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

Security Issues: "Access-Control-Allow-Credentials" should be false by default when using "quarkus.http.cors=true" #10447

Closed
ia3andy opened this issue Jul 3, 2020 · 16 comments · Fixed by #10660
Assignees
Labels
kind/bug Something isn't working
Milestone

Comments

@ia3andy
Copy link
Contributor

ia3andy commented Jul 3, 2020

Describe the bug
As soon as quarkus.http.cors=true is set, the Access-Control-Allow-Credentials: true is added as header.

It needs to be disabled for example to allow unknown requests, but avoid sending credentials (cookies, ..).

Expected behavior
Have an option to not add it in the header but still be able to use other CORS configs.

@ia3andy ia3andy added the kind/bug Something isn't working label Jul 3, 2020
@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2020

Here is the line of code:

response.headers().set(HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS, "true");

@ia3andy ia3andy changed the title It is not possible to disable "Access-Control-Allow-Credentials: true" when using "quarkus.http.cors=true" Security Issue: It is not possible to disable "Access-Control-Allow-Credentials: true" when using "quarkus.http.cors=true" Jul 3, 2020
@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2020

This is also the case in OpenApi extension:

response.headers().set("Access-Control-Allow-Credentials", "true");

We should also avoid ACAO: * and use the request origin:

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2020

For context:
If the site specifies the header Access-Control-Allow-Credentials: true in addition with an open Access-Control-Allow-Origin, third-party sites may be able to carry out privileged actions and retrieve sensitive information. Even if it does not, attackers may be able to bypass any IP-based access controls by proxying through users' browsers.

Steps of reproduction:
└──╼ $curl -I -H "Origin: evil.com" https://my-api.io
HTTP/1.1 200 OK
access-control-allow-origin: evil.com
access-control-allow-credentials: true
Cache-Control: no-cache, no-store, must-revalidate
Pragma: no-cache
Expires: 0
Content-Type: text/html
Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT
Content-Length: 3276
Accept-Ranges: bytes
Set-Cookie: 4203433b1528fb7d85e2ffa567cf2487=d18ab2b3c893cb05927c12a7a83f6e07; path=/; HttpOnly; Secure

CORS poorly implemented, best case for attack: Allowed access to cookies.
Remediation: Disable ACAC (Access-Control-Allow-Credentials) or set it to "false" if possible. Consider using CORS dynamic header generation or re-config to trusted URLs.

Defaulting Access-Control-Allow-Credential to true is therefore a bad practice and should only be set to true in addition to some other safeties (like setting trusted urls).

@ia3andy ia3andy changed the title Security Issue: It is not possible to disable "Access-Control-Allow-Credentials: true" when using "quarkus.http.cors=true" Security Issue: "Access-Control-Allow-Credentials" should be false by default when using "quarkus.http.cors=true" Jul 3, 2020
@ia3andy ia3andy changed the title Security Issue: "Access-Control-Allow-Credentials" should be false by default when using "quarkus.http.cors=true" Security Issues: "Access-Control-Allow-Credentials" should be false by default when using "quarkus.http.cors=true" Jul 3, 2020
@mcserra
Copy link
Contributor

mcserra commented Jul 3, 2020

Can I pick this?

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2020

@mcserra sure

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2020

The only problem we need to discuss is that it could break some existing apis, @gsmet any idea on how to deal with that?

@mcserra
Copy link
Contributor

mcserra commented Jul 3, 2020

Cool, thanks!

@sberyozkin
Copy link
Member

@ia3andy Hi, does it make sense to use it by default if one has:

quarkus.http.cors.origins=http://good.com

and switch to false only if origins is a wildcard ?

@sberyozkin
Copy link
Member

The only problem we need to discuss is that it could break some existing apis

Yes, it can break the existing applications, but your analysis shows that in case of the origin wildcard it is not safe, but if the expected origin has been matched then it is not so clear if switching to false is strictly necessary

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2020

@sberyozkin very good idea indeed!

So Access-Control-Allow-Credentials should be a new config option AND be true by default only if quarkus.http.cors.origins is present and not '*'.

It should be false by default when Access-Control-Allow-Origin is either * or dynamic from request origin.

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2020

We also need another PR for OpenApi to remove Access-Control-Allow-Credentials and to set Access-Control-Allow-Origin to the request origin instead of *.

@sberyozkin
Copy link
Member

yeah that is probably the right compromise, lets see if Guillaume and others agree, thanks

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2020

Another option so we don't break any API would be to display a WARN log when Access-Control-Allow-Credentials is not specified and Access-Control-Allow-Origin is open (wildcard or request origin)

@sberyozkin
Copy link
Member

sberyozkin commented Jul 3, 2020

We also need another PR for OpenApi

In smallrye-open-api ? Please open an issue there

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2020

@sberyozkin no it's in the Quarkus extension part of it:

response.headers().set("Access-Control-Allow-Credentials", "true");

@sberyozkin
Copy link
Member

Another option so we don't break any API would be to display a WARN log when Access-Control-Allow-Credentials is not specified and Access-Control-Allow-Origin is open (wildcard or request origin)

I'd just go for your original proposal to switch it to false when the origin is wildcard in 1.7.0 and update the migration guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants