Skip to content

fix: fix(privacy): eb_priv_apply_referrer writes one byte past the caller's buffer - #25

Draft
srpatcha wants to merge 2 commits into
masterfrom
autofix/privacy-referrer-overflow
Draft

fix: fix(privacy): eb_priv_apply_referrer writes one byte past the caller's buffer#25
srpatcha wants to merge 2 commits into
masterfrom
autofix/privacy-referrer-overflow

Conversation

@srpatcha

@srpatcha srpatcha commented Sep 6, 2026

Copy link
Copy Markdown
Member

Problem

eb_priv_apply_referrer() writes one byte past the caller's buffer whenever the
source origin is long enough to be truncated — src/privacy/privacy.c:115-116:

size_t len = e ? (size_t)(e-from) : strlen(from);
if (len >= max) len = max-1;
memcpy(ref, from, len); ref[len]='/'; ref[len+1]='\0';

The clamp reserves one byte, for the NUL. The code then writes two bytes
after the origin: the trailing '/' and the NUL. With len == max-1,
ref[len] is the last byte of the buffer and ref[len+1] is one past its end.

from is a page URL, so its length is attacker-controlled: any site can choose
an origin long enough to reach this branch for a given caller buffer.

Two smaller defects in the same function:

  • strncpy(ref, from, max-1) at :126 (same-origin) and :128 (full
    referrer) leaves ref unterminated when from is max-1 bytes or
    longer. strncpy only NUL-pads when the source is shorter than the count.
    Every caller reads ref as a C string, so this is an overread on the same
    input class that triggers the overflow above.
  • max == 0 was accepted. ref[0] = '\0' on the early-exit paths already
    writes to a zero-length buffer, and max-1 underflows to SIZE_MAX.

Root cause

The bound was derived from what memcpy writes, not from what the whole branch
writes. strncpy's partial-copy contract was assumed to include termination.

Reproduction

Compiled against unmodified src/privacy/privacy.c at origin/master bb37c5b
with -fsanitize=address, calling the origin-only policy with a 24-byte origin
and a 24-byte destination buffer:

==1528167==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6ca3a43e0058
WRITE of size 1 at 0x6ca3a43e0058 thread T0
    #0 ... in eb_priv_apply_referrer src/privacy/privacy.c:116
0x6ca3a43e0058 is located 0 bytes after 24-byte region [0x6ca3a43e0040,0x6ca3a43e0058)

After the change the same call returns https://averylongdom.c/ with no
sanitizer report.

The fix

  • Reserve two bytes in the origin-only branch: if (len > max - 2) len = max - 2;,
    guarded by max >= 2 so a one-byte buffer simply yields an empty referrer.
  • Add a referrer_copy() helper that does strncpy followed by an explicit
    dst[max-1] = '\0', and use it for both strncpy sites.
  • Reject max == 0 with the -1 the function already uses for bad arguments,
    and clear ref[0] once at the top so no exit path can leave the caller's
    buffer holding stale contents.

Files changed

  • src/privacy/privacy.c — the bound, the two copies, the max == 0 guard.
  • tests/test_privacy.c — three cases added to the existing suite:
    truncation stays inside the buffer (checked with a canary struct, because a
    one-byte stack overflow is invisible without a sanitizer), both strncpy
    policies terminate, and max == 0 returns -1 without touching the buffer.

A no-op statement removed, deliberately

case EB_REF_NONE: contained the bare expression statement
p->referrers_stripped;. It has no effect, and it cannot be turned into an
increment here because p is const eb_privacy_t * — so
eb_privacy_t::referrers_stripped has never left zero, while its sibling
trackers_stripped is incremented normally in eb_priv_clean_url(), which
takes a non-const pointer.

This PR removes the dead statement and leaves a comment saying why, rather than
changing the function's signature. Making the counter work is a public-API
change and belongs to a maintainer; it is recorded in the maintenance backlog.

Impact and risk

