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] SHR POP tokens are broken due to invalid alg (it should be PS256 not RS256) #4839

Closed
sfeitler opened this issue Jul 11, 2024 · 2 comments · Fixed by #4842
Closed

[Bug] SHR POP tokens are broken due to invalid alg (it should be PS256 not RS256) #4839

sfeitler opened this issue Jul 11, 2024 · 2 comments · Fixed by #4842
Assignees
Labels
bug confidential-client P2 regression Behavior that worked in a previous release that no longer works in a newer release
Milestone

Comments

@sfeitler
Copy link

sfeitler commented Jul 11, 2024

Library version used

4.60.3

.NET version

net 8.0

Scenario

ConfidentialClient - service to service (AcquireTokenForClient)

Is this a new or an existing app?

The app is in production, and I have upgraded to a new version of MSAL

Issue description and reproduction steps

I think the signed pop token produced by Microsoft.Identity.Client is not specifying the correct cryptographic algorithm in the JsonWebToken header.

I have a client application that uses ConfidentialClientApplicationBuilder AcquireTokenForClient to generate a signed pop token. Then my server application uses SignedHttpRequestHandler ValidateSignedHttpRequestAsync to validate the signature.

This code was working fine up through M.I.C version 4.59.1: the token generated by the client could be validated successfully in the server. However, in version 4.60.3 (the next one we tried), this validation starts failing with an error on signature validation (IDX23034).

Debugging deep into the validation code, I have figured out that the failure is because the validation code examines the algorithm from the JWT header and determines that the signature padding is Pkcs1. But with version 4.60 and higher, actually it is Pss.

To reproduce, use 4.60.3 or more recent to generate a signed pop token and then run ValidateSignedHttpRequestAsync on it.

I can provide sample code if necessary--I have a standalone application that generates a pop token and then validates it--but I think this issue can be seen just by reading the project code.


More details

I am pretty sure the issue stems from a recent fix in InMemoryCryptoProvider to use PSS for the padding. See PR #4633.

The code now reads:

    public byte[] Sign(byte[] payload)
    {
        return _signingKey.SignData(
            payload,
            HashAlgorithmName.SHA256,
            **RSASignaturePadding.Pss);**
    }

However, the crypto provider still returns "RS256" for the CryptographicAlgorithm:

    public string CryptographicAlgorithm { get => "RS256"; }

I think it needs to return "PS256" to get Pss signature padding (I got that value from https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/wiki/Supported-Algorithms).

Here's how it all goes down:

The value returned from CryptographicAlgorithm gets put into the JsonWebToken header in the "alg" field. Later when the signature is being verified in the server (using SignedHttpRequestHandler ValidateSignatureAsync), the alg field is read and used to determine the padding on the signature field. Padding determination is done in this code (from AsymmetricAdapter.cs):

        if (algorithm.Equals(SecurityAlgorithms.RsaSsaPssSha256) ||
            algorithm.Equals(SecurityAlgorithms.RsaSsaPssSha256Signature) ||
            algorithm.Equals(SecurityAlgorithms.RsaSsaPssSha384) ||
            algorithm.Equals(SecurityAlgorithms.RsaSsaPssSha384Signature) ||
            algorithm.Equals(SecurityAlgorithms.RsaSsaPssSha512) ||
            algorithm.Equals(SecurityAlgorithms.RsaSsaPssSha512Signature))
        {
            RSASignaturePadding = RSASignaturePadding.Pss;
        }
        else
        {
            // default RSASignaturePadding for other supported RSA algorithms is Pkcs1
            RSASignaturePadding = RSASignaturePadding.Pkcs1;
        }

Since the header alg field is not any of the "Pss" versions, the padding is set to Pkcs1. Then the signature validation fails because it's not using the correct padding.

Relevant code snippets

We are building the confidential client app as:
var uamiCcApp = ConfidentialClientApplicationBuilder
.Create(sniClientId)
.WithCertificate(cert, true)
.WithTenantId(tenantId)
.WithAuthority(new Uri(authConfig.Authority))
.WithExperimentalFeatures() // for PoP
.Build();

and the pop token as:
popConfig.HttpHost = host.Replace("https://", "");
popConfig.HttpMethod = httpMethod;
popConfig.HttpPath = path;
popConfig.SignHttpRequest = true;

                result = await ccApp.AcquireTokenForClient(scopes)
                    .WithSendX5C(true)
                    .WithProofOfPossession(popConfig)
                    .ExecuteAsync();

Expected behavior

The signature validates correctly.

Identity provider

Microsoft Entra ID (Work and School accounts and Personal Microsoft accounts)

Regression

MSAL Version: 4.59.1

Solution and workarounds

I think the solution is to modify InMemoryCryptoProvider to return PS256 for the algorithm:

public string CryptographicAlgorithm { get => "PS256"; }

I am trying this solution out locally by implementing a custom crypto provider, and so far it appears to work.

@sfeitler sfeitler added needs attention Delete label after triage untriaged Do not delete. Needed for Automation labels Jul 11, 2024
@sfeitler
Copy link
Author

I recorded a short video showing the code behavior as it is now, and with a custom crypto provider. The following link should be viewable by Microsoft employees:

https://microsoft-my.sharepoint.com/:v:/p/safeitle/EYXRLesI2F5PrxIF1ba60AEBdbEAjuiT7cajLPOyyCiMTA

@bgavrilMS bgavrilMS changed the title [Bug] Alg field in JsonWebToken header does not match signature padding algorithm [Bug] SHR POP tokens are broken due to invalid alg (it should be PS256 not RS256) Jul 11, 2024
@bgavrilMS bgavrilMS added bug P2 confidential-client regression Behavior that worked in a previous release that no longer works in a newer release and removed untriaged Do not delete. Needed for Automation needs attention Delete label after triage labels Jul 11, 2024
@bgavrilMS
Copy link
Member

Important note: the fix must be tested on both .NET FWK and .NET

@gladjohn gladjohn self-assigned this Jul 13, 2024
@gladjohn gladjohn added this to the 4.63.0 milestone Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug confidential-client P2 regression Behavior that worked in a previous release that no longer works in a newer release
Projects
Archived in project
3 participants