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

Extra characters at the end of encrypted JWT tokens don't prevent them from being considered valid #1641

Closed
kevinchalet opened this issue May 4, 2021 · 5 comments · Fixed by #2569
Assignees
Labels
Bug Product is not functioning as expected Customer reported Indicates issue was opened by customer IdentityModel8x Future breaking issues/features for IdentityModel 8x Investigate We are not quite sure what the issue is. P1 More important, prioritize highly
Milestone

Comments

@kevinchalet
Copy link
Contributor

Hey,

Yesterday, the following report was posted on the OpenIddict repository: openiddict/openiddict-core#1254.

I was able to reproduce it on my machine and confirmed this was caused by IdentityModel.
It looks like IdentityModel doesn't reject a JWE payload if it contains extra characters after the authentication tag.

Here's a repro:

using System.Diagnostics;
using System.Security.Claims;
using System.Security.Cryptography;
using Microsoft.IdentityModel.JsonWebTokens;
using Microsoft.IdentityModel.Tokens;

namespace ConsoleApp13
{
    class Program
    {
        static void Main(string[] args)
        {
            var identity = new ClaimsIdentity(TokenValidationParameters.DefaultAuthenticationType);
            var signingKey = new RsaSecurityKey(RSA.Create(2048))
            {
                KeyId = "id1"
            };

            var encryptionKey = new RsaSecurityKey(RSA.Create(2048))
            {
                KeyId = "id2"
            };

            var handler = new JsonWebTokenHandler();
            var token = handler.CreateToken(new SecurityTokenDescriptor
            {
                Audience = "audience",
                Issuer = "issuer",
                EncryptingCredentials = new EncryptingCredentials(encryptionKey, SecurityAlgorithms.RsaOAEP, SecurityAlgorithms.Aes256CbcHmacSha512),
                SigningCredentials = new SigningCredentials(signingKey, SecurityAlgorithms.RsaSha256Signature),
                Subject = identity
            });

            var parameters = new TokenValidationParameters
            {
                ValidAudience = "audience",
                ValidIssuer = "issuer",
                TokenDecryptionKey = encryptionKey,
                IssuerSigningKey = signingKey
            };

            var result = handler.ValidateToken(token + "A", parameters);
            Debug.Assert(!result.IsValid, "The token shouldn't be considered valid.");
        }
    }
}

@brentschmaltz @jennyf19 could you please take a look?

@mafurman mafurman added Bug Product is not functioning as expected Customer reported Indicates issue was opened by customer Investigate We are not quite sure what the issue is. labels May 4, 2021
@brentschmaltz
Copy link
Member

brentschmaltz commented May 5, 2021 via email

@kevinchalet
Copy link
Contributor Author

@brentschmaltz awesome, thanks!

Sometimes there are insignificant bits.

That was my feeling too, but in this particular case, the "extra last bits" are considered by IdentityModel as being part of the authentication tag (which is expected since it's technically part of it):

image

I suspect IdentityModel correctly base64decodes it but only uses a fixed-size array copy that ignores the extra bytes before calling the crypto primitives. Feeding the crypto primitives with the original tag or adding a check in IdentityModel to ensure the authentication tag length matches the expected length would certainly help fix that issue.

@kevinchalet
Copy link
Contributor Author

Someone else emailed me to tell me they were concerned about this behavior. Do you have any news?

@brentschmaltz
Copy link
Member

@kevinchalet nothing yet, should get some time soon.

@jennyf19 jennyf19 added the IdentityModel8x Future breaking issues/features for IdentityModel 8x label Feb 19, 2024
@jennyf19 jennyf19 added the P2 High, but not urgent. Needs to be addressed within the next couple of sprints label Mar 27, 2024
@jmprieur
Copy link
Contributor

Duplicate of #2201

@jmprieur jmprieur marked this as a duplicate of #2201 Apr 21, 2024
@jmprieur jmprieur added P1 More important, prioritize highly and removed P2 High, but not urgent. Needs to be addressed within the next couple of sprints labels Apr 21, 2024
@kellyyangsong kellyyangsong linked a pull request Apr 24, 2024 that will close this issue
@jennyf19 jennyf19 added this to the 7.5.2 milestone May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Product is not functioning as expected Customer reported Indicates issue was opened by customer IdentityModel8x Future breaking issues/features for IdentityModel 8x Investigate We are not quite sure what the issue is. P1 More important, prioritize highly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants