Hi, team. After examining the eventlog parsing code of the Rust version of this library, I think there are several areas that might need attention.
Unchecked input data slicing may lead to panics
As shown below:
// Parse EFI Spec Id Event structure
let spec_id_signature = data[index..index + 16].try_into().unwrap();
index += 16;
let spec_id_platform_cls = get_u32(data[index..index + 4].to_vec());
index += 4;
let spec_id_version_minor = get_u8(data[index..index + 1].to_vec());
index += 1;
...
All of the parsing logic directly slice the input data without a length check. If a user inputs malformed or incomplete data, the parser would panic. "Panicking" is not a valid error handling choice in a Rust library; it should return Err if there is any problem with the input data.
I suggest using a data decoding library rather than hand-rolling the parsing code from scratch. By using a library, you might get an input reader for free to get rid of manually managing the input data cursor. For example, the above code might become:
// Parse EFI Spec Id Event structure
let spec_id_signature = <[u8; 16]>::decode(input_reader)?;
let spec_id_platform_cls = u32::decode(input_reader)?;
let spec_id_version_minor = u8::decode(input_reader)?;
...
I'll send a PR to demonstrate the details of how to use a library to handle it.
.unwrap() might cause panic
.unwrap() should never be used in Rust except in unit tests or examples. The same applies to .expect("the reason") unless it is known that it would never fail. For example:
let hash: [u8; 32] = data.get(..32).context("insufficient data")?.try_into().expect("convert should never fail");
Unsafe enum transmutation could result in Undefined Behavior
There are some uses of unsafe transmute in the code. When using unsafe in Rust, you should be very careful, because unsafe Rust is harder than C/C++ to use correctly.
As you can see, there is at least one enum field ak_type in the transmuted struct. If the input data of the field ak_type is a value not in [2, 3], (for example, if some future platform got a third AttestationKeyType == 4), the code behavior becomes undefined.
The Rust compiler always optimizes the code based on the assumption that the ak_type never gets an invalid value.
Hi, team. After examining the eventlog parsing code of the Rust version of this library, I think there are several areas that might need attention.
Unchecked input data slicing may lead to panics
As shown below:
All of the parsing logic directly slice the input data without a length check. If a user inputs malformed or incomplete data, the parser would panic. "Panicking" is not a valid error handling choice in a Rust library; it should return
Err
if there is any problem with the input data.I suggest using a data decoding library rather than hand-rolling the parsing code from scratch. By using a library, you might get an input reader for free to get rid of manually managing the input data cursor. For example, the above code might become:
I'll send a PR to demonstrate the details of how to use a library to handle it.
.unwrap()
might cause panic.unwrap()
should never be used in Rust except in unit tests or examples. The same applies to.expect("the reason")
unless it is known that it would never fail. For example:Unsafe enum transmutation could result in Undefined Behavior
There are some uses of
unsafe transmute
in the code. When usingunsafe
in Rust, you should be very careful, because unsafe Rust is harder than C/C++ to use correctly.Take an example from this repo:
As you can see, there is at least one enum field
ak_type
in the transmuted struct. If the input data of the fieldak_type
is a value not in [2, 3], (for example, if some future platform got a third AttestationKeyType == 4), the code behavior becomes undefined. The Rust compiler always optimizes the code based on the assumption that theak_type
never gets an invalid value.See here for the demonstration code.
Additional areas for enhancement
&[u8]
rather thanVec<u8>
as function parameters.For example, the following definition will cause unnecessary call-site heap allocation.
Better:
A shorter equivalent could be:
And also
return Err(anyhow!("..."));
could be shortened toanyhow::bail!("...");
.match
on enum vars overif else
Using match, the compiler forces you to update your logic when you add variants to the enum.cargo clippy
.