From 336e176f38983bee45fe795ed9e672e446dbfbf9 Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Thu, 9 Apr 2020 15:35:35 -0400 Subject: [PATCH 1/7] Replace pbkdf2 by Argon2 in registrar --- Contributors.txt | 1 + oxide-auth/Cargo.toml | 2 + oxide-auth/src/lib.rs | 2 + oxide-auth/src/primitives/registrar.rs | 137 +++++++++---------------- 4 files changed, 52 insertions(+), 90 deletions(-) diff --git a/Contributors.txt b/Contributors.txt index 4909d571..fe4c4de5 100644 --- a/Contributors.txt +++ b/Contributors.txt @@ -4,3 +4,4 @@ HeroicKatora (initial author) ParisLiakos (Paris Liakos, gotham frontend) asonix (actix 1.0 update) +TheSpooler \ No newline at end of file diff --git a/oxide-auth/Cargo.toml b/oxide-auth/Cargo.toml index 3b762a53..a8b76dbe 100644 --- a/oxide-auth/Cargo.toml +++ b/oxide-auth/Cargo.toml @@ -14,8 +14,10 @@ license = "MIT OR Apache-2.0" autoexamples = false [dependencies] +argonautica = "0.2.0" base64 = "0.11" chrono = "0.4.2" +lazy_static = "1.4.0" serde = "1.0" serde_derive = "1.0" serde_json = "1.0" diff --git a/oxide-auth/src/lib.rs b/oxide-auth/src/lib.rs index 3d0c5a5d..b18c4dcf 100644 --- a/oxide-auth/src/lib.rs +++ b/oxide-auth/src/lib.rs @@ -65,8 +65,10 @@ //! [`Scopes`]: endpoint/trait.Scopes.html #![warn(missing_docs)] +extern crate argonautica; extern crate base64; extern crate chrono; +extern crate lazy_static; extern crate url; extern crate ring; extern crate rmp_serde; diff --git a/oxide-auth/src/primitives/registrar.rs b/oxide-auth/src/primitives/registrar.rs index f29a3594..16a1f3be 100644 --- a/oxide-auth/src/primitives/registrar.rs +++ b/oxide-auth/src/primitives/registrar.rs @@ -10,14 +10,16 @@ use std::cmp; use std::collections::HashMap; use std::fmt; use std::iter::{Extend, FromIterator}; -use std::num::NonZeroU32; use std::sync::{Arc, MutexGuard, RwLockWriteGuard}; use std::rc::Rc; +use argonautica::{ + Hasher, + input::SecretKey, + Verifier +}; +use lazy_static::lazy_static; use url::Url; -use ring::{digest, pbkdf2}; -use ring::error::Unspecified; -use ring::rand::{SystemRandom, SecureRandom}; /// Registrars provie a way to interact with clients. /// @@ -188,12 +190,6 @@ impl fmt::Debug for ClientType { } } -impl RegistrarError { - fn from(err: Unspecified) -> Self { - match err { Unspecified => RegistrarError::Unspecified } - } -} - impl Client { /// Create a public client. pub fn public(client_id: &str, redirect_uri: Url, default_scope: Scope) -> Client { @@ -281,8 +277,7 @@ impl cmp::PartialOrd for PreGrant { /// Determines how passphrases are stored and checked. /// -/// The provided library implementation is based on `Pbkdf2`. Other users may prefer to write their -/// own adaption with `Argon2`. If you do so, you could send a pull request my way. +/// The provided library implementation is based on `Argon2`. pub trait PasswordPolicy: Send + Sync { /// Transform the passphrase so it can be stored in the confidential client. fn store(&self, client_id: &str, passphrase: &[u8]) -> Vec; @@ -291,100 +286,58 @@ pub trait PasswordPolicy: Send + Sync { fn check(&self, client_id: &str, passphrase: &[u8], stored: &[u8]) -> Result<(), RegistrarError>; } -/// Store passwords using `Pbkdf2` to derive the stored value. -/// -/// Each instantiation generates a 16 byte random salt and prepends this additionally with the -/// username. This combined string is then used as the salt using the passphrase as the secret to -/// derive the output. The iteration count defaults to `65536` but can be customized. -pub struct Pbkdf2 { - /// A prebuilt random, or constructing one as needed. - random: Option, - iterations: NonZeroU32, -} - -impl Default for Pbkdf2 { - fn default() -> Self { - Pbkdf2 { - random: Some(SystemRandom::new()), - .. *PBKDF2_DEFAULTS - } - } +/// Store passwords using `Argon2` to derive the stored value. +#[derive(Debug)] +pub struct Argon2<'a> { + secret_key: SecretKey<'a> } -impl Clone for Pbkdf2 { - fn clone(&self) -> Self { - Pbkdf2 { - random: Some(SystemRandom::new()), - .. *self +impl<'a> Default for Argon2<'a> { + fn default() -> Self { + Argon2 { + secret_key: "Please provide a SecretKey! This is for your own protection.".into() } } } -impl fmt::Debug for Pbkdf2 { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.debug_struct("Pbkdf2") - .field("iterations", &self.iterations) - .field("random", &()) - .finish() +impl From for RegistrarError { + fn from(_err: std::string::FromUtf8Error) -> Self { + RegistrarError::PrimitiveError } } -impl Pbkdf2 { - /// Set the iteration count to `(1 << strength)`. - /// - /// This function will panic when the `strength` is larger or equal to `32`. - pub fn set_relative_strength(&mut self, strength: u8) { - assert!(strength < 32, "Strength value out of range (0-31): {}", strength); - self.iterations = NonZeroU32::new(1u32 << strength).unwrap(); - } - - fn salt(&self, user_identifier: &[u8]) -> Vec { - let mut vec = Vec::with_capacity(user_identifier.len() + 64); - let mut rnd_salt = [0; 16]; - - match self.random.as_ref() { - Some(random) => random.fill(&mut rnd_salt), - None => SystemRandom::new().fill(&mut rnd_salt), - }.expect("Failed to property initialize password storage salt"); - - vec.extend_from_slice(user_identifier); - vec.extend_from_slice(&rnd_salt[..]); - vec +impl From for RegistrarError { + fn from(_err: argonautica::Error) -> Self { + RegistrarError::Unspecified } } -// A default instance for pbkdf2, randomness is sampled from the system each time. -// -// TODO: in the future there might be a way to get static memory initialized with an rng at load -// time by the loader. Then, a constant instance of the random generator may be available and we -// could get rid of the `Option`. -static PBKDF2_DEFAULTS: &Pbkdf2 = &Pbkdf2 { - random: None, - iterations: unsafe { NonZeroU32::new_unchecked(1 << 16) }, -}; - -impl PasswordPolicy for Pbkdf2 { +impl<'a> PasswordPolicy for Argon2<'a> { fn store(&self, client_id: &str, passphrase: &[u8]) -> Vec { - let mut output = vec![0; 64]; - output.append(&mut self.salt(client_id.as_bytes())); - { - let (output, salt) = output.split_at_mut(64); - pbkdf2::derive(&digest::SHA256, self.iterations.into(), salt, passphrase, - output); - } - output + let mut hasher = Hasher::default(); + hasher + .with_secret_key(&self.secret_key) + .with_salt(client_id) + .with_password(passphrase) + .hash() + .unwrap() + .into_bytes() } fn check(&self, _client_id: &str /* Was interned */, passphrase: &[u8], stored: &[u8]) -> Result<(), RegistrarError> { - if stored.len() < 64 { - return Err(RegistrarError::PrimitiveError) - } - - let (verifier, salt) = stored.split_at(64); - pbkdf2::verify(&digest::SHA256, self.iterations.into(), salt, passphrase, verifier) + let mut verifier = Verifier::default(); + String::from_utf8(stored.to_vec()) .map_err(RegistrarError::from) + .and_then(|hash| + verifier + .with_hash(hash) + .with_password(passphrase) + .with_secret_key(&self.secret_key) + .verify() + .map_err(RegistrarError::from)) + .and_then(|valid| if valid { Ok(()) } else { Err(RegistrarError::Unspecified) } ) } } @@ -392,6 +345,10 @@ impl PasswordPolicy for Pbkdf2 { // Standard Implementations of Registrars // /////////////////////////////////////////////////////////////////////////////////////////////////// +lazy_static! { + static ref DEFAULT_PASSWORD_POLICY: Argon2<'static> = { Argon2::default() }; +} + impl ClientMap { /// Create an empty map without any clients in it. pub fn new() -> ClientMap { @@ -413,7 +370,7 @@ impl ClientMap { fn current_policy<'a>(policy: &'a Option>) -> &'a dyn PasswordPolicy { policy .as_ref().map(|boxed| &**boxed) - .unwrap_or(PBKDF2_DEFAULTS) + .unwrap_or(&*DEFAULT_PASSWORD_POLICY) } } @@ -616,7 +573,7 @@ mod tests { #[test] fn public_client() { - let policy = Pbkdf2::default(); + let policy = Argon2::default(); let client = Client::public( "ClientId", "https://example.com".parse().unwrap(), @@ -632,7 +589,7 @@ mod tests { #[test] fn confidential_client() { - let policy = Pbkdf2::default(); + let policy = Argon2::default(); let pass = b"AB3fAj6GJpdxmEVeNCyPoA=="; let client = Client::confidential( "ClientId", From 04608f84f04c89f81d34c1a100467e57e01ecb74 Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Thu, 9 Apr 2020 17:23:13 -0400 Subject: [PATCH 2/7] lazy_static, Option and remove leaky impls --- oxide-auth/Cargo.toml | 2 +- oxide-auth/src/lib.rs | 4 +-- oxide-auth/src/primitives/registrar.rs | 46 ++++++++++++-------------- 3 files changed, 24 insertions(+), 28 deletions(-) diff --git a/oxide-auth/Cargo.toml b/oxide-auth/Cargo.toml index a8b76dbe..a66f9c81 100644 --- a/oxide-auth/Cargo.toml +++ b/oxide-auth/Cargo.toml @@ -17,7 +17,7 @@ autoexamples = false argonautica = "0.2.0" base64 = "0.11" chrono = "0.4.2" -lazy_static = "1.4.0" +once_cell = "1.3.1" serde = "1.0" serde_derive = "1.0" serde_json = "1.0" diff --git a/oxide-auth/src/lib.rs b/oxide-auth/src/lib.rs index b18c4dcf..8eeeceb5 100644 --- a/oxide-auth/src/lib.rs +++ b/oxide-auth/src/lib.rs @@ -68,14 +68,14 @@ extern crate argonautica; extern crate base64; extern crate chrono; -extern crate lazy_static; -extern crate url; +extern crate once_cell; extern crate ring; extern crate rmp_serde; extern crate serde; #[macro_use] extern crate serde_derive; extern crate serde_json; +extern crate url; pub mod code_grant; pub mod endpoint; diff --git a/oxide-auth/src/primitives/registrar.rs b/oxide-auth/src/primitives/registrar.rs index 16a1f3be..38931e8a 100644 --- a/oxide-auth/src/primitives/registrar.rs +++ b/oxide-auth/src/primitives/registrar.rs @@ -18,7 +18,7 @@ use argonautica::{ input::SecretKey, Verifier }; -use lazy_static::lazy_static; +use once_cell::sync::Lazy; use url::Url; /// Registrars provie a way to interact with clients. @@ -289,34 +289,27 @@ pub trait PasswordPolicy: Send + Sync { /// Store passwords using `Argon2` to derive the stored value. #[derive(Debug)] pub struct Argon2<'a> { - secret_key: SecretKey<'a> + secret_key: Option> } impl<'a> Default for Argon2<'a> { fn default() -> Self { Argon2 { - secret_key: "Please provide a SecretKey! This is for your own protection.".into() + secret_key: None } } } -impl From for RegistrarError { - fn from(_err: std::string::FromUtf8Error) -> Self { - RegistrarError::PrimitiveError - } -} - -impl From for RegistrarError { - fn from(_err: argonautica::Error) -> Self { - RegistrarError::Unspecified - } -} - impl<'a> PasswordPolicy for Argon2<'a> { fn store(&self, client_id: &str, passphrase: &[u8]) -> Vec { let mut hasher = Hasher::default(); + let hasher = if let Some(secret_key) = &self.secret_key { + hasher.with_secret_key(secret_key) + } else { + hasher.opt_out_of_secret_key(true) + }; + hasher - .with_secret_key(&self.secret_key) .with_salt(client_id) .with_password(passphrase) .hash() @@ -328,15 +321,20 @@ impl<'a> PasswordPolicy for Argon2<'a> { -> Result<(), RegistrarError> { let mut verifier = Verifier::default(); + let verifier = if let Some(secret_key) = &self.secret_key { + verifier.with_secret_key(secret_key) + } else { + &mut verifier + }; + String::from_utf8(stored.to_vec()) - .map_err(RegistrarError::from) + .map_err(|_| RegistrarError::PrimitiveError) .and_then(|hash| verifier - .with_hash(hash) - .with_password(passphrase) - .with_secret_key(&self.secret_key) - .verify() - .map_err(RegistrarError::from)) + .with_hash(hash) + .with_password(passphrase) + .verify() + .map_err(|_| RegistrarError::Unspecified )) .and_then(|valid| if valid { Ok(()) } else { Err(RegistrarError::Unspecified) } ) } } @@ -345,9 +343,7 @@ impl<'a> PasswordPolicy for Argon2<'a> { // Standard Implementations of Registrars // /////////////////////////////////////////////////////////////////////////////////////////////////// -lazy_static! { - static ref DEFAULT_PASSWORD_POLICY: Argon2<'static> = { Argon2::default() }; -} +static DEFAULT_PASSWORD_POLICY: Lazy> = Lazy::new(|| { Argon2::default() }); impl ClientMap { /// Create an empty map without any clients in it. From 37f3b8e04d757cfd8647dcf21b9855cdf4579a6b Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Thu, 9 Apr 2020 17:45:42 -0400 Subject: [PATCH 3/7] Remove lifetime from Argon2 --- oxide-auth/src/primitives/registrar.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/oxide-auth/src/primitives/registrar.rs b/oxide-auth/src/primitives/registrar.rs index 38931e8a..211b6703 100644 --- a/oxide-auth/src/primitives/registrar.rs +++ b/oxide-auth/src/primitives/registrar.rs @@ -288,11 +288,11 @@ pub trait PasswordPolicy: Send + Sync { /// Store passwords using `Argon2` to derive the stored value. #[derive(Debug)] -pub struct Argon2<'a> { - secret_key: Option> +pub struct Argon2 { + secret_key: Option> } -impl<'a> Default for Argon2<'a> { +impl Default for Argon2 { fn default() -> Self { Argon2 { secret_key: None @@ -300,7 +300,7 @@ impl<'a> Default for Argon2<'a> { } } -impl<'a> PasswordPolicy for Argon2<'a> { +impl PasswordPolicy for Argon2 { fn store(&self, client_id: &str, passphrase: &[u8]) -> Vec { let mut hasher = Hasher::default(); let hasher = if let Some(secret_key) = &self.secret_key { @@ -343,7 +343,7 @@ impl<'a> PasswordPolicy for Argon2<'a> { // Standard Implementations of Registrars // /////////////////////////////////////////////////////////////////////////////////////////////////// -static DEFAULT_PASSWORD_POLICY: Lazy> = Lazy::new(|| { Argon2::default() }); +static DEFAULT_PASSWORD_POLICY: Lazy = Lazy::new(|| { Argon2::default() }); impl ClientMap { /// Create an empty map without any clients in it. From d9465752f88e7f7ad5ee87eb0ad7ed0b347d7c0e Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Fri, 10 Apr 2020 09:47:54 -0400 Subject: [PATCH 4/7] Fixing cirrus ci --- .cirrus.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.cirrus.yml b/.cirrus.yml index 2ced0550..3005afa5 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -1,6 +1,9 @@ stable_task: container: image: rust:latest + packages_script: + - apt-get update + - apt-get install -y clang libclang-dev cargo_cache: folder: $CARGO_HOME/registry fingerprint_script: cargo update && cat Cargo.lock @@ -18,6 +21,9 @@ stable_task: nightly_task: container: image: rustlang/rust:nightly + packages_script: + - apt-get update + - apt-get install -y clang libclang-dev cargo_cache: folder: $CARGO_HOME/registry fingerprint_script: cargo update && cat Cargo.lock @@ -36,6 +42,9 @@ nightly_task: ring_task: container: image: rust:latest + packages_script: + - apt-get update + - apt-get install -y clang libclang-dev ring_013_script: pushd tests/ring-0.13 && cargo build -v && popd ring_014_script: pushd tests/ring-0.14 && cargo build -v && popd @@ -48,4 +57,7 @@ release_task: doc_task: container: image: rustlang/rust:nightly + packages_script: + - apt-get update + - apt-get install -y clang libclang-dev script: cargo doc --no-deps --document-private-items --all-features From cdd031474118cb2992a35abf426e153e058ea2ac Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Sat, 11 Apr 2020 15:21:33 -0400 Subject: [PATCH 5/7] Removed ring from generator --- oxide-auth/Cargo.toml | 3 + oxide-auth/src/lib.rs | 5 +- oxide-auth/src/primitives/generator.rs | 80 ++++++++++++++++---------- 3 files changed, 56 insertions(+), 32 deletions(-) diff --git a/oxide-auth/Cargo.toml b/oxide-auth/Cargo.toml index 3b762a53..76ddd744 100644 --- a/oxide-auth/Cargo.toml +++ b/oxide-auth/Cargo.toml @@ -16,9 +16,12 @@ autoexamples = false [dependencies] base64 = "0.11" chrono = "0.4.2" +hmac = "0.7.1" serde = "1.0" serde_derive = "1.0" serde_json = "1.0" +sha2 = "0.8.1" +rand = "0.7.3" ring = ">=0.13,<0.15" rmp-serde = "0.14" url = "1.7" diff --git a/oxide-auth/src/lib.rs b/oxide-auth/src/lib.rs index 3d0c5a5d..93152c6c 100644 --- a/oxide-auth/src/lib.rs +++ b/oxide-auth/src/lib.rs @@ -67,13 +67,16 @@ extern crate base64; extern crate chrono; -extern crate url; +extern crate hmac; +extern crate rand; extern crate ring; extern crate rmp_serde; extern crate serde; #[macro_use] extern crate serde_derive; extern crate serde_json; +extern crate sha2; +extern crate url; pub mod code_grant; pub mod endpoint; diff --git a/oxide-auth/src/primitives/generator.rs b/oxide-auth/src/primitives/generator.rs index 8405ba5a..9a5ea72e 100644 --- a/oxide-auth/src/primitives/generator.rs +++ b/oxide-auth/src/primitives/generator.rs @@ -9,6 +9,7 @@ //! - `Assertion` cryptographically verifies the integrity of a token, trading security without //! persistent storage for the loss of revocability. It is thus unfit for some backends, which //! is not currently expressed in the type system or with traits. + use super::grant::{Value, Extensions, Grant}; use super::{Url, Time}; use super::scope::Scope; @@ -18,9 +19,8 @@ use std::rc::Rc; use std::sync::Arc; use base64::{encode, decode}; -use ring::digest::SHA256; -use ring::rand::{SystemRandom, SecureRandom}; -use ring::hmac::SigningKey; +use hmac::{crypto_mac::MacResult, Mac, Hmac}; +use rand::{rngs::OsRng, RngCore, thread_rng}; use rmp_serde; /// Generic token for a specific grant. @@ -49,7 +49,7 @@ pub trait TagGrant { /// Each byte is chosen randomly from the basic `rand::thread_rng`. This generator will always /// succeed. pub struct RandomGenerator { - random: SystemRandom, + random: OsRng, len: usize } @@ -57,14 +57,14 @@ impl RandomGenerator { /// Generates tokens with a specific byte length. pub fn new(length: usize) -> RandomGenerator { RandomGenerator { - random: SystemRandom::new(), + random: OsRng {}, len: length } } - fn generate(&self) -> String { + fn generate(&mut self) -> String { let mut result = vec![0; self.len]; - self.random.fill(result.as_mut_slice()) + self.random.try_fill_bytes(result.as_mut_slice()) .expect("Failed to generate random token"); encode(&result) } @@ -80,7 +80,7 @@ impl RandomGenerator { /// signing the same grant for different uses, i.e. separating authorization from bearer grants and /// refresh tokens. pub struct Assertion { - secret: SigningKey, + key: Vec } /// The cryptographic suite ensuring integrity of tokens. @@ -132,24 +132,24 @@ impl Assertion { /// padding or shortening of the supplied key material may be applied in the form dictated by /// the signature type. See the respective standards. /// - /// If future suites are added where this is not possible, his function may panic when supplied + /// If future suites are added where this is not possible, this function may panic when supplied /// with an incorrect key length. + /// + /// Currently, the implementation lacks the ability to really make use of another hasing mechanism than + /// hmac + sha256. pub fn new(kind: AssertionKind, key: &[u8]) -> Self { - let key = match kind { - AssertionKind::HmacSha256 => SigningKey::new(&SHA256, key), + match kind { + AssertionKind::HmacSha256 => Assertion { key: key.to_vec() }, AssertionKind::__NonExhaustive => unreachable!(), - }; - - Assertion { - secret: key, } } /// Construct an assertion instance whose tokens are only valid for the program execution. pub fn ephemeral() -> Self { - Assertion { - secret: SigningKey::generate(&SHA256, &SystemRandom::new()).unwrap(), - } + // XXX Why 64? + let mut rand_bytes: [u8; 64] = [0;64]; + thread_rng().fill_bytes(&mut rand_bytes); + Assertion { key: rand_bytes.to_vec() } } /// Get a reference to generator for the given tag. @@ -160,14 +160,21 @@ impl Assertion { fn extract<'a>(&self, token: &'a str) -> Result<(Grant, String), ()> { let decoded = decode(token).map_err(|_| ())?; let assertion: AssertGrant = rmp_serde::from_slice(&decoded).map_err(|_| ())?; - ring::hmac::verify_with_own_key(&self.secret, &assertion.0, &assertion.1).map_err(|_| ())?; + + let mut hmac = Hmac::::new_varkey(&self.key).unwrap(); + hmac.input(&assertion.0); + hmac.verify(assertion.1.as_slice()).map_err(|_| ())?; + let (_, serde_grant, tag): (u64, SerdeAssertionGrant, String) = rmp_serde::from_slice(&assertion.0).map_err(|_| ())?; + Ok((serde_grant.grant(), tag)) } - fn signature(&self, data: &[u8]) -> ring::hmac::Signature { - ring::hmac::sign(&self.secret, data) + fn signature(&self, data: &[u8]) -> MacResult< as Mac>::OutputSize> { + let mut hmac = Hmac::::new_varkey(&self.key).unwrap(); + hmac.input(data); + hmac.result() } fn counted_signature(&self, counter: u64, grant: &Grant) @@ -176,14 +183,16 @@ impl Assertion { let serde_grant = SerdeAssertionGrant::try_from(grant)?; let tosign = rmp_serde::to_vec(&(serde_grant, counter)).unwrap(); let signature = self.signature(&tosign); - Ok(base64::encode(&signature)) + Ok(base64::encode(&signature.code())) } fn generate_tagged(&self, counter: u64, grant: &Grant, tag: &str) -> Result { let serde_grant = SerdeAssertionGrant::try_from(grant)?; let tosign = rmp_serde::to_vec(&(counter, serde_grant, tag)).unwrap(); - let signature = self.signature(&tosign); - Ok(encode(&rmp_serde::to_vec(&AssertGrant(tosign, signature.as_ref().to_vec())).unwrap())) + let signature = self.signature(&tosign); + let assert = AssertGrant(tosign, signature.code().to_vec()); + + Ok(encode(&rmp_serde::to_vec(&assert).unwrap())) } } @@ -232,21 +241,30 @@ impl TagGrant for RandomGenerator { } } -impl<'a> TagGrant for &'a RandomGenerator { - fn tag(&mut self, _: u64, _: &Grant) -> Result { - Ok(self.generate()) - } -} +// impl<'a> TagGrant for &'a RandomGenerator { +// fn tag(&mut self, _: u64, _: &Grant) -> Result { +// // Ok(self.generate()) +// Err(()) +// } +// } impl TagGrant for Rc { fn tag(&mut self, _: u64, _: &Grant) -> Result { - Ok(self.generate()) + if let Some(rng) = Rc::get_mut(self) { + Ok(rng.generate()) + } else { + Err(()) + } } } impl TagGrant for Arc { fn tag(&mut self, _: u64, _: &Grant) -> Result { - Ok(self.generate()) + if let Some(rng) = Arc::get_mut(self) { + Ok(rng.generate()) + } else { + Err(()) + } } } From 4e28581089f08f81016f123096cc6aa6f8054f73 Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Sat, 11 Apr 2020 22:40:28 -0400 Subject: [PATCH 6/7] Move pkce to sha2 --- oxide-auth/src/code_grant/extensions/pkce.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/oxide-auth/src/code_grant/extensions/pkce.rs b/oxide-auth/src/code_grant/extensions/pkce.rs index 309f5a37..c836c074 100644 --- a/oxide-auth/src/code_grant/extensions/pkce.rs +++ b/oxide-auth/src/code_grant/extensions/pkce.rs @@ -3,8 +3,7 @@ use std::borrow::Cow; use primitives::grant::{GrantExtension, Value}; use base64; -use ring::digest::{SHA256, digest}; -use ring::constant_time::verify_slices_are_equal; +use sha2::{Digest, Sha256}; /// Proof Key for Code Exchange by OAuth Public Clients /// @@ -166,13 +165,12 @@ impl Method { fn verify(&self, verifier: &str) -> Result<(), ()> { match self { Method::Plain(encoded) => - verify_slices_are_equal(encoded.as_bytes(), verifier.as_bytes()) - .map_err(|_| ()), + if encoded.as_bytes() == verifier.as_bytes() { Ok(()) } else { Err(()) }, Method::Sha256(encoded) => { - let digest = digest(&SHA256, verifier.as_bytes()); - let b64digest = b64encode(digest.as_ref()); - verify_slices_are_equal(encoded.as_bytes(), b64digest.as_bytes()) - .map_err(|_| ()) + let mut hasher = Sha256::new(); + hasher.input(verifier.as_bytes()); + let b64digest = b64encode(&hasher.result()); + if encoded.as_bytes() == b64digest.as_bytes() { Ok(()) } else { Err(()) } } } } From 878d8035d8fa35754d9c2ee5d36f5ec25e657ca6 Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Mon, 13 Apr 2020 00:29:28 -0400 Subject: [PATCH 7/7] Fixing PR #84 --- oxide-auth/Cargo.toml | 1 + oxide-auth/src/code_grant/extensions/pkce.rs | 5 ++- oxide-auth/src/lib.rs | 1 + oxide-auth/src/primitives/generator.rs | 46 ++++++++------------ 4 files changed, 24 insertions(+), 29 deletions(-) diff --git a/oxide-auth/Cargo.toml b/oxide-auth/Cargo.toml index f28df8ef..4a8ae382 100644 --- a/oxide-auth/Cargo.toml +++ b/oxide-auth/Cargo.toml @@ -23,6 +23,7 @@ serde = "1.0" serde_derive = "1.0" serde_json = "1.0" sha2 = "0.8.1" +subtle = "2.2.2" rand = "0.7.3" rmp-serde = "0.14" url = "1.7" diff --git a/oxide-auth/src/code_grant/extensions/pkce.rs b/oxide-auth/src/code_grant/extensions/pkce.rs index c836c074..2177ee81 100644 --- a/oxide-auth/src/code_grant/extensions/pkce.rs +++ b/oxide-auth/src/code_grant/extensions/pkce.rs @@ -4,6 +4,7 @@ use primitives::grant::{GrantExtension, Value}; use base64; use sha2::{Digest, Sha256}; +use subtle::ConstantTimeEq; /// Proof Key for Code Exchange by OAuth Public Clients /// @@ -165,12 +166,12 @@ impl Method { fn verify(&self, verifier: &str) -> Result<(), ()> { match self { Method::Plain(encoded) => - if encoded.as_bytes() == verifier.as_bytes() { Ok(()) } else { Err(()) }, + if encoded.as_bytes().ct_eq(verifier.as_bytes()).into() { Ok(()) } else { Err(()) }, Method::Sha256(encoded) => { let mut hasher = Sha256::new(); hasher.input(verifier.as_bytes()); let b64digest = b64encode(&hasher.result()); - if encoded.as_bytes() == b64digest.as_bytes() { Ok(()) } else { Err(()) } + if encoded.as_bytes().ct_eq(b64digest.as_bytes()).into() { Ok(()) } else { Err(()) } } } } diff --git a/oxide-auth/src/lib.rs b/oxide-auth/src/lib.rs index 5aa7e53b..64ea6be2 100644 --- a/oxide-auth/src/lib.rs +++ b/oxide-auth/src/lib.rs @@ -77,6 +77,7 @@ extern crate serde; extern crate serde_derive; extern crate serde_json; extern crate sha2; +extern crate subtle; extern crate url; pub mod code_grant; diff --git a/oxide-auth/src/primitives/generator.rs b/oxide-auth/src/primitives/generator.rs index 9a5ea72e..42a5a564 100644 --- a/oxide-auth/src/primitives/generator.rs +++ b/oxide-auth/src/primitives/generator.rs @@ -62,9 +62,10 @@ impl RandomGenerator { } } - fn generate(&mut self) -> String { + fn generate(&self) -> String { let mut result = vec![0; self.len]; - self.random.try_fill_bytes(result.as_mut_slice()) + let mut rnd = self.random.clone(); + rnd.try_fill_bytes(result.as_mut_slice()) .expect("Failed to generate random token"); encode(&result) } @@ -80,7 +81,7 @@ impl RandomGenerator { /// signing the same grant for different uses, i.e. separating authorization from bearer grants and /// refresh tokens. pub struct Assertion { - key: Vec + hasher: Hmac:: } /// The cryptographic suite ensuring integrity of tokens. @@ -139,7 +140,7 @@ impl Assertion { /// hmac + sha256. pub fn new(kind: AssertionKind, key: &[u8]) -> Self { match kind { - AssertionKind::HmacSha256 => Assertion { key: key.to_vec() }, + AssertionKind::HmacSha256 => Assertion { hasher: Hmac::::new_varkey(key).unwrap() }, AssertionKind::__NonExhaustive => unreachable!(), } } @@ -149,7 +150,7 @@ impl Assertion { // XXX Why 64? let mut rand_bytes: [u8; 64] = [0;64]; thread_rng().fill_bytes(&mut rand_bytes); - Assertion { key: rand_bytes.to_vec() } + Assertion { hasher: Hmac::::new_varkey(&rand_bytes).unwrap() } } /// Get a reference to generator for the given tag. @@ -161,9 +162,9 @@ impl Assertion { let decoded = decode(token).map_err(|_| ())?; let assertion: AssertGrant = rmp_serde::from_slice(&decoded).map_err(|_| ())?; - let mut hmac = Hmac::::new_varkey(&self.key).unwrap(); - hmac.input(&assertion.0); - hmac.verify(assertion.1.as_slice()).map_err(|_| ())?; + let mut hasher = self.hasher.clone(); + hasher.input(&assertion.0); + hasher.verify(assertion.1.as_slice()).map_err(|_| ())?; let (_, serde_grant, tag): (u64, SerdeAssertionGrant, String) = rmp_serde::from_slice(&assertion.0).map_err(|_| ())?; @@ -172,9 +173,9 @@ impl Assertion { } fn signature(&self, data: &[u8]) -> MacResult< as Mac>::OutputSize> { - let mut hmac = Hmac::::new_varkey(&self.key).unwrap(); - hmac.input(data); - hmac.result() + let mut hasher = self.hasher.clone(); + hasher.input(data); + hasher.result() } fn counted_signature(&self, counter: u64, grant: &Grant) @@ -241,30 +242,21 @@ impl TagGrant for RandomGenerator { } } -// impl<'a> TagGrant for &'a RandomGenerator { -// fn tag(&mut self, _: u64, _: &Grant) -> Result { -// // Ok(self.generate()) -// Err(()) -// } -// } +impl<'a> TagGrant for &'a RandomGenerator { + fn tag(&mut self, _: u64, _: &Grant) -> Result { + Ok(self.generate()) + } +} impl TagGrant for Rc { fn tag(&mut self, _: u64, _: &Grant) -> Result { - if let Some(rng) = Rc::get_mut(self) { - Ok(rng.generate()) - } else { - Err(()) - } + Ok(self.generate()) } } impl TagGrant for Arc { fn tag(&mut self, _: u64, _: &Grant) -> Result { - if let Some(rng) = Arc::get_mut(self) { - Ok(rng.generate()) - } else { - Err(()) - } + Ok(self.generate()) } }