Skip to content

Commit

Permalink
Limits the number of recovery phrases to one (#727)
Browse files Browse the repository at this point in the history 
Limits the number of recovery phrases to one

This change is done in order to prevent an attacker adding a second protected recovery phrase.
Existing anchors with multiple recovery phrases can still use all of them but only make changes to recovery phrases after removing all but one.
  • Loading branch information
@frederikrothenberger
frederikrothenberger authored Jul 5, 2022
1 parent 5c999c4 commit b2a8760
Show file tree
Hide file tree
Showing 4 changed files with 205 additions and 23 deletions.
31 changes: 31 additions & 0 deletions src/canister_tests/src/framework.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ pub fn upgrade_ii_canister(env: &StateMachine, canister_id: CanisterId, wasm: Ve

pub const PUBKEY_1: &str = "test";
pub const PUBKEY_2: &str = "some other key";
pub const RECOVERY_PUBKEY_1: &str = "recovery 1";
pub const RECOVERY_PUBKEY_2: &str = "recovery 2";

pub fn principal_1() -> PrincipalId {
PrincipalId(Principal::self_authenticating(PUBKEY_1))
Expand All @@ -114,6 +116,13 @@ pub fn principal_1() -> PrincipalId {
pub fn principal_2() -> PrincipalId {
PrincipalId(Principal::self_authenticating(PUBKEY_2))
}
pub fn principal_recovery_1() -> PrincipalId {
PrincipalId(Principal::self_authenticating(RECOVERY_PUBKEY_1))
}

pub fn principal_recovery_2() -> PrincipalId {
PrincipalId(Principal::self_authenticating(RECOVERY_PUBKEY_2))
}

pub fn device_data_1() -> types::DeviceData {
types::DeviceData {
Expand All @@ -137,6 +146,28 @@ pub fn device_data_2() -> types::DeviceData {
}
}

pub fn recovery_device_data_1() -> types::DeviceData {
types::DeviceData {
pubkey: ByteBuf::from(RECOVERY_PUBKEY_1),
alias: "Recovery Phrase 1".to_string(),
credential_id: None,
purpose: types::Purpose::Recovery,
key_type: types::KeyType::SeedPhrase,
protection: types::DeviceProtection::Unprotected,
}
}

pub fn recovery_device_data_2() -> types::DeviceData {
types::DeviceData {
pubkey: ByteBuf::from(RECOVERY_PUBKEY_2),
alias: "Recovery Phrase 2".to_string(),
credential_id: None,
purpose: types::Purpose::Recovery,
key_type: types::KeyType::SeedPhrase,
protection: types::DeviceProtection::Unprotected,
}
}

/* Here are a few functions that are not directly related to II and could be upstreamed
* (were actually stolen from somewhere else)
*/
Expand Down
177 changes: 159 additions & 18 deletions src/canister_tests/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,9 @@ mod registration_tests {
use crate::{api, flows, framework};
use ic_error_types::ErrorCode::CanisterCalledTrap;
use ic_state_machine_tests::StateMachine;
use internet_identity_interface::{ChallengeAttempt, InternetIdentityInit, RegisterResponse};
use internet_identity_interface::{
ChallengeAttempt, DeviceProtection, InternetIdentityInit, RegisterResponse,
};
use regex::Regex;
use sdk_ic_types::Principal;
use std::time::Duration;
Expand Down Expand Up @@ -231,6 +233,34 @@ mod registration_tests {
Ok(())
}

/// Verifies that non-recovery devices cannot be registered as protected.
#[test]
fn should_not_register_non_recovery_device_as_protected() -> Result<(), CallError> {
let env = StateMachine::new();
let canister_id = framework::install_ii_canister(&env, framework::II_WASM.clone());
let mut device1 = device_data_1();
device1.protection = DeviceProtection::Protected;

let challenge = api::create_challenge(&&env, canister_id)?;
let result = api::register(
&env,
canister_id,
principal_1(),
&device1,
ChallengeAttempt {
chars: "a".to_string(),
key: challenge.challenge_key,
},
);

expect_user_error_with_message(
result,
CanisterCalledTrap,
Regex::new("Only recovery phrases can be protected but key type is Unknown").unwrap(),
);
Ok(())
}

/// Tests that the solution to the captcha needs to be correct.
#[test]
fn should_not_allow_wrong_captcha() -> Result<(), CallError> {
Expand Down Expand Up @@ -302,13 +332,18 @@ mod registration_tests {

/// Tests related to stable memory. In particular, the tests in this module make sure that II can be recovered from a stable memory backup.
mod stable_memory_tests {
use crate::framework::CallError;
use crate::framework::{
expect_user_error_with_message, principal_1, principal_recovery_1, principal_recovery_2,
recovery_device_data_1, recovery_device_data_2, CallError,
};
use crate::{api, framework};
use ic_error_types::ErrorCode::CanisterCalledTrap;
use ic_state_machine_tests::{PrincipalId, StateMachine};
use internet_identity_interface::DeviceData;
use internet_identity_interface::DeviceProtection::Unprotected;
use internet_identity_interface::KeyType::Unknown;
use internet_identity_interface::Purpose::Authentication;
use regex::Regex;
use sdk_ic_types::Principal;
use serde_bytes::ByteBuf;
use std::path::PathBuf;
Expand Down Expand Up @@ -483,6 +518,91 @@ mod stable_memory_tests {
assert_eq!(devices.len(), 1);
Ok(())
}

/// Verifies that an anchor with two recovery phrases can still use both.
/// This anchor is recovered from stable memory because the current version of II does not allow to create such anchors.
#[test]
fn should_not_break_on_multiple_legacy_recovery_phrases() -> Result<(), CallError> {
let env = StateMachine::new();
let canister_id = framework::install_ii_canister(&env, framework::II_WASM.clone());
let frontend_hostname = "frontend_hostname".to_string();
let session_key = ByteBuf::from("session_key");

let stable_memory_backup =
std::fs::read(PathBuf::from("stable_memory/multiple-recovery-phrases.bin")).unwrap();
env.set_stable_memory(canister_id, &stable_memory_backup);
// upgrade again to reset cached header info in II storage module
framework::upgrade_ii_canister(&env, canister_id, framework::II_WASM.clone());

api::prepare_delegation(
&env,
canister_id,
principal_recovery_1(),
10_000,
frontend_hostname.clone(),
session_key.clone(),
None,
)?;
api::prepare_delegation(
&env,
canister_id,
principal_recovery_2(),
10_000,
frontend_hostname,
session_key,
None,
)?;
Ok(())
}

/// Verifies that an existing account with two recovery phrases can only make changes after deleting one.
/// This anchor is recovered from stable memory because the current version of II does not allow to create such anchors.
#[test]
fn should_allow_modification_after_deleting_second_recovery_phrase() -> Result<(), CallError> {
let env = StateMachine::new();
let canister_id = framework::install_ii_canister(&env, framework::II_WASM.clone());

let stable_memory_backup =
std::fs::read(PathBuf::from("stable_memory/multiple-recovery-phrases.bin")).unwrap();
env.set_stable_memory(canister_id, &stable_memory_backup);
// upgrade again to reset cached header info in II storage module
framework::upgrade_ii_canister(&env, canister_id, framework::II_WASM.clone());

let mut recovery_1 = recovery_device_data_1();
recovery_1.alias = "new alias".to_string();
let result = api::update(
&env,
canister_id,
principal_1(),
10_000,
recovery_1.pubkey.clone(),
recovery_1.clone(),
);
expect_user_error_with_message(
result,
CanisterCalledTrap,
Regex::new("There is already a recovery phrase and only one is allowed\\.").unwrap(),
);

api::remove(
&env,
canister_id,
principal_1(),
10_000,
recovery_device_data_2().pubkey,
)?;

// successful after removing the other one
api::update(
&env,
canister_id,
principal_1(),
10_000,
recovery_1.pubkey.clone(),
recovery_1,
)?;
Ok(())
}
}

/// Tests related to local device management (add, remove, lookup, get_anchor_info).
Expand All @@ -491,7 +611,7 @@ mod stable_memory_tests {
mod device_management_tests {
use crate::framework::{
device_data_1, device_data_2, expect_user_error_with_message, principal_1, principal_2,
CallError,
recovery_device_data_1, recovery_device_data_2, CallError,
};
use crate::{api, flows, framework};
use ic_error_types::ErrorCode::CanisterCalledTrap;
Expand Down Expand Up @@ -550,6 +670,36 @@ mod device_management_tests {
Ok(())
}

/// Verifies that a second recovery phrase cannot be added.
#[test]
fn should_not_add_second_recovery_phrase() -> Result<(), CallError> {
let env = StateMachine::new();
let canister_id = framework::install_ii_canister(&env, framework::II_WASM.clone());
let user_number = flows::register_anchor(&env, canister_id);

api::add(
&env,
canister_id,
principal_1(),
user_number,
recovery_device_data_1(),
)?;
let result = api::add(
&env,
canister_id,
principal_1(),
user_number,
recovery_device_data_2(),
);

expect_user_error_with_message(
result,
CanisterCalledTrap,
Regex::new("There is already a recovery phrase and only one is allowed\\.").unwrap(),
);
Ok(())
}

/// Verifies that the devices cannot be added for other users.
#[test]
fn should_not_add_device_for_different_user() {
Expand Down Expand Up @@ -766,28 +916,19 @@ mod device_management_tests {
);
}

/// Verifies that unprotected devices can only be updated from themselves,
/// even if the authenticated device itself is protected
/// Verifies that non-recovery devices cannot be updated to be protected.
#[test]
fn should_not_update_protected_with_different_protected_device() {
fn should_not_update_non_recovery_device_to_be_protected() {
let env = StateMachine::new();
let canister_id = framework::install_ii_canister(&env, framework::II_WASM.clone());
let user_number = flows::register_anchor(&env, canister_id);

let mut device1 = device_data_1();
device1.protection = types::DeviceProtection::Protected;
device1.key_type = types::KeyType::SeedPhrase;
let user_number =
flows::register_anchor_with(&env, canister_id, principal_1(), &device1);

let mut device2 = device_data_2();
device2.protection = types::DeviceProtection::Protected;
device2.key_type = types::KeyType::SeedPhrase;

api::add(&env, canister_id, principal_1(), user_number, device2).unwrap();

let result = api::update(
&env,
canister_id,
principal_2(),
principal_1(),
user_number,
device1.pubkey.clone(),
device1.clone(), // data here doesnt' actually matter
Expand All @@ -796,7 +937,7 @@ mod device_management_tests {
expect_user_error_with_message(
result,
CanisterCalledTrap,
Regex::new("Device is protected. Must be authenticated with this device to mutate")
Regex::new("Only recovery phrases can be protected but key type is Unknown")
.unwrap(),
);
}
Expand Down
Binary file added src/canister_tests/stable_memory/multiple-recovery-phrases.bin
View file
Binary file not shown.
20 changes: 15 additions & 5 deletions src/internet_identity/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ async fn register(device_data: DeviceData, challenge_result: ChallengeAttempt) -
return RegisterResponse::BadChallenge;
}

check_device(&device_data);
check_device(&device_data, &vec![]);

if caller() != Principal::self_authenticating(device_data.pubkey.clone()) {
ic_cdk::trap(&format!(
Expand Down Expand Up @@ -438,8 +438,6 @@ async fn register(device_data: DeviceData, challenge_result: ChallengeAttempt) -
async fn add(user_number: UserNumber, device_data: DeviceData) {
const MAX_ENTRIES_PER_USER: usize = 10;

check_device(&device_data);

ensure_salt_set().await;

STATE.with(|s| {
Expand All @@ -451,6 +449,7 @@ async fn add(user_number: UserNumber, device_data: DeviceData) {
});

trap_if_not_authenticated(entries.iter().map(|e| &e.pubkey));
check_device(&device_data, &entries);

if entries
.iter()
Expand Down Expand Up @@ -523,7 +522,6 @@ fn mutate_device_or_trap(

#[update]
async fn update(user_number: UserNumber, device_key: DeviceKey, device_data: DeviceData) {
check_device(&device_data);
if device_key != device_data.pubkey {
trap("device key may not be updated");
}
Expand All @@ -537,6 +535,7 @@ async fn update(user_number: UserNumber, device_key: DeviceKey, device_data: Dev
});

trap_if_not_authenticated(entries.iter().map(|e| &e.pubkey));
check_device(&device_data, &entries);

mutate_device_or_trap(&mut entries, device_key, Some(device_data));

Expand Down Expand Up @@ -1136,12 +1135,13 @@ fn trap_if_not_authenticated<'a>(public_keys: impl Iterator<Item = &'a PublicKey
/// This checks some device invariants, in particular:
/// * Sizes of various fields do not exceed limits
/// * Only recovery phrases can be protected
/// * There can only be one recovery phrase
///
/// Otherwise, trap.
///
/// NOTE: while in the future we may lift this restriction, for now we do ensure that
/// protected devices are limited to recovery phrases, which the webapp expects.
fn check_device(device_data: &DeviceData) {
fn check_device(device_data: &DeviceData, existing_devices: &[DeviceDataInternal]) {
check_entry_limits(device_data);

if device_data.protection == DeviceProtection::Protected
Expand All @@ -1152,6 +1152,16 @@ fn check_device(device_data: &DeviceData) {
device_data.key_type
));
}

// if the device is a recovery phrase, check if a different recovery phrase already exists
if device_data.key_type == KeyType::SeedPhrase
&& existing_devices.iter().any(|existing_device| {
existing_device.pubkey != device_data.pubkey
&& existing_device.key_type == Some(KeyType::SeedPhrase)
})
{
trap("There is already a recovery phrase and only one is allowed.");
}
}

fn check_entry_limits(device_data: &DeviceData) {
Expand Down

0 comments on commit b2a8760

Please sign in to comment.