Confined to src/privacy/privacy.c. No header, ABI or manifest change. The
only behavioural differences are: a truncated origin-only referrer is now one
byte shorter (it has to be — the last byte was never inside the buffer), a
maximally long referrer is now terminated, and max == 0 returns -1 instead
of writing to a zero-length buffer.

Not addressed here, and filed in the backlog instead: eb_priv_clean_url() at
:61 and :65 uses the same unterminated strncpy(clean, url, max-1) pattern,
and eb_priv_should_allow_cookie() at :101-102 returns true from both
branches of its final conditional.

Surfaced by the scheduled maintenance sweep, 2026-09-06.

Verification

Executed in an isolated worktree branched from origin/master:

Check Result Duration Command
asan pass 1s bash -c cmake -B build/asan -G Ninja -DBUILD_TESTING=ON -DCMAKE_C_FLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer -g" -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,undefined" >/dev/null && cmake --build build/asan --target test_privacy --parallel 4 && ./build/asan/tests/test_privacy
build pass 8s cmake --build build/host --parallel 4
cmake pass 43s cmake -B build/host -G Ninja -DBUILD_TESTING=ON
cppcheck pass 0s /home/srpatcha/eos/.ai/toolchains/run-in-toolchain.sh /home/srpatcha/eos/.ai/autoreview/state/worktrees/eBrowser__privacy-referrer-overflow -- cppcheck --enable=warning,portability --inline-suppr --error-exitcode=1 -I include src/privacy/privacy.c tests/test_privacy.c
ctest pass 0s ctest --test-dir build/host --output-on-failure --no-tests=error --parallel 4

Opened by the scheduled autoreview pipeline (model claude-opus-5), branched from origin/master. No human has reviewed this yet. Close it freely if the fix is wrong - a bad automated PR is a bug worth reporting.

Fixes #28

…ller's buffer

Opened by the scheduled autoreview pipeline after review of open PRs.
Reviewed against the EmbeddedOS Master Design v2.0.

Files: src/privacy/privacy.c tests/test_privacy.c
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

Performance Regression Check ✅ All metrics within thresholds

Metric Value Status
Pipeline RPS 141,540.81 req/s
Pipeline Latency 7.1 µs/req
Full results
╔══════════════════════════════════════════════════════════════╗
║        eBrowser Performance Regression Results              ║
╠══════════════════════════════════════════════════════════════╣
║  Metric                    Value  Threshold   Status ║
║  ────────────────────────────────────────────────────────── ║
║  html_parse               44.1 us/op    10000 us/op     ✓  ║
║  css_parse                10.6 us/op      100 us/op     ✓  ║
║  dom_ops                 120.5 us/op     5000 us/op     ✓  ║
║  sha256_4kb               24.2 us/op      200 us/op     ✓  ║
║  xss_scan                  0.3 us/op       50 us/op     ✓  ║
║  firewall_check            0.5 us/op       50 us/op     ✓  ║
║  tracker_block             2.3 us/op      100 us/op     ✓  ║
║  url_clean                 0.1 us/op       10 us/op     ✓  ║
║  ext_match                 0.0 us/op        5 us/op     ✓  ║
║  pool_alloc                4.4 ns/op      500 ns/op     ✓  ║
║  pipeline                  7.1 us/op      100 us/op     ✓  ║
║  pipeline_rps         141540.8 rps    20000 rps     ✓  ║
║                                                              ║
║  VERDICT: ALL METRICS WITHIN THRESHOLDS ✓                              ║
╚══════════════════════════════════════════════════════════════╝

