Quarkus: CORS should return correct headers even if an authorization header is wrong

[belyaev-andrey]

Describe the bug
When CORS is enabled and "authorization" header is present and incorrect (wrong username and/or password), CORS filter does not return proper headers and browser doesn't even get HTTPS 401 because the response is rejected by the CORS policy.

Expected behavior
I guess we should add CORS filter BEFORE security check.

Actual behavior
Security (username/password) is verified before CORS even if endpoint contains @PermitAll annotation.

To Reproduce
I created a small Quarkus+ReactJS application, you can clone it from here: https://github.com/belyaev-andrey/quarkus-security-cors, tested for localhost only.

• Run quarkus backend by executing mvn compile quarkus:dev
• Run ReactJS client by executing npm start in ./client folder
• Open browser for http://localhost:3000 address
• Click on Hello secured incorrect password button
• See Access to fetch at 'http://localhost:8080/api/admin' from origin 'http://localhost:3000' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled. log entry in browser's console.
• You can check that it doesn't work for endpoints with anonymous access if you change line 16 in App.js file and use 'hello' endpoint instead of 'admin'.

Environment (please complete the following information):

• Output of uname -a or ver: Microsoft Windows [Version 10.0.18362.10014]
• Output of java -version:

java version "10.0.2" 2018-07-17
Java(TM) SE Runtime Environment 18.3 (build 10.0.2+13)
Java HotSpot(TM) 64-Bit Server VM 18.3 (build 10.0.2+13, mixed mode)

• GraalVM version (if different from Java): N/A
• Quarkus version or git rev: 0.21.1

[sberyozkin]

Hi, I'm not sure it will be a good strategy, changing the order to accommodate the applications doing the faulty Authorization header submissions.
The problem I see with it is that, if the CORS policy is more than just an 'allow all wildcard' but a more fine-grained one, then the attackers can just learn about it without even needing to bypass the authorization barrier.
This should be consistent with the way KC manages it. Does its CORS filter return the policy values even if no Authorization header is available ?

[belyaev-andrey]

Yes, it returns correct headers along with 401 code if there is no authorization header. Which is a bit inconsistent, to my mind because responses for a request without header and with the "incorrect" header should be the same - 401.

[sberyozkin]

Thanks, so what does it return when the Authorization value is incorrect ?

[belyaev-andrey]

Using my application and Fiddler, I got the following:

No authorization:

Request:

GET http://localhost:8080/api/admin HTTP/1.1
Host: localhost:8080
Connection: keep-alive
Sec-Fetch-Mode: cors
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.132 Safari/537.36
content-type: application/json
Accept: */*
Origin: http://localhost:3000
Sec-Fetch-Site: same-site
Referer: http://localhost:3000/
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9,fr;q=0.8

Response:

HTTP/1.1 401 Unauthorized
Connection: keep-alive
Access-Control-Allow-Origin: http://localhost:3000
Access-Control-Allow-Credentials: true
Content-Type: text/html;charset=UTF-8
Content-Length: 14
Date: Tue, 27 Aug 2019 12:32:02 GMT

Not authorized

Wrong authorization header:

Request:

GET http://localhost:8080/api/admin HTTP/1.1
Host: localhost:8080
Connection: keep-alive
Sec-Fetch-Mode: cors
authorization: Basic WrongYWRtaW46YWRtaW4=
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.132 Safari/537.36
content-type: application/json
Accept: */*
Origin: http://localhost:3000
Sec-Fetch-Site: same-site
Referer: http://localhost:3000/
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9,fr;q=0.8

Response:

HTTP/1.1 401 Unauthorized
Connection: keep-alive
WWW-Authenticate: Basic realm="Cors-Test"
Content-Type: text/html;charset=UTF-8
Content-Length: 71
Date: Tue, 27 Aug 2019 12:33:12 GMT

<html><head><title>Error</title></head><body>Unauthorized</body></html>

[sberyozkin]

The response in 2nd case (with the wrong authorization header) is what I'd expect as well (this is a response from the Keycloak, right ?).
Is it what the Quarkus CORS filter does as well ? IMHO it is correct by default, even if it is too strict.
The 1st case can be tackled separately (I agree it is inconsistent, whichever way you look at it :-) ).

[belyaev-andrey]

I'm using elytron, not keycloak, just for the sake of simplicity. And by not adding CORS headers in the second case the framework does not allow me to handle the response correctly in my ReactJS application because the browser just blocks it. That is the reason why I raised the issue. I'm OK with any response (401, 402, 410, 418, whatever) as soon as it is not blocked by the browser.

[sberyozkin]

Hi, sorry I should've been more clear.
IMHO, there should be a consistent handling of this issue in Quarkus, irrespectively of how the endpoint is protected, either with the quarkus-elytron-security (for example, Basic Auth) or with quarkus-keycloak (OIDC tokens), it does not really matter what sort of scheme is set in the Authorization header, the treatment of the CORS preflight requests should be consistent. It would be wrong IMHO if the Quarkus Elytron and Quarksu KC CORS handlers would react differently.
This is why I asked earlier about Keycloak, but I was too lazy and only typed 'KC', sorry :-)

So the 1 thing I'd like to find out is how KC reacts in cases where a CORS preflight request is issued alongside the invalid token. Let me ask on the forum first.
thanks

[belyaev-andrey]

Yep, I completely agree with you regarding request/response consistency. Also, I think it might worth implementing a simple backend application using other frameworks, e.g. Spring boot or Micronaut and look at their behavior. I think I'll add more backends to my sample soon :-)

[sberyozkin]

OK, KC always handles the preflight request, so I guess the Quarkus should handle it the same way by default.

[sberyozkin]

My proposal is to introduce a property instructing Quarkus to always handle preflights, by default it can be true to stay consistent with KC, and the order of the filters will be adjusted. If someone has concerns about exposing the fine grained CORS policy details in some security-sensitive set ups then this attribute can be disabled. If dealing with such an attribute is problematic/not possible then CORS preflight should always be done :-).

[belyaev-andrey]

While investigating this issue, I could not find a "standard" behavior for the CORS+auth combination. Introducing a setting for this looks like a good idea to me.

[sirf]

If the access control headers are not set for 4xx responses, the status code will not be exposed in a browser application, effectively masking the actual error.

Most real world applications I've come across where CORS headers can be enabled will set the headers even for non-2xx responses. Amazon S3 is a good example:
curl -v "https://eriks-testbucket.s3.eu-north-1.amazonaws.com/403.txt" -H "Origin: http://something"

[sberyozkin]

Some applications like masking the errors and return 200 even if it is 403 :-).
having a setting (always report - default, block them - optional, may be no one will ever use it but having it there could be useful for running Quarkus in ultra sensitive set ups I guess)

[sberyozkin]

@belyaev-andrey Can you please check the latest master code if you can ? @stuartwdouglas has done some fixes.

[sberyozkin]

Marking it as a Good First issue for Andrey or someone else from the community to verify the issue has been resolved and if not then consider a minor PR :-). @belyaev-andrey Andrey, thanks for opening this issue in the 1st place

[belyaev-andrey]

It works for me with version 0.28.1, but it looks like you don't take

quarkus.security.users.file.realm-name

property into account. I always see realm name "Quarkus" for the wrong password. I'll double check it and create an issue if it is confirmed.

[sberyozkin]

@belyaev-andrey thanks, good news (that this issue has been resolved), please open the one about quarkus.security.users.file.realm-name, thanks

[sberyozkin]

I've created #5284 to confirm and fix if needed