dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.12k stars 4.7k forks source link

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

Closed Vannevelj closed 1 year ago

Vannevelj commented 1 year ago

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.

https://github.com/dotnet/runtime/blob/6c0c96ccc775cad4119e2d5d3a1797c4126b4533/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/HMACCommon.cs#L55-L62

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

ghost commented 1 year ago

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](https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.hmacsha256?view=net-6.0) 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 ```cs 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); ``` ```xml Exe net7.0 enable enable ``` ### 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: ```cs 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. https://github.com/dotnet/runtime/blob/6c0c96ccc775cad4119e2d5d3a1797c4126b4533/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/HMACCommon.cs#L55-L62 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 commented 1 year ago

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

vcsjones commented 1 year ago

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 commented 1 year ago

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.

Vannevelj commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I opened a question around documentation over at https://github.com/Azure/azure-sdk-for-net/issues/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.