--- PERF_JSON_START ---
{
  "version": "2.0.0",
  "metrics": [
    {"name":"html_parse","value":44.12,"unit":"us/op","threshold":10000,"passed":true},
    {"name":"css_parse","value":10.58,"unit":"us/op","threshold":100,"passed":true},
    {"name":"dom_ops","value":120.52,"unit":"us/op","threshold":5000,"passed":true},
    {"name":"sha256_4kb","value":24.19,"unit":"us/op","threshold":200,"passed":true},
    {"name":"xss_scan","value":0.29,"unit":"us/op","threshold":50,"passed":true},
    {"name":"firewall_check","value":0.47,"unit":"us/op","threshold":50,"passed":true},
    {"name":"tracker_block","value":2.32,"unit":"us/op","threshold":100,"passed":true},
    {"name":"url_clean","value":0.15,"unit":"us/op","threshold":10,"passed":true},
    {"name":"ext_match","value":0.03,"unit":"us/op","threshold":5,"passed":true},
    {"name":"pool_alloc","value":4.38,"unit":"ns/op","threshold":500,"passed":true},
    {"name":"pipeline","value":7.07,"unit":"us/op","threshold":100,"passed":true},
    {"name":"pipeline_rps","value":141540.81,"unit":"rps","threshold":20000,"passed":true}
  ],
  "failures": 0,
  "total": 12
}
--- PERF_JSON_END ---

Commit: 53b8ef6

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — eBrowser#25 "fix(privacy): eb_priv_apply_referrer writes one byte past the caller's buffer"

head: 10295d5 author: srpatcha ci: fail

Verdict: The bounds fix is correct and I reproduced both the defect and the repair. What the PR does not address is what the function emits after truncating: a shortened origin is a different origin, and it still goes out as a referrer with a success return.

This PR was opened by this same automated pipeline. It carries no approval from me and needs a human decision before merge.

Findings

# Severity File:line Finding Recommended fix
1 Medium (P2) src/privacy/privacy.c:118-127 Truncation silently emits a referrer for a host that is not the source host, and returns 0 so the caller cannot tell. Compiled from this branch: eb_priv_apply_referrer(&p, "https://averylongdom.com/path/here", …, ref, 24) under EB_REF_ORIGIN_ONLY yields https://averylongdom.c/. That is a well-formed origin string for a domain the user never visited, handed to a third party by the module whose job is to limit what that third party learns. EB_REF_SAME_ORIGIN and the default (EB_REF_FULL) branch have the same shape through referrer_copy(): a truncated URL is not the URL. The overflow is gone, but the wrong-data path predates it and this PR is the change that makes truncation reachable and terminated rather than undefined. For a privacy control the safe truncation result is no referrer at all. In the EB_REF_ORIGIN_ONLY branch, replace the clamp with a refusal — if (len + 2 > max) return 0; with ref[0] already '\0' from the prologue — and do the same in referrer_copy(): emit nothing rather than a prefix. If a caller genuinely needs to distinguish "no referrer by policy" from "did not fit", that is a return-code change and belongs in its own PR with the compatibility note §23.2 asks for.
2 High (P1) .github/workflows/ci.yml:24-33 All three required Test (Python 3.x) checks fail and mergeStateStatus is BLOCKED. Not caused by this diff. Root cause confirmed from the job steps on #24's identical failure: Set up Python is the failing step, everything after it is skipped. actions/setup-python@v5 is configured with cache: pip, which needs a dependency file, and this repo has no requirements.txt, pyproject.toml, setup.py, setup.cfg or Pipfile. The following step then runs pip install -r requirements.txt against the same missing file. Out of scope here, but it is what blocks this PR. Either drop cache: pip and the -r requirements.txt install, or add the dependency file the workflow assumes. No open PR covers it (#21/#22/#23 are dependabot action bumps). Needs its own issue.
3 Low (P3) src/privacy/privacy.c:150 default: sends the full referrer. Today the enum has exactly four values (include/ebrowser/privacy.h:14) and all four are handled, so nothing is broken now — but the direction is fail-open: any future eb_referrer_policy_t member added without touching this switch silently gets the least private behaviour. .ai/security.md calls fail-open the failure mode this project keeps hitting. Latent, not live. Handle EB_REF_FULL explicitly and make default: emit nothing. That also lets -Wswitch flag the next unhandled value instead of the switch swallowing it.
4 Low (P3) include/ebrowser/privacy.h:23, src/privacy/privacy.c:109 Dead field, now provably dead. referrers_stripped is declared in the public struct and its only ever write was the no-op expression statement this PR removes. Grepping the tree, it is now written nowhere and read nowhere. The comment the PR leaves in its place explains why it was not fixed, which is the right call — but the struct still advertises a counter that is permanently zero to anyone reading the header. Decide one way in a follow-up: either drop p's const on this function and increment it (a public-signature change needing the §23.2 compatibility note), or remove the field. Leaving a public counter that is structurally incapable of moving is the worst of the three.

