Repository navigation
test(syscall-allowlist): assert a per-request syscall budget, not just the syscall set - #163
Merged
Merged
Conversation
The SET gate is one-directional by design and cannot catch a module that opens one extra file per request, because openat is already allowlisted for the reference probe's own /proc/self/fd walk (baseline.syscalls:52-60 names this hole in its own comment). Add a third assertion that ceilings the four names measured exact and zero-variance across bursts (openat=3N, accept4=N, getdents64=4N, readlink=11N), expressed per-request against the burst count rather than an absolute so it cannot silently re-scale. epoll_wait/recvfrom are excluded -- they wobbled +/-1 in the same measurement and would flake the gate. Verified: a throwaway instrumented copy of the ref module that opens /dev/null once per request pushes openat to 4N (80 vs a 60 ceiling at burst=20) and reds assertion 3 while assertion 2 (the set gate) stays green, confirming the hole this closes is real.
… errors The budget gate took the calls count as $(NF-1). strace's -c table only emits the `errors` column on rows that had errors, so a clean row is NF=5 and a row with failures is NF=6 -- and on exactly those rows $(NF-1) reads errors rather than calls. That fails in the dangerous direction. A budgeted openat that starts erroring would compare its small error count against the ceiling and pass while massively breached: injecting a 200-call/7-error openat row reads as 7 against a ceiling of 60. A gate that goes quiet under the hostile conditions this harness creates is the vacuous-gate class the scenario exists to catch. Anchor to $4 instead; the leading four columns are fixed whether or not errors is populated. Guarded on NF>=5 and a numeric $4. Re-verified both directions after the change: baseline openat=60 -> ok 3 (3/3 runs), and a throwaway ref-module copy opening /dev/null once per request gives openat=80 -> not ok 3, while assertion 2 stays green in both.
WalkthroughThe syscall allowlist scenario now runs a configurable burst, plans three TAP assertions, identifies responses served by the traced worker, and enforces syscall-family ceilings derived from ChangesSyscall allowlist budget validation
Sequence Diagram(s)sequenceDiagram
participant driver.sh
participant traced worker
participant strace -c
participant TAP output
driver.sh->>traced worker: Send burst of HTTP requests
traced worker-->>driver.sh: Return responses containing worker PID
driver.sh->>strace -c: Read syscall summary
strace -c-->>driver.sh: Return syscall counts
driver.sh->>TAP output: Report allowlist and budget assertions
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e4ede463-9e17-49ba-b646-1187f118d875
📒 Files selected for processing (1)
prober/scenarios/syscall-allowlist/driver.sh
eilandert
marked this pull request as draft
July 30, 2026 14:19
Member
Author
|
Moved to draft. The review found two blockers, and the scenario legs failing here are one of them — the ceilings are not all per-request.
My local sweep ran 5 repetitions across 4 burst sizes with zero variance, which I read as "constant". It only ever ran on one box, so it ruled out run-to-run noise and said nothing about environment dependence. The sweep never crossed the axis the value actually depends on. The budget also does not close the hole it was built for. Two more worth fixing before this lands: the ceiling's denominator is attempted requests, since the burst loop ignores connect/write/read failures — 15 of 20 served against a 4-per-request mutant totals exactly the 3×20 ceiling and passes. And the errors-column parse fixed in Reworking as: budget the family sums ( |
Addresses the four defects that had #163 rejected. Each is a way the budget could report green on the very behaviour it was added to catch. 1. Budget families, not names. openat and accept4 each have libc near-neighbours that are separately allowlisted, so a module reaching the kernel through open(2) once per request left openat at exactly its baseline: the set gate passed (open is allowlisted) and a name-wise openat ceiling passed (openat did not move). The hole the assertion exists to close survived it. The ceilings are now over openat+open and accept4+accept. 2. A budgeted family absent from the whole table is RED, not zero. Returning 0 for an absent row answers a question that could not be evaluated with the most comfortable possible answer, and every ceiling then passes. An absent MEMBER of a family that was observed still contributes zero -- no server calls the legacy open -- and a test pins that boundary so the rule cannot be over-tightened into a permanent red. 3. Drop readlink and getdents64 from the budget. They are not per-request constants: ngx_test_probe.c issues one readlink per entry in /proc/self/fd and a getdents64 per buffer-full of its directory walks, so both scale with the worker's FD population -- build config, module set, log files. CI run 30549769006 reads readlink=13N on nginx 1.29.0 and angie 1.12.0 while 1.28.0 sits at 11N. The local sweep that produced the 11N figure had zero variance because it never left one machine, which rules out run-to-run noise and says nothing about environment dependence. 4. The denominator is verified served responses, not the attempted burst. Every failure in the burst loop is deliberately swallowed so one bad iteration cannot kill the driver, so attempts say nothing about what the traced worker answered. With 15 of 20 served, a mutant issuing 4 openat per request totals 60 and sits exactly on a 3x20 ceiling -- passing while every served request breaches it. The strace summary parser moves to lib.sh as prober_strace_counts and prober_strace_family_sum, with syscall_budget_test.sh asserting it against committed fixtures. CI only ever captures clean rows, so the shapes that broke this parser -- an errors row, a missing family, a duplicated name -- are exactly the ones the scenario cannot exercise, and reverting the calls column to $(NF-1) would otherwise stay green forever. Two mutate.sh rows cover both. Control, rebuilt rather than inherited: a throwaway ref-module copy opening /dev/null once per request via open(2) reds assertion 3 at openat+open=80(>60) while assertion 2 stays green. Baseline is 3/3 green at served=20.
eilandert
marked this pull request as ready for review
July 30, 2026 14:47
Three defects found reviewing 5204c34, each a way the budget could report green on the behaviour it was added to catch. Remote CI was green 19/19 on all three with mutation reporting caught 117, survived 0: a vacuous-oracle defect crashes nothing and races nothing, so there is nothing for a sanitizer or a mutation row to trip on. 1. BLOCKER. SERVED counted any line matching ^HTTP/1.[01] [0-9]{3}, binding a counted response to no pid at all. strace attaches to ONE pid and -f does not follow a replacement worker -- the master forks it, not the tracee. A worker retiring mid-burst let its successor serve the rest off-tracee: all 20 counted, the numerator held only the handful the tracee answered, and a mutant's extra opens landed where nothing was counting. All three assertions passed. worker_processes 1 does not prevent it; sequential replacement is still one worker at a time. The predicate moves to lib.sh as prober_served_by, matching a final 200 whose probe body reports the traced pid. The same change closes the overbroad matcher: an interim 103, a 500, and the malformed 2000 each counted as a served response and none is one. 2. MAJOR. case "$name" in [a-z]*) validated only the FIRST character, so openat+open!, openat+open_at and openat+open* all returned rc=0 with the sum 60 -- openat's count alone, family-wide seen letting the one valid member mask its malformed sibling. That is the name-wise budget the family form exists to replace, silently restored by any future edit to a family string. Members are now validated end to end, and split with read -r -a so a glob member is not pathname-expanded into something other than what the caller wrote. Scope is stated in the test: shape validation cannot catch a typo that is itself syscall-shaped, which is indistinguishable from an absent member. 3. MAJOR. syscall_budget_test.sh had no impact.map row, so verify-impact failed closed: prober/syscall_budget_test.sh changed but maps to no target. Both PRIMARY edges added -- a lib.sh edit has to select the suite, or the parser's hostile cases go unrun. Docs: README and attack-environment still advertised the budget as unimplemented and the driver's header still said the plan was 1..2. Verification. Suite 16 -> 32 assertions; the 8 covering defect 2 and defect 1 red against the pre-fix lib.sh (negative control run both ways). Two mutate.sh rows added, first-character-only validation and the pid-blind denominator, both caught BY NAME: caught 4, survived 0, broken 0. Live scenario baseline stays 3/3 green at served=20 -- the pid check does not reject healthy responses -- while the rebuilt open(2)-per-request mutant still reds assertion 3 at openat+open=80(>60) with assertion 2 green, which is the proof the family sum remains load-bearing. verify-impact exits 0 and selects the suite. 27/27 local suites, shellcheck clean.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a89656ea-194c-43af-b7f6-bcf7539faae0
⛔ Files ignored due to path filters (1)
prober/impact.mapis excluded by!**/*.map
📒 Files selected for processing (6)
README.mddocs/attack-environment.mdprober/lib.shprober/mutate.shprober/scenarios/syscall-allowlist/driver.shprober/syscall_budget_test.sh
💤 Files with no reviewable changes (1)
- README.md
CodeRabbit found this on 931e801 and it reproduces. prober_served_by fed the entire response document to `grep -qE '^HTTP/1\.[01] 200( |$)'`. grep anchors ^/$ per LINE, so a status-shaped line ANYWHERE in the document satisfies it -- including one in the body, which this scenario echoes request-influenced content into. A 500 whose body carried `HTTP/1.1 200 OK` on a line of its own, with the traced pid in its JSON, counted as a served response. That inflates the syscall budget's denominator, which is the one thing the whole 931e801 commit existed to pin down: every per-response ceiling scales with SERVED, so crediting responses the worker never served loosens all of them at once. The committed control did not catch it because it encoded the same misunderstanding as the code. It used `X: HTTP/1.1 200 OK` -- a PREFIXED line, which fails the regex on the buggy and the fixed implementation alike, so it passed either way and proved nothing. The replacement uses an unprefixed body line and reds against the pre-fix lib.sh; a paired positive (a 200 with a multi-line body) guards against over-tightening the anchor into a permanent red. Both verified by stashing the fix and re-running: assertion 32 fails, 33 passes. The status line is now taken with ${resp%%$'\n'*} rather than head -n1 -- the driver calls this once per response in a burst loop, and a subprocess per call would be a syscall cost inside the scenario that measures syscall costs. Adds a mutate.sh row for the class, and repairs the anchor of the neighbouring served-pid row, which referenced the line this change edits and would otherwise have reported BROKEN while leaving the job green. Suite 32 -> 33 assertions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
syscall-allowlistchecks which syscalls the request path makes, but not how many. The reference probe walks/proc/self/fdon every request, soopenathas to be on the allowlist — which means a module that opens a file of its own per request is invisible to the gate. It still reads as "openat", already allowed.This adds assertion 3: a ceiling on the syscall families whose counts are structurally one-per-request, expressed per response the traced worker is proven to have served.
Severity: None (test coverage — no production behaviour change)
Mechanism
baseline.syscalls:52-60names this gap in its own comment. A set gate is one-directional by design and structurally cannot close it; only a count can.Four design calls carry the whole assertion, and each is the difference between a gate and a green light:
Families, not names. A ceiling on
openatalone closes nothing, because the libc near-neighbours are each separately allowlisted. A module reaching the kernel throughopen(2)once per request leavesopenatat exactly its baseline — the set gate passes (openis allowlisted) and a name-wiseopenatceiling passes (openatdid not move), on precisely the behaviour the assertion exists to catch. The ceilings are overopenat+openandaccept4+accept, at 3 and 1 per served response. Family members are validated end to end and split withread -r -a, so a malformed sibling cannot hide behind a valid one and a glob member is not pathname-expanded.An absent family is red, not zero. If not one member of a budgeted family appears in the table — changed strace format, truncated capture, a typo in the family string — returning 0 answers a question that could not be evaluated, with the most comfortable possible answer, and every ceiling then passes. An absent member of a family that was observed still contributes zero (no server calls the legacy
open); that boundary is pinned by its own assertion so the rule cannot be over-tightened into a permanent red.The denominator is bound to the traced pid. strace attaches to one worker and
-fdoes not follow a replacement — the master forks it, not the tracee. Counting any line matching^HTTP/1\.[01] [0-9]{3}therefore credits responses whose syscalls were never observed: a worker retiring mid-burst lets its successor serve the rest off-tracee, all 20 count, the numerator holds only what the tracee answered, and a mutant's extra opens land where nothing is counting.worker_processes 1does not prevent this; sequential replacement is still one worker at a time.prober_served_by(lib.sh) now matches a final 200 whose probe body reports the traced pid — which also closes the overbroad matcher, since an interim103, a500and a malformed2000each counted as a served response and none is one. The status match is pinned to the first line:grepanchors^/$per line, so testing the whole document accepted a status-shaped line in the body, and this scenario echoes request-influenced content into bodies.Served responses, not the attempted burst. Every failure in the burst loop is deliberately swallowed so one bad iteration cannot kill the driver, so attempts say nothing about what the worker answered. With 15 of 20 served, a mutant issuing 4
openatper request totals 60 and sits exactly on a 3×20 ceiling — passing while every served request breaches it.Ceilings are
K * SERVED, never absolutes: an absolute goes slack or starts flaking the moment someone edits the burst size, which is the same failure mode this scenario exists to catch, one level up.Only those two families are budgeted.
readlinkandgetdents64look like per-request constants on one box and are not:src/ngx_test_probe.cissues onereadlinkper entry in/proc/self/fdand agetdents64per buffer-full of its directory walks, so both scale with the worker's FD population — build config, module set, log files. CI run 30549769006 readsreadlink=13Non nginx 1.29.0 and angie 1.12.0 while 1.28.0 sits at 11N. A local sweep gave 11N with zero variance, which rules out run-to-run noise and says nothing about environment dependence.epoll_waitandrecvfromare excluded for the neighbouring reason — ±1 wobble from the kernel coalescing readiness.The parser moved out of the driver
The strace summary parser and the served-response predicate are now
prober_strace_counts,prober_strace_family_sumandprober_served_byinlib.sh, withprober/syscall_budget_test.shbehind them. Not tidiness: a parser that only runs inside a scenario can only be tested by booting a server, so the shapes that break it — an errors row, a missing family, a duplicated name, a response from a replacement worker — are exactly the ones CI never produces.The shape it was extracted for: calls was originally read as
$(NF-1). strace's-ctable emits theerrorscolumn only on rows that had errors, so a clean row is NF=5 and a failing row is NF=6 — and on exactly those rows$(NF-1)reads errors, low, in the passing direction. A 200-call/7-erroropenatrow read as 7 against a ceiling of 60. Now anchored to$4from the left, guarded onNF>=5and a numeric$4.All four defects this file guards share one property worth naming: each made the oracle report green on the behaviour it was added to catch, and none of them crashes or races. CI was green 19/19 with
mutationreportingsurvived 0on the code that carried three of them.Testing
Local nginx 1.29.0 + ref module,
.somtime checked againstsrc/ngx_test_probe.c.prober/syscall_budget_test.sh: 33/33 (1..33). The assertions coveringprober_served_bywere confirmed RED against the pre-fixlib.sh— negative control in both directions.ok 3atserved=20, green 3/3 consecutive runs. The pid binding does not reject healthy responses, which is the regression risk it carries.t/moduleopening/dev/nullonce per request viaopen(2), soopenatnever moves:not ok 3 - per-request syscall budget exceeded: openat+open=80(>60).openatceiling; the family sum is what reds. That green assertion 2 is the proof the set gate cannot see this.mutate.shrows for this scenario reportcaughtlocally, by name. The four present at931e801were each caught by name on CI'smutationjob (suite totalcaught 119, survived 0, broken 0); the fifth (status matched over the whole response, not the status line) is new ine473b7fand awaits that job's run.verify-impactexits 0; bothprober/impact.mapPRIMARY rows are present.931e801; the run fore473b7fis in progress.shellcheckSC1091 and theshfmtindentation diff on the driver both pre-exist onmain(same findings, 217 shfmt lines) and are left alone.