From 657914f7787a0ac5bdaad8c15d41a91425e1c8cf Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Tue, 20 Sep 2022 09:13:31 -0400
Subject: [PATCH 1/2] Add a new constant-time is_zero() check for RsaIdentity

There are some places in the protocol where we have an all-zero RSA
identity that does not truly represent a key, but rather represents
an absent or unknown key.  For these, it's better to use
`RsaIdentity::is_zero` instead of manually checking for a set of
zero bytes: it expresses the intent better, and ensures that the
operation is constant-time.

I am deliberately not introducing a more general IsZero trait here,
or implementing is_zero for anything else: This is the only one we
seem to need right now.  We can generalize it later if we have to.
---
 crates/tor-llcrypto/semver.md     |  2 ++
 crates/tor-llcrypto/src/pk/rsa.rs | 11 +++++++++++
 crates/tor-llcrypto/tests/misc.rs | 20 ++++++++++++++++++++
 3 files changed, 33 insertions(+)
 create mode 100644 crates/tor-llcrypto/semver.md

diff --git a/crates/tor-llcrypto/semver.md b/crates/tor-llcrypto/semver.md
new file mode 100644
index 000000000..4759b601c
--- /dev/null
+++ b/crates/tor-llcrypto/semver.md
@@ -0,0 +1,2 @@
+MODIFIED: New is_zero API on RsaIdentity.
+
diff --git a/crates/tor-llcrypto/src/pk/rsa.rs b/crates/tor-llcrypto/src/pk/rsa.rs
index 3ff326ccd..7376c71cd 100644
--- a/crates/tor-llcrypto/src/pk/rsa.rs
+++ b/crates/tor-llcrypto/src/pk/rsa.rs
@@ -159,6 +159,17 @@ impl RsaIdentity {
             Ok(()) => Some(RsaIdentity::from(array)),
         }
     }
+
+    /// Return true if this `RsaIdentity` is composed entirely of zero-valued
+    /// bytes.
+    ///
+    /// Such all-zero values should not be used internally, since they are not
+    /// the ID of any valid key.  Instead, they are used in some places in the
+    /// Tor protocols.
+    pub fn is_zero(&self) -> bool {
+        // We do a constant-time comparison to avoid side-channels.
+        self.id.ct_eq(&[0; RSA_ID_LEN]).into()
+    }
 }
 
 impl From<[u8; 20]> for RsaIdentity {
diff --git a/crates/tor-llcrypto/tests/misc.rs b/crates/tor-llcrypto/tests/misc.rs
index dd5572061..e1d86bb19 100644
--- a/crates/tor-llcrypto/tests/misc.rs
+++ b/crates/tor-llcrypto/tests/misc.rs
@@ -63,6 +63,26 @@ fn test_wrong_hex_rsa_ids() {
     assert!(RsaIdentity::from_hex("listen carefully, spider of destiny  -FZ").is_none());
 }
 
+#[test]
+fn test_rsa_is_zero() {
+    use ll::pk::rsa::RsaIdentity;
+    assert!(
+        RsaIdentity::from_hex("0000000000000000000000000000000000000000")
+            .unwrap()
+            .is_zero()
+    );
+    assert!(
+        !RsaIdentity::from_hex("000000000000000000000000000000000000000F")
+            .unwrap()
+            .is_zero()
+    );
+    assert!(
+        !RsaIdentity::from_hex("F000000000000000000000000000000000000000")
+            .unwrap()
+            .is_zero()
+    );
+}
+
 // TODO: Proper tests for RSA keys
 
 #[test]

From 3e922e5ededfc9c7915407cf30171342078cc30e Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Tue, 20 Sep 2022 09:36:49 -0400
Subject: [PATCH 2/2] Intoduce1: Use a constant-time check for all-zero
 RsaIdentity

As a matter of good crypto practice, we shouldn't use
short-circuiting checks to compare keys or key-like objects, since
the amount of time taken by those checks can leak information about
their inputs.

I don't think it's actually _necessary_ to use a constant-time
operation in this case, but let's establish the precedent.

This is a follow-up to !724.
---
 crates/tor-cell/src/relaycell/onion_service.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/crates/tor-cell/src/relaycell/onion_service.rs b/crates/tor-cell/src/relaycell/onion_service.rs
index c39396d0c..796bdde6a 100644
--- a/crates/tor-cell/src/relaycell/onion_service.rs
+++ b/crates/tor-cell/src/relaycell/onion_service.rs
@@ -7,6 +7,7 @@ use super::msg;
 use caret::caret_int;
 use tor_bytes::{EncodeError, EncodeResult, Error as BytesError, Readable, Result, Writeable};
 use tor_bytes::{Reader, Writer};
+use tor_llcrypto::pk::rsa::RsaIdentity;
 use tor_units::BoundedInt32;
 
 caret_int! {
@@ -269,8 +270,8 @@ impl msg::Body for Introduce1 {
         msg::RelayMsg::Introduce1(self)
     }
     fn decode_from_reader(r: &mut Reader<'_>) -> Result<Self> {
-        let legacy_key_id: [u8; 20] = r.extract()?;
-        if legacy_key_id.iter().any(|b| *b != 0_u8) {
+        let legacy_key_id: RsaIdentity = r.extract()?;
+        if !legacy_key_id.is_zero() {
             return Err(BytesError::BadMessage("legacy key id in Introduce1."));
         }
         let auth_key_type = r.take_u8()?.into();