Skip to content

Commit

Permalink
Move out check credProtectPolicy logic (#516)
Browse files Browse the repository at this point in the history 
* Move out check credProtectPolicy logic

Move the credProtectPolicy check outside credential ID decryption &
discoverable credential finding. Modify the unit tests, and add unit
tests for credProtectPolicy checking in non resident flows that were
originally missing.
  • Loading branch information
@hcyang-google
hcyang-google authored Jul 23, 2022
1 parent 9bb1a2f commit 8ef813c
Show file tree
Hide file tree
Showing 5 changed files with 205 additions and 101 deletions.
51 changes: 11 additions & 40 deletions src/ctap/credential_id.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ pub fn decrypt_credential_id(
env: &mut impl Env,
credential_id: Vec<u8>,
rp_id_hash: &[u8],
check_cred_protect: bool,
) -> Result<Option<PublicKeyCredentialSource>, Ctap2StatusCode> {
if credential_id.len() < MIN_CREDENTIAL_ID_SIZE {
return Ok(None);
Expand Down Expand Up @@ -240,9 +239,7 @@ pub fn decrypt_credential_id(
return Ok(None);
};

let is_protected = credential_source.cred_protect_policy
== Some(CredentialProtectionPolicy::UserVerificationRequired);
if rp_id_hash != credential_source.rp_id_hash || (check_cred_protect && is_protected) {
if rp_id_hash != credential_source.rp_id_hash {
return Ok(None);
}

Expand Down Expand Up @@ -279,7 +276,7 @@ mod test {
let rp_id_hash = [0x55; 32];
let encrypted_id =
encrypt_to_credential_id(&mut env, &private_key, &rp_id_hash, None).unwrap();
let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash, false)
let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash)
.unwrap()
.unwrap();

Expand Down Expand Up @@ -313,7 +310,7 @@ mod test {
encrypted_id.extend(&id_hmac);

assert_eq!(
decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash, false),
decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash),
Ok(None)
);
}
Expand All @@ -329,7 +326,7 @@ mod test {
let mut modified_id = encrypted_id.clone();
modified_id[i] ^= 0x01;
assert_eq!(
decrypt_credential_id(&mut env, modified_id, &rp_id_hash, false),
decrypt_credential_id(&mut env, modified_id, &rp_id_hash),
Ok(None)
);
}
Expand All @@ -356,12 +353,7 @@ mod test {

for length in (1..CBOR_CREDENTIAL_ID_SIZE).step_by(16) {
assert_eq!(
decrypt_credential_id(
&mut env,
encrypted_id[..length].to_vec(),
&rp_id_hash,
false
),
decrypt_credential_id(&mut env, encrypted_id[..length].to_vec(), &rp_id_hash),
Ok(None)
);
}
Expand Down Expand Up @@ -408,13 +400,13 @@ mod test {
let rp_id_hash = [0x55; 32];
let encrypted_id =
legacy_encrypt_to_credential_id(&mut env, ecdsa_key, &rp_id_hash).unwrap();
// When checking credProtect for legacy credentials the check will always pass because we didn't persist credProtect
// policy info in it.
let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash, true)
let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash)
.unwrap()
.unwrap();

assert_eq!(private_key, decrypted_source.private_key);
// Legacy credentials didn't persist credProtectPolicy info, so it should be treated as None.
assert!(decrypted_source.cred_protect_policy.is_none());
}

#[test]
Expand All @@ -429,7 +421,7 @@ mod test {
}

#[test]
fn test_check_cred_protect_fail() {
fn test_cred_protect_persisted() {
let mut env = TestEnv::new();
let private_key = PrivateKey::new(&mut env, SignatureAlgorithm::Es256);

Expand All @@ -442,34 +434,13 @@ mod test {
)
.unwrap();

assert_eq!(
decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash, true),
Ok(None)
);
}

#[test]
fn test_check_cred_protect_success() {
let mut env = TestEnv::new();
let private_key = PrivateKey::new(&mut env, SignatureAlgorithm::Es256);

let rp_id_hash = [0x55; 32];
let encrypted_id = encrypt_to_credential_id(
&mut env,
&private_key,
&rp_id_hash,
Some(CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList),
)
.unwrap();

let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash, true)
let decrypted_source = decrypt_credential_id(&mut env, encrypted_id, &rp_id_hash)
.unwrap()
.unwrap();

assert_eq!(decrypted_source.private_key, private_key);
assert_eq!(
decrypted_source.cred_protect_policy,
Some(CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList)
Some(CredentialProtectionPolicy::UserVerificationRequired)
);
}
}
7 changes: 3 additions & 4 deletions src/ctap/credential_management.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -872,10 +872,9 @@ mod test {
Ok(ResponseData::AuthenticatorCredentialManagement(None))
);

let updated_credential =
storage::find_credential(&mut env, "example.com", &[0x1D; 32], false)
.unwrap()
.unwrap();
let updated_credential = storage::find_credential(&mut env, "example.com", &[0x1D; 32])
.unwrap()
.unwrap();
assert_eq!(updated_credential.user_handle, vec![0x01]);
assert_eq!(&updated_credential.user_name.unwrap(), "new_name");
assert_eq!(
Expand Down
3 changes: 1 addition & 2 deletions src/ctap/ctap1.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ impl Ctap1Command {
flags: Ctap1Flags,
ctap_state: &mut CtapState,
) -> Result<Vec<u8>, Ctap1StatusCode> {
let credential_source = decrypt_credential_id(env, key_handle, &application, false)
let credential_source = decrypt_credential_id(env, key_handle, &application)
.map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?;
if let Some(credential_source) = credential_source {
let ecdsa_key = credential_source
Expand Down Expand Up @@ -440,7 +440,6 @@ mod test {
&mut env,
response[67..67 + CBOR_CREDENTIAL_ID_SIZE].to_vec(),
&application,
false
)
.unwrap()
.is_some());
Expand Down
184 changes: 175 additions & 9 deletions src/ctap/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,22 @@ impl CtapState {
Ok(())
}

fn check_cred_protect_for_listed_credential(
&mut self,
credential: &Option<PublicKeyCredentialSource>,
has_uv: bool,
) -> bool {
if let Some(credential) = credential {
has_uv
|| !matches!(
credential.cred_protect_policy,
Some(CredentialProtectionPolicy::UserVerificationRequired),
)
} else {
false
}
}

fn process_make_credential(
&mut self,
env: &mut impl Env,
Expand Down Expand Up @@ -809,9 +825,13 @@ impl CtapState {
let rp_id_hash = Sha256::hash(rp_id.as_bytes());
if let Some(exclude_list) = exclude_list {
for cred_desc in exclude_list {
if storage::find_credential(env, &rp_id, &cred_desc.key_id, !has_uv)?.is_some()
|| decrypt_credential_id(env, cred_desc.key_id, &rp_id_hash, !has_uv)?.is_some()
{
if self.check_cred_protect_for_listed_credential(
&storage::find_credential(env, &rp_id, &cred_desc.key_id)?,
has_uv,
) || self.check_cred_protect_for_listed_credential(
&decrypt_credential_id(env, cred_desc.key_id, &rp_id_hash)?,
has_uv,
) {
// Perform this check, so bad actors can't brute force exclude_list
// without user interaction.
let _ = check_user_presence(env, channel);
Expand Down Expand Up @@ -1078,14 +1098,12 @@ impl CtapState {
has_uv: bool,
) -> Result<Option<PublicKeyCredentialSource>, Ctap2StatusCode> {
for allowed_credential in allow_list {
let credential =
storage::find_credential(env, rp_id, &allowed_credential.key_id, !has_uv)?;
if credential.is_some() {
let credential = storage::find_credential(env, rp_id, &allowed_credential.key_id)?;
if self.check_cred_protect_for_listed_credential(&credential, has_uv) {
return Ok(credential);
}
let credential =
decrypt_credential_id(env, allowed_credential.key_id, rp_id_hash, !has_uv)?;
if credential.is_some() {
let credential = decrypt_credential_id(env, allowed_credential.key_id, rp_id_hash)?;
if self.check_cred_protect_for_listed_credential(&credential, has_uv) {
return Ok(credential);
}
}
Expand Down Expand Up @@ -1802,6 +1820,61 @@ mod test {
assert!(make_credential_response.is_ok());
}

#[test]
fn test_non_resident_process_make_credential_credential_with_cred_protect() {
let mut env = TestEnv::new();
let mut ctap_state = CtapState::new(&mut env, CtapInstant::new(0));

let test_policy = CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList;
let mut make_credential_params =
create_make_credential_parameters_with_cred_protect_policy(test_policy);
make_credential_params.options.rk = false;
let make_credential_response =
ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
assert!(make_credential_response.is_ok());
let credential_id = match make_credential_response.unwrap() {
ResponseData::AuthenticatorMakeCredential(make_credential_response) => {
let auth_data = make_credential_response.auth_data;
let offset = 37 + storage::aaguid(&mut env).unwrap().len();
assert_eq!(auth_data[offset], 0x00);
assert_eq!(auth_data[offset + 1] as usize, CBOR_CREDENTIAL_ID_SIZE);
auth_data[offset + 2..offset + 2 + CBOR_CREDENTIAL_ID_SIZE].to_vec()
}
_ => panic!("Invalid response type"),
};
let make_credential_params =
create_make_credential_parameters_with_exclude_list(&credential_id);
let make_credential_response =
ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
assert_eq!(
make_credential_response,
Err(Ctap2StatusCode::CTAP2_ERR_CREDENTIAL_EXCLUDED)
);

let test_policy = CredentialProtectionPolicy::UserVerificationRequired;
let mut make_credential_params =
create_make_credential_parameters_with_cred_protect_policy(test_policy);
make_credential_params.options.rk = false;
let make_credential_response =
ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
assert!(make_credential_response.is_ok());
let credential_id = match make_credential_response.unwrap() {
ResponseData::AuthenticatorMakeCredential(make_credential_response) => {
let auth_data = make_credential_response.auth_data;
let offset = 37 + storage::aaguid(&mut env).unwrap().len();
assert_eq!(auth_data[offset], 0x00);
assert_eq!(auth_data[offset + 1] as usize, CBOR_CREDENTIAL_ID_SIZE);
auth_data[offset + 2..offset + 2 + CBOR_CREDENTIAL_ID_SIZE].to_vec()
}
_ => panic!("Invalid response type"),
};
let make_credential_params =
create_make_credential_parameters_with_exclude_list(&credential_id);
let make_credential_response =
ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
assert!(make_credential_response.is_ok());
}

#[test]
fn test_process_make_credential_hmac_secret() {
let mut env = TestEnv::new();
Expand Down Expand Up @@ -2650,6 +2723,99 @@ mod test {
);
}

#[test]
fn test_non_resident_process_get_assertion_with_cred_protect() {
let mut env = TestEnv::new();
let mut ctap_state = CtapState::new(&mut env, CtapInstant::new(0));

let test_policy = CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList;
let mut make_credential_params =
create_make_credential_parameters_with_cred_protect_policy(test_policy);
make_credential_params.options.rk = false;
let make_credential_response =
ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
assert!(make_credential_response.is_ok());
let credential_id = match make_credential_response.unwrap() {
ResponseData::AuthenticatorMakeCredential(make_credential_response) => {
let auth_data = make_credential_response.auth_data;
let offset = 37 + storage::aaguid(&mut env).unwrap().len();
assert_eq!(auth_data[offset], 0x00);
assert_eq!(auth_data[offset + 1] as usize, CBOR_CREDENTIAL_ID_SIZE);
auth_data[offset + 2..offset + 2 + CBOR_CREDENTIAL_ID_SIZE].to_vec()
}
_ => panic!("Invalid response type"),
};
let cred_desc = PublicKeyCredentialDescriptor {
key_type: PublicKeyCredentialType::PublicKey,
key_id: credential_id,
transports: None,
};
let get_assertion_params = AuthenticatorGetAssertionParameters {
rp_id: String::from("example.com"),
client_data_hash: vec![0xCD],
allow_list: Some(vec![cred_desc]),
extensions: GetAssertionExtensions::default(),
options: GetAssertionOptions {
up: false,
uv: false,
},
pin_uv_auth_param: None,
pin_uv_auth_protocol: None,
};
let get_assertion_response = ctap_state.process_get_assertion(
&mut env,
get_assertion_params,
DUMMY_CHANNEL,
CtapInstant::new(0),
);
assert!(get_assertion_response.is_ok());

let test_policy = CredentialProtectionPolicy::UserVerificationRequired;
let mut make_credential_params =
create_make_credential_parameters_with_cred_protect_policy(test_policy);
make_credential_params.options.rk = false;
let make_credential_response =
ctap_state.process_make_credential(&mut env, make_credential_params, DUMMY_CHANNEL);
assert!(make_credential_response.is_ok());
let credential_id = match make_credential_response.unwrap() {
ResponseData::AuthenticatorMakeCredential(make_credential_response) => {
let auth_data = make_credential_response.auth_data;
let offset = 37 + storage::aaguid(&mut env).unwrap().len();
assert_eq!(auth_data[offset], 0x00);
assert_eq!(auth_data[offset + 1] as usize, CBOR_CREDENTIAL_ID_SIZE);
auth_data[offset + 2..offset + 2 + CBOR_CREDENTIAL_ID_SIZE].to_vec()
}
_ => panic!("Invalid response type"),
};
let cred_desc = PublicKeyCredentialDescriptor {
key_type: PublicKeyCredentialType::PublicKey,
key_id: credential_id,
transports: None,
};
let get_assertion_params = AuthenticatorGetAssertionParameters {
rp_id: String::from("example.com"),
client_data_hash: vec![0xCD],
allow_list: Some(vec![cred_desc]),
extensions: GetAssertionExtensions::default(),
options: GetAssertionOptions {
up: false,
uv: false,
},
pin_uv_auth_param: None,
pin_uv_auth_protocol: None,
};
let get_assertion_response = ctap_state.process_get_assertion(
&mut env,
get_assertion_params,
DUMMY_CHANNEL,
CtapInstant::new(0),
);
assert_eq!(
get_assertion_response,
Err(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS),
);
}

#[test]
fn test_process_get_assertion_with_cred_blob() {
let mut env = TestEnv::new();
Expand Down
Loading

0 comments on commit 8ef813c

Please sign in to comment.