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

update 3.5.5 #2204

Closed
elarlang opened this issue Oct 28, 2024 · 13 comments
Closed

update 3.5.5 #2204

elarlang opened this issue Oct 28, 2024 · 13 comments
Labels

Comments

@elarlang
Copy link
Collaborator

Spin-off from #2184

Current 3.5.5

# Description L1 L2 L3 CWE
3.5.5 [ADDED] Verify that only signing algorithms on an allowlist are allowed for a stateless token. 757

@randomstuff via #2184 (comment)

Should we verify as well that in a given context either MAC or public-key signature are used but not both interchangeably (notably as a way to prevent JWT key confusion type attacks)?

I think this is what @jmanico is referring to when talking about "signature type abuse" but if that's the case, the requirement should be more explicit about this (i.e. don't mix MAC and signatures).

@jmanico via #2184 (comment)

Another type of historical signature type abuse is when you forcibly set the JWT signature type to "none" and bypass signature validation. This is less of an issue when you set up an allow list of legal signatures and use modern JWT libraries.

@ryarmst
Copy link
Collaborator

ryarmst commented Oct 28, 2024

I would like to suggest the use of "Cryptographically Signed Token" in place of "Stateless Token" to further generalize. I assume there ought to be a corresponding documentation requirement, but this feels like it would coincide with V6.2.

@tghosth
Copy link
Collaborator

tghosth commented Oct 29, 2024

I would suggest:

# Description L1 L2 L3 CWE
3.5.5 [ADDED] Verify that only signing algorithms on an allow list can be used to create and verify Cryptographically Signed Token. The allow list should not include both symmetric and asymmetric algorithms and should not include the "None" algorithm. 757

I don't think we need an explicit documentation requirement...

@tghosth tghosth added 4) proposal for review Issue contains clear proposal for add/change something _5.0 - prep This needs to be addressed to prepare 5.0 labels Oct 29, 2024
@elarlang
Copy link
Collaborator Author

Styling things:

  • allow list > allowlist (based on Josh's previous edits)
  • Cryptographically Signed Token > lowercase

Content: the requirement describes what should not be in allowlist, but it does not describe, what should be there. At the moment it's negative requirement and still not clear, what exactly is allowed and what is not.

@jmanico
Copy link
Member

jmanico commented Oct 29, 2024

ADDED] Verify that only signing algorithms on an allowlist are allowed for a stateless token.

Some services support both HMAC's and Digital Signature Algorithms for various reasons (like when you are transitioning your token integrity method in a system to a new method, multi-tenant systems, etc).

HMAC's are cryptographic integrity algorithms and are not signing algorithms.

If you want to be generic, I suggest we drop the idea of "cryptographically signed", and instead just use "cryptographically authenticated" or "cryptographically secured" as a term.

@tghosth
Copy link
Collaborator

tghosth commented Oct 29, 2024

Ok so trying to respond to both elar and jim:

# Description L1 L2 L3 CWE
3.5.5 [ADDED] Verify that only algorithms on an allowlist can be used to create and verify cryptographically secured tokens. The allow list should include the permitted algorithms, ideally only either symmetric and asymmetric algorithms, and should not include the "None" algorithm. If both symmetric and asymmetric are needed, additional controls should prevent key confusion. 757

@elarlang
Copy link
Collaborator Author

  • "The allow list ..." - in first sentence there is "allowlist", in this one "allow list"
  • "ideally only either symmetric and asymmetric" - is here "and" correct or it should be "or"?

ping @randomstuff

@tghosth
Copy link
Collaborator

tghosth commented Oct 29, 2024

🤦

# Description L1 L2 L3 CWE
3.5.5 [ADDED] Verify that only algorithms on an allowlist can be used to create and verify cryptographically secured tokens. The allowlist should include the permitted algorithms, ideally only either symmetric or asymmetric algorithms, and should not include the "None" algorithm. If both symmetric and asymmetric are needed, additional controls should prevent key confusion. 757

@randomstuff
Copy link
Contributor

Should we add "in a given context" or something like that? ("Verify that only algorithms on an allowlist can be used to create and verify cryptographically secured tokens in a given context.") It is fine to use symmetric algorithms in one context and asymmetric algorithms in another context.

@tghosth
Copy link
Collaborator

tghosth commented Oct 30, 2024

Yeah I agree with that

🤦

# Description L1 L2 L3 CWE
3.5.5 [ADDED] Verify that only algorithms on an allowlist can be used to create and verify cryptographically secured tokens, for a given context. The allowlist should include the permitted algorithms, ideally only either symmetric or asymmetric algorithms, and should not include the "None" algorithm. If both symmetric and asymmetric are needed, additional controls should prevent key confusion. 757

@elarlang elarlang added 6) PR awaiting review and removed 4) proposal for review Issue contains clear proposal for add/change something labels Oct 30, 2024
@tghosth
Copy link
Collaborator

tghosth commented Oct 30, 2024

So the glossary says "Cryptographically Signed Token" but here we have used "cryptographically secured token".

Do we use "Cryptographically Signed Token" anywhere else

If no we should update the glossary and move on.

If yes, I think secured is more accurate than signed but we need to update to be consistent.

@elarlang

@elarlang
Copy link
Collaborator Author

This is more ping to @ryarmst and @randomstuff

@tghosth
Copy link
Collaborator

tghosth commented Oct 31, 2024

I don't think we need to bother them, I think secured is better anyway and we don't really use the term else where so I just committed 66adb0c and approved so if you are comfortable with that @elarlang then you can merge

@elarlang
Copy link
Collaborator Author

I merged it. If it needs further updates, we can improve it (again).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants