Restrict authentication to same-origin requests. #2627

jsha · 2020-07-12T19:27:51Z

In #2480, Access-Control-Allow-Origin: * was added to all requests. The goal,
as I understand it, was to allow other web sites to request data from
the public API - for instance the download counts for crates.

This does somewhat increase the risk of unwanted cross-site requests.
There are two cases to be concerned about, per
https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS:

"Simple" requests: GET, HEAD, or POST, and "preflighted" requests (for
our purposes, PUT).

Simple requests execute without an OPTIONS preflight, and the browser
decides whether the caller can read the response based on the
Access-Control-Allow-Origin (ACAO) header in the response. For GET,
the risk is that a third-party website could read private data - for
instance the email address in /api/v1/me. For POST, the risk is that
an important state mutation (like uploading a new crate) could happen
even if the calling web page isn't allowed via ACAO to read the response.

The above risk for GET is mitigated two ways:

Users who logged in recently get the SameSite setting on their
session cookie, so their cookie is not sent on cross-site requests
and they cannot be authenticated.
The CORS specification indicates that, on a request carrying
cookies, the calling origin cannot access the response unless the
ACAO header in the response explicitly names that origin; a wildcard
is not sufficient.

For POST, the risk is present regardless of whether ACAO: * is set.
Since POSTs aren't preflighted, they can mutate state even if ACAO
doesn't permit accessing the response. The risk for POST is mitigated
by:

SameSite cookie setting
crates.io doesn't currently implement any POST endpoints, only PUT.
But that's not a documented security feature and could change in the
future.

For preflighted PUT requests, the risk is that a third-party website
could send a request that mutates state, like uploading a new crate.
This is mitigated by:

SameSite cookie setting
Preflight request will fail because crates.io currently does not
support OPTIONS, and so will return 404. Nginx will additionally not
set ACAO on 404s. However, the fact that crates.io doesn't support
OPTIONS isn't currently documented as a security feature.
If OPTIONS support was added at some point in the future, a browser
that sent a credentialed request should stop after the preflight
response if the ACAO header in the response did not explicitly include
the calling origin (rather than a wildcard).

However, the above is a somewhat complex analysis for an important piece
of crates.io's security. I'm proposing to add another layer of protection:
When authenticating a request, authentication is refused if the Origin
header is set to anything other than the origin of crates.io itself.

I started to write a test for this, but could not figure out how to
instantiate something that implements RequestExt. I'd appreciate any
tips on how best to test this.

In rust-lang#2480, Access-Control-Allow-Origin: * was added to all requests. The goal, as I understand it, was to allow other web sites to request data from the public API - for instance the download counts for crates. This does somewhat increase the risk of unwanted cross-site requests. There are two cases to be concerned about, per https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS: "Simple" requests: GET, HEAD, or POST, and "preflighted" requests (for our purposes, PUT). Simple requests execute without an OPTIONS preflight, and the browser decides whether the caller can read the response based on the Access-Control-Allow-Origin (ACAO) header in the response. For GET, the risk is that a third-party website could read private data - for instance the email address in /api/v1/me. For POST, the risk is that an important state mutation (like uploading a new crate) could happen even if the calling web page isn't allowed via ACAO to read the response. The above risk for GET is mitigated two ways: - Users who logged in recently get the SameSite setting on their session cookie, so their cookie is not sent on cross-site requests and they cannot be authenticated. - The CORS specification indicates that, on a request carrying cookies, the calling origin cannot access the response unless the ACAO header in the response explicitly names that origin; a wildcard is not sufficient. For POST, the risk is present regardless of whether ACAO: * is set. Since POSTs aren't preflighted, they can mutate state even if ACAO doesn't permit accessing the response. The risk for POST is mitigated by: - SameSite cookie setting - crates.io doesn't currently implement any POST endpoints, only PUT. But that's not a documented security feature and could change in the future. For preflighted PUT requests, the risk is that a third-party website could send a request that mutates state, like uploading a new crate. This is mitigated by: - SameSite cookie setting - Preflight request will fail because crates.io currently does not support OPTIONS, and so will return 404. Nginx will additionally not set ACAO on 404s. However, the fact that crates.io doesn't support OPTIONS isn't currently documented as a security feature. - If OPTIONS support was added at some point in the future, a browser that sent a credentialed request should stop after the preflight response if the ACAO header in the response did not explicitly include the calling origin (rather than a wildcard). However, the above is a somewhat complex analysis for an important piece of crates.io's security. I'm proposing to add another layer of protection: When authenticating a request, authentication is refused if the Origin header is set to anything other than the origin of crates.io itself. I started to write a test for this, but could not figure out how to instantiate something that implements RequestExt. I'd appreciate any tips on how best to test this.

rust-highfive · 2020-07-12T19:27:54Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @carols10cents (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Also, emit useful error when sameorigin check fails.

jsha · 2020-07-12T22:24:15Z

I realized that in the test environment, and presumably also the production environment, the API is behind a proxy, and has to rely on the X-Forwarded-Proto and X-Forwarded-Host headers to properly understand the origin of the request. Updated to use those. Also in order to make this work, I needed to update the init-local-index.sh script (and regenerate my local index) so that API requests would be sent via the Ember proxy and thus have the correct host:port.

bors · 2020-07-21T15:23:41Z

☔ The latest upstream changes (presumably #2644) made this pull request unmergeable. Please resolve the merge conflicts.

jtgeibel

Hey @jsha, first I wanted to say thank you very much for your recent work reviewing our security stance. Your responsible disclosure, opened issues and PRs, and suggestions are extremely helpful!

I have a few suggested changes to this PR. For local testing, I often do the following:

checkout PR
npm install
ember build --prod
cargo run --bin server
Go to http://localhost:8888/

In this configuration, the backend will serve the static frontend files that were built in dist/. I've been meaning to describe this in the docs, but this mirrors the way files are served in production (except there nginx serves most of them before they hit the backend).

Below I propose a few options that should make it possible to continue do local testing without booting a node server on port 4200.

jtgeibel · 2020-07-22T00:33:40Z

script/init-local-index.sh

@@ -19,7 +19,7 @@ cd tmp/index-tmp
 cat > config.json <<-EOF
 {
  "dl": "http://localhost:8888/api/v1/crates",
-  "api": "http://localhost:8888/"
+  "api": "http://localhost:4200/"


I propose dropping this change.

jtgeibel · 2020-07-22T01:14:04Z

src/controllers/util.rs

@@ -28,6 +28,30 @@ impl AuthenticatedUser {
 impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
    /// Obtain `AuthenticatedUser` for the request or return an `Unauthorized` error
    fn authenticate(&self, conn: &PgConnection) -> AppResult<AuthenticatedUser> {
+        let forwarded_host = self.headers().get("x-forwarded-host");
+        let forwarded_proto = self.headers().get("x-forwarded-proto");
+        let expected_origin = match (forwarded_host, forwarded_proto) {


I propose having a comma separated list of origins in an environment variable like WEB_ALLOWED_ORIGINS=http://localhost:8888,http://localhost:4200. Any Origin headers in the request would need to be on this list. The example above could be added to .env.sample so that either testing approach works.

Alternatively, you could fall back to self.host() if the other headers aren't set. Note that for this approach you don't want to use self.scheme() because that is for the local socket. You'd probably want something like if self.app().config.env == crate::Env::Production { "https" } else { "http" }.

jtgeibel · 2020-07-22T01:35:42Z

crates.io doesn't currently implement any POST endpoints, only PUT.
But that's not a documented security feature and could change in the
future.

I agree that this should be documented somewhere. We've changed several endpoints from POST to PUT in the past, and we should have guidelines to avoid adding any POST in the future. I'll be honest that I haven't looked at this doc in a while, but docs/PR-REVIEW.md looks like a good place to mention guidelines like this.

jsha · 2020-07-22T05:56:44Z

Hey @jsha, first I wanted to say thank you very much for your recent work reviewing our security stance.

You're very welcome! I've enjoyed it. I feel like I've learned a lot about Rust reading the code, and the team's been really great to work with.

Go to http://localhost:8888/

Ah, this is good to know. The docs in CONTRIBUTING.md describe running the Node server proxying to the backend on :4200.

As of right now I have things working on either :4200 or :8888, so whichever workflow people prefer should still continue to work. Note that part of the setup instructions includes setting the GitHub callback URL to http://localhost:4200; to make things work I had to generate a new GitHub app for :8888. It would probably be handy to consolidate on one of the two workflows in the docs.

Alternatively, you could fall back to self.host() if the other headers aren't set.

I wound up choosing this route. I think it's simpler; curious to see what you think. I decided to hardcode "http:" for the fallback case because we know that if the backend is not frontend by a proxy, it's not using HTTPS.

jtgeibel · 2020-07-23T01:28:24Z

Thanks! Works for me locally and in a staging area.

@bors r+

bors · 2020-07-23T01:28:25Z

📌 Commit 1540521 has been approved by jtgeibel

bors · 2020-07-23T01:28:33Z

⌛ Testing commit 1540521 with merge 8010898...

bors · 2020-07-23T01:34:24Z

☀️ Test successful - checks-travis
Approved by: jtgeibel
Pushing 8010898 to master...

This adapts the scheme added in rust-lang#2627. When operating behind CloudFront, the Host received by nginx is the Heroku instance and does not match the user facing crates.io origin. Therefore it is necessary to obtain a list of allowed origins at boot time. If this header is not set, the server will panic and will not boot.

@JohnTitor

Obtain allowed origin list from the environment This adapts the scheme added in #2627. When operating behind CloudFront, the Host received by nginx is the Heroku instance and does not match the user facing crates.io origin. Therefore it is necessary to obtain a list of allowed origins at boot time. If this header is not set, the server will panic and will not boot. r? @JohnTitor cc @jsha

rust-highfive assigned carols10cents Jul 12, 2020

rust-highfive added the S-waiting-on-review label Jul 12, 2020

Use x-forwarded-{host,proto} headers.

346cd98

Also, emit useful error when sameorigin check fails.

jtgeibel requested changes Jul 22, 2020

View reviewed changes

jsha added 3 commits July 21, 2020 22:08

Merge branch 'master' into sameorigin

04ddd4b

Revert init-local-index.

8523b79

Factor out verify_origin and handle no-proxy case.

1540521

bors merged commit 8010898 into rust-lang:master Jul 23, 2020

jbr mentioned this pull request Jul 26, 2020

Cannot generate new token - An error occurred while saving this token, must be logged in to perform that action #2658

Closed

jtgeibel mentioned this pull request Jul 26, 2020

Obtain allowed origin list from the environment #2659

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Restrict authentication to same-origin requests. #2627

Restrict authentication to same-origin requests. #2627

Uh oh!

jsha commented Jul 12, 2020

Uh oh!

rust-highfive commented Jul 12, 2020

Uh oh!

jsha commented Jul 12, 2020

Uh oh!

bors commented Jul 21, 2020

Uh oh!

jtgeibel left a comment

Uh oh!

jtgeibel Jul 22, 2020

Uh oh!

jtgeibel Jul 22, 2020

Uh oh!

jtgeibel commented Jul 22, 2020

Uh oh!

jsha commented Jul 22, 2020

Uh oh!

jtgeibel commented Jul 23, 2020

Uh oh!

bors commented Jul 23, 2020

Uh oh!

bors commented Jul 23, 2020

Uh oh!

bors commented Jul 23, 2020

Uh oh!

Uh oh!

Restrict authentication to same-origin requests. #2627

Restrict authentication to same-origin requests. #2627

Uh oh!

Conversation

jsha commented Jul 12, 2020

Uh oh!

rust-highfive commented Jul 12, 2020

Uh oh!

jsha commented Jul 12, 2020

Uh oh!

bors commented Jul 21, 2020

Uh oh!

jtgeibel left a comment

Choose a reason for hiding this comment

Uh oh!

jtgeibel Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

jtgeibel Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

jtgeibel commented Jul 22, 2020

Uh oh!

jsha commented Jul 22, 2020

Uh oh!

jtgeibel commented Jul 23, 2020

Uh oh!

bors commented Jul 23, 2020

Uh oh!

bors commented Jul 23, 2020

Uh oh!

bors commented Jul 23, 2020

Uh oh!

Uh oh!