From bb1ee04e6e704a4d6c2430f89a93b918aa1b9f23 Mon Sep 17 00:00:00 2001
From: Germano Percossi <germano.percossi@gmail.com>
Date: Fri, 28 Jun 2024 17:53:06 +0100
Subject: [PATCH] Prevent panic when creating a Schnorr from slice (#1056)

Introduced new safe builders, same as bign256 and sm2:
- `split_at` can panic if it receives a slice shorter than `Self::BYTES / 2`
- `split_at` is now use in the new `from_bytes` that accepts only byte arrays
  or the right size
- `from_slice` uses a safe conversion that does not panic when the slice is too short
- `try_from` slice uses `from_slice` internally, so it does not panic when a short slice
  is passed
- introduce safe type conversion from SignatureBytes
---
 k256/src/schnorr.rs | 60 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 12 deletions(-)

diff --git a/k256/src/schnorr.rs b/k256/src/schnorr.rs
index 111c549e..1036ecaa 100644
--- a/k256/src/schnorr.rs
+++ b/k256/src/schnorr.rs
@@ -117,6 +117,31 @@ impl Signature {
     fn split(&self) -> (&FieldElement, &NonZeroScalar) {
         (self.r(), self.s())
     }
+
+    /// Parse a Secp256k1 signature from a byte array.
+    pub fn from_bytes(bytes: &SignatureBytes) -> Result<Self> {
+        let (r_bytes, s_bytes) = bytes.split_at(Self::BYTE_SIZE / 2);
+
+        let r: FieldElement =
+            Option::from(FieldElement::from_bytes(FieldBytes::from_slice(r_bytes)))
+                .ok_or_else(Error::new)?;
+
+        // one of the rules for valid signatures: !is_infinite(R);
+        if r.is_zero().into() {
+            return Err(Error::new());
+        }
+
+        let s = NonZeroScalar::try_from(s_bytes).map_err(|_| Error::new())?;
+
+        Ok(Self { r, s })
+    }
+
+    /// Parse a Secp256k1 signature from a byte slice.
+    pub fn from_slice(bytes: &[u8]) -> Result<Self> {
+        SignatureBytes::try_from(bytes)
+            .map_err(|_| Error::new())?
+            .try_into()
+    }
 }
 
 impl Eq for Signature {}
@@ -139,24 +164,27 @@ impl PartialEq for Signature {
     }
 }
 
-impl TryFrom<&[u8]> for Signature {
+impl TryFrom<SignatureBytes> for Signature {
     type Error = Error;
 
-    fn try_from(bytes: &[u8]) -> Result<Signature> {
-        let (r_bytes, s_bytes) = bytes.split_at(Self::BYTE_SIZE / 2);
+    fn try_from(signature: SignatureBytes) -> Result<Signature> {
+        Signature::from_bytes(&signature)
+    }
+}
 
-        let r: FieldElement =
-            Option::from(FieldElement::from_bytes(FieldBytes::from_slice(r_bytes)))
-                .ok_or_else(Error::new)?;
+impl TryFrom<&SignatureBytes> for Signature {
+    type Error = Error;
 
-        // one of the rules for valid signatures: !is_infinite(R);
-        if r.is_zero().into() {
-            return Err(Error::new());
-        }
+    fn try_from(signature: &SignatureBytes) -> Result<Signature> {
+        Signature::from_bytes(signature)
+    }
+}
 
-        let s = NonZeroScalar::try_from(s_bytes).map_err(|_| Error::new())?;
+impl TryFrom<&[u8]> for Signature {
+    type Error = Error;
 
-        Ok(Self { r, s })
+    fn try_from(bytes: &[u8]) -> Result<Signature> {
+        Signature::from_slice(bytes)
     }
 }
 
@@ -509,4 +537,12 @@ mod tests {
             );
         }
     }
+
+    #[test]
+    fn try_from() {
+        // Pass an invalid signature (shorter than Self::BYTES / 2) and make sure
+        // it does not panic, but return Err
+        let invalid_signature = [111; 24];
+        assert_eq!(Signature::try_from(&invalid_signature[..]).is_err(), true);
+    }
 }