OWASP / ASVS

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

3.5.2 doesn't seem like it is in context #1522

Closed tghosth closed 6 months ago

tghosth commented 1 year ago

3.5.2 sounds more like it should either:

  1. Be in section 2.10 where we discuss intra-service authentication
  2. It should be made more generic to be clear that this is a general guideline that temporary session tokens should be used after authentication and never a static identifier.
# Description L1 L2 L3 CWE NIST §
3.5.2 [GRAMMAR] Verify that the application uses session tokens rather than static API secrets and keys, except with legacy implementations. 798

History:

# Description L1 L2 L3 CWE NIST §
3.5.2 Verify the application uses session tokens rather than static API secrets and keys, except with legacy implementations. - 798 tbd
3.5.2 Verify that single factor, static API secrets and keys are not used, except with legacy implementations. - 798 tbd
3.3.1 Verify single factor unchanging API secrets and keys are not used, except with legacy implementations. 1.0
jmanico commented 1 year ago

I would just say:

Verify that the application uses framework-specific session tokens or cryptographically signed JSON Web Tokens for session management. Static API secrets and keys should be avoided.

elarlang commented 1 year ago

Probably we need to start from the question: what problem it addresses?

From the text I don't read it as machine-to-machine communication specific.

As a quick-fix I would get rid of phrase ", except with legacy implementations".

Sjord commented 1 year ago

It seems this section is for applications that authenticate with OpenID connect and supply a JWT to the user, instead of a random session cookie. I am not sure how "static API secrets and keys" can be an alternative to that, so in this context this requirement doesn't make sense to me.

For intra-service authentication, static API secrets and keys seem acceptable to me.

tghosth commented 1 year ago

Some more details:

History:

# Description L1 L2 L3 CWE NIST §
3.5.2 Verify the application uses session tokens rather than static API secrets and keys, except with legacy implementations. - 798 tbd
3.5.2 Verify that single factor, static API secrets and keys are not used, except with legacy implementations. - 798 tbd
3.3.1 Verify single factor unchanging API secrets and keys are not used, except with legacy implementations. 1.0

See also 2.10.1:

# Description L1 L2 L3 CWE NIST §
2.10.1 Verify that intra-service secrets do not rely on unchanging credentials such as passwords, API keys or shared accounts with privileged access. 287
2.10.1 Verify that intra-service secrets do not rely on unchanging credentials such as passwords, API keys or shared accounts with privileged access. OS assisted HSM 287 5.1.1.1
2.10.1 Verify that intra-service secrets do not rely on unchanging passwords, such as API keys or shared accounts with privileged access. OS assisted HSM 287 5.1.1.1
2.10.1 Verify that integration secrets do not rely on unchanging passwords, such as API keys or shared privileged accounts. OS assisted HSM 287 5.1.1.1
2.11.1 Verify that integration secrets do not rely on unchanging passwords, such as API keys or shared privileged accounts. Software OS assisted HSM 5.1.1.1
2.11.1 Integration secrets SHOULD NOT rely on unchanging passwords, such as API keys or shared privileged accounts. Software OS assisted HSM 5.1.1.1
2.11.1 Integration secrets SHOULD NOT rely on unchanging passwords, such as API keys or shared privileged accounts. If passwords are required, the credential should not be a default account and stored with sufficient protection to prevent offline recovery attacks, including local system access. Software OS assisted HSM 5.1.1.1
2.21 Integration secrets SHOULD NOT rely on unchanging memorized secrets, such as API keys or shared privileged accounts. If memorized secrets are required, the credential should not be a default account, and stored with sufficient protection to prevent offline recovery attacks, including local system access. Software OS assisted HSM 5.1.1.1
2.21 Integration memorized secrets SHOULD NOT rely on unchanging memorized secrets, such as API keys or shared privileged accounts. If memorized secrets are required, the credential should not be a default account, and stored with sufficient protection to prevent offline recovery attacks, including local system access. Software OS assisted HSM 4.0 5.1.1.1

From Andrew's deck: image

From Andrew's deck: image

tghosth commented 1 year ago

There has been discussion of 2.10.1 at #1032

tghosth commented 1 year ago

3.5.2 sounds more like it should either:

  1. Be in section 2.10 where we discuss intra-service authentication
  2. It should be made more generic to be clear that this is a general guideline that temporary session tokens should be used after authentication and never a static identifier.

I am leaning towards option 2 here. 2.10.1 seems like it is going to change slightly anyway. The history of 3.5.2 is a bit weird but I think the idea is partially covered by 2.10.1 and the rest is kind of a generic basis for session management.

elarlang commented 1 year ago

After reading it again, I still have the same question: what problem it solves?

tghosth commented 1 year ago

I think the problem it solves is that we don't have a basic level requirement which says, use session tokens not fixed values which I think is why Jim came up with:

