Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jjwt accepts signature that is also base64 encoded #192

Closed
kleewho opened this issue Nov 22, 2016 · 5 comments
Closed

Jjwt accepts signature that is also base64 encoded #192

kleewho opened this issue Nov 22, 2016 · 5 comments
Milestone

Comments

@kleewho
Copy link

kleewho commented Nov 22, 2016

Somehow by accident jjwt will accept base64 encoded signatures although according to RFC it should be base64url encoded. The problematic part is that you decode base64 so first you convert signature from base64url to base64. But this convertion doesn't check for presence of + or / before conversion (which would meant that we have wrong string). Here is the function Base64UrlCoded#decode:

@Override
    public byte[] decode(String encoded) {
        char[] chars = encoded.toCharArray(); //always ASCII - one char == 1 byte

        //Base64 requires padding to be in place before decoding, so add it if necessary:
        chars = ensurePadding(chars);

        //Replace url-friendly chars back to normal Base64 chars:
        for (int i = 0; i < chars.length; i++) {
            if (chars[i] == '-') {
                chars[i] = '+';
            } else if (chars[i] == '_') {
                chars[i] = '/';
            }
        }

        String base64Text = new String(chars);

        return TextCodec.BASE64.decode(base64Text);
    }

and here simplest solution:

@Override
    public byte[] decode(String encoded) {
        char[] chars = encoded.toCharArray(); //always ASCII - one char == 1 byte

        //Base64 requires padding to be in place before decoding, so add it if necessary:
        chars = ensurePadding(chars);

        //Replace url-friendly chars back to normal Base64 chars:
        for (int i = 0; i < chars.length; i++) {
            if (chars[i] == + || chars[i] == '/') {
                throw new SignatureException("Wrong signature encoding. Use base64url encoding");
            }
            if (chars[i] == '-') {
                chars[i] = '+';
            } else if (chars[i] == '_') {
                chars[i] = '/';
            }
        }

        String base64Text = new String(chars);

        return TextCodec.BASE64.decode(base64Text);
    }

Or maybe instead of javax.xml.bind.DatatypeConverter use something that has support for Base64Url https://google.github.io/guava/releases/19.0/api/docs/com/google/common/io/BaseEncoding.html?

@kleewho
Copy link
Author

kleewho commented Nov 30, 2016

I would gladly fix that but I need to know what is your preference

@lhazlewood
Copy link
Contributor

I don't see this as an issue? I mean, this could be seen as a 'feature, not a bug'.

In other words, JJWT will correctly base64url decode signatures as expected. It also might base64 decode signatures too. What is the problem?

@kleewho
Copy link
Author

kleewho commented Jan 11, 2017

The problem is that it will work for subset of all possible inputs which makes this feature, not a bug coders bad time, when he needs to figure out why only some of the inputs fail.

@lhazlewood lhazlewood modified the milestones: 1.0, 0.10.0 Jul 5, 2018
@lhazlewood
Copy link
Contributor

lhazlewood commented Jul 11, 2018

Only Base64Url is allowed now per #333 which has been merged to master and will be released in 0.10.0

@lhazlewood
Copy link
Contributor

Released in 0.10.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants