Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional Rigorous Range Check to Prevent Potential Overflow Vulnerability in LessThan(8) Usage #83

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

Koukyosyumei
Copy link

@Koukyosyumei Koukyosyumei commented Jan 9, 2025

Hi!

Thank you for this fantastic project! While exploring the code, I noticed that LessThan(8) is used in several templates to validate the range of the input msg. However, this function does not check the bit-length of msg, which can result in potential security vulnerabilities.

Issue Overview:

The LessThan(N) function in Circomlib has a known overflow vulnerability. When handling values exceeding N bits, it can produce unexpected results. For instance, in this project, the intended behavior seems to be ensuring LessThan(8) outputs out = 0 if msg[i] exceeds 255.

However, for an input like:
msg[i] = 21888242871839275222246405745257275088548364400416034343698204186575808495616
(which I discovered using a fuzzing tool), the LessThan(8) function incorrectly outputs out = 1, allowing a valid proof to be generated.

example

in_range_checks[i] <== LessThan(8)([msg[i], 255]);

I confirmed that:

  • A proof for BodyHashRegex(1) cannot be generated with an input like {"msg": ["256"]}.
  • However, a proof can be generated for {"msg": ["21888242871839275222246405745257275088548364400416034343698204186575808495616"]}, despite this input clearly exceeding the intended range.

Affected Templates

The issue also impacts the following templates:

  • BodyHashRegex
  • EmailAddrRegex
  • EmailAddrWithNameRegex
  • EmailDomainRegex
  • FromAllRegex
  • MessageIdRegex
  • ReversedBracketRegex
  • SubjectAllRegex
  • TimestampRegex
  • ToAddrRegex
  • ToAllRegex

Users of these templates should exercise caution. Although using these templates to the standard utf-8 body is safe, replace LessThan with SemiSafeLessThan If stricter input validation is needed.

Changes in This PR:

To address this issue, I have introduced a new option -i --is_safe <true/false> option to the zk-regex CLI. When set to true, the generated Circom template uses SemiSafeLessThan for range check, which calls Num2Bits and validates the bit-length of the input. This ensures that inputs are constrained correctly and prevents unintended overflow behavior.

Example

$ zk-regex decomposed -d ./simple_regex_decomposed.json -c ./simple_regex.circom -t SimpleRegex -g true -i true
$ cat ./simple_regex.circom
pragma circom 2.1.5;

include "@zk-email/zk-regex-circom/circuits/regex_helpers.circom";

// regex: email was meant for @[a-z]+.
template SimpleRegex(msg_bytes) {
        signal input msg[msg_bytes];
        signal output out;

        var num_bytes = msg_bytes+1;
        signal in[num_bytes];
        signal in_range_checks[msg_bytes];
        in[0]<==255;
        for (var i = 0; i < msg_bytes; i++) {
                in_range_checks[i] <== SemiSafeLessThan(8)([msg[i], 255]);
                in_range_checks[i] === 1;
                in[i+1] <== msg[i];
        }

Additional Context:

For further details on this type of vulnerability, see:
https://github.com/BlakeMScurr/comparator-overflow

Acknolwdgement

Special thanks to @Divide-By-0 and @SoraSuegami for confirming this potential vulnerability!

@Koukyosyumei Koukyosyumei marked this pull request as ready for review January 10, 2025 18:07
@Koukyosyumei Koukyosyumei changed the title Fix Potential Overflow Vulnerability in LessThan(8) Usage [WIP] Fix Potential Overflow Vulnerability in LessThan(8) Usage Jan 10, 2025
@Koukyosyumei
Copy link
Author

Koukyosyumei commented Jan 10, 2025

Hi @Divide-By-0 @SoraSuegami,

Thanks again for your support!

I think a quick fix could be adding an optional argument to the zk-regex CLI, allowing users to choose between LessThan and SemiSafeLessThan. I named it SemiSafeLessThan because it only checks the first element—since the second element is always 255, it’s not validated here.

However, I worry that users might forget to enable or disable this option, leading to unintended behavior. Defining the is_safe option explicitly in the Circom file as the template parameter might be safer, as it requires users to deliberately specify a value for this parameter.

Let me know what you think!

@Koukyosyumei Koukyosyumei changed the title [WIP] Fix Potential Overflow Vulnerability in LessThan(8) Usage Fix Potential Overflow Vulnerability in LessThan(8) Usage Jan 13, 2025
@Koukyosyumei Koukyosyumei changed the title Fix Potential Overflow Vulnerability in LessThan(8) Usage [WIP] Fix Potential Overflow Vulnerability in LessThan(8) Usage Jan 13, 2025
@Koukyosyumei Koukyosyumei changed the title [WIP] Fix Potential Overflow Vulnerability in LessThan(8) Usage Fix Potential Overflow Vulnerability in LessThan(8) Usage Jan 14, 2025
@Koukyosyumei
Copy link
Author

Hi @Divide-By-0 @SoraSuegami, this PR is now ready for review!!

@Koukyosyumei Koukyosyumei changed the title Fix Potential Overflow Vulnerability in LessThan(8) Usage Optional Rigorous Range Check to Prevent Potential Overflow Vulnerability in LessThan(8) Usage Jan 14, 2025
@Koukyosyumei
Copy link
Author

@Divide-By-0 @shreyas-londhe
Hi! If you need additional information or improvement before merging this update, please let me know!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants