Verify TAs - #1399
Verify TAs#1399Angelina Vu (athvu) wants to merge 8 commits into
Conversation
|
Why make this a compile time feature: |
| } else { | ||
| let ta_bin = Self::rpc_get_ta_bin(ta_uuid)?; | ||
| if !self.store_ta_bin(ta_uuid, &ta_bin) { | ||
| if !self.store_ta_bin(ta_uuid, &ta_bin, TaSource::Dynamic) { |
There was a problem hiding this comment.
fyi, the place to store_ta_bin changed with the open Dynamic TA Support PR (#1213).
So, this should be revisited later
06bc0ae to
27170aa
Compare
Signed-off-by: Angelina Vu <angelinavu@microsoft.com>
Signed-off-by: Angelina Vu <angelinavu@microsoft.com>
b259c1e to
96b466b
Compare
Dynamic TAs must be signed and verified. Signed-off-by: Angelina Vu <angelinavu@microsoft.com>
… is not given. Signed-off-by: Angelina Vu <angelinavu@microsoft.com>
Signed-off-by: Angelina Vu <angelinavu@microsoft.com>
Signed-off-by: Angelina Vu <angelinavu@microsoft.com>
96b466b to
1c4ac88
Compare
Signed-off-by: Angelina Vu <angelinavu@microsoft.com>
1c4ac88 to
01e58f5
Compare
|
🤖 SemverChecks 🤖 Click for details |
There was a problem hiding this comment.
Two things should be considered:
- this PR is not yet wired with
ta_signing_cert,ta_svn,ta_digest, andta_dynamic.ta_dynamicis still debatable because it is related to not only this PR but also #1213. However, the former three should be wired here. - It doesn't have an end-to-end test. We can implement one using
litebox_runner_optee_on_linux_userland, whichsign_encrypt.pyone of the example TAs using a randomly generated RSA key pair and checks whether the userland runner can verify/run it.
| const SHDR_VERSION_LEN: usize = 4; | ||
|
|
||
| /// An RSA public key used to verify signed `.ta` files | ||
| pub struct TaVerifyKey(rsa::RsaPublicKey); |
There was a problem hiding this comment.
All these implementations (TaVerifyKey, parse_and_verify_ta, ...) should be moved to litebox_shim_optee. We are trying to avoid having actual implementation in common crates. This would also allow us to revert the changes in ci.yml and Cargo.toml.
| pub fn parse_and_verify_ta<'a>( | ||
| ta_data: &'a [u8], | ||
| verify_key: &TaVerifyKey, | ||
| ) -> Result<(TaHead, &'a [u8]), &'static str> { |
There was a problem hiding this comment.
nits: using thiserror should be better. returning error strings is a bit fragile.
| signed_message.extend_from_slice(uuid_and_version); | ||
| signed_message.extend_from_slice(img); | ||
| verify_shdr_signature(&signed_message, sig, shdr.algo, &verify_key.0)?; | ||
| let ta_head = parse_ta_head(img).ok_or("Invalid TA ELF binary")?; |
There was a problem hiding this comment.
We should check whether UUIDs in uuid_and_version and ta_head are equal.
| verify_shdr_signature(&signed_message, sig, shdr.algo, &verify_key.0)?; | ||
| let ta_head = parse_ta_head(img).ok_or("Invalid TA ELF binary")?; | ||
|
|
||
| Ok((ta_head, img)) |
There was a problem hiding this comment.
We should return version as well because a raw TA image doesn't contain version info. assign it to ta_svn.
| } | ||
| let hash_size = shdr.hash_size as usize; |
There was a problem hiding this comment.
nits: Better to check whether other fields (e.g., img_type) is valid and whether hash_size is correct (SHA256).
| /// UUID (from `.ta_head` section) doesn't match the provided UUID or parsing failed. | ||
| pub(crate) fn store_ta_bin(&self, ta_uuid: &TeeUuid, ta_bin: &[u8]) -> bool { | ||
| self.ta_uuid_map.insert(*ta_uuid, ta_bin.into()) | ||
| pub(crate) fn store_ta_bin(&self, ta_uuid: &TeeUuid, ta_bin: &[u8], source: TaSource) -> bool { |
There was a problem hiding this comment.
nits: Rather than relying on the source argument, we could check ta_bin itself to identify whether it is signed or not.
| ta_signing_cert: &'static [u8], | ||
| ta_verify_key: &'static [u8], |
There was a problem hiding this comment.
These two are the same one.
| /// and from `optee_os/core/include/tee_api_types.h` | ||
| /// ```c | ||
| /// typedef struct { | ||
| /// uint32_t timeLow; | ||
| /// uint16_t timeMid; | ||
| /// uint16_t timeHiAndVersion; | ||
| /// uint8_t clockSeqAndNode[8]; | ||
| /// } TEE_UUID; |
There was a problem hiding this comment.
not needed. this UUID thing is already in the same file.
| signature: &[u8], | ||
| algo: u32, | ||
| rsa_pub_key: &rsa::RsaPublicKey, | ||
| ) -> Result<(), &'static str> { |
There was a problem hiding this comment.
same. thiserror is preferred.
| /// How the TA binary was loaded | ||
| source: TaSource, |
There was a problem hiding this comment.
take a look at ta_dynamic.
| let end = img_offset | ||
| .checked_add(img_size) | ||
| .ok_or("Invalid signed TA file")?; | ||
| if end > ta_data.len() { |
There was a problem hiding this comment.
why not end != ta_data.len()? do we need to accept trailing bytes?
Allow for TA signature verification. A verification key can be embedded through the LITEBOX_TA_VERIFY_KEY build variable.
Built-in TAs can either be unsigned .elf or signed .ta files. Dynamic TAs must be signed.