orchestracities / boost

BOOST4.0 EIDS FIWARE CIM Connector
MIT License
0 stars 0 forks source link

JWT validation false positives #14

Closed c0c0n3 closed 4 years ago

c0c0n3 commented 4 years ago

Validation works for the most part but there are some gotchas. Specifically, to jwt-go (the lib we use under the bonnet for validation) a field with a 0 value is the same as the field not being there! So e.g. this token (pseudo code):

{ alg: RS256 }.{ exp: 0 }.valid-rs256-signature

passes validation even though it expired at the beginning of the epoch!

Instead of delegating validation to jwt-go, consider rolling out your own. Specifically, for the following JWT registered claims: exp ("expires at"), iat ("issued at"), nbf ("not before").

chicco785 commented 4 years ago

the behaviour may be correct. i.e. the validation may just verify the signature, but nothing about expiry dates or content, which you will have to verify separately.

c0c0n3 commented 4 years ago

Note to self. If rolling out custom validation, be extra careful about security implications!!!

For example, checking the signature algo ∂ (HMAC, RSA, ...) declared in the JWT alg header is critical, since jwt-go will check the token according to ∂. Specifically, we could say that in (very!) abstract terms,jwt-go implements a signature verification function

F: Algo x JWT -> Bool 

so that for a JWT token t

t's signature is valid <=> F(∂, t) = T   where ∂ = t.alg

Because of this, an attacker could use our public key K as an HS256 shared secret to sign a forged token u:

header = { alg: HS256, ... }
payload = { ... }
signature = hs256-sign(K, header + payload)

Since in this case F(HS256, u) = T, we'd be fooled into thinking u got signed with the private key paired to K!!!

For a better explanation of the problem, see e.g.

c0c0n3 commented 4 years ago

@chicco785

the behaviour may be correct

Quite plainly, I think it's a bug in jwt-go, courtesy of Go's weak type system. But I think you're talking about the scope of our validation? i.e. we shouldn't validate any registered claims? But then who or what's going to check the token hasn't expired? Can Orion do that? If the adapter returns success to the Mixer, then the message gets passed on to Orion...

However, we could implement a custom policy resource like they do in the IBM adapter so that you could check e.g. the exp field using an Istio config expression.

chicco785 commented 4 years ago

orion can't do that. i am simply saying that "validation" as jwt validation is one thing (and i suppose the library does what's needed), then you need to apply some logic based on the content of the token to finalise the authorisation. i.e. the fact that a jwt token is valid, does not mean you are authorised to do something with that token.

On Thu, 9 Jan 2020 at 14:50, c0c0n3 notifications@github.com wrote:

@chicco785 https://github.com/chicco785

the behaviour may be correct

Quite plainly, I think it's a bug in jwt-go, courtesy of Go's weak type system. But I think you're talking about the scope of our validation? i.e. we shouldn't validate any registered claims? But then who or what's going to check the token hasn't expired? Can Orion do that?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/orchestracities/boost/issues/14?email_source=notifications&email_token=AAF5YASXBQLB2L3IQBCHIZLQ44TUHA5CNFSM4KEYL2E2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIQLT4Q#issuecomment-572570098, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAF5YAVZCGIQEPHMV25JGYLQ44TUHANCNFSM4KEYL2EQ .

-- Dr. FEDERICO MICHELE FACCA CTO, Head of Martel Lab +41 788075838 MARTEL INNOVATE https://www.martel-innovate.com/ - INNOVATION, WE MAKE IT HAPPEN Click HERE to download Martel reports and white papers! https://www.martel-innovate.com/premium-content/ Follow us on TWITTER https://twitter.com/Martel_Innovate

c0c0n3 commented 4 years ago

orion can't do that

yep, just I just wanted to double-check with you, thanks.

