1.11.2 - [Clarification/For Discussion] for "unsynchronized state"

[csfreak92]

ASVS 4.0 - 1.11.2 verification requirement is:

Verify that all high-value business logic flows, including authentication, session management and access control, do not share unsynchronized state.

I am confused about what the term "unsynchronized state" means here. There was no clarification or heading in ASVS documentation explaining this sub-section about this term. Can someone from the community or project leads please clarify what this means? And maybe we could add a sentence or two in this ASVS verification requirement as an edit to reflect it?

[tghosth]

Good point, leaving this for when we prepare 4.1

[csfreak92]

@tghosth, revisiting this issue since it still haunts our pentesters about what this means. Any idea about the "unsynchronized state" term here?

[jmanico]

I suggest just delete this requirement, it makes no sense. Care to PR this @csfreak92 ? It needs to go!

[elarlang]

I think we should be quite careful with deleting requirement in case we just don't clearly understand what it says - the risk is that requirement actually have good meaning but just bad wording.

In this case, linked CWE points to:
https://cwe.mitre.org/data/definitions/362.html

I can use this requirement for authentication when authentication state is not handled properly (it's not possible to verify is the same client reporting code for OAuth who actually started the authentication process) and therefore causing Race-Condition. Pure Race-Condition you can report to current 11.1.6, this one you can use for missing state. I feel I'm lacking English skills to explain that, so if it was not understandable then we can do another attempt.

[jmanico]

The CWE helps me understand this more. Can you change "synchronized state" to "business logic workflow" so the issue is more clear? Otherwise, I'm +1 on a requirement for this topic. I get it now.

[csfreak92]

Thanks for pointing that out @elarlang! That helped me understand this more.

@jmanico, in that case I would reword this into: Verify that all high-value business logic flows, including authentication, session management and access control, prevent race condition issues by not sharing business logic workflow What do you think?

[jmanico]

+1 I like this direction a lot. We’re getting close to a PR I’d enjoy merging to main. One of the main attacks and I see in this area is step skipping attacks, like skipping the payment step in a e-commerce workflow. Can we someone emphasize non-security flows a bit better? Maybe?

…

-- Jim Manico @manicode

On Mar 15, 2021, at 3:23 AM, Ralph Andalis ***@***.***> wrote:  Thanks for pointing that out @elarlang! That helped me understand this more. @jmanico, in that case I would reword this into: Verify that all high-value business logic flows, including authentication, session management and access control, prevent race condition issues by not sharing business logic workflow What do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[elarlang]

Just in case - for skipping steps there is requirement 11.1.1

Now this proposal is getting quite close to "Race-Condition" requirement 1.11.3 (also covered by 11.1.6).

[csfreak92]

Elar was right, this now looks like 1.11.3 and if we add step skipping attacks, then this is more like 11.1.1. So, how do you propose we make it better @elarlang? My main problem was the word unsynchronized state, but maybe that is how it really should be and we should just add an introduction sentence or two in V1.11 Business Logic Architectural Requirements explaining the term unsynchronized state? Thoughts?

[elarlang]

I don't know :) But I agree, this requirement may need some improvement - it describes what should NOT be done, but should describe what should be done.

I interpret this "state for authentication" as "session for authentication". Example:
Client starts authentication from application A via application SSO
If client is sent from A to SSO without having session/state fixed on application A then it's not possible to check, is this the same client who is requesting application A with "authentication decision" from SSO.
Usually - application A should send state parameter (or something like that) to SSO and then validate this state parameter, when client is sent from SSO back to application A. If there is no state in use, then it opens race-condition.

To which category my scenario suits, it's also good question. Is "keeping state" business logic, I'm not sure.

[jmanico]

Now that I see 11.1.1 and 11.1.3 and other JWT related requirements, I really do think we should close this out, it’s already well covered.

…

-- Jim Manico @manicode

On Mar 15, 2021, at 11:52 AM, Elar Lang ***@***.***> wrote:  I don't know :) But I agree, that this requirements may need some improvement - it describes what should NOT be done, but should describe what should be done. I interpret this "state for authentication" as "session for authentication". Example: Client starts authentication from application A via application SSO If client is sent from A to SSO without having session/state fixed on application A then it's not possible to check, is this the same client who is requesting application A with "authentication decision" from SSO. Usually - application A should send state parameter (or something like that) to SSO and then validate this state parameter, when client is sent from SSO back to application A. If there is no state in use, then it opens race-condition. To which category my scenario suits, it's also good question. Is "keeping state" business logic, I'm not sure. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[elarlang]

Where is covered "keeping state"?

[jmanico]

It's implied in 11.1.1

Verify the application will only process business logic flows for the same user in sequential step order and without skipping steps.

In order to track proper sequences you need to track state. Perhaps augment this requirement as opposed to a new one?

[csfreak92]

I agree, state tracking is done if proper sequences are being tracked. With that said, then 11.1.1 covers it so should we remove 1.11.2 then?

[jmanico]

Yes. I would accept a PR that removes 1.11.2 since it's a duplicate and hard to parse. I would like @elarlang to be on board... Elar, would you be ok with removing 1.11.2 and enhancing 1.11.1 in some way?

[elarlang]

In wishful thinking you can say that you can use requirement 11.1.1 for situation where state is not checked.
I would like have requirement which says clearly "Verify that there is state checked". I'm not sure 11.1.1 is the best one for that and maybe we need something clear and separate.

I need to analyze it a bit more, I'll come back to that later if it's ok.

[jmanico]

Absolutely. I'm willing to keep 11.1.2 if the language is improved. :)

…

On 3/17/21 6:57 AM, Elar Lang wrote: In wishful thinking you can say that you can use requirement 11.1.1 for situation where state is not checked. I would like have requirement which says clearly "Verify that there is state checked". I'm not sure 11.1.1 is the best one for that and maybe we need something clear and separate. I need to analyze it a bit more, I'll come back to that later if it's ok. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#737 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEBYCKGZNHPZWHD5QGXAS3TECDLHANCNFSM4MK3VKSA>.

[csfreak92]

Hey guys, just revisiting this requirement the other day and I kept reading all the related controls for a bunch of times. I agree with Elar that we still need 1.11.2, since neither 11.1.1 nor 11.1.3 talk about the state in an explicit way and it would be good for developers to be aware of that. Now, having said that, my revised control for 1.11.2 is missing a piece for defining the phrase "checking of state" mean and I can propose a PR soon. If you could help me out with the words for "state", then I think we will be in good shape.

In my understanding and as per the examples given previously, an application's state is used for determining whether a user has been authenticated, has a valid session, and has proper authorization to a specific function/API call. In the example mentioned by Elar, state could be used by the SSO to determine which party started the authentication process. State could be represented by a parameter or a token sent to the SSO in this case.

So, I think maybe we could make the current 1.11.2 to this - Verify that all high-value business logic flows, including authentication, session management and access control, checks the application state to prevent race conditions and business logic flaws. How does that sound, @elarlang , @jmanico?

If my proposed revision still sounds a bit off, then I would say we can leave it as is and then just add an introductory paragraph in V1.11 Business Logic Architecture section to explain what we mean by "unsynchronized state" as it is the unclear term. Thoughts?

[avasiljeva]

I'm following this discussion with a big interests, since in our group nobody can effectively interpret 1.11.2 as well.
Now it is starting to get clearer to me.

Slight adjustment on your @csfreak92 proposal:
Verify that all high-value business logic flows, including authentication, session management and access control, maintain a consistent application state and check it to prevent race conditions and business logic flaws

How does it sound?

[elarlang]

For me there is one precondition to move on with proposals - I need to understand, what is the purpose for entire "V1 Architecture, Design and Threat Modeling" from point of view - what should be there and what should not be there.

After that we can define, is some requirement actually belongs to that category or should be in some other category. In this given example, for me it seems that this requirement (1.11.2) is by current wording more "implementation" check, than architecture check.

[jmanico]

I'm following this discussion with a big interests, since in our group nobody can effectively interpret 1.11.2 as well.
Now it is starting to get clearer to me.

Slight adjustment on your @csfreak92 proposal:
Verify that all high-value business logic flows, including authentication, session management and access control, maintain a consistent application state and check it to prevent race conditions and business logic flaws

How does it sound?

I think this proposal is ready or almost ready for a PR. I like it!

[jmanico]

For me there is one precondition to move on with proposals - I need to understand, what is the purpose for entire "V1 Architecture, Design and Threat Modeling" from point of view - what should be there and what should not be there.

After that we can define, is some requirement actually belongs to that category or should be in some other category. In this given example, for me it seems that this requirement (1.11.2) is by current wording more "implementation" check, than architecture check.

This has been a long conversation and we are close to resolution. Let's focus on fixing the issue in this thread and open a new thread to move it to a new category if needed so we do not lose track of the progress made, please.

[wet-certitude]

I've tried to reconstruct the meaning of 1.11.2 based on this discussion. However, I'm not sure I quite get it.

From what I've gathered "state" is supposed to be the state of a user on the application, as in whether the user is logged in, which roles they have or where in the authentication process they currently are (in regards to e.g. to OAuth). In plain English a state could be Client X is in the process of authenticating against the IdP or User U is logged in on Client X and has the role 'admin'.

The new stipulation is:

Verify that business logic flows, including authentication, session management and access control, maintain a consistent application state and check to prevent race conditions and business logic flaws.

Based on that interpretation of "state" I tried to dissect this sentence:

"maintain a consistent application state" is somewhat unclear as there is no definition of what "consistent" means in this context. When thinking about race conditions, etc. "consistent" to me means the "state" is atomic. For example, when the user logs out, all parts of the application must be aware that the state is Client X is not authenticated at the same time. Vulnerabilities could result from parts of the application maintaining a different state (e.g. that the user is still logged in) due to race conditions. "business logic flaws" I think is very broad and can cover, I believe, a wide variety of flaws.

By my interpretation, this stipulation basically mandates that there should be no flaws (race condition or otherwise) when handling user state. A simplified requirement would be:
Verify that the application handles user state (e.g. login status) in a way that attackers cannot influence it in an undesired way.
However, this is a VERY broad statement and may therefore not be useful.

I might be way off with my interpretation. If that is the case, please provide a hypothetical vulnerability that violates this requirement, so I can understand its intention.

[elarlang]

Checking old and closed issues and I'm going to re-open this issue to address questions and concerns by @wet-certitude .

In general, @wet-certitude - if no-one reacts on comments on closed issues (like here), it makes sense to open new one.

[jmanico]

I am removing myself as the assignee for this bug because the conversation is circuitous and we are making no progress, and I'd like other stakeholders here to make the decision

Before I go....

I again suggest we delete this requirement 1.11.12 because (1) no one understands it clearly and (2) it's covered elsewhere well enough

When this much confusion abounds, it's time to remove the requirement.

[tghosth]

So I would consider changing this to:

Verify that all application flows maintain including authentication, session management and access control, maintain a consistent user state to prevent race conditions and business logic flaws.

Do you think that provides sufficient coverage?

[jmanico]

This class of race conditions is so ultra rare in web apps, I would drop the requirement. I'd rather talk about race conditions separate from auth if we must talk about it. - Jim

[tghosth]

@wet-certitude do you have any feedback on my suggestion above.
#737 (comment)

If I don't get feedback then I might just make the change.

[elarlang]

Ok, quick-fix is done for 1.11.2, but as it is implementation requirement, then we should move it to V11.1 category.

Now the good question is - is it unique requirement or it duplicates V11.1.1.

#	Description	L1	L2	L3	CWE
1.11.2	[MODIFIED] Verify that all application flows including authentication, session management and access control, maintain a consistent application and user state to prevent race conditions and business logic flaws.		✓	✓	362
11.1.1	Verify that the application will only process business logic flows for the same user in sequential step order and without skipping steps.	✓	✓	✓	841

[tghosth]

Please open a new issue on this as I agree some thought it needed there.

[elarlang]

Please open a new issue on this as I agree some thought it needed there.

#1303