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.

Sign up for GitHub

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

Jump to bottom

Issue #4128 - Add missing padding and use URL decoder #4129

Merged
merged 1 commit into from Oct 7, 2019
Merged

Issue #4128 - Add missing padding and use URL decoder #4129

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 30 additions & 3 deletions jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdCredentials.java
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@
import java.net.HttpURLConnection;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Base64;
import java.util.Map;

Expand Down Expand Up @@ -158,9 +159,9 @@ protected Map<String, Object> decodeJWT(String jwt) throws IOException
if (sections.length != 3)
throw new IllegalArgumentException("JWT does not contain 3 sections");

Base64.Decoder decoder = Base64.getDecoder();
String jwtHeaderString = new String(decoder.decode(sections[0]), StandardCharsets.UTF_8);
String jwtClaimString = new String(decoder.decode(sections[1]), StandardCharsets.UTF_8);
Base64.Decoder decoder = Base64.getUrlDecoder();
String jwtHeaderString = new String(decoder.decode(padJWTSection(sections[0])), StandardCharsets.UTF_8);
String jwtClaimString = new String(decoder.decode(padJWTSection(sections[1])), StandardCharsets.UTF_8);
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
String jwtSignature = sections[2];

Map<String, Object> jwtHeader = (Map)JSON.parse(jwtHeaderString);
Expand All @@ -175,6 +176,32 @@ and the Token Endpoint (which it is in this flow), the TLS server validation
return (Map)JSON.parse(jwtClaimString);
}

private static byte[] padJWTSection(String unpaddedEncodedJwtSection)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the case here with === is necessary.

This could be simplified using the example from
https://tools.ietf.org/html/rfc7515#appendix-C

Something like:

static byte[] b64DecodeWithoutPadding(String arg)
{
    String s = arg;
    switch (s.length() % 4) // Pad with trailing '='s
    {
        case 0: break; // No pad chars in this case
        case 2: s += "=="; break; // Two pad chars
        case 3: s += "="; break; // One pad char
        default: throw new IllegalArgumentException("Illegal base64url string!");
    }
    return Base64.getUrlDecoder().decode(s); // Standard base64 decoder
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, that is simpler. Will look into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lachlan-roberts , @joakime et a. maybe you can help with this, but it seems the above code is licensed under BSD. See https://trustee.ietf.org/documents/IETF-TLP-5_001.html. So, is it OK to bring this in? Does something need to be done, like an attribution or something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travisspencer how about we write this in a different way with the same resulting logic.

remainder    action       (4-remainder)%4
-----------------------------------------
0            0 padding        0
1            error            -
2            2 padding        2
3            1 padding        1

private static byte[] padBase64(byte[] unpaddedBase64)
{
    int remainder = unpaddedBase64.length % 4;
    if (remainder == 1)
        throw new IllegalArgumentException("Not valid Base64");

    int paddingNeeded = (4 - remainder) % 4;
    byte[] paddedBase64 = Arrays.copyOf(unpaddedBase64, unpaddedBase64.length + paddingNeeded);
    Arrays.fill(paddedBase64, unpaddedBase64.length, paddedBase64.length, (byte)'=');

    return paddedBase64;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good one @lachlan-roberts . That special case for a remainder of 1 took me a sec, but it's because a base64-endoded string will always be an even number. I see.

I think that an if statement like I have below is good to avoid the unnecessary array copy and I'd like to assign unpaddedBase64.length to a variable for readability. Will make those changes and update this PR.

{
int length = unpaddedEncodedJwtSection.length();
int remainder = length % 4;

if (remainder == 1)
// A valid base64-encoded string will never be have an odd number of characters.
throw new IllegalArgumentException("Not valid Base64-encoded string");

byte[] paddedEncodedJwtSection;

if (remainder > 0)
{
int paddingNeeded = (4 - remainder) % 4;

paddedEncodedJwtSection = Arrays.copyOf(unpaddedEncodedJwtSection.getBytes(), length + paddingNeeded);
Arrays.fill(paddedEncodedJwtSection, length, paddedEncodedJwtSection.length, (byte)'=');
}
else
{
paddedEncodedJwtSection = unpaddedEncodedJwtSection.getBytes();
}

return paddedEncodedJwtSection;
}

private Map<String, Object> claimAuthCode(String authCode) throws IOException
{
if (LOG.isDebugEnabled())
Expand Down