Auto merge of #2637 - rust-lang:sg-security-advisory-2020-07-14, r=jtgeibel

Generate API tokens with a secure RNG, store hashed

Note: The contents of this PR have already been deployed to production, as this was a security issue. The code in this PR is the minimum number of changes needed to rebase what was deployed today onto master. Unfortunately, since this was done in parallel with some refactoring to the errors module, this means some of the added code doesn't look like the rest of the code base. However, master currently does not reflect what is in production, so any cleanup should come as a followup PR. The purpose of this is mainly to get master in sync with prod after going through bors.

This addresses a security issue with the way crates.io generates API
tokens. Prior to this commit, they were generated using the PostgreSQL
`random` function, which is not a secure PRNG. Additionally, the tokens
were stored in plain text, which would give an attacker who managed to
compromise our database access to all user's API tokens. This commit
addresses both changes.

An advisory about the problem was posted at
https://blog.rust-lang.org/2020/07/14/crates-io-security-advisory.html

The tokens are now generated using the OS's random number generator,
which maps to C's `getrandom` function, which is secure and
unpredictable.

The tokens are hashed using sha256. The choice to use a fast hashing
function such as this one instead of one typically used for passwords
(such as bcrypt or argon2) was intentional. Unlike passwords, our API
tokens are known to be 32 characters and are truly random, giving us 192
bits of entropy. This means that even with a fast hashing function,
actually finding a token from that hash before the death of human
civilization is infeasible.

Additionally, unlike passwords, API tokens need to be checked on every
request where they're used, instead of once at sign in. This means that
a slower hashing function would put significantly more load on our
server than they would when used for passwords.

We opted to use sha256 instead of bcrypt with a lower cost due to the
mandatory salt in bcrypt. If we salt the values before hashing them, the
tokens can no longer directly be used to identify themselves, and we
would need to include another identifier in the token given to the user.
While this is feasible, it leads to a very obtuse looking token, and
more complex code.

r? @jtgeibel

Author: bors

migrations/2020-07-13-125308_remove_api_token_default/down.sql

@@ -0,0 +1 @@
+ALTER TABLE api_tokens ALTER COLUMN token SET DEFAULT random_string(32);

migrations/2020-07-13-125308_remove_api_token_default/up.sql

@@ -0,0 +1 @@
+ALTER TABLE api_tokens ALTER COLUMN token DROP DEFAULT;

migrations/2020-07-13-182546_remove_api_tokens_index/down.sql

@@ -0,0 +1,2 @@
+CREATE INDEX api_tokens_token_idx ON api_tokens (token);
+ALTER TABLE api_tokens ADD CONSTRAINT api_tokens_token_key UNIQUE (token);

migrations/2020-07-13-182546_remove_api_tokens_index/up.sql

@@ -0,0 +1,2 @@
+ALTER TABLE api_tokens DROP CONSTRAINT api_tokens_token_key;
+DROP INDEX api_tokens_token_idx;

migrations/2020-07-14-113112_change_api_token_to_bytea/down.sql

@@ -0,0 +1,4 @@
+ALTER TABLE api_tokens
+  ALTER COLUMN token
+  TYPE text USING encode(token, 'escape');
+DROP INDEX api_tokens_token_idx;

migrations/2020-07-14-113112_change_api_token_to_bytea/up.sql

@@ -0,0 +1,5 @@
+CREATE EXTENSION IF NOT EXISTS pgcrypto;
+ALTER TABLE api_tokens
+  ALTER COLUMN token
+  TYPE bytea USING decode(token, 'escape');
+CREATE UNIQUE INDEX ON api_tokens (token);

script/ci/prune-cache.sh

@@ -12,7 +12,7 @@ du -hs target/debug
 
 crate_name="cargo-registry"
 test_name="all"
