Auto merge of #2627 - jsha:sameorigin, r=jtgeibel

Restrict authentication to same-origin requests.

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.

Author: bors

src/controllers/util.rs

@@ -6,6 +6,7 @@ use crate::models::{ApiToken, User};
 use crate::util::errors::{
     forbidden, internal, AppError, AppResult, ChainError, InsecurelyGeneratedTokenRevoked,
 };
+use conduit::Host;
 
 #[derive(Debug)]
 pub struct AuthenticatedUser {
@@ -28,9 +29,50 @@ impl AuthenticatedUser {
     }
 }
 
+// The Origin header (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Origin)
+// is sent with CORS requests and POST requests, and indicates where the request comes from.
+// We don't want to accept authenticated requests that originated from other sites, so this
+// function returns an error if the Origin header doesn't match what we expect "this site" to
+// be: https://crates.io in production, or http://localhost:port/ in development.
+fn verify_origin(req: &dyn RequestExt) -> AppResult<()> {
+    let headers = req.headers();
+    // If x-forwarded-host and -proto are present, trust those to tell us what the proto and host
+    // are; otherwise (in local dev) trust the Host header and the scheme.
+    let forwarded_host = headers.get("x-forwarded-host");
+    let forwarded_proto = headers.get("x-forwarded-proto");
+    let expected_origin = match (forwarded_host, forwarded_proto) {
+        (Some(host), Some(proto)) => format!(
+            "{}://{}",
+            proto.to_str().unwrap_or_default(),
+            host.to_str().unwrap_or_default()
+        ),
+        // For the default case we assume HTTP, because we know we're not serving behind a reverse
+        // proxy, and we also know that crates by itself doesn't serve HTTPS.
+        _ => match req.host() {
+            Host::Name(a) => format!("http://{}", a),
+            Host::Socket(a) => format!("http://{}", a.to_string()),
+        },
+    };
+
+    let bad_origin = headers
+        .get_all(header::ORIGIN)
+        .iter()
+        .find(|h| h.to_str().unwrap_or_default() != expected_origin);
+    if let Some(bad_origin) = bad_origin {
+        let error_message = format!(
+            "only same-origin requests can be authenticated. expected {}, got {:?}",
+            expected_origin, bad_origin
+        );
+        return Err(internal(&error_message))
+            .chain_error(|| Box::new(forbidden()) as Box<dyn AppError>);
+    }
+    Ok(())
+}
+
 impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
     /// Obtain `AuthenticatedUser` for the request or return an `Forbidden` error
     fn authenticate(&mut self) -> AppResult<AuthenticatedUser> {
+        verify_origin(self)?;
         if let Some(id) = self.extensions().find::<TrustedUserId>().map(|x| x.0) {
             log_request::add_custom_metadata(self, "uid", id);
             Ok(AuthenticatedUser {