search

OWASP / ASVS

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

3.5.3 Stateless Token Signing should be L1 #839

Closed marcwickenden closed 3 years ago

marcwickenden commented 4 years ago

Level 1 does not appear to include any checks to ensure stateless tokens such as JWTs have signatures enforced. This feels like an oversight to me. Case in point, we're doing a L1 test this week on an app and found the JWT signature was not checked at all meaning trivial forgery of tokens and therefore identities.

Under a strict L1 test we would not have checked this but I would argue that an opportunistic threat would identify this weakness, particularly as it could be largely automated.

Did I miss a relevant control somewhere else or would you agree 3.5.3 should also be included at L1?

jmanico commented 4 years ago

+1 this is the most absolute basics when it comes to JWT security.

-- Jim Manico @Manicode

On Sep 9, 2020, at 9:44 PM, Marc Wickenden notifications@github.com wrote:

 Level 1 does not appear to include any checks to ensure stateless tokens such as JWTs have signatures enforced. This feels like an oversight to me. Case in point, we're doing a L1 test this week on an app and found the JWT signature was not checked at all meaning trivial forgery of tokens and therefore identities.

Under a strict L1 test we would not have checked this but I would argue that an opportunistic threat would identify this weakness, particularly as it could be largely automated.

Did I miss a relevant control somewhere else or would you agree 3.5.3 should also be included at L1?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

jmanico commented 3 years ago

Would you kindly submit a PR on this? Let's do it!

elarlang commented 3 years ago

If you touch it already, should it clearly mention, that alg:None is not ok?

jmanico commented 3 years ago

Most of the libraries have fixed the problem of alg:none and I have not seen that problem since like 2017. Am I wrong? Do you see these problems in recent tests @elarlang ?

elarlang commented 3 years ago

No, alg:none I have not seen lately. (offtopic here) Only weak secrets - maybe we need some separate requirement for that as well.

roelstorms commented 3 years ago

Was going through ASVS and noticed the same. +1 Is someone working on a pull request? Else I can give it a go.

elarlang commented 3 years ago

Signing JWT tokens should be checked via level 1.

If we change some requirement to level 1, then every single bit of that should be pen-testable.

I'm not 100% about this requirement, especially about "other countermeasures" part.

V3.5.3 Verify that stateless session tokens use digital signatures, encryption, and other countermeasures to protect against tampering, enveloping, replay, null cipher, and key substitution attacks.

I think with changing level to 1, we need to make this requirement itself more strict. If necessary, split it for different defenses.

I need some help or directions to fully understand this requirement at the moment:

jmanico commented 3 years ago

Signing JWT tokens should be checked via level 1.

I agree 1000%.

V3.5.3 Verify that stateless session tokens use digital signatures, encryption, and other countermeasures to protect against tampering, enveloping, replay, null cipher, and key substitution attacks.

This is overkill, encryption is not necessary in a JWT if it does not contain sensitive data (except for the userid).

jmanico commented 3 years ago

VERY FEW controls protect against replay, this is one of the big dangers of JWT and session tokens, I'd like to drop some of the BS and split this into 2 requirements or simplify this one. I agree with your take on this @elarlang

jmanico commented 3 years ago

No, alg:none I have not seen lately. (offtopic here) Only weak secrets - maybe we need some separate requirement for that as well.

I think this is also a good idea

roelstorms commented 3 years ago

VERY FEW controls protect against replay, this is one of the big dangers of JWT and session tokens, I'd like to drop some of the BS and split this into 2 requirements or simplify this one. I agree with your take on this @elarlang

V3.4 Cookie based session management does not include the protection against replay attacks. In practice I never see protection against replay on session management level. Is defense against replay attacks a desirable property on session management level or is it sufficient to have this implemented by the application where needed?

In my opinion, this can be dropped from V3.5.3.

No idea what is meant with enveloping. I tried to find out who wrote this requirement via git blame. There is one commit that kind of rephrases this requirements:

https://github.com/OWASP/ASVS/commit/0f5ac563056132e650dbaa11cdf34d697c848bf1

But it is @vanderaj who first introduced the terminology:

https://github.com/OWASP/ASVS/commit/64194cfc4e4b3229acb20e03812f626bc247ed43

My best guess is that it adds a second envelop around the JWT or it tampers with the envelop attributes such as alg.

This is overkill, encryption is not necessary in a JWT if it does not contain sensitive data (except for the userid).

It might be overkill but it does protect against tampering so it is a valid solution.

No, alg:none I have not seen lately. (offtopic here) Only weak secrets - maybe we need some separate requirement for that as well.

I think this is also a good idea

V6.4.1 states that secrets are created securely. This could imply that you are not allowed to use weak secrets for token signing.

jmanico commented 3 years ago

I agree on dropping replay from 3.5.3.

It might be overkill but it does protect against tampering so it is a valid solution.

Digital signatures protect against tampering already, you do not need JWT Encryption for this property. Since we advise (or should advise) to keep sensitive data out of JWT's there is no need to encrypt JWT's. It's overkill.

No idea what is meant with enveloping.

@vanderaj is VERY smart. Enveloping is an old SOAP trick, not a JWT need. I do think we should drop this however.

I think V6.4.1 is the right place to add a little more about strong secrets.

danielcuthbert commented 3 years ago

We've re-worked 3.5.3 to be clearer:

| 3.5.3 | [MODIFIED, LEVEL 2 > 1] Verify that stateless session tokens make use of digital signatures to protect against tampering. | ✓ | ✓ | ✓ | 345 | |

https://github.com/OWASP/ASVS/commit/771a33a1c2d1ce52190b1d4c970e20e1c526afc7