Repository: google/OpenSK
File: src/ctap/crypto_wrapper.rs (Rust)

Code before:
/// - 32 bytes: HMAC-SHA256 over everything else.
///
/// For v2 we write the following data for EdDSA over curve Ed25519 (algorithm -8, curve 6):
/// -  1 byte : version number
/// - 16 bytes: initialization vector for AES-256,
/// - 32 bytes: Ed25519 private key for the credential,
/// - 32 bytes: relying party ID hashed with SHA256,
/// - 32 bytes: HMAC-SHA256 over everything else.
pub fn encrypt_key_handle(
    env: &mut impl Env,
    private_key: &PrivateKey,
    application: &[u8; 32],
) -> Result<Vec<u8>, Ctap2StatusCode> {
    let master_keys = storage::master_keys(env)?;
    let aes_enc_key = crypto::aes256::EncryptionKey::new(&master_keys.encryption);

    let mut plaintext = [0; 64];
    let version;
    match private_key {
        PrivateKey::Ecdsa(ecdsa_key) => {
            ecdsa_key.to_bytes(array_mut_ref!(plaintext, 0, 32));
            version = ECDSA_CREDENTIAL_ID_VERSION;
        }
        #[cfg(feature = "with_ed25519")]
        PrivateKey::Ed25519(keypair) => {
            plaintext[0..32].copy_from_slice(&keypair.secret.to_bytes());
            version = ED25519_CREDENTIAL_ID_VERSION;
        }
    };
    plaintext[32..64].copy_from_slice(application);
    let mut encrypted_id = aes256_cbc_encrypt(env.rng(), &aes_enc_key, &plaintext, true)?;
    encrypted_id.insert(0, version);

    let id_hmac = hmac_256::<Sha256>(&master_keys.hmac, &encrypted_id[..]);
    encrypted_id.extend(&id_hmac);
    Ok(encrypted_id)
}

/// Decrypts a credential ID and writes the private key into a PublicKeyCredentialSource.
///
/// Returns None if
/// - the format does not match any known versions,
/// - the HMAC test fails or
/// - the relying party does not match the decrypted relying party ID hash.
///
/// This functions reads:
/// - legacy credentials (no version number),
/// - v1 (ECDSA)
/// - v2 (EdDSA over curve Ed25519)
pub fn decrypt_credential_source(
    env: &mut impl Env,

Review comment (suggestion):
Documenting my thoughts here (not actionable): As long as all private keys are 32 bytes, one optoin is the `PrivateKey.to_bytes()` function and to check its output length to match the 32 byte (which should always succeed). But yours is more extensible, so I like it more.

Code after:
    let mut encrypted_id = aes256_cbc_encrypt(env.rng(), &aes_enc_key, &plaintext, true)?;
    encrypted_id.insert(0, version);

    let id_hmac = hmac_256::<Sha256>(&master_keys.hmac, &encrypted_id[..]);
    encrypted_id.extend(&id_hmac);
    Ok(encrypted_id)
}

/// Decrypts a credential ID and writes the private key into a PublicKeyCredentialSource.
///
/// Returns None if
/// - the format does not match any known versions,
/// - the HMAC test fails or
/// - the relying party does not match the decrypted relying party ID hash.
///
/// This functions reads:
/// - legacy credentials (no version number),
/// - v1 (ECDSA)
/// - v2 (EdDSA over curve Ed25519)
pub fn decrypt_credential_source(
    env: &mut impl Env,
    credential_id: Vec<u8>,
    rp_id_hash: &[u8],
) -> Result<Option<PublicKeyCredentialSource>, Ctap2StatusCode> {
    if credential_id.len() < LEGACY_CREDENTIAL_ID_SIZE {
        return Ok(None);
    }
    let master_keys = storage::master_keys(env)?;
    let hmac_message_size = credential_id.len() - 32;
    if !verify_hmac_256::<Sha256>(
        &master_keys.hmac,
        &credential_id[..hmac_message_size],
        array_ref![credential_id, hmac_message_size, 32],
    ) {
        return Ok(None);
    }

    let (payload, algorithm) = if credential_id.len() == LEGACY_CREDENTIAL_ID_SIZE {
        (&credential_id[..hmac_message_size], ES256_ALGORITHM)
    } else {
        // Version number check
        let algorithm = match credential_id[0] {
            ECDSA_CREDENTIAL_ID_VERSION => ES256_ALGORITHM,
            #[cfg(feature = "ed25519")]
            ED25519_CREDENTIAL_ID_VERSION => EDDSA_ALGORITHM,
            _ => return Ok(None),
        };
        (&credential_id[1..hmac_message_size], algorithm)
    };
    if payload.len() != 80 {
        // We shouldn't have HMAC'ed anything of different length. The check is cheap though.