From 43dbbf14a54a486778204bb4988cd7ddf125d204 Mon Sep 17 00:00:00 2001
From: Patrick Amrein <47777208+ubamrein@users.noreply.github.com>
Date: Mon, 29 Nov 2021 00:32:05 +0100
Subject: [PATCH] Parse `n` and `e` as BigInts (#53)

* add bigint dependency
parse n and e as bigints to prevent signed/unsigned issues

* add  rsa test
---
 Cargo.toml         |  1 +
 src/core/crypto.rs | 70 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 23469a0c..cd1bed5e 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -40,6 +40,7 @@ serde_path_to_error = "0.1"
 serde-value = "0.6"
 untrusted = "0.7"
 url = { version = "2.1", features = ["serde"] }
+num-bigint = "0.4.3"
 
 [dev-dependencies]
 color-backtrace = { version = "0.4" }
diff --git a/src/core/crypto.rs b/src/core/crypto.rs
index 346532b1..bfc0e9ed 100644
--- a/src/core/crypto.rs
+++ b/src/core/crypto.rs
@@ -1,3 +1,4 @@
+use num_bigint::{BigInt, Sign};
 use ring::hmac;
 use ring::rand::SecureRandom;
 use ring::signature as ring_signature;
@@ -94,10 +95,16 @@ pub fn verify_rsa_signature(
     msg: &[u8],
     signature: &[u8],
 ) -> Result<(), SignatureVerificationError> {
-    let (n, e) = rsa_public_key(&key).map_err(SignatureVerificationError::InvalidKey)?;
+    let (n, e) = rsa_public_key(key).map_err(SignatureVerificationError::InvalidKey)?;
+    // let's n and e as a big integers to prevent issues with leading zeros
+    // according to https://datatracker.ietf.org/doc/html/rfc7518#section-6.3.1.1
+    // `n` is alwasy unsigned (hence has sign plus)
+
+    let n_bigint = BigInt::from_bytes_be(Sign::Plus, n.deref());
+    let e_bigint = BigInt::from_bytes_be(Sign::Plus, e.deref());
     let public_key = ring_signature::RsaPublicKeyComponents {
-        n: n.deref(),
-        e: e.deref(),
+        n: &n_bigint.to_bytes_be().1,
+        e: &e_bigint.to_bytes_be().1,
     };
 
     public_key
@@ -131,3 +138,60 @@ pub fn verify_ec_signature(
         .verify(msg, signature)
         .map_err(|_| SignatureVerificationError::CryptoError("EC Signature was wrong".to_string()))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::ops::Deref;
+
+    use crate::{
+        core::{crypto::rsa_public_key, CoreJsonWebKey},
+        SignatureVerificationError,
+    };
+
+    #[test]
+    fn test_leading_zeros_are_parsed_correctly() {
+        // The message we signed
+        let msg = "THIS IS A SIGNATURE TEST";
+        let signature = base64::decode_config("bg0ohqKwYHAiODeG6qkJ-6IhodN7LGPxAh4hbWeIoBdSXrXMt8Ft8U0BV7vANPvF56h20XB9C0021x2kt7iAbMgPNcZ7LCuXMPPq04DrBpMHafH5BXBwnyDKJKrzDm5sfr6OgEkcxSLHaSJ6gTWQ3waPt6_SeH2-Fi74rg13MHyX-0iqz7bZveoBbGIs5yQCwvXgrDS9zW5LUwUHozHfE6FuSi_Z92ioXeu7FHHDg1KFfg3hs8ZLx4wAX15Vw2GCQOzvyNdbItxXRLnrN1NPqxFquVNo5RGlx6ihR1Jfe7y_n0NSR2q2TuU4cIwR0LRwEaANy5SDqtleQPrTEn8nGQ", base64::URL_SAFE_NO_PAD).unwrap();
+        // RSA pub key with leading 0
+        let key : CoreJsonWebKey = serde_json::from_value(serde_json::json!(
+            {
+            "kty": "RSA",
+            "e": "AQAB",
+            "use": "sig",
+            "kid": "TEST_KEY_ID",
+            "alg": "RS256",
+            "n": "AN0M6Y760b9Ok2PxDOps1TgSmiOaR9mLIfUHtZ_o-6JypOckGcl1CxrteyokOb3WyDsfIAN9fFNrycv5YoLKO7sh0IcfzNEXFgzK84HTBcGuqhN8NV98Z6N9EryUrgJYsJeVoPYm0MzkDe4NyWHhnq-9OyNCQzVELH0NhhViQqRyM92OPrJcQlk8s3ZvcgRmkd-rEtRua8SbS3GEvfvgweVy5-qcJCGoziKfx-IteMOm6yKoHvqisKb91N-qw_kSS4YQUx-DZVDo2g24F7VIbcYzJGUOU674HUF1j-wJyXzG3VV8lAXD8hABs5Lh87gr8_hIZD5gbYBJRObJk9XZbfk"
+            }
+        )).unwrap();
+
+        // Old way of verifying the jwt, take the modulus directly form the JWK
+        let (n, e) = rsa_public_key(&key)
+            .map_err(SignatureVerificationError::InvalidKey)
+            .unwrap();
+
+        let public_key = ring_signature::RsaPublicKeyComponents {
+            n: n.deref(),
+            e: e.deref(),
+        };
+        // This fails, since ring expects the keys to have no leading zeros
+        assert! {
+            public_key
+                .verify(
+                    &ring_signature::RSA_PKCS1_2048_8192_SHA256,
+                    msg.as_bytes(),
+                    &signature,
+                ).is_err()
+        };
+        // This should succeed as the function uses big-integers to actually harmonize parsing
+        assert! {
+            verify_rsa_signature(
+                &key,
+                &ring_signature::RSA_PKCS1_2048_8192_SHA256,
+                msg.as_bytes(),
+                &signature,
+            ).is_ok()
+        }
+    }
+}