[Bug] secure_boot_verify_payload fails with InvalidSignature when using ECDSA_NIST_P384_SHA384 crypto suite

[longlongyang]

secure_boot_verify_payload fails with InvalidSignature when using ECDSA_NIST_P384_SHA384 crypto suite with strip = "symbols" set in cargo.toml.
Location:

td-shim/td-shim/src/bin/td-shim/verifier.rs

Lines 162 to 164 in 552e40d

	signature_verifier
	.verify(self.image, self.signature)
	.map_err(|_e| VerifyErr::InvalidSignature)

How to reproduce the bug:

• If no panic is observed using the code change Reproducible build: introduce a tool to help reproducible build #268,

  Try add
  log::info!("Just visit the image memory, and the bug will be triggered: {:?}\n", self.image[100]);
  to the line before line 162
  

  td-shim/td-shim/src/bin/td-shim/verifier.rs

  Line 162 in 552e40d

  signature_verifier

• if panic is observed using the code change Reproducible build: introduce a tool to help reproducible build #268, then you have reproduced the bug.

Note: The trigger condition of this bug is not fixed. At very first, I caught the bug without visiting the image(message) memory, and the bug is gone after have a visiting in the image memory; And in the other day, I have to visit the image memory in order to trigger the bug.

FYR:
The signer:

td-shim/td-shim-tools/src/signer.rs

Lines 103 to 122 in 552e40d

	let public_key = ecdsa_keypair.public_key().as_ref();
	// 0x4 -- Uncompressed
	// 0x0 -- Compressed
	if public_key[0] != 0x4 {
	error!("Invalid ECDSA data format: {}", public_key[0],);
	return Err(io::Error::new(
	io::ErrorKind::InvalidInput,
	"Invalid ECDSA data format",
	));
	}
	let signature = ecdsa_keypair
	.sign(&rng, self.signed_image.as_slice())
	.map_err(|e| {
	error!("Failed to sign message with ECDSA: {}", e);
	io::Error::new(io::ErrorKind::Other, "failed to sign message")
	})?;
	self.signed_image.extend_from_slice(&public_key[1..]);
	self.signed_image.extend_from_slice(signature.as_ref());

The verifier:

td-shim/td-shim/src/bin/td-shim/verifier.rs

Lines 55 to 89 in 552e40d

	let header = signed_payload
	.pread::<PayloadSignHeader>(0)
	.map_err(|_e| VerifyErr::InvalidContent)?;
	let mut offset = header.length as usize;
	if
	/*offset <= size_of::<PayloadSignHeader>() || offset >= signed_payload.len() || */
	&header.type_guid != SIGNED_PAYLOAD_FILE_HEADER_GUID.as_bytes() {
	return Err(VerifyErr::InvalidContent);
	}
	// The image to be verified contains signing header and payload ELF/PE image
	let image = &signed_payload[0..offset];
	let mut formated_public_key: Vec<u8> = Vec::new();
	let verify_alg: &'static dyn VerificationAlgorithm;
	let signature;
	let public_key;
	match header.signing_algorithm {
	PAYLOAD_SIGN_ECDSA_NIST_P384_SHA384 => {
	if signed_payload.len() < offset + 192 {
	return Err(VerifyErr::InvalidContent);
	}
	// Public key (X: first 48 bytes, Y: second 48 bytes)
	public_key = &signed_payload[offset..offset + 96];
	offset += 96;
	// Signature: (R: first 48 bytes, S: second 48 byts)
	signature = &signed_payload[offset..offset + 96];
	// Uncompressed public key
	formated_public_key.push(0x04);
	formated_public_key.extend_from_slice(public_key);
	verify_alg = &signature::ECDSA_P384_SHA384_FIXED;

[longlongyang]

may be caused by #279

[ariel-adam]

@longlongyang is this issue still relevant or can be closed?
If it's still relevant to what release do you think we should map it to (mid-November, end-December, mid-February etc...)?

[ariel-adam]

@longlongyang is this issue still relevant or can be closed?
If it's still relevant to what release do you think we should map it to (mid-November, end-December, mid-February etc...)?

[longlongyang]

@longlongyang is this issue still relevant or can be closed? If it's still relevant to what release do you think we should map it to (mid-November, end-December, mid-February etc...)?

Not much relevant I think, This is an unexpected observation when enabling striping symbols. But since we now have a tool to do the striping work, this can be closed , or map it to mid-February to keep track of it

[ariel-adam]

Similar comment, I will close it for now and you can open when relevant