Not a finding, checked and cleared:

  • The overflow is real and the fix removes it. Against origin/master's privacy.c, a 24-byte buffer with an 8-byte canary comes back as 00 23 23 23 …ref[24] clobbered. Against this branch the canary is fully intact. Compiled both from source, same harness.
  • The new tests are not vacuous. Built against origin/master's privacy.c with -fsanitize=address,undefined, test_referrer_always_terminates aborts with stack-buffer-overflow … READ of size 17 in strlen at tests/test_privacy.c:163 — the unterminated strncpy. Against this branch all 26 cases pass clean under the same sanitizers.
  • Ordering of the max == 0 guard against the new ref[0] = '\0' prologue is correct: the guard returns first, so a zero-length buffer is never written. test_referrer_rejects_zero_length_buffer covers exactly that.
  • if (max >= 2) { if (len > max - 2) len = max - 2; … } is right at the boundary. max == 1 writes only the prologue NUL; otherwise the last byte touched is ref[max-1]. No off-by-one remains.
  • The PR body's verification table is backed by state/verify/eBrowser__privacy-referrer-overflow.*: 20/20 ctest, plus a real -fsanitize=address,undefined build of test_privacy that ran and passed. The 0s entries are whole-second rounding of a 0.39 s suite.

Architecture conformance

Conforms. eBrowser is Tier 5 — Applications (§21); §20.1 treats it as a reference application, not a peer of the kernel. The diff stays inside src/privacy/ and tests/ in that repo. No #include, link line or manifest entry points up a tier, and referrer_copy() is static, so nothing new is exported.

§23.2 is not engaged: eb_priv_apply_referrer()'s signature, the eb_privacy_t layout and eb_referrer_policy_t are all unchanged. The PR is explicit that fixing referrers_stripped would require a signature change and declines to make it here — that is the right reading of the compatibility contract, and the reason findings 1 and 4 are recommendations for follow-ups rather than demands on this diff.

§21.1 is not engaged — no subsystem or repository boundary moves.

Proposed changes

  1. Make truncation emit nothing (finding 1). In EB_REF_ORIGIN_ONLY, replace the clamp:
        /* A shortened origin is a different origin. Send none rather than a
         * plausible-looking wrong one. ref[0] is already '\0'. */
        if (len + 2 > max) return 0;
        memcpy(ref, from, len); ref[len] = '/'; ref[len + 1] = '\0';

and give referrer_copy() the same treatment — if (strlen(src) >= max) { dst[0] = '\0'; return; } before the copy. Then extend test_referrer_origin_only_truncates_within_bounds to assert the result is empty rather than merely in-bounds, and test_referrer_always_terminates likewise.

  1. Handle EB_REF_FULL explicitly and make default: emit nothing (finding 3). Two lines, and it turns the next added enum value into a compiler warning.

  2. Leave findings 2 and 4 to their own issues. Finding 2 is repo-wide CI; finding 4 needs a public-API decision.

Not checked

  • I did not run this repo's CMake build or ctest myself. My evidence is the pipeline's recorded logs under state/verify/ plus a standalone harness I compiled from this branch's src/privacy/privacy.c and tracker_blocker.c against the repo headers with gcc -fsanitize=address,undefined. That is not the repo's real build configuration.
  • No fuzzing of eb_priv_apply_referrer(), which is the coverage .ai/security.md asks for on an externally reachable parser. The three new cases are hand-picked boundaries, not a search. I did not check whether the repo's fuzz.yml targets this function.
  • I did not trace the callers of eb_priv_apply_referrer() to establish what buffer sizes reach it in practice, so I cannot say how often the truncation path in finding 1 is actually taken, only that it is reachable and wrong when taken.
  • I did not review the URL-origin parsing itself (strstr(from, "://") / strchr(s+3, '/')) beyond the bounds question. Whether it handles userinfo, ports, IPv6 literals or a scheme-relative URL correctly is untested here, and the same-origin comparison in EB_REF_SAME_ORIGIN rests on it.
  • CI: step conclusions read through the API; the full job logs were not retrievable while the run was in progress. Finding 2's root cause is inferred from the failing step name plus the confirmed absence of every dependency file that step requires, not from log text.

Automated architecture review of 10295d5ba482 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Performance Regression Check ✅ All metrics within thresholds

Metric Value Status
Pipeline RPS 123,389.76 req/s
Pipeline Latency 8.1 µs/req
Full results
╔══════════════════════════════════════════════════════════════╗
║        eBrowser Performance Regression Results              ║
╠══════════════════════════════════════════════════════════════╣
║  Metric                    Value  Threshold   Status ║
║  ────────────────────────────────────────────────────────── ║
║  html_parse               88.7 us/op    10000 us/op     ✓  ║
║  css_parse                 8.8 us/op      100 us/op     ✓  ║
║  dom_ops                 235.2 us/op     5000 us/op     ✓  ║
║  sha256_4kb               20.1 us/op      200 us/op     ✓  ║
║  xss_scan                  0.3 us/op       50 us/op     ✓  ║
║  firewall_check            0.5 us/op       50 us/op     ✓  ║
║  tracker_block             3.4 us/op      100 us/op     ✓  ║
║  url_clean                 0.2 us/op       10 us/op     ✓  ║
║  ext_match                 0.0 us/op        5 us/op     ✓  ║
║  pool_alloc                6.6 ns/op      500 ns/op     ✓  ║
║  pipeline                  8.1 us/op      100 us/op     ✓  ║
║  pipeline_rps         123389.8 rps    20000 rps     ✓  ║
║                                                              ║
║  VERDICT: ALL METRICS WITHIN THRESHOLDS ✓                              ║
╚══════════════════════════════════════════════════════════════╝

--- PERF_JSON_START ---
{
  "version": "2.0.0",
  "metrics": [
    {"name":"html_parse","value":88.70,"unit":"us/op","threshold":10000,"passed":true},
    {"name":"css_parse","value":8.78,"unit":"us/op","threshold":100,"passed":true},
    {"name":"dom_ops","value":235.20,"unit":"us/op","threshold":5000,"passed":true},
    {"name":"sha256_4kb","value":20.10,"unit":"us/op","threshold":200,"passed":true},
    {"name":"xss_scan","value":0.32,"unit":"us/op","threshold":50,"passed":true},
    {"name":"firewall_check","value":0.50,"unit":"us/op","threshold":50,"passed":true},
    {"name":"tracker_block","value":3.39,"unit":"us/op","threshold":100,"passed":true},
    {"name":"url_clean","value":0.19,"unit":"us/op","threshold":10,"passed":true},
    {"name":"ext_match","value":0.04,"unit":"us/op","threshold":5,"passed":true},
    {"name":"pool_alloc","value":6.65,"unit":"ns/op","threshold":500,"passed":true},
    {"name":"pipeline","value":8.10,"unit":"us/op","threshold":100,"passed":true},
    {"name":"pipeline_rps","value":123389.76,"unit":"rps","threshold":20000,"passed":true}
  ],
  "failures": 0,
  "total": 12
}
--- PERF_JSON_END ---

Commit: 822ef32

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — eBrowser#25 "fix(privacy): eb_priv_apply_referrer writes one byte past the caller's buffer"

head: 7496d8a author: srpatcha ci: fail (3 × Test (Python 3.x)) draft: true

Verdict: Follow-up. 7496d8a makes every referrer path fail closed and I verified each one by execution — the truncated-origin leak and the fail-open default: are both gone, with the overflow canary still intact. The code is now ahead of its description: the PR body still documents the previous approach, and quotes as the "after" result the exact output this commit was written to eliminate.

This PR was opened by this automated pipeline. It carries no approval and still needs a human decision before merge.

Prior findings

Prior # Status Evidence
1 — truncation emits a referrer for a host the user never visited Resolved in 7496d8a privacy.c:134 now refuses instead of clamping, and referrer_copy() (:105-110) returns without writing when the source does not fit. Re-verified by execution: the previously-reported call with a 24-byte origin into a 24-byte buffer now yields an empty referrer with the canary intact, where it previously yielded https://averylongdom.c/.
2 — all three Test (Python 3.x) checks fail; setup-python cache: pip with no dependency file Untouched — still open, still blocking Re-verified on origin/master: .github/workflows/ci.yml:26 sets cache: pip, :32 runs pip install -r requirements.txt, and the repository has no requirements.txt, pyproject.toml, setup.py, setup.cfg or Pipfile. Same root cause as eBrowser#24.
3 — default: sent the full referrer (fail-open on a future enum value) Resolved at runtime in 7496d8a EB_REF_FULL is now an explicit case (:124-126) and default: returns an empty referrer (:149-150). Verified: a policy cast to 99 returns 0 with ref[0] == '\0', and test_referrer_unknown_policy_fails_closed pins it. The compile-time half of the recommendation was not achieved — see finding 6.
4 — referrers_stripped is a public counter that can never move Untouched, deliberately The explanatory comment at :119-122 is retained. Still the right call for a bounds fix; still needs its own public-API decision.

Findings

