Azure-activedirectory-identitymodel-extensions-for-dotnet: System.IdentityModel.Tokens.Jwt is using the key after disposing System.Security.Cryptography. ECDsaOpenSsl

[VladislavMorozov]

https://github.com/dotnet/corefx/issues/42631

[brentschmaltz]

Before adding or using a SignatureProvider, we should check if the key material has been disposed. If so, don't add to the cache or use the SignatureProvider.

[martincostello]

I just stumbled across this issue while investigating an ObjectDisposedException for ECDsa after trying to update some code from version 5.3.0 to 5.6.0.

Test Name:  GenerateAsync_Caches_Jwt_Until_Expired
Test FullName:  AspNet.Security.OAuth.Providers.Tests (netcoreapp3.1).AspNet.Security.OAuth.Apple.AppleClientSecretGeneratorTests.GenerateAsync_Caches_Jwt_Until_Expired
Test Source:    C:\Coding\AspNet.Security.OAuth.Providers\test\AspNet.Security.OAuth.Providers.Tests\Apple\AppleClientSecretGeneratorTests.cs : line 78
Test Outcome:   Failed
Test Duration:  0:00:00

Test Name:  GenerateAsync_Caches_Jwt_Until_Expired
Test Outcome:   Failed
Result StackTrace:  
at System.Security.Cryptography.ECCngKey.ThrowIfDisposed()
   at System.Security.Cryptography.ECCngKey.GetDuplicatedKeyHandle(Int32 callerKeySizeProperty)
   at System.Security.Cryptography.ECDsaImplementation.ECDsaCng.SignHash(Byte[] hash)
   at Microsoft.IdentityModel.Tokens.AsymmetricAdapter.SignWithECDsa(Byte[] bytes)
   at Microsoft.IdentityModel.Tokens.AsymmetricAdapter.Sign(Byte[] bytes)
   at Microsoft.IdentityModel.Tokens.AsymmetricSignatureProvider.Sign(Byte[] input)
   at Microsoft.IdentityModel.JsonWebTokens.JwtTokenUtilities.CreateEncodedSignature(String input, SigningCredentials signingCredentials)
   at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.CreateJwtSecurityTokenPrivate(String issuer, String audience, ClaimsIdentity subject, Nullable`1 notBefore, Nullable`1 expires, Nullable`1 issuedAt, SigningCredentials signingCredentials, EncryptingCredentials encryptingCredentials)
   at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.CreateJwtSecurityToken(SecurityTokenDescriptor tokenDescriptor)
   at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.CreateEncodedJwt(SecurityTokenDescriptor tokenDescriptor)
   at AspNet.Security.OAuth.Apple.Internal.DefaultAppleClientSecretGenerator.GenerateNewSecretAsync(AppleGenerateClientSecretContext context) in C:\Coding\AspNet.Security.OAuth.Providers\src\AspNet.Security.OAuth.Apple\Internal\DefaultAppleClientSecretGenerator.cs:line 86
   at AspNet.Security.OAuth.Apple.Internal.DefaultAppleClientSecretGenerator.GenerateAsync(AppleGenerateClientSecretContext context) in C:\Coding\AspNet.Security.OAuth.Providers\src\AspNet.Security.OAuth.Apple\Internal\DefaultAppleClientSecretGenerator.cs:line 48
   at AspNet.Security.OAuth.Apple.AppleClientSecretGeneratorTests.<>c__DisplayClass2_0.<<GenerateAsync_Caches_Jwt_Until_Expired>b__0>d.MoveNext() in C:\Coding\AspNet.Security.OAuth.Providers\test\AspNet.Security.OAuth.Providers.Tests\Apple\AppleClientSecretGeneratorTests.cs:line 93
--- End of stack trace from previous location where exception was thrown ---
   at AspNet.Security.OAuth.Apple.AppleClientSecretGeneratorTests.GenerateTokenAsync(AppleAuthenticationOptions options, Func`3 actAndAssert) in C:\Coding\AspNet.Security.OAuth.Providers\test\AspNet.Security.OAuth.Providers.Tests\Apple\AppleClientSecretGeneratorTests.cs:line 129
   at AspNet.Security.OAuth.Apple.AppleClientSecretGeneratorTests.GenerateAsync_Caches_Jwt_Until_Expired() in C:\Coding\AspNet.Security.OAuth.Providers\test\AspNet.Security.OAuth.Providers.Tests\Apple\AppleClientSecretGeneratorTests.cs:line 90
--- End of stack trace from previous location where exception was thrown ---
Result Message: 
System.ObjectDisposedException : Cannot access a disposed object.
Object name: 'ECDsa'.

It looks like the change to use token caching by default in #1198 breaks the following code by causing the ECDsa created by the first call to the method (and then disposed of) to be retained and used on subsequent calls, causing regeneration of the secret after the provider's own cache expires to fail due to an ObjectDisposedException being thrown.

https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/53d23133c07b311c088dd05ae8cd8d76ce18a164/src/AspNet.Security.OAuth.Apple/Internal/DefaultAppleClientSecretGenerator.cs#L82-L87

[brentschmaltz]

@martincostello yes the 'using' statement causes the dispose and with the current code, this pattern will fault. We agree the IdentityModel runtime should fix this.

