Incorrect validation of authorization response

[pixtron]

Expected Behavior

Describe expected behavior

According to Section 3.1.2.7 of the openid spec, the client MUST validate the response according to RFC 6749, especially Sections 4.1.2 and 10.12. The openid spec does not specifically mention Section 4.1.3 of rfc6479 but Error responses should also be validated.

Describe the problem

Actual Behavior

This is the current validation:

https://github.com/openid/AppAuth-JS/blob/f34322dc6f8a0be667da7e1c7c3227686ff45842/src/node_support/node_request_handler.ts#L63-L70

• if the redirect uri is called with the state parameter only the client will currently handle the response. Either "code" or "error" parameter must be present.
• if "state" parameter was present in request, it must be present in the response as well and match the exact value sent in the request.

Steps to reproduce the behavior

1.) perform an authorization request but do not complete consent screen 2a.) Open http://127.0.0.1:8000/?code=someothervalidcode -> will exchange the code for an access token, allowing an atacker to potentially perform a CSRF attack (see rfc6479 10.12) 2b.) Open http://127.0.0.1:8000/?state=somestate -> will close the server and try to exchange code null for an access token.

Environment

• AppAuth-JS version: 1.2.6
• AppAuth-JS Environment: Node

[tikurahul]

A couple of things mitigate the CSRF attack:

• PKCE ensures that it’s not enough to intercept the code and the state. The token service will not issue tokens unless you can provide the verifier. In such cases, the clients error handling mechanism can restart the authorization request. AppAuth-JS uses this by default.

• The node server is usually spun up on a random port number. Then the token issuer can check if the redirect URIs matched when the token request was made (apart from the other usual checks)

• Requests need a valid code and state which is random to get the tokens in the absence of PKCE. So a malicious entity making forged requests will need to know that. The most damage they can do is to close the server (which falls back to the regular error handing mechanism). This is actually better than the opposite where the server remains open, and they can keep making requests. If you are in this bucket I would argue you have bigger problems.

[tikurahul]

All the authorization callback is doing is taking parameters it got from a well known endpoint. It’s also a way for a well known AP to verify that it’s a known client making the request.

The checks for state, verifiers are done before tokens are issued.

[pixtron]

PKCE ensures that it’s not enough to intercept the code and the state. The token service will not issue tokens unless you can provide the verifier. In such cases, the clients error handling mechanism can restart the authorization request. AppAuth-JS uses this by default.

CSRF is not about "intercepting" the code nor the state, it is about injecting a valid code for another user than the one trying to log in (eg. uploading files to an attackers Google Drive instead to the users Google Drive). For an OP it is not mandatory to implement PKCE. If the OP does not implement PKCE the CSRF Attack would not be "mitigated", although appauth sends a code_challenge.

The node server is usually spun up on a random port number. Then the token issuer can check if the redirect URIs matched when the token request was made (apart from the other usual checks)

Relying on a random value 0...65535 doesn't seem to be a secure way to mitigate a CSRF attack neither. 1.) As the port number is part of the redirect_uri, and the OP MUST check the redirect_uri. As an OP is not required to allow any port number, it is most likely the port number will be hardcoded in the apps using appauth. As the apps are not confidential (otherwise we could hardcode a client_secret) it will be easy for an attacker to figure out the port number used. 2.) Even if every authorization request uses a random port number 0...65535 an attacker might send out requests to all possible port numbers.

Requests need a valid code and state which is random to get the tokens in the absence of PKCE. So a malicious entity making forged requests will need to know that.

As the state is not checked anywhere, when using NodeBasedHandler an attacker does not need to know the state param used and might just send a request with a code parameter or include any random state parameter.

The most damage they can do is to close the server (which falls back to the regular error handing mechanism).

Whenever the server is closed the authorization promise resolves with an AuthorizationRequestResponse: https://github.com/openid/AppAuth-JS/blob/f34322dc6f8a0be667da7e1c7c3227686ff45842/src/node_support/node_request_handler.ts#L98-L104

In the case of a CSRF Attack the AuthorizationRequestResponse contains a malicious code, in the case of an invalid request (state set, but neither error nor code) it contains a null value as code. Contrary to the Specs appauth delegates the validation of the authorization response to the OP.

This is actually better than the opposite where the server remains open, and they can keep making requests. If you are in this bucket I would argue you have bigger problems.

I did not ask to keep the server open on erronous or malicious requests. Thats open for discussion. IMHO either emit an error && close server or ignore response && keep server open.

All the authorization callback is doing is taking parameters it got from a well known endpoint. It’s also a way for a well known AP to verify that it’s a known client making the request.

The response should be validated according to the specs:

From Openid Core 1.0 Section 3.1.2.7:

When using the Authorization Code Flow, the Client MUST validate the response according to RFC 6749, especially Sections 4.1.2 and 10.12.

From RFC 6749 Section 4.1.2:

code REQUIRED. The authorization code generated by the authorization server. ... state REQUIRED if the "state" parameter was present in the client authorization request. The exact value received from the client.

From RFC 6749 Section 4.1.2.1. Error Response

error REQUIRED. A single ASCII [USASCII] error code from the following ...

From RFC 6749 Section 10.12:

... The client MUST implement CSRF protection for its redirection URI. This is typically accomplished by requiring any request sent to the redirection URI endpoint to include a value that binds the request to the user-agent's authenticated state (e.g., a hash of the session cookie used to authenticate the user-agent). The client SHOULD utilize the "state" request parameter to deliver this value to the authorization server when making an authorization request. ...

RedirectBasedHandler does check that state from the response does match the state sent in the request. At least this check should be done in NodeBasedHandler as well, although it would be better to get fully compliant to the specs. https://github.com/openid/AppAuth-JS/blob/f34322dc6f8a0be667da7e1c7c3227686ff45842/src/redirect_based_handler.ts#L105

The checks for state, verifiers are done before tokens are issued. Could you point out where exactly the state is checked? Can't verify this.