simply saying that "validation" as jwt validation is one thing (and i suppose the library does what's needed)

As far as signature verification goes, jwt-go can verify the signature correctly as long as you check the JWT alg field yourself---see my earlier comment about it. If you don't do that, verification goes out of the window.

However, jwt-go can also check some of the registered claims, notably the exp field, but like I said there's a bug in the lib whereby a token with a valid signature and an exp field set to 0 is considered valid.

then you need to apply some logic based on the content of the token to finalise the authorisation...the fact that a jwt token is valid, does not mean you are authorised to do something with that token.

Exactly, so let's stick to validation here :-) To me, RSA token validation has the following meaning:

  1. Check the token is well-formed.
  2. Verify the payload hasn't been tampered with.
  3. Identify payload author.
  4. Verify the payload is current, if possible.

RSA signature verification gives us (2) and (3) whereas the additional checks on the exp and nbf fields give us (4). I think we both agree the adapter should do (1), (2) and (3), but perhaps you're not too keen on (4). Should we drop (4) then? Why though?

chicco785 commented 4 years ago

simply we can do (4) outside the library, I suspect that's not complex.

On Thu, 9 Jan 2020 at 17:10, c0c0n3 notifications@github.com wrote:

orion can't do that

yep, just I just wanted to double-check with you, thanks.

simply saying that "validation" as jwt validation is one thing (and i suppose the library does what's needed)

As far as signature verification goes, jwt-go can verify the signature correctly as long as you check the JWT alg field yourself---see my earlier comment https://github.com/orchestracities/boost/issues/14#issuecomment-572567394 about it. If you don't do that, verification goes out of the window.

However, jwt-go can also check some of the registered claims, notably the exp field, but like I said there's a bug in the lib whereby a token with a valid signature and an exp field set to 0 is considered valid.

then you need to apply some logic based on the content of the token to finalise the authorisation...the fact that a jwt token is valid, does not mean you are authorised to do something with that token.

Exactly, so let's stick to validation here :-) To me, RSA token validation has the following meaning:

  1. Check the token is well-formed.
  2. Verify the payload hasn't been tampered with.
  3. Identify payload author.
  4. Verify the payload is current, if possible.

RSA signature verification gives us (2) and (3) whereas the additional checks on the exp and nbf fields give us (4). I think we both agree the adapter should do (1), (2) and (3), but perhaps you're not too keen on (4). Should we drop (4) then? Why though?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/orchestracities/boost/issues/14?email_source=notifications&email_token=AAF5YAR6RLTXFNKKHDI6HVDQ45D6ZA5CNFSM4KEYL2E2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIQ2QHY#issuecomment-572631071, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAF5YARC2WKRZZUVRFADJTDQ45D6ZANCNFSM4KEYL2EQ .

-- Dr. FEDERICO MICHELE FACCA CTO, Head of Martel Lab +41 788075838 MARTEL INNOVATE https://www.martel-innovate.com/ - INNOVATION, WE MAKE IT HAPPEN Click HERE to download Martel reports and white papers! https://www.martel-innovate.com/premium-content/ Follow us on TWITTER https://twitter.com/Martel_Innovate

c0c0n3 commented 4 years ago

agreed, cool bananas!

gboege commented 4 years ago

A little late, but I agree. Especially if we also think of future options of maybe caching JWT? We will have other Access Right functions with XACML. In that context checking for iss/nbf <= now() <= exp.

c0c0n3 commented 4 years ago

simply we can do (4) outside the library

Well, we actually have to do it now. In fact I've just unearthed a nasty bug in jwt-go whereby a token with a non-numeric exp field is considered valid! And again, this has to do with Go's weak type system. A token with a payload of e.g.

{ exp: "put null any non numeric value you like here" }

passes validation. Fortunately, we won't cache it since our code actually does the right thing and considers it expired but since we rely on jwt-go to tell us if the token is useable for the current call, we give the Mixer the green light.

So we have to replace jwt-go validation of (4) with our own. Here's a nasty scenario to convince you we should. Say the server that issues the token has a silly serialization bug whereby the exp field value becomes a string

{ exp: "12345678" }

Everytime the client passes it on to us we okay it so potentially the client could use it inadvertently for a long time past the server's intended expiry date. This makes it more likely to be hijacked and if an attacker gets hold of one of those tokens, they'll be able to use it forever!