Authorize_code revoked before client_id validation

[tl-francesco-oddo]

Issue / Steps to reproduce the problem

The method ValidateAuthorizationCodeRequestAsync in ./src/IdentityServer4/src/Validation/Default/TokenRequestValidator.cs invokes RemoveAuthorizationCodeAsync to revoke the code before checking the client_id sent in the body of the request is valid (i.e. checking if the client_id value provided by the user-agent matches the one stored in the back-end and associated to the authorization_code).

The impact of this is that a malicious client could discover/leak somehow a valid authorization_code from a victim front-channel (via other vectors), then send it along its client_credentials to prevent the legitimate client from consuming it. Not a critical issue but still potentially feasible.

Let's assume a request A sent by the malicious client with its client credentials but the victim authorization code and a request B sent by the legitimate client with the correct client credentials and its authorization code. Two scenarios can happen:

1. if the two requests are received sequentially with a minimal time difference, request A will fail the check against the client binding, while request B gets an access token, then the code is invalidated (expected)
2. if the two requests are received with just a few seconds difference from one another, request B will fail authentication since request A's execution of RemoveAuthorizationCodeAsync completes before B can reach the lookup step GetAuthorizationCodeAsync and consume its authorization_code.

./src/IdentityServer4/src/Validation/Default/TokenRequestValidator.cs (code snippets not relevant are omitted).

 private async Task<TokenRequestValidationResult> ValidateAuthorizationCodeRequestAsync(NameValueCollection parameters)
        {
[...]

            /////////////////////////////////////////////
            // validate authorization code
            /////////////////////////////////////////////
            var code = parameters.Get(OidcConstants.TokenRequest.Code);
[...]

            _validatedRequest.AuthorizationCodeHandle = code;

            var authZcode = await _authorizationCodeStore.GetAuthorizationCodeAsync(code);
            if (authZcode == null)
            {
                LogError("Invalid authorization code", new { code });
                return Invalid(OidcConstants.TokenErrors.InvalidGrant);
            }

            await _authorizationCodeStore.RemoveAuthorizationCodeAsync(code);

[...]

            /////////////////////////////////////////////
            // validate client binding
            /////////////////////////////////////////////
            if (authZcode.ClientId != _validatedRequest.Client.ClientId)
            {
               LogError("Client is trying to use a code from a different client", new { clientId = _validatedRequest.Client.ClientId, codeClient = authZcode.ClientId });
                return Invalid(OidcConstants.TokenErrors.InvalidGrant);
            }

[...]
        }

As far as I can tell the validation of the client ID should be "moved up" immediately after retrieving the code and checking its value exists, and the code should be invalidated only when the client ID is valid. Unless it is intended for the authorization_code to be invalidated regardless of the client ID attempting to consume it. Could you confirm?

[tl-francesco-oddo]

Any update about this?

[leastprivilege]

We will look into it. But I am travelling right now.

[leastprivilege]

#4501

[tl-francesco-oddo]

Thanks for confirming and patching the issue. I have also requested a CVE ID for this issue.

[brockallen]

I have also requested a CVE ID for this issue.

Why?

[tl-francesco-oddo]

Why?

Because it is a security vulnerability? :-)

[leastprivilege]

If you are after a CVE, you seem to be a security professional. Why didn't you use responsible disclosure then?

The way I see this is, with the previous validation logic, one legitimate client would be able to invalidate the authorization code of another legitimate client. How exactly is this a vulnerability? What's the damage that happens in the worst case?

[tl-francesco-oddo]

The fact that both clients are legitimate is irrelevant. One legitimate client shouldn't be able to invalidate the authorisation code of another and prevent the second client from completing the authorisation code flow. It is obviously a minor issue with low exploitability and availability impact - that's why I just opened a ticket directly on GitHub - but in my opinion it is still security relevant.

I don't mind about the CVE if you are inclined to consider it a logic bug. I thought it was a good thing to ask for it to keep track of the issue and associate it to a patched version. And anyway what harm does it make even if it were a CVSS < 3 scored vulnerability? "Your framework" might be an open source project but I don't need to tell you it is very popular and widely used and has obviously a very critical function.

Having said that I am glad you fixed it.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.