In the meantime, if CreateSigningCredentials (https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/53d23133c07b311c088dd05ae8cd8d76ce18a164/src/AspNet.Security.OAuth.Apple/Internal/DefaultAppleClientSecretGenerator.cs#L113)

`
var cryptoProviderFactory = new CryptoProviderFactory();
cryptoProviderFactory.CacheSignatureProviders = false;
var key = new ECDsaSecurityKey(algorithm) { KeyId = keyId };
var signingCredentials = new SigningCredentials(key, SecurityAlgorithms.EcdsaSha256Signature){ CryptoProviderFactory = cryptoProviderFactory};
return signingCredentials;

`

There is a high level Default that will turn off caching.
CryptoProviderFactory.DefaultCacheSignatureProviders = false;

[martincostello]

Yeah I saw the high-level default, but as it's static it's not something I want to be altering from deep within the bowels of a library. Users can set it themselves if they need to as a workaround though.

The code workaround seems a bit heavy-handed as a workaround to put in a library (if it's then going to be made redundant by a fix), so I'll just leave things as-is for now.

I just wanted to add to the issue so that any fix also caters for this use-case.

[brentschmaltz]

@VladislavMorozov
your code from the other thread is:

for (var i = 0; i < 100; i++)
{
    using (var algorithm = ECDsa.Create()) // this code doesn't work correct
    {
        tokenDescriptor.SigningCredentials = new SigningCredentials(new ECDsaSecurityKey(algorithm)
        {
            KeyId = "A"
        }, SecurityAlgorithms.EcdsaSha256);

        var token = tokenHandler.CreateEncodedJwt(tokenDescriptor);
    }
}

I am trying to understand when you would use this pattern?
For deterministic control, we would need to add IDisposible on ECDsaSecurityKey.

[VladislavMorozov]

@brentschmaltz I want to create tokens that are signed with the ES256 algorithm on the Linux machine. I didn't find another way.
var algorithm = ECDsa.Create() is IDisposible, so I use using construction. I can register ECDsa.Create() in DI container, but I think that the problem will remain.

[brentschmaltz]

Can you try using a unique KeyId?

[VladislavMorozov]

@brentschmaltz Of course on production, I use a unique KeyId. In my issue just a small example.

[shadow-cs]

I have come across this issue as well. I'm creating a new ECDsa instance each time (and disposing it each time, if I don't one of my analyzers bugs me about it 😉). The second call to signing method fails with ObjectDisposedException. I'm using number of private keys. I'd have to create a dictionary for each key I guess to bypass the error.

[brentschmaltz]

@shadow-cs @VladislavMorozov AH, ..., if you dispose the ECDsa instance the signature provider has been cached with the ECDsa key inside. The next call will find a cached SignatureProvider.

You can turn caching off:
1.at the App level by writting at startup:

CryptoProviderFactory.DefaultCacheSignatureProviders

1. at the call level by writing where MyCustomCryptoProviderFactory overrides : CreateForSigning(key, algorithm) and calls CreateForSigning(key, algorithm, false);

using (var algorithm = ECDsa.Create()) // this code doesn't work correct
{
    tokenDescriptor.SigningCredentials =    
        new SigningCredentials(
            new ECDsaSecurityKey(algorithm)
            {
                KeyId = "A"
            }, SecurityAlgorithms.EcdsaSha256)
    { CryptoProviderFactory = MyCustomCryptProviderFactory };

    var token = tokenHandler.CreateEncodedJwt(tokenDescriptor);
}

[arthg]

I am experiencing a similar exception when I upgrade from 5.6.0 to 6.5.0.
(Microsoft.IdentityModel.Tokens and System.IdentityModel.Tokens.Jwt)

In this case it is happening on 2nd call after an RSA instance disposed.

System.ObjectDisposedException : Cannot access a disposed object.
Object name: 'RSA'.

Demo code, not production. First call emits an valid RSA256 token, second call throws:

            private static string GetJwt(string pk)
            {
                var privateKeyBuffer = PrivateRsaKey.GetBytes(pk);
                using var rsa = RSA.Create(); // remove the using and no throw
                rsa.ImportRSAPrivateKey(privateKeyBuffer, out _);
                var rsaSecurityKey = new RsaSecurityKey(rsa);
                var signingCredentials = new SigningCredentials(rsaSecurityKey, SecurityAlgorithms.RsaSha256);

                var header = new JwtHeader(signingCredentials);
                var token = new JwtSecurityToken(header, payload);
                return new JwtSecurityTokenHandler().WriteToken(token);  // throws System.ObjectDisposedException 
            }

When the using is removed the exception is not thrown

I attempted to use the workaround described here: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1302#issuecomment-606776893

Maybe I am not correctly implementing the workaround but I am not able to mitigate the exception.

Has anyone had success with the workaround or other experience to share / recommend a solution?

Update: I finally was able to avoid the exception with minimally invasive change.

            var signingCredentials = new SigningCredentials(rsaSecurityKey, SecurityAlgorithms.RsaSha256)
            {
                CryptoProviderFactory = new CryptoProviderFactory { CacheSignatureProviders = false }
            };

That is all. I was getting a false signal because I needed to make the change in 2 places:

1. The client code (integration test) using the private key
2. The server code using the public key

[brentschmaltz]

@martincostello @keegan-caruso closing and attaching to #1434