chore(crypto): CRP-1488 Update the wycheproof test data

icwizardmonke · May 22, 2023 · df535c0 · df535c0
1 parent cf25f0d
commit df535c0
Show file tree

Hide file tree

Showing 21 changed files with 129 additions and 91 deletions.
diff --git a/Cargo.Bazel.StaticOpenSSL.json.lock b/Cargo.Bazel.StaticOpenSSL.json.lock
@@ -1,5 +1,5 @@
 {
-  "checksum": "9ab4c70d688fc847051fa83ea67a5b26978dd0d77273e25e582079017c625ed6",
+  "checksum": "87a5af9327ba381852c335988c62c22781e11ca3f694a38d9056d943286614e3",
   "crates": {
     "abnf 0.12.0": {
       "name": "abnf",
@@ -13766,7 +13766,7 @@
               "target": "wsl"
             },
             {
-              "id": "wycheproof 0.4.0",
+              "id": "wycheproof 0.5.0",
               "target": "wycheproof"
             },
             {
@@ -50151,8 +50151,8 @@
         },
         {
           "Binary": {
-            "crate_name": "exit",
-            "crate_root": "src/bin/exit.rs",
+            "crate_name": "sleep",
+            "crate_root": "src/bin/sleep.rs",
             "srcs": {
               "include": [
                 "**/*.rs"
@@ -50175,8 +50175,8 @@
         },
         {
           "Binary": {
-            "crate_name": "sleep",
-            "crate_root": "src/bin/sleep.rs",
+            "crate_name": "exit",
+            "crate_root": "src/bin/exit.rs",
             "srcs": {
               "include": [
                 "**/*.rs"
@@ -54654,13 +54654,13 @@
       },
       "license": "MIT"
     },
-    "wycheproof 0.4.0": {
+    "wycheproof 0.5.0": {
       "name": "wycheproof",
-      "version": "0.4.0",
+      "version": "0.5.0",
       "repository": {
         "Http": {
-          "url": "https://crates.io/api/v1/crates/wycheproof/0.4.0/download",
-          "sha256": "183c789620c674b79dac33cd3aadb6c8006b66cba6a680402235aaebc743e3df"
+          "url": "https://crates.io/api/v1/crates/wycheproof/0.5.0/download",
+          "sha256": "ccaef718f69c19f7f66faacc10f747180ceb4883a53c35548be4cac4cec1591c"
         }
       },
       "targets": [
@@ -54703,8 +54703,8 @@
           ],
           "selects": {}
         },
-        "edition": "2018",
-        "version": "0.4.0"
+        "edition": "2021",
+        "version": "0.5.0"
       },
       "license": "Apache-2.0"
     },

diff --git a/Cargo.Bazel.StaticOpenSSL.toml.lock b/Cargo.Bazel.StaticOpenSSL.toml.lock
@@ -10084,9 +10084,9 @@ checksum = "f8dab7ac864710bdea6594becbea5b5050333cf34fefb0dc319567eb347950d4"
 
 [[package]]
 name = "wycheproof"
-version = "0.4.0"
+version = "0.5.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "183c789620c674b79dac33cd3aadb6c8006b66cba6a680402235aaebc743e3df"
+checksum = "ccaef718f69c19f7f66faacc10f747180ceb4883a53c35548be4cac4cec1591c"
 dependencies = [
  "base64 0.13.1",
  "hex",

diff --git a/Cargo.Bazel.json.lock b/Cargo.Bazel.json.lock
@@ -1,5 +1,5 @@
 {
-  "checksum": "ab80b814e81e0f34b3bede1529dc3855eb18e6f5dd5fe7ca547337e23d1a7b3c",
+  "checksum": "6ee9224073d1c20dd9523a8a061a5500f3716a0c62b19612a3da665af44d9c13",
   "crates": {
     "abnf 0.12.0": {
       "name": "abnf",
@@ -13766,7 +13766,7 @@
               "target": "wsl"
             },
             {
-              "id": "wycheproof 0.4.0",
+              "id": "wycheproof 0.5.0",
               "target": "wycheproof"
             },
             {
@@ -54630,13 +54630,13 @@
       },
       "license": "MIT"
     },
-    "wycheproof 0.4.0": {
+    "wycheproof 0.5.0": {
       "name": "wycheproof",
-      "version": "0.4.0",
+      "version": "0.5.0",
       "repository": {
         "Http": {
-          "url": "https://crates.io/api/v1/crates/wycheproof/0.4.0/download",
-          "sha256": "183c789620c674b79dac33cd3aadb6c8006b66cba6a680402235aaebc743e3df"
+          "url": "https://crates.io/api/v1/crates/wycheproof/0.5.0/download",
+          "sha256": "ccaef718f69c19f7f66faacc10f747180ceb4883a53c35548be4cac4cec1591c"
         }
       },
       "targets": [
@@ -54679,8 +54679,8 @@
           ],
           "selects": {}
         },
-        "edition": "2018",
-        "version": "0.4.0"
+        "edition": "2021",
+        "version": "0.5.0"
       },
       "license": "Apache-2.0"
     },

diff --git a/Cargo.Bazel.toml.lock b/Cargo.Bazel.toml.lock
@@ -10084,9 +10084,9 @@ checksum = "f8dab7ac864710bdea6594becbea5b5050333cf34fefb0dc319567eb347950d4"
 
 [[package]]
 name = "wycheproof"
-version = "0.4.0"
+version = "0.5.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "183c789620c674b79dac33cd3aadb6c8006b66cba6a680402235aaebc743e3df"
+checksum = "ccaef718f69c19f7f66faacc10f747180ceb4883a53c35548be4cac4cec1591c"
 dependencies = [
  "base64 0.13.1",
  "hex",

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/bazel/external_crates.bzl b/bazel/external_crates.bzl
@@ -1237,7 +1237,7 @@ def external_crates_repository(name, static_openssl, cargo_lockfile, lockfile):
                 version = "^0.1.0",
             ),
             "wycheproof": crate.spec(
-                version = "^0.4",
+                version = "^0.5",
             ),
             "x509-parser": crate.spec(
                 version = "^0.12.0",

diff --git a/rs/crypto/ecdsa_secp256k1/Cargo.toml b/rs/crypto/ecdsa_secp256k1/Cargo.toml
@@ -15,4 +15,4 @@ lazy_static = "1.4"
 
 [dev-dependencies]
 hex = "0.4"
-wycheproof = "0.4"
+wycheproof = "0.5"
diff --git a/rs/crypto/ecdsa_secp256k1/tests/tests.rs b/rs/crypto/ecdsa_secp256k1/tests/tests.rs
@@ -16,12 +16,7 @@ fn should_pass_wycheproof_ecdsa_secp256k1_verification_tests() -> Result<(), Key
             // The Wycheproof ECDSA tests do not normalize s so we must use
             // the verification method that accepts either valid s
             let accepted = pk.verify_signature_with_malleability(&test.msg, &test.sig);
-
-            if accepted {
-                assert_eq!(test.result, wycheproof::TestResult::Valid);
-            } else if test.result != wycheproof::TestResult::Invalid {
-                assert!(test.flags.contains(&TestFlag::SigSize));
-            }
+            assert_eq!(accepted, test.result == wycheproof::TestResult::Valid);
         }
     }
 

diff --git a/rs/crypto/ecdsa_secp256r1/Cargo.toml b/rs/crypto/ecdsa_secp256r1/Cargo.toml
@@ -17,4 +17,4 @@ ic-crypto-getrandom-for-wasm = { path = "../getrandom_for_wasm" }
 
 [dev-dependencies]
 hex = "0.4"
-wycheproof = "0.4"
+wycheproof = "0.5"
diff --git a/rs/crypto/ecdsa_secp256r1/tests/tests.rs b/rs/crypto/ecdsa_secp256r1/tests/tests.rs
@@ -17,8 +17,8 @@ fn should_pass_wycheproof_ecdsa_secp256r1_verification_tests() -> Result<(), Key
 
             if accepted {
                 assert_eq!(test.result, wycheproof::TestResult::Valid);
-            } else if test.result != wycheproof::TestResult::Invalid {
-                assert!(test.flags.contains(&TestFlag::SigSize));
+            } else {
+                assert_eq!(test.result, wycheproof::TestResult::Invalid);
             }
         }
     }

diff --git a/rs/crypto/internal/crypto_lib/basic_sig/ecdsa_secp256k1/Cargo.toml b/rs/crypto/internal/crypto_lib/basic_sig/ecdsa_secp256k1/Cargo.toml
@@ -26,4 +26,4 @@ proptest-derive = "0.3.0"
 rand = "0.8"
 regex = "1.5"
 strum = "0.23.0"
-wycheproof = "0.4"
+wycheproof = "0.5"
diff --git a/rs/crypto/internal/crypto_lib/basic_sig/ecdsa_secp256k1/tests/wycheproof.rs b/rs/crypto/internal/crypto_lib/basic_sig/ecdsa_secp256k1/tests/wycheproof.rs
@@ -19,9 +19,9 @@ fn should_pass_wycheproof_test_vectors() {
         };
 
         for test in &test_group.tests {
-            let sig = match types::SignatureBytes::try_from(test.sig.clone()).ok() {
+            let sig = match types::SignatureBytes::try_from(test.sig.to_vec()).ok() {
                 None => {
-                    assert!(test.result.must_fail() || test.flags.contains(&TestFlag::SigSize));
+                    assert!(test.result.must_fail());
                     continue;
                 }
                 Some(sig) => sig,

diff --git a/rs/crypto/internal/crypto_lib/basic_sig/ecdsa_secp256r1/Cargo.toml b/rs/crypto/internal/crypto_lib/basic_sig/ecdsa_secp256r1/Cargo.toml
@@ -26,4 +26,4 @@ proptest = "1.0"
 proptest-derive = "0.3.0"
 regex = "1.5"
 strum = "0.23.0"
-wycheproof = "0.4"
+wycheproof = "0.5"
diff --git a/rs/crypto/internal/crypto_lib/basic_sig/ecdsa_secp256r1/tests/wycheproof.rs b/rs/crypto/internal/crypto_lib/basic_sig/ecdsa_secp256r1/tests/wycheproof.rs
@@ -19,9 +19,9 @@ fn should_pass_wycheproof_test_vectors() {
         };
 
         for test in &test_group.tests {
-            let sig = match types::SignatureBytes::try_from(test.sig.clone()).ok() {
+            let sig = match types::SignatureBytes::try_from(test.sig.to_vec()).ok() {
                 None => {
-                    assert!(test.result.must_fail() || test.flags.contains(&TestFlag::SigSize));
+                    assert!(test.result.must_fail());
                     continue;
                 }
                 Some(sig) => sig,

diff --git a/rs/crypto/internal/crypto_lib/basic_sig/ed25519/Cargo.toml b/rs/crypto/internal/crypto_lib/basic_sig/ed25519/Cargo.toml
@@ -29,4 +29,4 @@ proptest = "1.0"
 proptest-derive = "0.3.0"
 serde_cbor = "0.11.1"
 strum = "0.23.0"
-wycheproof = "0.4"
+wycheproof = "0.5"
diff --git a/rs/crypto/internal/crypto_lib/basic_sig/ed25519/src/api/tests.rs b/rs/crypto/internal/crypto_lib/basic_sig/ed25519/src/api/tests.rs
@@ -164,9 +164,8 @@ mod sign {
 }
 
 mod wycheproof {
-    use crate::api::SecretArray;
-    use crate::types::{PublicKeyBytes, SecretKeyBytes, SignatureBytes};
-    use crate::{sign, verify};
+    use crate::types::{PublicKeyBytes, SignatureBytes};
+    use crate::verify;
     use std::convert::TryInto;
 
     #[test]
@@ -175,10 +174,14 @@ mod wycheproof {
             .expect("Unable to load tests");
 
         for test_group in test_set.test_groups {
-            let pk = PublicKeyBytes(test_group.key.pk.try_into().expect("Unexpected key size"));
-
-            let sk_bytes: [u8; 32] = test_group.key.sk.try_into().expect("Unexpected key size");
-            let sk = SecretKeyBytes(SecretArray::new_and_dont_zeroize_argument(&sk_bytes));
+            let pk = PublicKeyBytes(
+                test_group
+                    .key
+                    .pk
+                    .as_ref()
+                    .try_into()
+                    .expect("Unexpected key size"),
+            );
 
             for test in test_group.tests {
                 /*
@@ -188,20 +191,16 @@ mod wycheproof {
                 if test.sig.len() != 64 {
                     continue;
                 }
-                let test_sig =
-                    SignatureBytes(test.sig.try_into().expect("Unexpected signature size"));
-
-                let gen_sig = sign(&test.msg, &sk).expect("Generating signature failed");
+                let test_sig = SignatureBytes(
+                    test.sig
+                        .as_ref()
+                        .try_into()
+                        .expect("Unexpected signature size"),
+                );
 
                 if test.result == wycheproof::TestResult::Valid {
-                    // If test is valid verify that our generated signature matches (Ed25519 should
-                    // be deterministic) and that the signature verifies
                     assert!(verify(&test_sig, &test.msg, &pk).is_ok());
-                    assert_eq!(test_sig, gen_sig);
                 } else {
-                    // Otherwise check that the test signature fails but our generated signature
-                    // is accepted
-                    assert!(verify(&gen_sig, &test.msg, &pk).is_ok());
                     assert!(verify(&test_sig, &test.msg, &pk).is_err());
                 }
             }

diff --git a/rs/crypto/internal/crypto_lib/basic_sig/rsa_pkcs1/Cargo.toml b/rs/crypto/internal/crypto_lib/basic_sig/rsa_pkcs1/Cargo.toml
@@ -18,4 +18,4 @@ ic-crypto-getrandom-for-wasm = { path = "../../../../getrandom_for_wasm" }
 hex = "0.4.2"
 serde = { version = "1.0.115", features = ["derive"] }
 serde_json = "1.0.54"
-wycheproof = "0.4.0"
+wycheproof = "0.5"
diff --git a/rs/crypto/internal/crypto_lib/basic_sig/rsa_pkcs1/src/lib.rs b/rs/crypto/internal/crypto_lib/basic_sig/rsa_pkcs1/src/lib.rs
@@ -159,6 +159,37 @@ impl RsaPublicKey {
     /// (https://datatracker.ietf.org/doc/html/rfc8017#section-8.2) and used by
     /// the IC for webauthn (https://docs.dfinity.systems/spec/public/#webauthn)
     pub fn verify_pkcs1_sha256(&self, message: &[u8], signature: &[u8]) -> CryptoResult<()> {
+        // The version of rsa crate that we use has a signature malleability bug
+        // https://github.com/RustCrypto/RSA/issues/272 which does not seem
+        // directly to be any real problem, but worth avoiding.
+        //
+        // There is an updated version of the crate but upgrading
+        // requires significant changes, see CRP-2038
+        let modulus_bytes = (self.key.n().bits() + 7) / 8; // rounding up
+
+        if signature.len() > modulus_bytes {
+            return Err(CryptoError::SignatureVerification {
+                algorithm: AlgorithmId::RsaSha256,
+                public_key_bytes: self.as_der().to_vec(),
+                sig_bytes: signature.to_vec(),
+                internal_error: format!(
+                    "Signature is {} bytes but public modulus only {}",
+                    signature.len(),
+                    modulus_bytes
+                ),
+            });
+        }
+        let sig = rsa::BigUint::from_bytes_be(signature);
+
+        if &sig > self.key.n() {
+            return Err(CryptoError::SignatureVerification {
+                algorithm: AlgorithmId::RsaSha256,
+                public_key_bytes: self.as_der().to_vec(),
+                sig_bytes: signature.to_vec(),
+                internal_error: "Signature is larger than public modulus".to_string(),
+            });
+        }
+
         let digest = Sha256::hash(message);
         let padding = rsa::PaddingScheme::PKCS1v15Sign {
             hash: Some(rsa::Hash::SHA2_256),

diff --git a/rs/crypto/internal/crypto_lib/basic_sig/rsa_pkcs1/tests/tests.rs b/rs/crypto/internal/crypto_lib/basic_sig/rsa_pkcs1/tests/tests.rs
@@ -75,6 +75,40 @@ fn should_be_able_to_verify_rsa_signature() {
     assert!(key.verify_pkcs1_sha256(&msg, &sig).is_err());
 }
 
+#[test]
+fn should_reject_valid_rsa_signature_with_extra_zeros() {
+    let der = hex::decode("30820122300D06092A864886F70D01010105000382010F003082010A0282010100A7078A1A8FDE64C537AE5CA8D4B3A9139D68050CF76E45E77DBE47CECEB162F7095ADB6260998775203AA42A444F865DEB995C2B70B548ECEE01695DEB069ED18744C12FD24AEACDA4B2B7A5E97E7167CAF7D4B8904CE20CA9A8928978CA957FF2D9FCAE0859618B0AD74C164FAF5AB1DE7D7228A89BD3F8B497CEF9E45E1203CC40EE252140157C331A584F3916E569A8C39573D542A3577FB12332EBD3C9F421C9EF8A23D5ACF6BA439F7C3D6B73BA4E56B9B8EFBC42A2E5E734B99FDF7AB046813E43C65C926793919A7AE54F71AAF57C6876001A0558BC847D7555B1AE71F56A70272D786BE69A23A21A56C426371BD9882D40E7ECA6B7DA5D8169B7030F0203010001").unwrap();
+    let key = RsaPublicKey::from_der_spki(&der).unwrap();
+
+    let sig = hex::decode("0000000000000000007416E0A20E46CEF9FC09FA87D4C324502839EB8DEAFEF7CA5ADEC1044523232E66B32F4A497AA84FC4069182AD4A921B43DBCBD3ACCA870F887299692E23555086169F89EA1DD4856DC9FEB4E96B1661F803B784B4BE9A0E36B739A38126996912D92343688DB58F24CF8066250E2B04EE166A1C9C924D1AA9DED87D8A24E07CF35B02CA487B1632BA2508FF2B28F880983926A75D67EB83292BF77EE9B283337D841F04253C846BD66E63E50D8B326DCE1EC67A95A9D31DBDF3DCA5E8C09CA8CCE2026A3A5AE56250EC57CDE67A745FA1B1CC83473BA167AD1F8311A3D071184D03380B80C7921457CE282B9222FE805E506B53C5F798917B1A45044D2E896D").unwrap();
+    let msg = hex::decode("616263").unwrap();
+
+    assert!(key.verify_pkcs1_sha256(&msg, &sig).is_err());
+}
+
+#[test]
+fn should_reject_valid_rsa_signature_with_signature_greater_than_modulus() {
+    let der = hex::decode("30820122300D06092A864886F70D01010105000382010F003082010A02820101009D698E1ECBFDDF304B77CE796A1AED1078CB15688510BEE181C622A674131176DD89B0E594C3B8E8934793952A22B36B428389F0902809A45278D52371A6299C7B1966C29AD1E2F221AC5ABB25B3C09810FDA816F9F38652C1DCD5D439764454D1CBF7C5E7D2AB4092134A010EC69C8BBCA7C32D8C6FC4888C37F86595124C99D38321C5C73E03180660617311A01D7759A64339CF285C0C0CF0026ECD79C00B141E640E62924B88CF7038713FFB25E8DB7E61306D781C35C4AF70EF3EEF5013A1D50D6180B7DE5D717642A3293B59AB78CA988D0F7493848734425419B47BA3CC1041E45D9370844FC2CAFC0A7E28400D33B338CD74B8BF74980064F9CD77750203010001").unwrap();
+    let key = RsaPublicKey::from_der_spki(&der).unwrap();
+
+    let msg = hex::decode("68656C6C6F").unwrap();
+
+    // First check the valid signature
+    let sig = hex::decode("09F17E7A37E0485152670F35F78718EF46285D3CA9C03EFEDEACB81CC3322B50647D750581497AC2A6B6B2A408903B54EBCE5531104F6FDA50504E88EDA03786EB43A063A496BA68BEE192E820EACC08A731BEB33B945E81729077E9BBE8E2362CE97F19C3F9D06B326AC5004B3E8326FCF434206DFDDB37FA84F21C9E019BE94649BAAAFDAC3A4C7F060EBDB3DBB08DF13201A3B573632BB083F8E1B726232F52DD1E628C9F58A18D31535FD099EEE7384F2078004B2FDFD8A0FFE2BE457BD32502E5F8668FA6F3545ED82D00D6EAE656CF7BAED73F80938D0826D3B4C3586A9DF72B10908FE0AF8F44D41D05B676AEABF85A8A0C61E9784D50B921606601DE").unwrap();
+
+    assert!(key.verify_pkcs1_sha256(&msg, &sig).is_ok());
+
+    // Now try to verify sig + modulus, which should fail:
+
+    let sig_plus = hex::decode("a75b0c9903de27819ddeddaf61a205ffbef372a52ed0fde06072dac337453cc7420725eb160d33ab39fe463932b2eec02e51df21a077797ea2c923ac5f466123665d07263f689d5ae08deda3469e8ca0b82f66ca3587e4d4346d4dbdf55f268afeb576dfabcc7babc47e0f015a051fb2b99bf74dfa6d9fc086bcea823313e88319ccdc70c4ea3d6485667030c57bce054ad844dd849bbf37bd73fb50849fe33a66fb8270ef31a42a5ca18bd1109514d013cd81a86dc34c159d5070d1fd34cbe6c6d7f359e7478550c5d51ad02a124491cf9a143be6b41418143c6927ce77d40e6a076cf4ee235133df079f1910349eeeb92c0dc2d9d6a237c1e8b9865a337953").unwrap();
+
+    // Verify that we are not hitting the check that the signatures are of unexpected length:
+    assert_eq!(sig.len(), sig_plus.len());
+
+    // Verify that signatures greater than the modulus are rejected:
+    assert!(key.verify_pkcs1_sha256(&msg, &sig_plus).is_err());
+}
+
 #[test]
 fn should_be_able_to_serialize_and_deserialize_rsa_pubkey() {
     let der = hex::decode("30820122300D06092A864886F70D01010105000382010F003082010A0282010100A7078A1A8FDE64C537AE5CA8D4B3A9139D68050CF76E45E77DBE47CECEB162F7095ADB6260998775203AA42A444F865DEB995C2B70B548ECEE01695DEB069ED18744C12FD24AEACDA4B2B7A5E97E7167CAF7D4B8904CE20CA9A8928978CA957FF2D9FCAE0859618B0AD74C164FAF5AB1DE7D7228A89BD3F8B497CEF9E45E1203CC40EE252140157C331A584F3916E569A8C39573D542A3577FB12332EBD3C9F421C9EF8A23D5ACF6BA439F7C3D6B73BA4E56B9B8EFBC42A2E5E734B99FDF7AB046813E43C65C926793919A7AE54F71AAF57C6876001A0558BC847D7555B1AE71F56A70272D786BE69A23A21A56C426371BD9882D40E7ECA6B7DA5D8169B7030F0203010001").unwrap();