OWASP / ASVS

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

3.7.1 seems to have got confused from its original purpose #1523

Closed tghosth closed 1 week ago

tghosth commented 1 year ago

The table below shows how this requirement has evolved since it was added:

Point in time # Description L1 L2 L3 CWE NIST §
Historic 3.7.1 Verify that all post-reset, post-registration, and post-authentication high value or adminstrative functionality verifies the user session is both fully logged in and has a valid post-authentication role before permitting any changes or transactions, especially in relation to user profile updates, password changes, MFA enrollment, and administrative functionality.
Current 3.7.1 [MODIFIED] Verify the application requires re-authentication or secondary factor verification before allowing any sensitive transactions or account modifications. 306

In principle, it is in the "Defenses Against Session Management Exploits" section but practically speaking it is the only item here and it is designed to specifically handle the "Half-open Attack".

I have two questions:

What do people think?

Sjord commented 1 year ago

Does the current wording reflect the original intent?

No. I think the current requirement is fine, but it doesn't match the original intent, and it doesn't fit in the current chapter about half-open attacks.

Is the original requirement specific and unique enough to require its own requirement?

The original requirement basically said "require authentication". It made the distinction that a user must be fully logged in, but this isn't a thing, normally.

I think "require authentication" could be a valid requirement. This would cover half-open attacks and other authentication bypasses, and possibly also shared user accounts.

Also relevant:

1.2.4 Verify that all authentication pathways and identity management APIs implement consistent authentication security control strength

tghosth commented 1 year ago

Who else has opinions on this @elarlang @jmanico @danielcuthbert?

jmanico commented 1 year ago

I think this change is radically better, I love it!

elarlang commented 1 year ago

We need current 3.7.1.

I agree with other comments like it seems to be quite different from original intent (if needed bring back old requirement with original intent) and it seems to be in a bit wrong place (under wrong title).

Related comment: https://github.com/OWASP/ASVS/issues/1496#issuecomment-1382720743

jmanico commented 1 year ago

I prefer the current, cleaner 3.7.1 even with the content change.

tghosth commented 1 year ago

Comment form @elarlang at #1496

Is suggestion meant to be new requirement on update to current 3.7.1?

I would suggest something similar to 3.7.1 but more focused and I would be tempted to consider this a V2 authentication issue rather than V3 session management..

Without any extra problems with authentication or authorization - 3.7.1 can happen only when user's session is taken over and it's defense against escalation from session takeover to account takeover.

So 3.7.1 requires that user re-authenticates (V2) to be sure, that in case of session takeover (V3) user is legit and authorized (V4) to do this sensitive action. From this I would say, it's more authorization requirement.

I don't think there is a question as to whether they are authorized, I think it is more making sure that they are still the correct authenticated person, That is why I suggested V2. What do you think @elarlang

tghosth commented 1 year ago

(Note that I opened #1642 to resolve #1496 which deals purely with the wording.)

elarlang commented 1 year ago

Updated requirement:

# Description L1 L2 L3 CWE NIST §
3.7.1 [MODIFIED] Verify that the application requires re-authentication or secondary verification before allowing highly sensitive transactions or modifications to account profile or authentication settings. 306

So, the situation is - wording is improved but you propose to move it to authentication category?

Without any extra problems with authentication or authorization - 3.7.1 can happen only when user's session is taken over and it's defense against escalation from session takeover to account takeover.

I watch it as "authentication is just a tool" in this context to provide defense on situation, when victim's session is taken over.

In short - I'm ok when it stays where it is at the moment. If there is good reasoning or candidate category for authentication, I can discuss it.

jmanico commented 1 year ago

I’m ok to leave it as is too.

tghosth commented 1 year ago

Ok, I think this needs to move somewhere so I am going to leave this issue open but mark it for the big rework

jmanico commented 5 months ago

I would suggest we move this to access control. This is really about protecting individual high-risk features, its not about protecting authentication or the session, per say (although I agree with Elar's and other's comments that session hijacking is what makes this important).

The current version looks a little too specific:

3.7.1 [MODIFIED] Verify that the application requires re-authentication or secondary verification before allowing highly sensitive transactions, modifications to account profile or authentication settings, or a large export of sensitive data. 306

I suggest we move it back to:

Current 3.7.1 [MODIFIED] Verify the application requires re-authentication or secondary factor verification before allowing any sensitive transactions or account modifications. 306

Account modification is really special and deserves mention. But perhaps change "sensitive transactions" to "high-risk transactions"?

ryarmst commented 1 week ago

Following discussion, consensus was to keep this requirement in V3 (see #2270 for new section description proposal). I would like to suggest the following wording:

Verify that the application requires re-authentication or secondary verification before allowing highly sensitive transactions or modifications to sensitive account attributes such as authentication settings.

IMO authentication settings are worth calling out.

tghosth commented 1 week ago

So the change effectively gets rid of "or a large export of sensitive data". Was there a specific reason for that?

ryarmst commented 1 week ago

@tghosth Not sure, but IMO it is perhaps challenging to define and test and may be infrequently applicable.

tghosth commented 1 week ago

@ryarmst as we discussed , you point out that export is a slightly different problem and is an only an example anyway so let's please PR your proposal in

ryarmst commented 1 week ago

@tghosth Please see #2292