elliptic-curve: include curve OID in SEC1 private keys (#1707)

The OID identifies the particular elliptic curve the key is for. Without it, OpenSSL can't parse these keys. See #1706.
RustCrypto · Oct 22, 2024 · a1ade1b · a1ade1b
1 parent 8c7c761
commit a1ade1b
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 13 deletions.
diff --git a/elliptic-curve/src/secret_key.rs b/elliptic-curve/src/secret_key.rs
@@ -29,7 +29,7 @@ use {
         sec1::{EncodedPoint, ModulusSize, ValidatePublicKey},
         FieldBytesSize,
     },
-    sec1::der,
+    sec1::der::{self, oid::AssociatedOid},
 };
 
 #[cfg(all(feature = "alloc", feature = "arithmetic", feature = "sec1"))]
@@ -184,7 +184,7 @@ where
     #[cfg(feature = "sec1")]
     pub fn from_sec1_der(der_bytes: &[u8]) -> Result<Self>
     where
-        C: Curve + ValidatePublicKey,
+        C: AssociatedOid + Curve + ValidatePublicKey,
         FieldBytesSize<C>: ModulusSize,
     {
         sec1::EcPrivateKey::try_from(der_bytes)?
@@ -196,17 +196,18 @@ where
     #[cfg(all(feature = "alloc", feature = "arithmetic", feature = "sec1"))]
     pub fn to_sec1_der(&self) -> der::Result<Zeroizing<Vec<u8>>>
     where
-        C: CurveArithmetic,
+        C: AssociatedOid + CurveArithmetic,
         AffinePoint<C>: FromEncodedPoint<C> + ToEncodedPoint<C>,
         FieldBytesSize<C>: ModulusSize,
     {
         let private_key_bytes = Zeroizing::new(self.to_bytes());
         let public_key_bytes = self.public_key().to_encoded_point(false);
+        let parameters = sec1::EcParameters::NamedCurve(C::OID);
 
         let ec_private_key = Zeroizing::new(
             sec1::EcPrivateKey {
                 private_key: &private_key_bytes,
-                parameters: None,
+                parameters: Some(parameters),
                 public_key: Some(public_key_bytes.as_bytes()),
             }
             .to_der()?,
@@ -225,7 +226,7 @@ where
     #[cfg(feature = "pem")]
     pub fn from_sec1_pem(s: &str) -> Result<Self>
     where
-        C: Curve + ValidatePublicKey,
+        C: AssociatedOid + Curve + ValidatePublicKey,
         FieldBytesSize<C>: ModulusSize,
     {
         let (label, der_bytes) = pem::decode_vec(s.as_bytes()).map_err(|_| Error)?;
@@ -244,7 +245,7 @@ where
     #[cfg(feature = "pem")]
     pub fn to_sec1_pem(&self, line_ending: pem::LineEnding) -> Result<Zeroizing<String>>
     where
-        C: CurveArithmetic,
+        C: AssociatedOid + CurveArithmetic,
         AffinePoint<C>: FromEncodedPoint<C> + ToEncodedPoint<C>,
         FieldBytesSize<C>: ModulusSize,
     {
@@ -344,16 +345,21 @@ where
 #[cfg(feature = "sec1")]
 impl<C> TryFrom<sec1::EcPrivateKey<'_>> for SecretKey<C>
 where
-    C: Curve + ValidatePublicKey,
+    C: AssociatedOid + Curve + ValidatePublicKey,
     FieldBytesSize<C>: ModulusSize,
 {
     type Error = der::Error;
 
     fn try_from(sec1_private_key: sec1::EcPrivateKey<'_>) -> der::Result<Self> {
+        if let Some(sec1::EcParameters::NamedCurve(curve_oid)) = sec1_private_key.parameters {
+            if C::OID != curve_oid {
+                return Err(der::Tag::ObjectIdentifier.value_error());
+            }
+        }
+
         let secret_key = Self::from_slice(sec1_private_key.private_key)
-            .map_err(|_| der::Tag::Sequence.value_error())?;
+            .map_err(|_| der::Tag::OctetString.value_error())?;
 
-        // TODO(tarcieri): validate `sec1_private_key.params`?
         if let Some(pk_bytes) = sec1_private_key.public_key {
             let pk = EncodedPoint::<C>::from_bytes(pk_bytes)
                 .map_err(|_| der::Tag::BitString.value_error())?;

diff --git a/elliptic-curve/src/secret_key/pkcs8.rs b/elliptic-curve/src/secret_key/pkcs8.rs
@@ -17,9 +17,10 @@ use {
         AffinePoint, CurveArithmetic,
     },
     pkcs8::{
-        der::{self, asn1::OctetStringRef},
+        der::{self, asn1::OctetStringRef, Encode},
         EncodePrivateKey,
     },
+    zeroize::Zeroizing,
 };
 
 // Imports for actual PEM support
@@ -54,8 +55,7 @@ where
             .algorithm
             .assert_oids(ALGORITHM_OID, C::OID)?;
 
-        let ec_private_key = EcPrivateKey::from_der(private_key_info.private_key.as_bytes())?;
-        Ok(Self::try_from(ec_private_key)?)
+        Ok(EcPrivateKey::from_der(private_key_info.private_key.as_bytes())?.try_into()?)
     }
 }
 
@@ -72,7 +72,19 @@ where
             parameters: Some((&C::OID).into()),
         };
 
-        let ec_private_key = self.to_sec1_der()?;
+        let private_key_bytes = Zeroizing::new(self.to_bytes());
+        let public_key_bytes = self.public_key().to_encoded_point(false);
+
+        // TODO(tarcieri): unify with `to_sec1_der()` by building an owned `EcPrivateKey`
+        let ec_private_key = Zeroizing::new(
+            EcPrivateKey {
+                private_key: &private_key_bytes,
+                parameters: None,
+                public_key: Some(public_key_bytes.as_bytes()),
+            }
+            .to_der()?,
+        );
+
         let pkcs8_key = pkcs8::PrivateKeyInfoRef::new(
             algorithm_identifier,
             OctetStringRef::new(&ec_private_key)?,