Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

merge 1.11.2 and 11.1.1 (edit: handle 1.11.2) #1303

Closed
elarlang opened this issue Jun 23, 2022 · 8 comments · Fixed by #2158
Closed

merge 1.11.2 and 11.1.1 (edit: handle 1.11.2) #1303

elarlang opened this issue Jun 23, 2022 · 8 comments · Fixed by #2158
Assignees
Labels
4b Major-rework These issues need to be part of a full chapter rework 6) PR awaiting review josh/elar V1 _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

spin-off from #737 (comment)

V1.11 Business Logic Architecture

# 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

V11.1 Business Logic Security

# Description L1 L2 L3 CWE
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

(abstract) proposal - merge 1.11.2 and 11.1.1 to 11.1.1 (or at least move 1.11.2 to V11.1 category)

@elarlang elarlang added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos labels Jun 23, 2022
@elarlang elarlang self-assigned this Jun 23, 2022
@jmanico
Copy link
Member

jmanico commented Jul 6, 2022

The problem with 11.1.1 is that many workflows are not sequential, but are conditional based on user input. Perhaps?

[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 and that required steps are not allowed to be sipped.

@elarlang
Copy link
Collaborator Author

elarlang commented Jul 6, 2022

The problem with 11.1.1 is that many workflows are not sequential, but are conditional based on user input. Perhaps?

For this kind of functionality we don't apply 11.1.1.

About proposal - we need to keep separate "step order" and "race condition", those are separate problems and 11.1.1 should cover only sequential steps. I think my initial proposal is not precise - we just need to merge current 1.11.2 to V11.1 subcategory (or, if it is unique enough, just move to V11.1 subcategory).

Race condition is covered by 11.1.6:

# Description L1 L2 L3 CWE
11.1.6 Verify that the application does not suffer from "Time Of Check to Time Of Use" (TOCTOU) issues or other race conditions for sensitive operations. 367

@Sjord
Copy link
Contributor

Sjord commented Aug 13, 2022

Verify that all application flows including authentication, session management and access control, maintain a consistent application and user state

I interpret this as keeping information in multiple places, and these can become inconsistent with each other. E.g. logging out of the application does not log you out on the identitiy provider, and then you can log back in to the application without reauthorizing. Or blocking a user does not terminate the current sessions, so his session says he is allowed to use the application but the database does not. This seems really an architectural requirement to me, and not specific to race conditions.

@jmanico
Copy link
Member

jmanico commented Sep 28, 2022

From https://github.com/OWASP/ASVS/blob/master/5.0/en/0x19-V11-BusLogic.md

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

I want to state again this is not about sequential. It's about protecting steps in a business logic sequence that could hurt your business.

For example, in an eCommerce flow. I may have:

  1. add to cart
  2. checkout
  3. Pay the money
  4. enter shipping
  5. submit order and ship the product

The danger in this sequence is only in step 5. Only that step needs to be protected, and it does not need to be sequential.

@tghosth tghosth self-assigned this Dec 7, 2022
@tghosth tghosth added _5.0 - prep This needs to be addressed to prepare 5.0 josh/elar and removed 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos labels Dec 7, 2022
@tghosth
Copy link
Collaborator

tghosth commented Jan 25, 2023

History of 1.11.2:

# 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
1.11.2 Verify that all high-value business logic flows, including authentication, session management and access control, do not share unsynchronized state. 362
1.15.2 Verify that all high value business logic flows, including authentication, session management and access control, do not share unsynchronized shared state. 362

The requirement was originally added by this commit 2c18f12 but the background is not clear.

I think @Sjord's interpretation makes the most sense, even if it was not the original intention of the requirement.

Do we think we would therefore need to transform this into a 1.2, 1.3 or 1.4 requirement? How would a tester verify this, by reviewing design/architecture documentation?

@tghosth tghosth added 4b Major-rework These issues need to be part of a full chapter rework V1 and removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Jul 10, 2023
@tghosth
Copy link
Collaborator

tghosth commented Oct 15, 2024

Ok, I think that 11.1.1 has a clear purpose and reasoning.

I can sort of see what 1.11.2 is talking about but I think it is too vague to implement or enforce, we could try putting it in a recommendation but even then I think it is problematic.

I would suggest deleting 1.11.2

@elarlang elarlang changed the title merge 1.11.2 and 11.1.1 merge 1.11.2 and 11.1.1 (edit: handle 1.11.2) Oct 16, 2024
@elarlang
Copy link
Collaborator Author

Current 1.11.2:

V1.11.2 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.

CWE for 1.11.2 points to "CWE-362: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition')"

Many parts of this requirement are covered with a more precise requirement:

  • Session management, Session Termination
    • V3.8.5 Verify that the application terminates all active sessions when a user account is disabled or deleted (such as an employee leaving the company).
    • V3.8.6 Verify that application administrators are able to terminate active sessions for an individual user or for all users.
  • Authorization

User state and race condition is also covered in 11.1.6 (updated wording from #2138):

Verify that all high-value business logic flows, as well as authentication, session management, and access control, are thread-safe, resistant to time-of-check and time-of-use (TOCTOU) race conditions, and utilize synchronization and locking mechanisms for sensitive operations to maintain internal data consistency and user state.

For me it seems, that valid proposal is to delete 1.11.2 as "MERGED TO 11.1.6".

@elarlang elarlang added the 4) proposal for review Issue contains clear proposal for add/change something label Oct 16, 2024
@tghosth
Copy link
Collaborator

tghosth commented Oct 16, 2024

Looks good to me

elarlang pushed a commit to elarlang/ASVS that referenced this issue Oct 16, 2024
@elarlang elarlang added 6) PR awaiting review and removed 4) proposal for review Issue contains clear proposal for add/change something labels Oct 16, 2024
@elarlang elarlang linked a pull request Oct 16, 2024 that will close this issue
tghosth pushed a commit that referenced this issue Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4b Major-rework These issues need to be part of a full chapter rework 6) PR awaiting review josh/elar V1 _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants