T8968: fix: bounded brace expansion to prevent DoS in braces.c#37
T8968: fix: bounded brace expansion to prevent DoS in braces.c#37orbisai0security wants to merge 3 commits into
Conversation
The brace expansion code does not impose limits on the number of results generated
|
All contributors have signed the CLA ✍️ ✅ |
|
❌ |
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. 📝 WalkthroughWalkthroughLimits brace-expansion growth by adding BRACE_EXPANSION_LIMIT (braces.c:46), runtime guards in mkseq (braces.c:318–329) and array_concat (braces.c:628–638), replaces unsafe strncpy calls with memcpy + explicit NULs (braces.c:141,192,275–276,649–650), and adds an invariant test with SIGALRM timeout (tests/test_invariant_braces.c:1–98). ChangesBrace expansion overflow protection
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@braces.c`:
- Around line 616-625: The brace-expansion limit is only checked in the
Cartesian concatenation (array_concat) path, leaving sequence generation
functions like expand_seqterm and mkseq able to produce arbitrarily large
sequences (e.g. {1..1000000000}); add a guard in expand_seqterm/mkseq that
computes the expected sequence length (or checks growth as you build) and if it
would exceed BRACE_EXPANSION_LIMIT, emit the same error path (use report_error
with a descriptive message and call throw_to_top_level where appropriate),
release any allocated state before returning, and return an empty/partial result
consistent with current failure handling so the DoS vector is closed. Ensure you
reference BRACE_EXPANSION_LIMIT and follow the same cleanup pattern used in
array_concat/brace expansion code (strvec_dispose, strvec_copy behavior) for
consistency.
In `@tests/test_invariant_braces.c`:
- Around line 24-28: The current test payloads in tests/test_invariant_braces.c
never hit the MAX_RESULTS guard; replace or add a payload with at least 17
binary groups (e.g. "{a,b}" repeated 17 times) so the expansion would produce
131072 items > MAX_RESULTS, and update the assertions near the brace expansion
checks (the assertions around lines 56-58) to verify that the expansion stops or
is capped by MAX_RESULTS (use the MAX_RESULTS symbol in the assertion) rather
than producing full expansion; ensure the test asserts bounded/early-stop
behavior rather than exact full-count output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a77e7ccf-426d-40a9-bbda-5f8f7360039e
📒 Files selected for processing (2)
braces.ctests/test_invariant_braces.c
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codeql-analysis-call / Analyze (c)
- GitHub Check: Summary
🧰 Additional context used
🪛 Clang (14.0.6)
tests/test_invariant_braces.c
[error] 1-1: 'check.h' file not found
(clang-diagnostic-error)
🔍 Remote MCP Context7
Perfect! Now I have sufficient context. Let me compile the findings.
Summary of Additional Context Found
Brace Expansion Vulnerability Pattern
The core vulnerability pattern involves repeated numeric brace ranges that trigger exponential expansion. For example, the input {0..99}{0..99}{0..99}{0..99}{0..99} produces 100^5 = 10,000,000,000 combinations, which can quickly overwhelm the event loop and heap memory, resulting in process termination. An attacker can cause excessive CPU and memory consumption, potentially crashing the process and the services relying on this library, by supplying patterns with repeated numeric brace ranges that trigger exponential expansion.
Industry-Standard Limit Value
Industry standard implementations (such as the @isaacs/brace-expansion library fix for CVE-2026-25547) set the expansion limit to 100,000 by default. The standard fix approach introduces a limit on the number of expansions and modifies the internal recursive function to accept and enforce the max limit during its execution, checking against the max limit to terminate the expansion process early if the limit is reached.
Additional Vulnerability: Early Limit Application
It's worth noting that a subsequent vulnerability (CVE-2026-45149) was discovered where the max check was applied after the entire sequence had been generated, not during the generation loop, causing the loop to still iterate millions of times even with a small max limit, and the max limit being enforced only at the output combination step, which was too late to prevent the resource-heavy generation. This means the PR's fix should ensure the limit is enforced during expansion, not after.
Test Framework Context
The Check framework is a unit testing framework for C that runs tests in a separate address space, so both assertion failures and code errors that cause segmentation faults or other signals can be caught. All tests run with a default timeout of 4 seconds; if the test is not finished within that time, it is killed and logged as an error. Check uses setitimer (if available) to get subsecond unit test timeouts, and if setitimer is unavailable, it falls back on alarm.
Key Review Considerations
-
Limit Value Choice: The PR's
BRACE_EXPANSION_LIMITconstant should be compared against the 100,000 standard used by similar fixes to ensure it's reasonable. -
Early Enforcement: The fix should enforce the limit during the expansion process (in the loop), not after generating all results, to prevent resource exhaustion before the check occurs.
-
Test Coverage: The test file's timeout approach using
setitimer(ITIMER_REAL, ...)is appropriate and aligns with Check framework's standard timeout mechanisms.
[web_search::]
🔇 Additional comments (1)
braces.c (1)
46-47: LGTM!Also applies to: 141-142, 192-194, 275-276
|
I have read the CLA Document and I hereby sign the CLA |
|
✅ Changes Applied I've updated the code based on your feedback: Two issues to fix based on the code review:
Files modified:
The changes have been pushed to this PR branch. Please review! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== braces.c: mkseq and array_concat regions ==="
grep -n "BRACE_EXPANSION_LIMIT\|mkseq\|strlcpy\|snprintf\|strncpy\|strcpy" braces.c | head -80
echo ""
echo "=== braces.c: mkseq full function (context) ==="
grep -n "mkseq" braces.c
echo ""
echo "=== tests/test_invariant_braces.c full content ==="
cat tests/test_invariant_braces.cLength of output: 3951 🏁 Script executed: #!/bin/bash
echo "=== mkseq function body (lines 311-360) ==="
sed -n '311,380p' braces.c
echo ""
echo "=== array_concat around limit check (lines 620-670) ==="
sed -n '615,680p' braces.c
echo ""
echo "=== brace_expand preamble/amble copy region ==="
sed -n '100,200p' braces.cLength of output: 5606
✅
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_invariant_braces.c (2)
10-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign test bound with implementation cap.
Line 10 sets
MAX_RESULTSto 100000, but implementation enforcesBRACE_EXPANSION_LIMIT65536. The assertion at Lines 60–62 can pass even when production cap regresses above 65536 but below 100000.Proposed fix
-#define MAX_RESULTS 100000 +#define MAX_RESULTS 65536 /* Keep in sync with BRACE_EXPANSION_LIMIT in braces.c */Also applies to: 60-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_invariant_braces.c` at line 10, The test defines MAX_RESULTS as 100000 which is higher than the implementation cap BRACE_EXPANSION_LIMIT (65536), so update the test to align with the implementation cap: change MAX_RESULTS to 65536 or, better, reference the implementation constant (BRACE_EXPANSION_LIMIT) instead of a hardcoded value so the assertion around the brace expansion bounds (the assertion at the block using MAX_RESULTS / lines 60–62) will always match the production limit.
8-8:⚠️ Potential issue | 🟡 MinorFix
brace_expandprototype mismatch intests/test_invariant_braces.c
tests/test_invariant_braces.c:8declaresbrace_expand(const char *), but the production signature inbraces.cischar **brace_expand(char *text)(non-const). This prototype drift can mask contract/const-correctness issues;tests/test_invariant_braces.c:44also passes string literals viaconst char*.braces.c’s parsing code treatstextas read-only (index advancement only), so runtime mutation risk is unlikely, but align the test signature/callsite anyway.Proposed fix
-extern char **brace_expand(const char *str); +extern char **brace_expand(char *str); @@ - char **results = brace_expand(payloads[i]); + char *input = strdup(payloads[i]); + ck_assert_msg(input != NULL, "strdup failed"); + char **results = brace_expand(input); + free(input);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_invariant_braces.c` at line 8, Update the prototype in the test to match the production symbol: change the extern declaration to use the same signature as the implementation (brace_expand with a non-const char* parameter), i.e. replace the current extern char **brace_expand(const char *str); with extern char **brace_expand(char *text); and verify the test call site(s) (e.g. the call around the existing test at the reference to brace_expand) still compile against that symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_invariant_braces.c`:
- Line 13: timed_out is shared with the SIGALRM handler (handle_alarm) so change
its declaration from "static volatile int timed_out = 0;" to "static volatile
sig_atomic_t timed_out = 0;" and ensure <signal.h> is included if not already;
update any reads/writes of timed_out (e.g., in handle_alarm and test
teardown/reset) to use the same symbol/type but no other semantic changes.
---
Outside diff comments:
In `@tests/test_invariant_braces.c`:
- Line 10: The test defines MAX_RESULTS as 100000 which is higher than the
implementation cap BRACE_EXPANSION_LIMIT (65536), so update the test to align
with the implementation cap: change MAX_RESULTS to 65536 or, better, reference
the implementation constant (BRACE_EXPANSION_LIMIT) instead of a hardcoded value
so the assertion around the brace expansion bounds (the assertion at the block
using MAX_RESULTS / lines 60–62) will always match the production limit.
- Line 8: Update the prototype in the test to match the production symbol:
change the extern declaration to use the same signature as the implementation
(brace_expand with a non-const char* parameter), i.e. replace the current extern
char **brace_expand(const char *str); with extern char **brace_expand(char
*text); and verify the test call site(s) (e.g. the call around the existing test
at the reference to brace_expand) still compile against that symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3a42ae65-1d3e-41a4-9f9b-d90fb63eee48
📒 Files selected for processing (2)
braces.ctests/test_invariant_braces.c
🚧 Files skipped from review as they are similar to previous changes (1)
- braces.c
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codeql-analysis-call / Analyze (c)
- GitHub Check: Summary
🧰 Additional context used
🔍 Remote MCP Context7
The Context7 search for "Check" testing framework didn't return the C unit testing library (it returned testing frameworks for other languages). Let me search more specifically for the Check C framework.Great! I've gathered sufficient information on the key areas. Let me compile the additional context that's relevant to reviewing this PR.
Summary of Additional Relevant Context Found
Brace Expansion Security Vulnerabilities
Brace expansion is vulnerable to "Allocation of Resources Without Limits or Throttling" due to unbounded brace range expansion, allowing attackers to cause excessive CPU and memory consumption by supplying patterns with repeated numeric brace ranges that trigger exponential expansion. For example, patterns like {0..99}{0..99}{0..99}{0..99}{0..99} can trigger exponential expansion leading to excessive CPU and memory consumption, potentially crashing the process. The vulnerability stems from unbounded brace range expansion where the library eagerly generates every possible combination synchronously, and due to exponential growth in expansion, even small malicious inputs can consume excessive CPU and memory resources.
The specific attack vector aligns with the PR's V-004 vulnerability: When expanding a single large numeric range (e.g., {1..10000000}), the max check is applied after the entire sequence has been generated, not during generation. Consequently, even with limits, the loop still iterates millions of times, building a full intermediate array.
C String Safety: strcpy vs memcpy Best Practices
The memcpy() function transfers a specified number of bytes from the source to the destination buffer irrespective of null-terminating characters, and typically outperforms strcpy, as it does not need to check for null-terminating characters during copying, which is particularly beneficial when copying large amounts of data. A key difference between memcpy() and strcpy() is how they handle the null terminator: memcpy() does not add a null terminator to the copied data (the programmer must ensure this), while strcpy() automatically adds a null terminator.
Buffer overflow is the most common issue with strcpy, occurring when the source string's length surpasses the destination buffer size, potentially causing data corruption or even code execution by an attacker. Best practice recommendations include using memcpy with explicit validated lengths.
Check Framework Timeout Behavior
To be certain that a test won't hang indefinitely, all tests in Check are run with a timeout (default 4 seconds). If the test is not finished within that time, it is killed and logged as an error. The timeout for a specific test case can be changed with the tcase_set_timeout() function, and the default timeout can be changed with the environment variable CK_DEFAULT_TIMEOUT.
[::web_search::],
| #define MAX_RESULTS 100000 | ||
| #define TIMEOUT_SECONDS 5 | ||
|
|
||
| static volatile int timed_out = 0; |
There was a problem hiding this comment.
Use volatile sig_atomic_t for timed_out shared with the SIGALRM handler (tests/test_invariant_braces.c, line 13).
timed_out is written in the async signal handler (handle_alarm, lines 17–20) and read/reset in normal control flow, so the flag type should be volatile sig_atomic_t rather than int.
Proposed fix
-static volatile int timed_out = 0;
+static volatile sig_atomic_t timed_out = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static volatile int timed_out = 0; | |
| static volatile sig_atomic_t timed_out = 0; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_invariant_braces.c` at line 13, timed_out is shared with the
SIGALRM handler (handle_alarm) so change its declaration from "static volatile
int timed_out = 0;" to "static volatile sig_atomic_t timed_out = 0;" and ensure
<signal.h> is included if not already; update any reads/writes of timed_out
(e.g., in handle_alarm and test teardown/reset) to use the same symbol/type but
no other semantic changes.
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 invalid-task-id label must be absent to mergeWaiting for
This rule is failing.Block merge while the invalid-task-id label is present. Set by the per-repo product T-ID rule (product repos only); dormant where the label is never applied.
|
|
@orbisai0security — Mergify flagged this PR with the Why: the PR title must match
This title has no VyOS task ID. If it maps to a Phorge task (vyos.dev/T…), please prefix the title with ℹ️ This check is currently advisory (it doesn't block merge yet) but is slated to become a required check, so it's worth fixing now. 🤖 Generated by robots |
orbisai0security
changed the title
I've created a task and updated the title. Anything else remaining on this PR? |
3 participants
Summary
Fix high severity security issue in
braces.c.Vulnerability
V-004braces.c:339Description: The brace expansion code does not impose limits on the number of results generated. Combinatorial brace expansions like {a,b}{c,d}{e,f}...{y,z} with many terms produce a Cartesian product that grows exponentially. The code allocates memory for all combinations and performs strcpy for each, consuming unbounded CPU and memory.
Evidence
Exploitation scenario: An attacker who can provide input to bash (e.g., through a web application calling system() or popen() with user input) sends: echo {a,b}{a,b}{a,b}{a,b}{a,b}{a,b}{a,b}{a,b}{a,b}{a,b}{a,b}{a,b}{a,b}{a,.
Scanner confirmation: multi_agent_ai rule
V-004flagged this pattern.Production code: This file is in the production codebase, not test-only code.
Changes
braces.cVerification
Security Invariant
Regression test
This test guards against regressions — it's useful independent of the code change above.
Automated security fix by OrbisAI Security