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

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

Closed
tghosth opened this issue Jan 12, 2023 · 27 comments · Fixed by #1743
Closed

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

tghosth opened this issue Jan 12, 2023 · 27 comments · Fixed by #1743
Assignees
Labels
6) PR awaiting review _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@tghosth
Copy link
Collaborator

tghosth commented Jan 12, 2023

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
3.5.2 Verify that single factor, static API secrets and keys are not used, except with legacy implementations. - 798
3.3.1 Verify single factor unchanging API secrets and keys are not used, except with legacy implementations. 1.0
@tghosth tghosth added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet _5.0 - prep This needs to be addressed to prepare 5.0 Community wanted We would like feedback from the community to guide our decision otherwise we will progress Leaders decision Big decisions, like re-structuring or concept changes labels Jan 12, 2023
@jmanico
Copy link
Member

jmanico commented Jan 12, 2023

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
Copy link
Collaborator

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
Copy link
Contributor

Sjord commented Jan 17, 2023

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
Copy link
Collaborator Author

tghosth commented May 23, 2023

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
3.5.2 Verify that single factor, static API secrets and keys are not used, except with legacy implementations. - 798
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
Copy link
Collaborator Author

tghosth commented May 23, 2023

There has been discussion of 2.10.1 at #1032

@tghosth
Copy link
Collaborator Author

tghosth commented May 23, 2023

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
Copy link
Collaborator

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

@tghosth
Copy link
Collaborator Author

tghosth commented Jun 12, 2023

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 tghosth removed the Leaders decision Big decisions, like re-structuring or concept changes label Sep 14, 2023
@tghosth
Copy link
Collaborator Author

tghosth commented Sep 14, 2023

@elarlang what do you think about this very basic session management requirement which Jim suggested here #1522 (comment)

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
Copy link
Collaborator

elarlang commented Sep 14, 2023

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
Copy link
Collaborator Author

tghosth commented Sep 18, 2023

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
Copy link
Collaborator Author

tghosth commented Sep 28, 2023

@elarlang do you approve this wording?

@elarlang
Copy link
Collaborator

elarlang commented Sep 28, 2023

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
Copy link
Collaborator Author

tghosth commented Sep 28, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator Author

tghosth commented Oct 1, 2023

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
Copy link
Collaborator

elarlang commented Oct 1, 2023

ok with text. no CWE?

@tghosth
Copy link
Collaborator Author

tghosth commented Oct 3, 2023

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

@tghosth
Copy link
Collaborator Author

tghosth commented Oct 3, 2023

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

@tghosth
Copy link
Collaborator Author

tghosth commented Oct 3, 2023

Opened #1743 to resolve

@tghosth tghosth added 6) PR awaiting review and removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet Community wanted We would like feedback from the community to guide our decision otherwise we will progress labels Oct 3, 2023
elarlang pushed a commit to elarlang/ASVS that referenced this issue Nov 9, 2023
elarlang added a commit to elarlang/ASVS that referenced this issue Nov 9, 2023
@craig-shony
Copy link

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 tghosth reopened this Mar 13, 2024
@tghosth
Copy link
Collaborator Author

tghosth commented Mar 13, 2024

@elarlang what do you think?

@tghosth tghosth added the next meeting Filter for leaders label Mar 13, 2024
@Sjord
Copy link
Contributor

Sjord commented Mar 14, 2024

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
Copy link
Collaborator Author

tghosth commented Mar 21, 2024

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

Copy link

octo-reminder bot commented Mar 21, 2024

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

Josh to look at this one

@tghosth tghosth removed the next meeting Filter for leaders label Mar 21, 2024
Copy link

octo-reminder bot commented Mar 21, 2024

🔔 @tghosth

Josh to look at this one

@tghosth
Copy link
Collaborator Author

tghosth commented Apr 3, 2024

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

@tghosth tghosth closed this as completed Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6) PR awaiting review _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.

6 participants