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

[Bug] JWE is still valid after appending data to the Authenticaton Tag #2201

Closed
3 of 14 tasks
vladpirlog opened this issue Aug 2, 2023 · 7 comments · Fixed by #2569
Closed
3 of 14 tasks

[Bug] JWE is still valid after appending data to the Authenticaton Tag #2201

vladpirlog opened this issue Aug 2, 2023 · 7 comments · Fixed by #2569
Assignees
Labels
Bug Product is not functioning as expected Duplicate A similar issue already exists P1 More important, prioritize highly
Milestone

Comments

@vladpirlog
Copy link

Which version of Microsoft.IdentityModel are you using?
System.IdentityModel.Tokens.Jwt 7.1.0-preview

Where is the issue?

  • M.IM.JsonWebTokens
  • M.IM.KeyVaultExtensions
  • M.IM.Logging
  • M.IM.ManagedKeyVaultSecurityKey
  • M.IM.Protocols
  • M.IM.Protocols.OpenIdConnect
  • M.IM.Protocols.SignedHttpRequest
  • M.IM.Protocols.WsFederation
  • M.IM.TestExtensions
  • M.IM.Tokens
  • M.IM.Tokens.Saml
  • M.IM.Validators
  • M.IM.Xml
  • S.IM.Tokens.Jwt
  • Other (please describe)

Is this a new or an existing app?
This is a new app or an experiment.

Repro

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

var jwtHandler = new JsonWebTokenHandler();

var claims = new Dictionary<string, object>
{
    { "claim1", "value1" },
    { "claim2", "value2" },
};

var signingKey = RSA.Create();
var validationKey = RSA.Create(signingKey.ExportParameters(false));

var decryptionKey = RSA.Create();
var encryptionKey = RSA.Create(decryptionKey.ExportParameters(false));

var token = jwtHandler.CreateToken(new SecurityTokenDescriptor
{
    Claims = claims,
    SigningCredentials = new SigningCredentials(
        new RsaSecurityKey(signingKey),
        SecurityAlgorithms.RsaSha256),
    EncryptingCredentials = new EncryptingCredentials(
        new RsaSecurityKey(encryptionKey),
        SecurityAlgorithms.RsaOAEP,
        SecurityAlgorithms.Aes256CbcHmacSha512)
});

// altering the JWE by appending some stuff to the Authentication Tag
var invalidToken = token + "the_token_has_been_tampered_with";

var validationResult = jwtHandler.ValidateToken(invalidToken, new()
{
    IssuerSigningKey = new RsaSecurityKey(validationKey),
    TokenDecryptionKey = new RsaSecurityKey(decryptionKey),
    ValidateAudience = false,
    ValidateIssuer = false,
    ValidateLifetime = false
});

if (validationResult.IsValid) // token is valid when it shouldn't be
    throw new Exception("The invalid token has been incorrectly marked as valid!");

Expected behavior
The validation of the JWE should fail, as the Authentication Tag has been altered, thus no longer valid.

Extracted from RFC 7516, Section 5.2, regarding JWE decryption:

2.   Base64url decode the encoded representations of the JWE
       Protected Header, the JWE Encrypted Key, the JWE Initialization
       Vector, the JWE Ciphertext, the JWE Authentication Tag, and the
       JWE AAD, following the restriction that no line breaks,
       whitespace, or other additional characters have been used.

Actual behavior
The JWE is successfully validated when the Authentication Tag has been altered.

Possible solution
Check the actual length of the Authentication Tag and fail the validation if it does not match the expected value, according to the JWE encryption algorithm used.

@brentschmaltz brentschmaltz added Bug Product is not functioning as expected and removed needs attention untriaged labels Feb 21, 2024
@brentschmaltz
Copy link
Member

This is not a security issue, but should be fixed as it keeps coming up.

@jmprieur
Copy link
Contributor

jmprieur commented Apr 21, 2024

@kellyyangsong, I think it's a duplicate of #1641 (also in scope for .NET9)

@jmprieur jmprieur marked this as a duplicate of #1641 Apr 21, 2024
@jmprieur jmprieur added the Duplicate A similar issue already exists label Apr 21, 2024
@kellyyangsong
Copy link
Contributor

kellyyangsong commented Apr 23, 2024

Currently

var payload = new JObject()
{
    { JwtRegisteredClaimNames.Email, "[email protected]" },
    { JwtRegisteredClaimNames.GivenName, "Bob" },
    { JwtRegisteredClaimNames.Iss, "http://Default.Issuer.com"},
    { JwtRegisteredClaimNames.Aud, "http://Default.Audience.com" },
    { JwtRegisteredClaimNames.Iat, EpochTime.GetIntDate(DateTime.Parse("2017-03-17T18:33:37.095Z")).ToString() },
    { JwtRegisteredClaimNames.Nbf, EpochTime.GetIntDate(DateTime.Parse("2017-03-17T18:33:37.080Z")).ToString() },
    { JwtRegisteredClaimNames.Exp, EpochTime.GetIntDate(DateTime.Parse("2025-03-17T18:33:37.080Z")).ToString() },
}.ToString();
 
