shim: skip verification for multiple signatures once one of them is matched

[dennis-tseng99]

If a single signature is correctly verified for an image with multiple signatures, then the verify_one_signature() should be skipped to avoid cpu-consumption. In the mean time, it is wise to at least check the format of the remaining signatures to reduce the risk of corrupted or malformed signatures.

[mikebeaton]

@dennis-tseng99 - I think this change may not be correct:

76c0447e

"If at any point any assertion about the
binary or signature list being well-formed fails, the binary is
immediately rejected, though we do allow skipping over signatures
which have an unsupported sig->Hdr.wCertificateType."

[dennis-tseng99]

@mikebeaton, Thanks for your comment. Hope not to misunderstand your meaning.

If at any point any assertion about the
binary or signature list being well-formed fails, the binary is
immediately rejected

EDIT: if the format check for sig-list is necessary, then we should skip verify_one_signature() once one of signatures is correctly verified to avoid the high cpu consumption.

@mikebeaton To enhance data integrity, I do not remove the format check for rest signatures to avoid mal-format.

TPM measurement is cpu heavier than malformed checking. Verifying one valid signature and measuring its corresponding vendor cert’s TPM should be enough to ensure security and integrity, reducing extra overhead.

[mikebeaton]

@mikebeaton, Thanks for your comment. Hope not to misunderstand your meaning.

If at any point any assertion about the
binary or signature list being well-formed fails, the binary is
immediately rejected

Actually, in this PR, do-while would not stop until verification gets SUCCESS or offset is too large. Even the well-formed fails happen, the binary would not be immediately rejected.

In the PR, verification would stop if it gets SUCCESS. Before the PR verification would fail after a success, if the binary was found to be malformed after the success. And I think the commit message I quoted means that that is the intended behaviour.

I'm not a maintainer. So if you don't agree with my reading, of course wait for the maintainers to comment!

EDIT: To put it another way, I think the requirements are, binary must be signed by at least one sig, and entire sig list must be valid. The second requirement is not checked with this PR, but should be, if I've understood the code and comment correctly.

[mikebeaton]

Hi! I still think this is unfortunately probably wrong. If modifying code like this then (I humbly suggest!) it is important to check whether the code being skipped has any side effects that might be wanted. In this case, the calls to verify_one_signature which you are skipping make calls to tpm_measure_variable - so I believe the answer is yes, there are non-trivial, and wanted, side effects to 'pointlessly' verifying all signatures.