# Severity File:line Finding Recommended fix
1 Medium (P2) PR body, "The fix", "Reproduction", "Files changed" The body describes the previous head and is now wrong in four places. (a) "Reserve two bytes in the origin-only branch: if (len > max - 2) len = max - 2;" — that clamp was deleted; the branch now refuses. (b) "Add a referrer_copy() helper that does strncpy followed by an explicit dst[max-1] = '\0'" — referrer_copy() no longer uses strncpy at all; it is strlen → refuse → memcpy (:105-110). (c) "After the change the same call returns https://averylongdom.c/ with no sanitizer report" — it now returns an empty referrer, and that string is precisely the leak the previous review flagged and 7496d8a removed. Quoting it as the desired outcome inverts the meaning of the fix. (d) "three cases added … truncation stays inside the buffer, both strncpy policies terminate" — those two tests were renamed and their assertions inverted, and there are now five referrer cases, not three. .ai/reviewer.md treats an unsupported claim as the finding; a claim that describes code that was deleted is the same problem. Rewrite the "The fix", "Reproduction" and "Files changed" sections against 7496d8a. The reproduction section is worth keeping — the ASan trace against origin/master is still valid evidence of the original overflow — but its "after" line must read "returns an empty referrer" rather than a truncated origin.
2 Medium (P2) src/privacy/privacy.c:112-152 A public function's observable output changed for existing callers, with nothing said about it. eb_priv_apply_referrer() still returns 0 in every non-error case, so a caller cannot distinguish "policy says no referrer" from "your buffer was too small". What changed is what lands in ref: a caller passing a buffer shorter than the URL previously got a truncated string, and now gets an empty one. This is the correct change — a partial origin is a different origin, and it is what the previous review asked for — but brief §8 wants the compatibility impact and migration path stated, and the body currently states the opposite. Anything downstream that treated a non-empty ref as "we have a referrer" will now silently send none. One paragraph in the body: which callers are affected (those with max smaller than the source URL), what they now observe, and why silence is the safe outcome for a privacy control. If a caller genuinely needs to tell the two cases apart, that is a return-code change and belongs in its own PR with its own note — as the previous review said and this one still agrees.
3 Medium (P2) tests/test_privacy.c:167 EB_REF_SAME_ORIGIN's success path is untested. The policy appears exactly once in the whole suite, inside test_referrer_fails_closed_when_full_url_is_too_long, where the expected result is an empty referrer. Nothing asserts that a same-origin navigation with a buffer that fits actually produces the referrer — so the origin-comparison logic at :140-145 (fl != tl || strncmp(from, to, fl)) has no coverage at all, and a change that made this branch always refuse would pass the suite green. That matters more after this commit than before it, because "returns empty" is now the correct answer to several inputs and is therefore a weak signal. The author added exactly this test for EB_REF_FULL (test_referrer_full_copies_complete_url), so the pattern is right there. Add the mirror: test_referrer_same_origin_copies_url_when_origins_match (same host, different paths, 64-byte buffer, assert strcmp(ref, from) == 0) and test_referrer_same_origin_refuses_cross_origin (different hosts, assert ref[0] == '\0'). Two tests, and they are the only thing standing between the same-origin policy and a silent regression.
4 Low (P3) src/privacy/privacy.c:125, :146 referrer_copy() was changed from void to bool specifically so it can report that the source did not fit, and neither call site reads the result. The behaviour is still correct — ref[0] = '\0' at :117 means a refused copy leaves the right value — so nothing is broken; the new signal is simply dead. -Wall -Wextra (CMakeLists.txt:59) will not flag it without warn_unused_result. Either drop the return to void and rely on the prologue as the code actually does, or keep the bool and add __attribute__((warn_unused_result)) so the next caller cannot ignore it. Half-measures here are how the discarded-result pattern gets in.
5 Low (P3) src/privacy/privacy.c:134 if (len > max - 1 || len + 1 >= max) return 0; — the second clause implies the first for every max >= 1, and max == 0 already returned -1 at :114, so the first clause can never be the one that fires. I verified the combined condition is exactly right at the boundary: an origin of 14 bytes into max == 16 is accepted and writes 15 characters plus the NUL, and 15 bytes into max == 16 is refused. Correct, just doubled. if (len + 2 > max) return 0; states the requirement directly — two bytes are written after the origin — and is the form the comment above it already describes.
6 Low (P3) src/privacy/privacy.c:118-151 The compile-time half of prior finding 3 was not obtained. All four enumerators are now explicit cases, but default: is still present, and -Wswitch is suppressed by any default: label — so a fifth eb_referrer_policy_t value added later still produces no warning. It will fail closed at runtime, which is the part that mattered, but the compiler will not point at the switch. Keeping default: is nonetheless necessary here, since test_referrer_unknown_policy_fails_closed casts 99 into the enum and that value must land somewhere. Add -Wswitch-enum to the warning set in CMakeLists.txt:59. It warns on a missing enumerator even when default: is present, which gets both properties at once. Note it will also flag other switches in the tree, so it is a small sweep rather than a one-liner.

Verified clean this round, by execution against head 7496d8a under -fsanitize=address,undefined, with no sanitizer diagnostics on any case:

Case Result
origin-only, 24-byte origin into a 24-byte buffer rc=0, empty referrer, 8-byte canary intact
origin-only, origin that fits rc=0, https://a.com/
origin-only, origin length 14 into max=16 (exact fit) rc=0, https://abc.de/, 15 chars — last byte used, none past it
origin-only, origin length 15 into max=16 (one too big) rc=0, empty
EB_REF_FULL, fits rc=0, complete URL copied
EB_REF_FULL, too long, buffer pre-filled with 'A' rc=0, empty — the old strncpy would have left it unterminated
policy cast to 99 rc=0, empty; pre-existing "stale" contents cleared
max == 0 rc=-1, buffer untouched
max == 1, origin-only rc=0, empty

Also confirmed: the renamed regression tests keep their teeth. test_referrer_origin_only_fails_closed_when_too_long retains the canary loop, so it still catches the original one-byte overflow; and its new ASSERT(buf.ref[0] == '\0') would fail against origin/master, where strncpy leaves the buffer holding a URL prefix — so the unterminated-copy regression is still covered too, by assertion rather than by sanitizer. The RUN() macro in this file also gained the s_fail snapshot, so a failing case no longer prints PASS.

Architecture conformance

Conforms, unchanged from the previous review. eBrowser is Tier 5 — Applications (§21) and §20.1 treats it as a reference application rather than a peer of the kernel, so a bounds-and-policy fix inside src/privacy/ carries no wider obligation. The diff stays inside src/privacy/ and tests/; no #include, link line or manifest entry points up a tier, and referrer_copy() remains static, so nothing new is exported. §23.2 is not engaged at the type level: eb_priv_apply_referrer()'s signature, the eb_privacy_t layout and eb_referrer_policy_t are all unchanged — finding 2 is about observable behaviour, not the ABI.

Worth naming explicitly: .ai/security.md's "fail closed" section is the standard this commit is measured against — "a verification step that cannot run must fail, not pass" — and 7496d8a is a clean instance of applying it. The referrer path is not boot or crypto, so eBoot's stricter reading does not apply, but the module's whole purpose is limiting what a third party learns, and a fail-open truncation was directly contrary to it. No design-doc proposal appended; the master design says nothing about referrer policy and does not need to, since §20.1 scopes eBrowser as a demonstrator.

Proposed changes

  1. Rewrite the body's "The fix", "Reproduction" and "Files changed" sections against 7496d8a, and add the compatibility paragraph (findings 1 and 2). This is the largest remaining gap and it is prose, not code.
  2. Add the two EB_REF_SAME_ORIGIN tests (finding 3) — the only real coverage hole left in the function.
  3. Simplify :134 to if (len + 2 > max) return 0; and decide referrer_copy()'s return type (findings 4 and 5). Two lines.
  4. -Wswitch-enum (finding 6) is a separate small sweep, not part of this PR.
  5. Prior finding 2 is what actually blocks this, exactly as on #24: a four-line workflow fix — drop cache: pip, drop pip install -r requirements.txt, keep the explicit tool install — that unblocks every PR in this repository. It should be its own PR; no open PR here covers it (#21/#22/#23 are Dependabot action bumps).

Not checked

  • I did not run this repository's CMake build or its ctest suite. Everything in the verified table above came from a standalone harness compiled from this branch's src/privacy/privacy.c and src/privacy/tracker_blocker.c against the repo headers with gcc -I include -fsanitize=address,undefined, in a temporary extraction. That is not the repo's real build configuration, and it does not run tests/test_privacy.c itself — I read the test source and reasoned about what it asserts rather than executing the suite.
  • The claim that the renamed tests still fail against origin/master is reasoning from the assertions plus the previous review's recorded ASan run, not a re-execution of the new tests against the old source.
  • No fuzzing. .ai/security.md asks for fuzz coverage on an externally reachable parser and from is an attacker-influenced page URL. The five cases here are hand-picked boundaries. I did not check whether the repo's fuzz harness reaches eb_priv_apply_referrer().
  • The URL-origin parsing itself is still unreviewed beyond the bounds question. strstr(from, "://") / strchr(s+3, '/') and the strncmp-based comparison at :140-145 have not been assessed against userinfo (https://a@evil.com/), explicit ports, IPv6 literals, or scheme-relative URLs — and finding 3 means the suite would not catch a defect there either. This is the most likely place for a real vulnerability in this function and neither this PR nor this review has looked at it.
  • I did not trace the callers of eb_priv_apply_referrer(), so I cannot say what buffer sizes reach it in practice or how often finding 2's behaviour change is observable.
  • I did not read the failing CI job logs. Prior finding 2's root cause is re-confirmed from the workflow file plus the confirmed absence of every dependency file it references, not from log text.
  • The Performance Check job is green and its 12 metrics are within thresholds, but none exercise the referrer path; I did not measure the cost of the extra strlen in referrer_copy(), and would not expect it to matter.
  • Nothing was pushed and no merge was attempted. The clone was read through git show/git archive against FETCH_HEAD; its working tree is unchanged.

Automated architecture review of 7496d8a1440a — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

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.

Prevent the referrer policy helper from writing past the caller buffer

1 participant