diff --git a/extmod/foundation-rust/include/foundation.h b/extmod/foundation-rust/include/foundation.h index 4f71e66f8..008690258 100644 --- a/extmod/foundation-rust/include/foundation.h +++ b/extmod/foundation-rust/include/foundation.h @@ -171,6 +171,14 @@ typedef enum { * The second signature verification failed. */ FIRMWARE_RESULT_FAILED_SIGNATURE2, + /** + * Missing user public key + */ + FIRMWARE_RESULT_MISSING_USER_PUBLIC_KEY, + /** + * Invalid hash length + */ + FIRMWARE_RESULT_INVALID_HASH_LENGTH, } FirmwareResult_Tag; typedef struct { diff --git a/extmod/foundation-rust/src/firmware.rs b/extmod/foundation-rust/src/firmware.rs index 87f943032..b54c34954 100644 --- a/extmod/foundation-rust/src/firmware.rs +++ b/extmod/foundation-rust/src/firmware.rs @@ -53,6 +53,10 @@ pub enum FirmwareResult { FailedSignature1, /// The second signature verification failed. FailedSignature2, + /// Missing user public key + MissingUserPublicKey, + /// Invalid hash length + InvalidHashLength, } impl From for FirmwareResult { @@ -85,9 +89,7 @@ impl From for FirmwareResult { } VerifySignatureError::FailedSignature1 { .. } => FailedSignature1, VerifySignatureError::FailedSignature2 { .. } => FailedSignature2, - // No need to implement this, see comment on - // verify_update_signatures. - VerifySignatureError::MissingUserPublicKey => unimplemented!(), + VerifySignatureError::MissingUserPublicKey => MissingUserPublicKey, } } } @@ -159,8 +161,13 @@ pub extern "C" fn verify_update_signatures( result: &mut FirmwareResult, ) { let header = unsafe { slice::from_raw_parts(header, header_len) }; - let firmware_hash = sha256d::Hash::from_slice(hash) - .expect("hash should be of correct length"); + let firmware_hash = match sha256d::Hash::from_slice(hash) { + Ok(v) => v, + Err(_) => { + *result = FirmwareResult::InvalidHashLength; + return; + } + }; let user_public_key = user_public_key .map(|v| { @@ -169,8 +176,10 @@ pub extern "C" fn verify_update_signatures( (&mut buf[1..]).copy_from_slice(v); buf }) - .map(|v| { - PublicKey::from_slice(&v).expect("user public key should be valid") + .and_then(|v| { + // If we fail to parse the public key, it means that the user + // installed an invalid public key to the secure element. + PublicKey::from_slice(&v).ok() }); let header = @@ -188,15 +197,6 @@ pub extern "C" fn verify_update_signatures( Ok(()) => { *result = FirmwareResult::SignaturesOk; } - // The code calling this function must make sure that there's an user - // public key provided to us before verifying signatures. - // - // When verifying signatures is because we are committed to the - // update, i.e. the user has accepted to do it, so we must have - // presented an error if there was not an user public key earlier. - Err(VerifySignatureError::MissingUserPublicKey) => { - unreachable!("we always provide a user public key") - } Err(e) => *result = FirmwareResult::from(e), } } diff --git a/ports/stm32/boards/Passport/modpassport.c b/ports/stm32/boards/Passport/modpassport.c index d785d9902..414f0e41f 100644 --- a/ports/stm32/boards/Passport/modpassport.c +++ b/ports/stm32/boards/Passport/modpassport.c @@ -135,6 +135,14 @@ STATIC mp_obj_t mod_passport_verify_update_header(mp_obj_t header) { MP_ERROR_TEXT("Same Public Key was used for both signatures (%"PRIu32")."), result.SAME_PUBLIC_KEY.index); break; + case FIRMWARE_RESULT_MISSING_USER_PUBLIC_KEY: + mp_raise_msg(&mp_type_InvalidFirmwareUpdate, + MP_ERROR_TEXT("Missing user public key")); + break; + case FIRMWARE_RESULT_INVALID_HASH_LENGTH: + mp_raise_msg(&mp_type_InvalidFirmwareUpdate, + MP_ERROR_TEXT("Invalid hash length")); + break; default: break; } @@ -145,9 +153,21 @@ STATIC mp_obj_t mod_passport_verify_update_header(mp_obj_t header) { STATIC MP_DEFINE_CONST_FUN_OBJ_1(mod_passport_verify_update_header_obj, mod_passport_verify_update_header); +STATIC bool array_is_zero(uint8_t *buf, size_t size) +{ + for (int i = 0; i < size; i++) { + if (buf[i] != 0) { + return false; + } + } + + return true; +} + + STATIC mp_obj_t mod_passport_verify_update_signatures(mp_obj_t header, mp_obj_t validation_hash) { - uint8_t buf[72]; - uint8_t public_key[64]; + uint8_t buf[72] = {0}; + uint8_t public_key[64] = {0}; mp_buffer_info_t header_info; mp_get_buffer_raise(header, &header_info, MP_BUFFER_READ); @@ -163,9 +183,16 @@ STATIC mp_obj_t mod_passport_verify_update_signatures(mp_obj_t header, mp_obj_t uint32_t firmware_timestamp = se_get_firmware_timestamp(current_board_hash); se_pair_unlock(); - bool has_user_key = se_read_data_slot(KEYNUM_user_fw_pubkey, buf, sizeof(buf)) == 0; - memcpy(public_key, buf, sizeof(public_key)); + bool has_user_key = false; + if (se_read_data_slot(KEYNUM_user_fw_pubkey, buf, sizeof(buf)) == 0) { + memcpy(public_key, buf, sizeof(public_key)); + + has_user_key = !array_is_zero(public_key, sizeof(public_key)); + } + // If we fail reading the user key from the secure element just pass NULL + // and the verification won't take into account the user key and will + // return an error if it is an user signed firmware. FirmwareResult result = {0}; foundation_firmware_verify_update_signatures(header_info.buf, header_info.len, @@ -232,6 +259,15 @@ STATIC mp_obj_t mod_passport_verify_update_signatures(mp_obj_t header, mp_obj_t case FIRMWARE_RESULT_FAILED_SIGNATURE2: mp_raise_msg(&mp_type_InvalidFirmwareUpdate, MP_ERROR_TEXT("Second signature verification failed")); + break; + case FIRMWARE_RESULT_MISSING_USER_PUBLIC_KEY: + mp_raise_msg(&mp_type_InvalidFirmwareUpdate, + MP_ERROR_TEXT("Missing user public key")); + break; + case FIRMWARE_RESULT_INVALID_HASH_LENGTH: + mp_raise_msg(&mp_type_InvalidFirmwareUpdate, + MP_ERROR_TEXT("Invalid hash length")); + break; default: break; }