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

HMACSHA256 doesn't pad key when the secret is under 64 bytes #80180

Closed
Vannevelj opened this issue Jan 4, 2023 · 7 comments
Closed

HMACSHA256 doesn't pad key when the secret is under 64 bytes #80180

Vannevelj opened this issue Jan 4, 2023 · 7 comments
Assignees
Labels
area-System.Security question Answer questions and provide assistance, not an issue with source code or documentation.
Milestone

Comments

@Vannevelj
Copy link

Description

The HMACSHA256(Byte[]) constructor says

The secret key for HMACSHA256 encryption. The key can be any length. However, the recommended size is 64 bytes. If the key is more than 64 bytes long, it is hashed (using SHA-256) to derive a 64-byte key. If it is less than 64 bytes long, it is padded to 64 bytes.

However I'm not seeing that play out in practice and the key remains as it is. If I then try to use it it is unable to validate the JWT. Padding it myself fixes the issue and the token can be validated.

Reproduction Steps

I've created the following repro:

https://jwt.io/#debugger-io?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6InRlc3RAZXhhbXBsZS5jb20ifQ.LRuMOc2ie7o3belYB9TMC44fEdvH2laZ_sIxZKRM8yg

using System.Security.Cryptography;
using System.IdentityModel.Tokens.Jwt;
using Microsoft.IdentityModel.Tokens;
using System.Text;

var token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6InRlc3RAZXhhbXBsZS5jb20ifQ.LRuMOc2ie7o3belYB9TMC44fEdvH2laZ_sIxZKRM8yg";
var secret = "secret_value";
var tokenHandler = new JwtSecurityTokenHandler();
var hmac = new HMACSHA256(Encoding.UTF8.GetBytes(secret));

var hmacKey = hmac.Key;
Console.WriteLine(hmacKey.Length);
// Array.Resize(ref hmacKey, 64); <-- uncomment for fix
// Console.WriteLine(hmacKey.Length);
var validationParameters = new TokenValidationParameters
{
    ValidateLifetime = false,
    ValidateAudience = false,
    ValidateIssuer = false,
    ValidateIssuerSigningKey = true,
    IssuerSigningKey = new SymmetricSecurityKey(hmacKey)
};

var result = await tokenHandler.ValidateTokenAsync(token, validationParameters);
Console.WriteLine(result.IsValid);
<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net7.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="System.IdentityModel.Tokens.Jwt" Version="6.25.0" />
  </ItemGroup>
</Project>

Expected behavior

Output should show True since the secret is valid

Actual behavior

Output shows False

Regression?

Tried on .NET 6 and .NET 7 and fails on both.

Known Workarounds

Manually resizing the key to a 64 byte array appears to work:

Array.Resize(ref hmacKey, 64); 

Configuration

No response

Other information

I had a peek around the relevant code to see if I might be missing something. Looking at the ChangeKeyImpl code it seems like we only modify the key if it is longer than the secret and not when it is shorter.

