From 0c8c87362b5cae9d48fae1dcdf3b5536e36641e5 Mon Sep 17 00:00:00 2001
From: ArmanKolozyan <92608959+ArmanKolozyan@users.noreply.github.com>
Date: Tue, 3 Jun 2025 13:19:53 +0200
Subject: [PATCH] fix: added missing range checks in ShaBytesDynamic (#579)

* fix: added missing range checks for Sha1Bytes

* more descriptive comments around range checks

* added range assumption of Sha1Bytes

* added range assumption of Sha1General

---------

Co-authored-by: nicoshark <i.am.nicoshark@gmail.com>
Co-authored-by: turnoffthiscomputer <98749896+remicolin@users.noreply.github.com>
---
 .../hasher/shaBytes/dynamic/sha1Bytes.circom  |  6 +++++
 .../hasher/shaBytes/shaBytesDynamic.circom    | 27 +++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/circuits/circuits/utils/crypto/hasher/shaBytes/dynamic/sha1Bytes.circom b/circuits/circuits/utils/crypto/hasher/shaBytes/dynamic/sha1Bytes.circom
index d45616059..a25ab6149 100644
--- a/circuits/circuits/utils/crypto/hasher/shaBytes/dynamic/sha1Bytes.circom
+++ b/circuits/circuits/utils/crypto/hasher/shaBytes/dynamic/sha1Bytes.circom
@@ -6,6 +6,9 @@ include "@openpassport/zk-email-circuits/utils/array.circom";
 include "circomlib/circuits/bitify.circom";
 
 //Adapted from @openpassport/zk-email-circuits/helpers/sha.circom
+// Assumption: The value `in_len_padded_bytes * 8` must fit within `ceil(log2(max_num_bytes * 8))` bits.
+// This range constraint is assumed (but not enforced) by the underlying `Sha1General` template.
+// It must be enforced externally, via a Num2Bits check, to prevent incorrect hash outputs.
 template Sha1Bytes(max_num_bytes) {
     signal input in_padded[max_num_bytes];
     signal input in_len_padded_bytes;
@@ -33,6 +36,9 @@ template Sha1Bytes(max_num_bytes) {
 
 //Adapted from @openpassport/zk-email-circuits/helpers/sha256general.circom
 //Sha1 template from https://github.com/dmpierre/sha1-circom/blob/fe18319cf72b9f3b83d0cea8f49a1f04482c125b/circuits/sha1.circom
+// Assumption: The value of `in_len_padded_bits` must fit within `ceil(log2(maxBitsPadded))` bits.
+// This constraint is required to ensure soundness of the LessEqThan comparator. 
+// It is not enforced here: it must be guaranteed by the "caller", via a Num2Bits check!
 template Sha1General(maxBitsPadded) {
     assert(maxBitsPadded % 512 == 0);
     var maxBitsPaddedBits = log2Ceil(maxBitsPadded);
diff --git a/circuits/circuits/utils/crypto/hasher/shaBytes/shaBytesDynamic.circom b/circuits/circuits/utils/crypto/hasher/shaBytes/shaBytesDynamic.circom
index 1803f0130..cdf945aa7 100644
--- a/circuits/circuits/utils/crypto/hasher/shaBytes/shaBytesDynamic.circom
+++ b/circuits/circuits/utils/crypto/hasher/shaBytes/shaBytesDynamic.circom
@@ -5,6 +5,7 @@ include "./dynamic/sha224Bytes.circom";
 include "@openpassport/zk-email-circuits/lib/sha.circom";
 include "./dynamic/sha384Bytes.circom";
 include "./dynamic/sha512Bytes.circom";
+include "circomlib/circuits/bitify.circom"; // needed for Num2Bits
 
 /// @title ShaBytesDynamic
 /// @notice Computes the hash of an input message using a specified hash length and padded input
@@ -26,12 +27,38 @@ template ShaBytesDynamic(hashLen, max_num_bytes) {
         hash_bits <== Sha384Bytes(max_num_bytes)(in_padded, in_len_padded_bytes);
     }
     if (hashLen == 256) {
+
+        // Range check for the padded input length (in_len_padded_bytes).
+        // This check enforces that `in_len_padded_bytes * 8` can be represented using 
+        // `ceil(log2(max_num_bytes * 8))` bits, which is a requirement assumed by the 
+        // underlying SHA templates. Without this check, out-of-range values could 
+        // silently bypass internal constraints, leading to incorrect hash outputs.
+        // For more information, see:
+        // https://github.com/zkemail/zk-email-verify/blob/b193cf0c760456b837b2bbcf7b2c72d5bb3f43c3/packages/circuits/lib/sha.circom#L87
+        var maxBitsPadded = max_num_bytes * 8;
+        var maxBitsPaddedBits = ceil(log2(maxBitsPadded));
+        component rangeCheck = Num2Bits(maxBitsPaddedBits);
+        rangeCheck.in <== in_len_padded_bytes * 8;
+
         hash_bits <== Sha256Bytes(max_num_bytes)(in_padded, in_len_padded_bytes);
     }
     if (hashLen == 224) { 
         hash_bits <== Sha224Bytes(max_num_bytes)(in_padded, in_len_padded_bytes);
     }
     if (hashLen == 160) {
+
+        // Range check for the padded input length (in_len_padded_bytes).
+        // This check enforces that `in_len_padded_bytes * 8` can be represented using 
+        // `ceil(log2(max_num_bytes * 8))` bits, which is a requirement assumed by the 
+        // underlying SHA templates. Without this check, out-of-range values could 
+        // silently bypass internal constraints, leading to incorrect hash outputs.
+        // For more information, see:
+        // https://github.com/selfxyz/self/pull/579#issuecomment-2922842294
+        var maxBitsPadded = max_num_bytes * 8;
+        var maxBitsPaddedBits = ceil(log2(maxBitsPadded));
+        component rangeCheck = Num2Bits(maxBitsPaddedBits);
+        rangeCheck.in <== in_len_padded_bytes * 8;
+        
         hash_bits <== Sha1Bytes(max_num_bytes)(in_padded, in_len_padded_bytes);
     }