search

OWASP / ASVS

Application Security Verification Standard
Creative Commons Attribution Share Alike 4.0 International
2.69k stars 657 forks source link

merge 1.11.2 and 11.1.1 #1303

Open elarlang opened 2 years ago

elarlang commented 2 years ago

spin-off from https://github.com/OWASP/ASVS/issues/737#issuecomment-1162087686

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)

jmanico commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 tbd

The requirement was originally added by this commit https://github.com/OWASP/ASVS/commit/2c18f120c358eac684b0092af1785f3c1bca3595 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?