yourkarma / JWT

A JSON Web Token implementation in Objective-C.
MIT License
351 stars 107 forks source link

Incorrectly reported signature verification failure #181

Open ChrisMash opened 6 years ago

ChrisMash commented 6 years ago

New Issue Checklist

Issue Info

Info Value
Platform Name osx
Platform Version 10.12.6 Sierra
Library Version Latest
Integration Method example app
Xcode Version Xcode 9.2
Repro rate all the time (100%)
Demo project link N/A

Issue Description and Steps

Using the Swift desktop example app I enter the following details (from http://jwtbuilder.jamiekurtz.com/):

algorithm: HS256

secret: qwertyuiopasdfghjklzxcvbnm123456

token: eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJPbmxpbmUgSldUIEJ1aWxkZXIiLCJpYXQiOjE1MTkzODAxMzksImV4cCI6MTU1MDkxNjEzOSwiYXVkIjoid3d3LmV4YW1wbGUuY29tIiwic3ViIjoianJvY2tldEBleGFtcGxlLmNvbSIsIkdpdmVuTmFtZSI6IkpvaG5ueSIsIlN1cm5hbWUiOiJSb2NrZXQiLCJFbWFpbCI6Impyb2NrZXRAZXhhbXBsZS5jb20iLCJSb2xlIjpbIk1hbmFnZXIiLCJQcm9qZWN0IEFkbWluaXN0cmF0b3IiXX0.eWNPV4xRgkzE-ysEnP5We6gGj1FLXUxteSGhHO3axR8

The expectation is that the signature is determined to be valid (as jwt.io reports) but it is reported as invalid. Nothing useful on the logs, but included here anyhow:

JWT ENCODED TOKEN Optional("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJPbmxpbmUgSldUIEJ1aWxkZXIiLCJpYXQiOjE1MTkzODAxMzksImV4cCI6MTU1MDkxNjEzOSwiYXVkIjoid3d3LmV4YW1wbGUuY29tIiwic3ViIjoianJvY2tldEBleGFtcGxlLmNvbSIsIkdpdmVuTmFtZSI6IkpvaG5ueSIsIlN1cm5hbWUiOiJSb2NrZXQiLCJFbWFpbCI6Impyb2NrZXRAZXhhbXBsZS5jb20iLCJSb2xlIjpbIk1hbmFnZXIiLCJQcm9qZWN0IEFkbWluaXN0cmF0b3IiXX0.eWNPV4xRgkzE-ysEnP5We6gGj1FLXUxteSGhHO3axR8") JWT Algorithm NAME HS256 JWT ERROR: Optional("<JWTCodingResultTypeError: 0x608000008410>") -> Optional("Invalid signature! It seems that signed part of jwt mismatch generated part by algorithm provided in header.") JWT RESULT: nil -> nil JWT ENCODED TOKEN Optional("eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJPbmxpbmUgSldUIEJ1aWxkZXIiLCJpYXQiOjE1MTkzODAxMzksImV4cCI6MTU1MDkxNjEzOSwiYXVkIjoid3d3LmV4YW1wbGUuY29tIiwic3ViIjoianJvY2tldEBleGFtcGxlLmNvbSIsIkdpdmVuTmFtZSI6IkpvaG5ueSIsIlN1cm5hbWUiOiJSb2NrZXQiLCJFbWFpbCI6Impyb2NrZXRAZXhhbXBsZS5jb20iLCJSb2xlIjpbIk1hbmFnZXIiLCJQcm9qZWN0IEFkbWluaXN0cmF0b3IiXX0.eWNPV4xRgkzE-ysEnP5We6gGj1FLXUxteSGhHO3axR8") JWT Algorithm NAME HS256 JWT ERROR: nil -> nil JWT RESULT: Optional("<JWTCodingResultTypeSuccess: 0x60800005a4c0>") -> Optional("[AnyHashable(\"payload\"): {\n Email = \"jrocket@example.com\";\n GivenName = Johnny;\n Role = (\n Manager,\n \"Project Administrator\"\n );\n Surname = Rocket;\n}, AnyHashable(\"header\"): {\n alg = HS256;\n typ = JWT;\n}]")

lolgear commented 6 years ago

@ChrisMash

Hi! Thanks for reporting this issue! I tested different variations of token and secret.

It seems that long secret doesn't work properly now. For example, base "secret" secret works nice.

ChrisMash commented 6 years ago

Thanks for the quick response @lolgear, can you explain a bit more? I don't understand what you mean by 'base "secret" secret works nice'

ChrisMash commented 6 years ago

Actually, I think I see what you're saying. If I reduce the length of the default secret used on http://jwtbuilder.jamiekurtz.com/ then it works. Equally if I make it longer it also works... Adding a 7 to the end is ok, and removing the 6 is ok.

It seems a bit inconsistent, which is a bit worrying if I want to use this code to validate all manner of different tokens as there could be some combinations that end up failing incorrectly :(

lolgear commented 6 years ago

@ChrisMash could you test also different tokens of the same length? ( equal, less by one letter, greater by one letter )

Nice issue although :)

ChrisMash commented 6 years ago

So adding to the length and removing from the length makes it pass verification. But changing the last character doesn't. So it kinda looks like a key length of 32 (for HS256 at least) causes an issue.

Interestingly a key length of 64 also fails, while a key length of 63 does not.

lolgear commented 6 years ago

@ChrisMash I detected a bug.


// JWTBase64Coder
+ (NSData *)dataWithString:(NSString *)string {
    // check if base64.
    if (string == nil) {
        return nil;
    }

    // check that string is base64 encoded
    NSData *data = [[NSData alloc] initWithBase64EncodedString:string options:0];

    NSString *stringToPass = data != nil ? string : [[string dataUsingEncoding:NSUTF8StringEncoding] base64EncodedStringWithOptions:0];

    NSData *result = [self dataWithBase64UrlEncodedString:stringToPass];
    return result;
}

This check doesn't work properly for this case. I suppose it could be removed. This check was added as convenient way to extract data from both strings formats - base64 and plain.

ChrisMash commented 6 years ago

@lolgear yep, I see that commenting out data != nil ? string : makes it function correctly. I imagine that would equally break different uses of the function so not the right fix for general use!

lolgear commented 6 years ago

@ChrisMash I suppose that it was changed only for convenient conversion in JWTDesktop. :)

ChrisMash commented 6 years ago

Ahh yes, in my own use case I'll know if it's base64 encoded or not so can apply the correct logic

lolgear commented 6 years ago

@ChrisMash I am happy to merge your PR if you wish :)

lolgear commented 6 years ago

@ChrisMash Could you check this fix?

ChrisMash commented 6 years ago

@lolgear looks great, many thanks!