// If _blockSize is -1 the key isn't going to be extractable by the object holder,
// so there's no point in recalculating it in managed code.
if (key.Length > _blockSize && _blockSize > 0)
{
// Perform RFC 2104, section 2 key adjustment.
modifiedKey = _hashAlgorithmId switch
{
HashAlgorithmNames.SHA256 => SHA256.HashData(key),

That line of code hasn't changed in 7 years though so perhaps I'm barking up the wrong tree.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 4, 2023
@ghost
Copy link

ghost commented Jan 4, 2023

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

The HMACSHA256(Byte[]) constructor says

The secret key for HMACSHA256 encryption. The key can be any length. However, the recommended size is 64 bytes. If the key is more than 64 bytes long, it is hashed (using SHA-256) to derive a 64-byte key. If it is less than 64 bytes long, it is padded to 64 bytes.

However I'm not seeing that play out in practice and the key remains as it is. If I then try to use it it is unable to validate the JWT. Padding it myself fixes the issue and the token can be validated.

Reproduction Steps

I've created the following repro:

https://jwt.io/#debugger-io?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6InRlc3RAZXhhbXBsZS5jb20ifQ.LRuMOc2ie7o3belYB9TMC44fEdvH2laZ_sIxZKRM8yg

using System.Security.Cryptography;
using System.IdentityModel.Tokens.Jwt;
using Microsoft.IdentityModel.Tokens;
using System.Text;

var token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6InRlc3RAZXhhbXBsZS5jb20ifQ.LRuMOc2ie7o3belYB9TMC44fEdvH2laZ_sIxZKRM8yg";
var secret = "secret_value";
var tokenHandler = new JwtSecurityTokenHandler();
var hmac = new HMACSHA256(Encoding.UTF8.GetBytes(secret));

var hmacKey = hmac.Key;
Console.WriteLine(hmacKey.Length);
// Array.Resize(ref hmacKey, 64); <-- uncomment for fix
// Console.WriteLine(hmacKey.Length);
var validationParameters = new TokenValidationParameters
{
    ValidateLifetime = false,
    ValidateAudience = false,
    ValidateIssuer = false,
    ValidateIssuerSigningKey = true,
    IssuerSigningKey = new SymmetricSecurityKey(hmacKey)
};

var result = await tokenHandler.ValidateTokenAsync(token, validationParameters);
Console.WriteLine(result.IsValid);
<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net7.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="System.IdentityModel.Tokens.Jwt" Version="6.25.0" />
  </ItemGroup>
</Project>

Expected behavior

Output should show True since the secret is valid

Actual behavior

Output shows False

Regression?

Tried on .NET 6 and .NET 7 and fails on both.

Known Workarounds

Manually resizing the key to a 64 byte array appears to work:

Array.Resize(ref hmacKey, 64); 

Configuration

No response

Other information

I had a peek around the relevant code to see if I might be missing something. Looking at the ChangeKeyImpl code it seems like we only modify the key if it is longer than the secret and not when it is shorter.

// If _blockSize is -1 the key isn't going to be extractable by the object holder,
// so there's no point in recalculating it in managed code.
if (key.Length > _blockSize && _blockSize > 0)
{
// Perform RFC 2104, section 2 key adjustment.
modifiedKey = _hashAlgorithmId switch
{
HashAlgorithmNames.SHA256 => SHA256.HashData(key),

That line of code hasn't changed in 7 years though so perhaps I'm barking up the wrong tree.

Author: Vannevelj
Assignees: -
Labels:

area-System.Security

Milestone: -

@vcsjones
Copy link
Member

vcsjones commented Jan 4, 2023

I can take a look to dig in to the details here.

@vcsjones vcsjones self-assigned this Jan 4, 2023
@vcsjones
Copy link
Member

vcsjones commented Jan 4, 2023

So, as a first observation, even .NET Framework does not zero-pad the key. This prints out "00".

using System;
using System.Security.Cryptography;

byte[] key = new byte[1];
HMACSHA256 hmac = new HMACSHA256(key);
Console.WriteLine(BitConverter.ToString(hmac.Key));

The "zero pad the key" is an internal implementation of how HMAC works, which is what I think the documentation is trying to convey. We don't literally zero-pad the key.

What I was more curious about was why you have to zero pad your key with the JWT library. After all, with the way HMAC works, this produces the same result:

using System;
using System.Security.Cryptography;

using HMACSHA256 hmac = new HMACSHA256();
hmac.Key = new byte[] { 1 };
Console.WriteLine(Convert.ToHexString(hmac.ComputeHash("yabba dabba doo"u8.ToArray())));

hmac.Key = new byte[] { 1, 0, 0, 0, 0, 0 };
Console.WriteLine(Convert.ToHexString(hmac.ComputeHash("yabba dabba doo"u8.ToArray())));

But it's not that it thinks the key is invalid. When you are supplying the short token, it is actually throwing an exception. We need to throw a IdentityModelEventSource.ShowPII = true; in there to see the details.

But now with:

IdentityModelEventSource.ShowPII = true;
var result = await tokenHandler.ValidateTokenAsync(token, validationParameters);
Console.WriteLine(result.Exception.Message);

we get:

Exceptions caught:
'System.ArgumentOutOfRangeException: IDX10653: The encryption algorithm 'HS256' requires a key size of at least '128' bits. Key 'Microsoft.IdentityModel.Tokens.SymmetricSecurityKey, KeyId: '', InternalId: 'hDGBbPVvCjs5XXtj1gl7i56LIXkzY_0NRabLZLEFDrE'.', is of size: '96'. (Parameter 'key')

Ah. So the library is trying to stop you from using what it thinks is a too-weak HS256 key. It thinks your key is 96 bits, which is right, as your key secret_value is 12 octets.

The zero padding is just circumventing the "your key is too small" check.


So in summary, there is no behavior regression. The documentation is a little confusing around this, but the behavior observed goes far back in to .NET Framework.

The behavioral difference observed in the JWT library is because the library is trying to be helpful by telling you your key is too short. Manually padding with zeros acts as an escape hatch if you really want to use a short HS256 key.

@vcsjones
Copy link
Member

vcsjones commented Jan 4, 2023

Just to prove the point about the JWT library, if you change your Array.Resize to Array.Resize(ref hmacKey, 16);, then it will also validate the JWT. That gets it above the 128-bit security check, but is still below the 512-bit (64 byte) block size boundary of HMACSHA256.

@vcsjones vcsjones added this to the Future milestone Jan 4, 2023
@vcsjones vcsjones added question Answer questions and provide assistance, not an issue with source code or documentation. and removed untriaged New issue has not been triaged by the area owner labels Jan 4, 2023
@Vannevelj
Copy link
Author

Interesting, that makes sense. Thanks for the swift response and adjusting the messaging, that should help a lot! Would it be useful to document this apparent minimum-length requirement from the security check?

I'm integrating an existing system so I'll stick with my padding workaround for now but I'll aim to move to a stronger secret in the future.

@vcsjones
Copy link
Member

vcsjones commented Jan 4, 2023

Would it be useful to document this apparent minimum-length requirement from the security check?

I don't think it would hurt to raise an issue. Would you be interested in writing one up in the Azure SDK repository? I think the right place for documentation issues is https://github.com/Azure/azure-sdk-for-net/issues. If it isn't, I'm sure someone will help ferry it to the right location.

@vcsjones
Copy link
Member

vcsjones commented Jan 4, 2023

I opened a question around documentation over at Azure/azure-sdk-for-net#33300.

Summarizing:

  1. The zero padding is not observed on the Key property of HMACSHA256, and as far as I can see, has always been the case. The documentation has been corrected.
  2. The library was enforcing a minimum key size requirement that the zero padding was circumventing.
  3. A query has been opened for the appropriate module to see if there are documentation improvements that can be made for SymmetricSecurityKey.

I don't think there is any remaining work to perform for this issue. I'm going to close this out, but please feel free to re-open if you believe there are still issues that need to be addressed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

No branches or pull requests

2 participants