var jsonWebTokenHandler = new JsonWebTokenHandler();
var signingCredentials = Default.SymmetricSigningCredentials;
var encryptingCredentials = new EncryptingCredentials(KeyingMaterial.RsaSecurityKey_2048, SecurityAlgorithms.RsaPKCS1, SecurityAlgorithms.Aes128CbcHmacSha256);
var jwe = jsonWebTokenHandler.CreateToken(payload, signingCredentials, encryptingCredentials);
 
// altering the JWE by appending some stuff to the Authentication Tag
var invalidToken = jwe + "the_token_has_been_tampered_with";
 
var tokenValidationParameters = new TokenValidationParameters
{
    TokenDecryptionKey = KeyingMaterial.RsaSecurityKey_2048,
    IssuerSigningKey = Default.SymmetricSigningKey256,
    ValidAudience = "http://Default.Audience.com",
    ValidIssuer = "http://Default.Issuer.com",
};
var tokenValidationResult = await jsonWebTokenHandler.ValidateTokenAsync(invalidToken, tokenValidationParameters).ConfigureAwait(false);
 
// tokenValidationResult.IsValid == true

Desired Behavior

// tokenValidationResult.IsValid == false

How

  • We will evaluate that authenticationTag is the correct length based on the given Algorithm.
  • This will be wrapped in an app context switch.
  • auth tag length validation will be on by default.
Supported Algorithms Auth Tag size BaseUrl64 encoded length
Aes128Gcm 128 bits 24
Aes192Gcm 128 bits 24
Aes256Gcm 128 bits 24
Aes128CbcHmacSha256 128 bits 24
Aes192CbcHmacSha384 192 bits 32
Aes256CbcHmacSha512 256 bits 44

@kellyyangsong
Copy link
Contributor

@kevinchalet fyi on the comment above, which is a duplicate of #1641

@kevinchalet
Copy link
Contributor

Thanks for fixing that 👍🏻

That said, I'm not so sure it's the best approach:

  • You're adding yet another mapping (one already exists in SymmetricSignatureProvider for the same exact purpose).
  • The validation logic is completely skipped if the algorithm is not recognized, which seems fragile.
  • Your mapping dictionary includes AES-GCM algorithms but it's unnecessary as they are never handled in DecryptWithAesCbc, that only deals with AES-CBC (hence the name I guess 😄).

I wonder if simply replacing _authenticatedkeys.Value.HmacKey.Key.Length by authenticationTag.Length here isn't a better fix:

if (!_symmetricSignatureProvider.Value.Verify(macBytes, 0, macBytes.Length, authenticationTag, 0, _authenticatedkeys.Value.HmacKey.Key.Length, Algorithm))

With that in place, I'd expect SymmetricSignatureProvider.Verify() to end up properly validating the "signature" length (or HMAC to be 100% correct):

// Check that signature length matches algorithm.
// If we don't have an entry for the algorithm in our dictionary, that is probably a bug.
// This is why a new message was created, rather than using IDX10640.
if (!ExpectedSignatureSizeInBytes.TryGetValue(algorithmToValidate, out int expectedSignatureLength))
throw LogHelper.LogExceptionMessage(new ArgumentException(
LogHelper.FormatInvariant(
LogMessages.IDX10718,
LogHelper.MarkAsNonPII(algorithmToValidate),
LogHelper.MarkAsNonPII(Algorithm))));
if (expectedSignatureLength != signatureLength)
throw LogHelper.LogExceptionMessage(new ArgumentException(
LogHelper.FormatInvariant(
LogMessages.IDX10719,
LogHelper.MarkAsNonPII(signatureLength),
LogHelper.MarkAsNonPII(expectedSignatureLength))));

@jennyf19 jennyf19 added this to the 7.5.2 milestone May 1, 2024
@kellyyangsong
Copy link
Contributor

kellyyangsong commented May 1, 2024

@kevinchalet thanks for sharing your thoughts! We took some of your points into consideration - such as utilizing the existing mapping in SymmetricSignatureProvider. Another point you raised, replacing _authenticatedkeys.Value.HmacKey.Key.Length with authenticationTag.Length, is something that we discussed internally too, but existing partners asked for an app context switch. The switch will be removed upon the next major release, during which we can make the replacement you suggested, too. Really appreciate your engagement here, Kevin!

@kevinchalet
Copy link
Contributor

Really appreciate your engagement here, Kevin!

My pleasure! Thanks for tagging me and for fixing this issue 👍🏻

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 Duplicate A similar issue already exists P1 More important, prioritize highly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants