decode fails with 'none' algorithm and verify

[danleyden]

When providing a token that has the algorithm 'none' to the decode method, a decode error is raised from line 87 with the error message 'Not enough or too many segments'.

This can be suppressed by disabling verification when decoding, but that has knock-on effects of other claims not being verified.

I believe the correct behaviour should be that, when the algorithm is 'none' and the token is otherwise valid, the decode method should return successfully.

It is worth noting that this behaviour provides (accidentally, I believe) protection against certain attacks. To provide support for this without exposing users to additional vulnerabilities, I would suggest also adding some flag (that defaults safely) that allows the developer to 'turn on' acceptance of the 'none' algorithm.

[excpt]

Does your none-token have a . (dot) as the last character?

[danleyden]

it does indeed.

Generated using JWT.encode(payload, 'some key that gets ignored', 'none', my_header)

I believe the RFC calls for the trailing '.' in the case of the 'none' algorithm (see section 6.1), so I believe the token is correct. This is to indicate that the signature is actually the empty string.

[excpt]

I just needed to know this. This is a bug. Thanks for reporting. :)

[excpt]

@danleyden If you find some time you can checkout the rspec-test-refactoring branch and test the updated code.

[danleyden]

Thanks @excpt - that looks like it fixes the bug.

However, that does open the door for third parties tampering with the token in the default use case..

Take the case where I send a valid, signed (not encrypted) token with e.g. RS256. A man in the middle could intercept that token, remove the signature and alter the alg claim to 'none' and send it on as a valid token. In this case, unless the implementer has set the algorithm when calling verify (a slightly buried option) the token will be accepted.

Given that, by fixing this, we open the door to that attack vector (albeit through the developer not using the library fully), I would be tempted not to merge that fix in until there is a better story around the none algorithm, such as forcing the developer to specifically enable its use.

[excpt]

Thanks for testing. I will remove that fix and the documentation is already updated.