fix jwt decoded with missing characters in body

[akanass]

Hello,

If someone delete characters in JWT body and send it in an auth route, exception will be thrown by JWT.decode(token) method.

Perform a try-catch around the call to solve the problem.

Unit tests are updated and coverage is 100%.

Can you merge ASAP and publish a new version on npm if you are agree with that.

Thanks.

[nelsonic]

@njl07 thanks for getting involved!
would you mind clarifying what pull request is this fixing?
can you please create an issue to explain what the problem is you are seeing?
thanks!

[nelsonic]

JWT2 uses the _asynchronous_ version of JWT.decode to avoid blocking the V8 event loop.
This try/catch uses the _synchronous_ (_blocking_) version of JWT.decode which will introduce a bottle-neck into the authorization process which could easily be exploited by a DOS attacker.

[akanass]

No issue was created for this.

I had this problem when I generate a JWT and have it in query parameters and delete some characters in body part.

I sent JWT in my server and exception was thrown when plugin tried to decode token.

[nelsonic]

is your code somewhere on GitHub so we can help debug it?

[akanass]

No _asynchronous_ method exists for JWT.decode

I've just move your code to surround it with try-catch

[akanass]

I've make a unit test to show the problem

[akanass]

If you take your code, generate a JWT and delete some characters in body part, exception will be thrown in JWT.decode function.

Are you agree with that?

[akanass]

I've done test with your code and exception is:

stack: SyntaxError: Uncaught error: Unexpected token 
Debug: internal, implementation, error 
    at Object.parse (native)
    SyntaxError: Uncaught error: Unexpected token 
    at Object.jwsDecode [as decode] (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/jsonwebtoken/node_modules/jws/lib/verify-stream.js:71:20)
    at Object.JWT.decode (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/jsonwebtoken/index.js:11:21)
    at Object.authenticate (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi-auth-jwt2/lib/index.js:44:19)
    at Object.parse (native)
    at Object.jwsDecode [as decode] (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/jsonwebtoken/node_modules/jws/lib/verify-stream.js:71:20)
    at Object.JWT.decode (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/jsonwebtoken/index.js:11:21)
    at Object.authenticate (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi-auth-jwt2/lib/index.js:44:19)
    at /Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/auth.js:227:30
    at internals.Protect.run (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/protect.js:56:5)
    at authenticate (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/auth.js:218:26)
    at internals.Auth._authenticate (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/auth.js:348:5)
    at internals.Auth.authenticate (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/auth.js:177:17)
    at /Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/request.js:370:13
    at /Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/auth.js:227:30
    at internals.Protect.run (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/protect.js:56:5)
    at authenticate (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/auth.js:218:26)
    at internals.Auth._authenticate (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/auth.js:348:5)
    at internals.Auth.authenticate (/Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/auth.js:177:17)
    at /Users/njl/Programmation/InTech/WEGA-PROD/wega-manager/node_modules/hapi/lib/request.js:370:13

[akanass]

Hello,

What's the status of this PR? Do you accept it or are you another issue?

I must push in production tomorrow and I would like to know if can trust this library to have no errors when manipulating token in URL.

Thanks.

[nelsonic]

@njl07 thanks for showing enthusiasm for getting the PR merged. We are in the middle of our day and have our own deadline. I promise to look at it this evening. you will be able to push your code to prod. 👍

[nelsonic]

Given that jsonwebtoken (this module's main dependency) is _not_ operating truly _asynchronously_, see:
https://github.com/auth0/node-jsonwebtoken/blob/6a715a13992c888db77cc5b274e5fd28633e4c76/index.js#L198
it seems reasonable to wrap the call to jwt.verify in a try/catch ...
@alanshaw / @eventhough / @diffsky / @dherault / @walling any objection to me merging this PR?

[nelsonic]

@njl07 from reading the jsonwebtoken source it appears that the only reason for throwing an error is the lack of a callback - https://github.com/auth0/node-jsonwebtoken/blob/6a715a13992c888db77cc5b274e5fd28633e4c76/index.js#L106
Otherwise it returns the error as expected.

Would you mind plasting the complete stack trace of the error you are seeing when the body of the JWT is tampered with?

Thanks!

[akanass]

Hello guys,

Exception thrown is not by jsonwebtoken itself but by jws, _the main module dependency_ to sign JWT, when it try to decode base64 payload. https://github.com/brianloveswords/node-jws/blob/master/lib/verify-stream.js#L37

That you show in https://github.com/auth0/node-jsonwebtoken/blob/6a715a13992c888db77cc5b274e5fd28633e4c76/index.js#L198 is not the JWT.decode function result but the JWT.verify result which is _asynchronously_ and in https://github.com/auth0/node-jsonwebtoken/blob/6a715a13992c888db77cc5b274e5fd28633e4c76/index.js#L106 it's the declaration of the custom callback if it's not define in JWT.verify parameter.

My PR is a fix of your own code which pass JWT.decode function in parameter of the keyFunc to retrieve publicKey or secret to verify the token.

But JWT.decode function throw an exception when a problem occurred in the decode process and jsonwebtoken know that because in JWT.verify function, call to JWT.decode is surround with try-catch https://github.com/auth0/node-jsonwebtoken/blob/6a715a13992c888db77cc5b274e5fd28633e4c76/index.js#L170.

To fix this issue, I think, the best way is to delete the decoded parameter in keyFunc signature https://github.com/dwyl/hapi-auth-jwt2/blob/master/lib/index.js#L44 and have only the callback.

You don't use decoded in function to retrieve publicKey or secret because you don't have element in decoded to know where these data are. If decoded contains element to know where is publicKey or secret, I think it's not secure so call to JWT.decode it's not necessary at this step.

After retrieve publicKey or secret, you call JWT.verify which decode the token and surround function with try-catch so no problem will occurred.

According that, my PR is not necessary but you must change your code to delete decoded in keyFunc and all problems will be resolved.

Are you agree with that?

Thanks.

[alanshaw]

Thanks for your time @njl07 - this is a good catch and something we should fix asap.

@nelsonic the issue is that the JWT.decode can throw, if the payload is malformed JSON. We can't verify, because we (potentially) don't yet have the key. So we need to try/catch around the call to decode.

I think it's reasonable to merge this PR.

In related news, we're decoding the JWT twice (once for the key func and again when we verify), even if options.key isn't a function. We can optimize and skip the double decode if the key isn't a function.

[akanass]

Thanks @alanshaw. Like explain in my last comment, I think it's better to delete decoded parameter in keyFunc because we don't need to have it yet.

It use only in JWT.verify and jsonwebtoken does the job fine for this exception.

My PR works to fix your code but it's better to optimize code and let verify to decode token

[walling]

@njl07, correct me if I'm wrong, but isn't keyFunc used to select a secret key based on the header and/or payload. Maybe you want to be able to select different secret keys for different jti's. I'm not depending on this use case; I just wanted to mention it.

I agree that the extra JWT.decode could be avoided if the keyFunc feature is not used.

[akanass]

@walling I'm agree with you with the fact that we can be able to select different secret keys for different jti's.

So decoded is required to get data and find good element to retrieve secret if it's not explicit and can show how to get this secret.

[alanshaw]

I vote for merge this now and optimize is separate PR

[nelsonic]

Agreed. Merge now to prevent server crash from throw and optimise to strip double-decoding #soon

[nelsonic]

@njl07 Version 5.0.3 is on NPM. 📦
Thanks for spotting and fixing this bug! 👍
Welcome to dwyl ✅ ❤️