-bin_names="background-worker delete-crate delete-version enqueue-job monitor populate render-readmes server test-pagerduty transfer-crates"
+bin_names="background-worker delete-crate delete-version enqueue-job monitor populate render-readmes server test-pagerduty transfer-crates verify-token"
 
 normalized_crate_name=${crate_name//-/_}
 rm -vf target/debug/$normalized_crate_name-*

src/bin/verify-token.rs

@@ -0,0 +1,14 @@
+// Look up a username by API token. Used by staff to verify someone's identity
+// by having an API token given. If an error occurs, including being unable to
+// find a user with that API token, the error will be displayed.
+
+use cargo_registry::{db, models::User, util::errors::AppResult};
+use std::env;
+
+fn main() -> AppResult<()> {
+    let conn = db::connect_now()?;
+    let token = env::args().nth(1).expect("API token argument required");
+    let user = User::find_by_api_token(&conn, &token)?;
+    println!("The token belongs to user {}", user.gh_login);
+    Ok(())
+}

src/controllers/util.rs

@@ -2,7 +2,9 @@ use super::prelude::*;
 
 use crate::middleware::current_user::TrustedUserId;
 use crate::models::{ApiToken, User};
-use crate::util::errors::{forbidden, internal, AppResult, ChainError};
+use crate::util::errors::{
+    forbidden, internal, AppError, AppResult, ChainError, InsecurelyGeneratedTokenRevoked,
+};
 
 #[derive(Debug)]
 pub struct AuthenticatedUser {
@@ -42,8 +44,13 @@ impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
                         user_id: token.user_id,
                         token_id: Some(token.id),
                     })
