Fix padding on EC coordinates that are not expected sizes
When encoding public EC keys, we should ensure that we don't have
extra leading 00s that make the key larger than expected.
Additionally, we should insert leading 00s if the key would otherwise
be too short.
Include new test data to verify these edge cases.
Bug: 286384153
Test: atest libhwtrust_tests
(cherry picked from https://android-review.googlesource.com/q/commit:6f75372e2b16584e893ac25426dc8692867ac724)
Merged-In: I28418e6d8b788576a410b974e38c5a6d7fc96578
Change-Id: I28418e6d8b788576a410b974e38c5a6d7fc96578
diff --git a/remote_provisioning/hwtrust/src/cbor/publickey.rs b/remote_provisioning/hwtrust/src/cbor/publickey.rs
index 69f9799..709daff 100644
--- a/remote_provisioning/hwtrust/src/cbor/publickey.rs
+++ b/remote_provisioning/hwtrust/src/cbor/publickey.rs
@@ -61,11 +61,14 @@
key.public_key()
.affine_coordinates_gfp(group, &mut x, &mut y, &mut ctx)
.context("Get EC coordinates")?;
- let crv = match ec {
- EcKind::P256 => iana::EllipticCurve::P_256,
- EcKind::P384 => iana::EllipticCurve::P_384,
+ let (crv, coord_len) = match ec {
+ EcKind::P256 => (iana::EllipticCurve::P_256, 32),
+ EcKind::P384 => (iana::EllipticCurve::P_384, 48),
};
- CoseKeyBuilder::new_ec2_pub_key(crv, x.to_vec(), y.to_vec())
+
+ let x = adjust_coord(x.to_vec(), coord_len);
+ let y = adjust_coord(y.to_vec(), coord_len);
+ CoseKeyBuilder::new_ec2_pub_key(crv, x, y)
}
};
Ok(builder
@@ -75,6 +78,21 @@
}
}
+fn adjust_coord(mut coordinate: Vec<u8>, length: usize) -> Vec<u8> {
+ // Use loops "just in case". However we should never see a coordinate with more than one
+ // extra leading byte. The chances of more than one trailing byte is also quite small --
+ // roughly 1/65000.
+ while coordinate.len() > length && coordinate[0] == 00 {
+ coordinate.remove(0);
+ }
+
+ while coordinate.len() < length {
+ coordinate.insert(0, 0);
+ }
+
+ coordinate
+}
+
fn pkey_from_okp_key(cose_key: &CoseKey) -> Result<PKey<Public>> {
ensure!(cose_key.kty == KeyType::Assigned(iana::KeyType::OKP));
ensure!(cose_key.alg == Some(Algorithm::Assigned(iana::Algorithm::EdDSA)));
@@ -158,7 +176,10 @@
#[cfg(test)]
mod tests {
use super::*;
- use crate::publickey::testkeys::{PrivateKey, ED25519_KEY_PEM, P256_KEY_PEM};
+ use crate::publickey::testkeys::{
+ PrivateKey, ED25519_KEY_PEM, P256_KEY_PEM, P256_KEY_WITH_LEADING_ZEROS_PEM,
+ P384_KEY_WITH_LEADING_ZEROS_PEM,
+ };
use coset::{CoseSign1Builder, HeaderBuilder};
impl PrivateKey {
@@ -269,4 +290,40 @@
let new_key = PublicKey::from_cose_key(&value).unwrap();
assert!(key.pkey().public_eq(new_key.pkey()));
}
+
+ #[test]
+ fn from_p256_pkey_with_leading_zeros() {
+ for pem in P256_KEY_WITH_LEADING_ZEROS_PEM {
+ let key = PrivateKey::from_pem(pem).public_key();
+ let cose_key = key.to_cose_key().unwrap();
+
+ let x =
+ get_label_value_as_bytes(&cose_key, Label::Int(iana::Ec2KeyParameter::X.to_i64()))
+ .unwrap();
+ assert_eq!(x.len(), 32, "X coordinate is the wrong size\n{}", pem);
+
+ let y =
+ get_label_value_as_bytes(&cose_key, Label::Int(iana::Ec2KeyParameter::Y.to_i64()))
+ .unwrap();
+ assert_eq!(y.len(), 32, "Y coordinate is the wrong size\n{}", pem);
+ }
+ }
+
+ #[test]
+ fn from_p384_pkey_with_leading_zeros() {
+ for pem in P384_KEY_WITH_LEADING_ZEROS_PEM {
+ let key = PrivateKey::from_pem(pem).public_key();
+ let cose_key = key.to_cose_key().unwrap();
+
+ let x =
+ get_label_value_as_bytes(&cose_key, Label::Int(iana::Ec2KeyParameter::X.to_i64()))
+ .unwrap();
+ assert_eq!(x.len(), 48, "X coordinate is the wrong size\n{}", pem);
+
+ let y =
+ get_label_value_as_bytes(&cose_key, Label::Int(iana::Ec2KeyParameter::Y.to_i64()))
+ .unwrap();
+ assert_eq!(y.len(), 48, "Y coordinate is the wrong size\n{}", pem);
+ }
+ }
}
diff --git a/remote_provisioning/hwtrust/src/publickey.rs b/remote_provisioning/hwtrust/src/publickey.rs
index 905d6ac..562ec37 100644
--- a/remote_provisioning/hwtrust/src/publickey.rs
+++ b/remote_provisioning/hwtrust/src/publickey.rs
@@ -211,6 +211,25 @@
-----END PRIVATE KEY-----\n",
];
+ /// A selection of EC keys that should have leading zeros in their coordinates
+ pub const P256_KEY_WITH_LEADING_ZEROS_PEM: &[&str] = &[
+ // 31 byte Y coordinate:
+ "-----BEGIN PRIVATE KEY-----\n\
+ MEECAQAwEwYHKoZIzj0CAQYIKoZIzj0DAQcEJzAlAgEBBCCWbRSB3imI03F5YNVq\n\
+ 8AN8ZbyzW/h+5BQ53caD5VkWJg==\n\
+ -----END PRIVATE KEY-----\n",
+ // 31 byte X coordinate:
+ "-----BEGIN PRIVATE KEY-----\n\
+ MEECAQAwEwYHKoZIzj0CAQYIKoZIzj0DAQcEJzAlAgEBBCDe5E5WqNmCLxtsCNTc\n\
+ UOb9CPXCn6l3CZpbrp0aivb+Bw==\n\
+ -----END PRIVATE KEY-----\n",
+ // X & Y both have MSB set, and some stacks will add a padding byte
+ "-----BEGIN PRIVATE KEY-----\n\
+ MEECAQAwEwYHKoZIzj0CAQYIKoZIzj0DAQcEJzAlAgEBBCCWOWcXPDEVZ4Qz3EBK\n\
+ uvSqhD9HmxDGxcNe3yxKi9pazw==\n\
+ -----END PRIVATE KEY-----\n",
+ ];
+
/// A selection of elliptic curve P-384 private keys.
pub const P384_KEY_PEM: &[&str] = &["-----BEGIN PRIVATE KEY-----\n\
MIG2AgEAMBAGByqGSM49AgEGBSuBBAAiBIGeMIGbAgEBBDBMZ414LiUpcuNTNq5W\n\
@@ -219,6 +238,25 @@
uuHi+VayOreTX1/qlUoxgBT+XTI0nTdLn6WwO6vVO1NIkGEVnYvB2eM=\n\
-----END PRIVATE KEY-----\n"];
+ /// A selection of EC keys that should have leading zeros in their coordinates
+ pub const P384_KEY_WITH_LEADING_ZEROS_PEM: &[&str] = &[
+ // 47 byte Y coordinate:
+ "-----BEGIN PRIVATE KEY-----\n\
+ ME4CAQAwEAYHKoZIzj0CAQYFK4EEACIENzA1AgEBBDCzgVHCz7wgmSdb7/IixYik\n\
+ 3AuQceCtBTiFrJpgpGFluwgLUR0S2NpzIuty4M7xU74=\n\
+ -----END PRIVATE KEY-----\n",
+ // 47 byte X coordinate:
+ "-----BEGIN PRIVATE KEY-----\n\
+ ME4CAQAwEAYHKoZIzj0CAQYFK4EEACIENzA1AgEBBDBoW+8zbvwf5fYOS8YPyPEH\n\
+ jHP71Vr1MnRYRp/yG1wbthW2XEu0UWbp4qrZ5WTnZPg=\n\
+ -----END PRIVATE KEY-----\n",
+ // X & Y both have MSB set, and some stacks will add a padding byte
+ "-----BEGIN PRIVATE KEY-----\n\
+ ME4CAQAwEAYHKoZIzj0CAQYFK4EEACIENzA1AgEBBDD2A69j5M/6oc6/WGoYln4t\n\
+ Alnn0C6kpJz1EVC+eH6y0YNrcGamz8pPY4NkzUB/tj4=\n\
+ -----END PRIVATE KEY-----\n",
+ ];
+
/// A selection of elliptic curve P-521 private keys.
pub const P521_KEY_PEM: &[&str] = &["-----BEGIN PRIVATE KEY-----\n\
MIHuAgEAMBAGByqGSM49AgEGBSuBBAAjBIHWMIHTAgEBBEIBQuD8Db3jT2yPYR5t\n\