Verify that the application uses framework-specific session tokens or cryptographically signed JSON Web Tokens for session management. Static API secrets and keys should be avoided.

tghosth commented 1 year ago

@elarlang what do you think about this very basic session management requirement which Jim suggested here https://github.com/OWASP/ASVS/issues/1522#issuecomment-1380700455

Verify that the application uses framework-specific session tokens or cryptographically signed JSON Web Tokens for session management. Static API secrets and keys should be avoided.

elarlang commented 1 year ago

Is the context here machine-to-machine integrations or it covers all "usual" end-users with their browsers as well? If it is machine to machine, How it's different from current 2.10.1?

# Description L1 L2 L3 CWE NIST §
2.10.1 Verify that intra-service secrets do not rely on unchanging credentials such as passwords, API keys or shared accounts with privileged access. 287

For 2.10.1 there is separate issue opened: #1032

At the moment the requirement is in "Token based session management", but I can not read this category out from the requirement text.

What is "framework-specific" session token?

tghosth commented 1 year ago

Is the context here machine-to-machine integrations or it covers all "usual" end-users with their browsers as well? If it is machine to machine, How it's different from current 2.10.1?

I think we are talking about usual end users here and that service to service is something else handled in 2.10.1.

What is "framework-specific" session token?

This is a good question and I think actually we should be more consistent with the wording we are using in 3.2 so I would propose the following:

# Description L1 L2 L3 CWE NIST §
3.1.3 [ADDED] Verify that the application uses opaque or cryptographically signed tokens for session management. Static API secrets and keys should be avoided.
tghosth commented 1 year ago

@elarlang do you approve this wording?

elarlang commented 1 year ago
We already have those requirements: # Description L1 L2 L3 CWE NIST §
3.2.2 [MODIFIED] Verify that opaque session tokens possess at least 128 bits of entropy. (C6) 331 7.1
3.2.4 [MODIFIED] Verify that opaque session tokens are generated using a secure random function. (C6) 330 7.1
3.5.2 [GRAMMAR] Verify that the application uses session tokens rather than static API secrets and keys, except with legacy implementations. 798

Why we need proposed 3.1.3? What problem it solves? Feels like some overlapping.

tghosth commented 1 year ago

Ah yeah, I think it is intended to replace 3.5.2 with a generic requirement for the need for session management so it should be this:

# Description L1 L2 L3 CWE NIST §
3.1.3 [MOVED FROM 3.5.2, MODIFIED] Verify that the application uses opaque or cryptographically signed tokens for session management. Static API secrets and keys should be avoided.

@elarlang

elarlang commented 1 year ago

Maybe some duplication with "session tokens" Verify that the application uses opaque session tokens or cryptographically signed tokens for session management. Static API secrets and keys should be avoided.

Otherwise (I personally) may read it like: Verify that the application uses ( opaque OR cryptographically ) signed tokens for session management. Static API secrets and keys should be avoided.

... and it rises the question - why we need to sign opaque tokens,

tghosth commented 1 year ago

Fair point, how about this @elarlang:

# Description L1 L2 L3 CWE NIST §
3.1.3 [MOVED FROM 3.5.2, MODIFIED] Verify that the application uses either cryptographically signed or opaque tokens for session management. Static API secrets and keys should be avoided.
elarlang commented 1 year ago

ok with text. no CWE?

tghosth commented 1 year ago

I cannot find a good CWE match so I would rather just get this in for now.

tghosth commented 1 year ago

Although actually 3.5.2 originally had CWE-798 which is not ideal but it is something.

tghosth commented 1 year ago

Opened #1743 to resolve

craig-shony commented 6 months ago

Sorry to rehash this, but I think the original concern was that "Static API secrets and keys should be avoided." would get interpreted as almost a stand alone statement. I think this is a valid concern as while reading this, though I did eventually lean toward "this only applies to client-side sessions", there was some thought of whether or not ASVS was wanting me to ditch static API secrets m2m.

tghosth commented 6 months ago

@elarlang what do you think?

Sjord commented 6 months ago

ASVS was wanting me to ditch static API secrets m2m.

There is more discussion about this in 2.10.1 Verify that intra-service secrets do not rely on unchanging credentials such as passwords, API keys · Issue #1032 · OWASP/ASVS.

tghosth commented 6 months ago

@set-reminder 10 minutes Josh to look at this one

octo-reminder[bot] commented 6 months ago

Reminder Thursday, March 21, 2024 1:09 PM (GMT+01:00)

Josh to look at this one

octo-reminder[bot] commented 6 months ago

🔔 @tghosth

Josh to look at this one

tghosth commented 6 months ago

Hi @craig-shony, I agree with @Sjord that the best place to discuss this would be in #1032 where I think we are already deep into this discussion :)

If you don't agree, tag me with some reasoning and I can reopen this