-                    .chain_error(|| internal("invalid token"))
-                    .chain_error(forbidden)
+                    .map_err(|e| {
+                        if e.is::<InsecurelyGeneratedTokenRevoked>() {
+                            e
+                        } else {
+                            e.chain(internal("invalid token")).chain(forbidden())
+                        }
+                    })
             } else {
                 // Unable to authenticate the user
                 Err(internal("no cookie session or auth header found")).chain_error(forbidden)

src/models.rs

@@ -11,7 +11,7 @@ pub use self::krate::{Crate, CrateVersions, NewCrate, RecentCrateDownloads};
 pub use self::owner::{CrateOwner, Owner, OwnerKind};
 pub use self::rights::Rights;
 pub use self::team::{NewTeam, Team};
-pub use self::token::ApiToken;
+pub use self::token::{ApiToken, CreatedApiToken};
 pub use self::user::{NewUser, User};
 pub use self::version::{NewVersion, Version};
 

src/models/token.rs

@@ -1,20 +1,26 @@
 use chrono::NaiveDateTime;
 use diesel::prelude::*;
+use diesel::sql_types::{Bytea, Text};
 
 use crate::models::User;
 use crate::schema::api_tokens;
+use crate::util::errors::{AppResult, InsecurelyGeneratedTokenRevoked};
 use crate::util::rfc3339;
 use crate::views::EncodableApiTokenWithToken;
 
+const TOKEN_LENGTH: usize = 32;
+const TOKEN_PREFIX: &str = "cio"; // Crates.IO
+
 /// The model representing a row in the `api_tokens` database table.
 #[derive(Clone, Debug, PartialEq, Eq, Identifiable, Queryable, Associations, Serialize)]
 #[belongs_to(User)]
 pub struct ApiToken {
     pub id: i32,
     #[serde(skip)]
     pub user_id: i32,
+    // Nothing should ever access the plaintext token.
     #[serde(skip)]
-    pub token: String,
+    token: Vec<u8>,
     pub name: String,
     #[serde(with = "rfc3339")]
     pub created_at: NaiveDateTime,
@@ -24,36 +30,41 @@ pub struct ApiToken {
     pub revoked: bool,
 }
 
+diesel::sql_function! {
+    fn digest(input: Text, method: Text) -> Bytea;
+}
+
 impl ApiToken {
     /// Generates a new named API token for a user
-    pub fn insert(conn: &PgConnection, user_id: i32, name: &str) -> QueryResult<ApiToken> {
-        diesel::insert_into(api_tokens::table)
-            .values((api_tokens::user_id.eq(user_id), api_tokens::name.eq(name)))
-            .get_result::<ApiToken>(conn)
-    }
+    pub fn insert(conn: &PgConnection, user_id: i32, name: &str) -> AppResult<CreatedApiToken> {
+        let plaintext = format!(
+            "{}{}",
+            TOKEN_PREFIX,
+            crate::util::generate_secure_alphanumeric_string(TOKEN_LENGTH)
+        );
 
-    /// Converts this `ApiToken` model into an `EncodableApiToken` including
-    /// the actual token value for JSON serialization.  This should only be
-    /// used when initially creating a new token to minimize the chance of
-    /// token leaks.
-    pub fn encodable_with_token(self) -> EncodableApiTokenWithToken {
-        EncodableApiTokenWithToken {
-            id: self.id,
-            name: self.name,
-            token: self.token,
-            revoked: self.revoked,
-            created_at: self.created_at,
-            last_used_at: self.last_used_at,
-        }
+        let model = diesel::insert_into(api_tokens::table)
+            .values((
+                api_tokens::user_id.eq(user_id),
+                api_tokens::name.eq(name),
+                api_tokens::token.eq(digest(&plaintext, "sha256")),
+            ))
+            .get_result::<ApiToken>(conn)?;
+
+        Ok(CreatedApiToken { plaintext, model })
     }
 
-    pub fn find_by_api_token(conn: &PgConnection, token_: &str) -> QueryResult<ApiToken> {
-        use crate::schema::api_tokens::dsl::{api_tokens, last_used_at, revoked, token};
+    pub fn find_by_api_token(conn: &PgConnection, token_: &str) -> AppResult<ApiToken> {
+        use crate::schema::api_tokens::dsl::*;
         use diesel::{dsl::now, update};
 
+        if !token_.starts_with(TOKEN_PREFIX) {
+            return Err(InsecurelyGeneratedTokenRevoked::boxed());
+        }
+
         let tokens = api_tokens
-            .filter(token.eq(token_))
-            .filter(revoked.eq(false));
+            .filter(revoked.eq(false))
+            .filter(token.eq(digest(token_, "sha256")));
 
         // If the database is in read only mode, we can't update last_used_at.
         // Try updating in a new transaction, if that fails, fall back to reading
@@ -63,6 +74,37 @@ impl ApiToken {
                 .get_result::<ApiToken>(conn)
         })
         .or_else(|_| tokens.first(conn))
+        .map_err(Into::into)
+    }
+}
+
+pub struct CreatedApiToken {
+    pub model: ApiToken,
+    pub plaintext: String,
+}
+
+impl CreatedApiToken {
+    /// Converts this `CreatedApiToken` into an `EncodableApiToken` including
+    /// the actual token value for JSON serialization.
+    pub fn encodable_with_token(self) -> EncodableApiTokenWithToken {
+        EncodableApiTokenWithToken {
+            id: self.model.id,
+            name: self.model.name,
+            token: self.plaintext,
+            revoked: self.model.revoked,
+            created_at: self.model.created_at,
+            last_used_at: self.model.last_used_at,
+        }
+    }
+}
+
+// Use a custom implementation of Debug to hide the plaintext token.
+impl std::fmt::Debug for CreatedApiToken {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("CreatedApiToken")
+            .field("model", &self.model)
+            .field("plaintext", &"(sensitive)")
+            .finish()
     }
 }
 
@@ -71,12 +113,20 @@ mod tests {
     use super::*;
     use chrono::NaiveDate;
 
+    #[test]
+    fn test_token_constants() {
+        // Changing this by itself will implicitly revoke all existing tokens.
+        // If this test needs to be change, make sure you're handling tokens
+        // with the old prefix or that you wanted to revoke them.
+        assert_eq!("cio", TOKEN_PREFIX);
+    }
+
     #[test]
     fn api_token_serializes_to_rfc3339() {
         let tok = ApiToken {
             id: 12345,
             user_id: 23456,
-            token: "".to_string(),
+            token: Vec::new(),
             revoked: false,
             name: "".to_string(),
             created_at: NaiveDate::from_ymd(2017, 1, 6).and_hms(14, 23, 11),

src/models/user.rs

@@ -110,10 +110,10 @@ impl User {
     }
 
     /// Queries the database for a user with a certain `api_token` value.
-    pub fn find_by_api_token(conn: &PgConnection, token: &str) -> QueryResult<User> {
+    pub fn find_by_api_token(conn: &PgConnection, token: &str) -> AppResult<User> {
         let api_token = ApiToken::find_by_api_token(conn, token)?;
 
-        Self::find(conn, api_token.user_id)
+        Ok(Self::find(conn, api_token.user_id)?)
     }
 
     pub fn owning(krate: &Crate, conn: &PgConnection) -> QueryResult<Vec<Owner>> {

src/schema.rs

@@ -22,10 +22,10 @@ table! {
         user_id -> Int4,
         /// The `token` column of the `api_tokens` table.
         ///
-        /// Its SQL type is `Varchar`.
+        /// Its SQL type is `Bytea`.
         ///
         /// (Automatically generated by Diesel.)
-        token -> Varchar,
+        token -> Bytea,
         /// The `name` column of the `api_tokens` table.
         ///
         /// Its SQL type is `Varchar`.

src/tests/authentication.rs

@@ -36,7 +36,7 @@ fn anonymous_user_unauthorized() {
 fn token_auth_cannot_find_token() {
     let (app, anon) = TestApp::init().empty();
     let mut request = anon.request_builder(Method::GET, URL);
-    request.header(header::AUTHORIZATION, "fake-token");
+    request.header(header::AUTHORIZATION, "cio1tkfake-token");
 
     let (status, body) = into_parts(call(&app, request));
     assert_eq!(status, StatusCode::FORBIDDEN);

src/tests/krate.rs

@@ -725,7 +725,7 @@ fn new_wrong_token() {
     // Try to publish with the wrong token (by changing the token in the database)
     app.db(|conn| {
         diesel::update(api_tokens::table)
-            .set(api_tokens::token.eq("bad"))
+            .set(api_tokens::token.eq(b"bad" as &[u8]))
             .execute(conn)
             .unwrap();
     });

src/tests/token.rs

@@ -1,4 +1,8 @@
-use crate::{user::UserShowPrivateResponse, util::StatusCode, RequestHelper, TestApp};
+use crate::{
+    user::UserShowPrivateResponse,
+    util::{header, StatusCode},
+    RequestHelper, TestApp,
+};
 use cargo_registry::{
     models::ApiToken,
     schema::api_tokens,
@@ -67,7 +71,10 @@ fn list_tokens() {
             .into_iter()
             .map(|t| t.name)
             .collect::<HashSet<_>>(),
-        tokens.into_iter().map(|t| t.name).collect::<HashSet<_>>()
+        tokens
+            .into_iter()
+            .map(|t| t.model.name)
+            .collect::<HashSet<_>>()
     );
 }
 
@@ -88,7 +95,7 @@ fn list_tokens_exclude_revoked() {
 
     // Revoke the first token.
     let _json: RevokedResponse = user
-        .delete(&format!("/api/v1/me/tokens/{}", tokens[0].id))
+        .delete(&format!("/api/v1/me/tokens/{}", tokens[0].model.id))
         .good();
 
     // Check that we now have one less token being listed.
@@ -97,7 +104,7 @@ fn list_tokens_exclude_revoked() {
     assert!(json
         .api_tokens
         .iter()
-        .find(|token| token.name == tokens[0].name)
+        .find(|token| token.name == tokens[0].model.name)
         .is_none());
 }
 
@@ -167,7 +174,6 @@ fn create_token_success() {
     let tokens = app.db(|conn| t!(ApiToken::belonging_to(user.as_model()).load::<ApiToken>(conn)));
     assert_eq!(tokens.len(), 1);
     assert_eq!(tokens[0].name, "bar");
-    assert_eq!(tokens[0].token, json.api_token.token);
     assert_eq!(tokens[0].revoked, false);
     assert_eq!(tokens[0].last_used_at, None);
 }
@@ -297,3 +303,17 @@ fn using_token_updates_last_used_at() {
     // based on the start of the database transaction so it doesn't work in
     // this test framework.
 }
+
+#[test]
+fn old_tokens_give_specific_error_message() {
+    let url = "/api/v1/me";
+    let (_, anon) = TestApp::init().empty();
+
+    let mut request = anon.get_request(url);
+    request.header(header::AUTHORIZATION, "oldtoken");
+    let json = anon
+        .run::<()>(request)
+        .bad_with_status(StatusCode::UNAUTHORIZED);
+
+    assert_contains!(json.errors[0].detail, "revoked");
+}

src/tests/user.rs

@@ -355,7 +355,7 @@ fn user_total_downloads_no_crates() {
 fn updating_existing_user_doesnt_change_api_token() {
     let (app, _, user, token) = TestApp::init().with_token();
     let gh_id = user.as_model().gh_id;
-    let token = &token.as_model().token;
+    let token = token.plaintext();
 
     let user = app.db(|conn| {
         // Reuse gh_id but use new gh_login and gh_access_token