Skip to content

fix(parser): quote-aware element skip + strict duplicate-q rejection#55

Open
eilandert wants to merge 1 commit into
masterfrom
audit-r2-parser-quotes
Open

fix(parser): quote-aware element skip + strict duplicate-q rejection#55
eilandert wants to merge 1 commit into
masterfrom
audit-r2-parser-quotes

Conversation

@eilandert

Copy link
Copy Markdown
Owner

Second-pass audit follow-ups on the Accept-Encoding parser and its tests.

Correctness (medium)

The element-skip in ngx_http_zstd_accept_encoding() was not quoted-string aware, while eval_qvalue()'s value-skip was. A quoted comma inside a non-q parameter split the element and fabricated a phantom coding token:

Accept-Encoding: gzip;x="a, zstd";q=1   ->   accepted zstd (wrong)

Both skip sites now route through a single ngx_http_zstd_skip_quoted() helper (RFC 9110 §5.6.4 quoted-string, honoring quoted-pair escapes, strictly length-bounded). eval_qvalue() also rejects a repeated q parameter (RFC 9110 §12.4.2 permits at most one weight).

Client controls its own header, so this is correctness, not a security issue.

Fuzzing

  • fuzz/extract_parser.sh slices the new helper into the standalone target.
  • The reference oracle learns simple (no-escape) quoted-string parameters so it is confident on the quoted-comma class it previously skipped, and bails to "unsure" on stray quotes in name/token positions so it never disagrees with production on malformed input.
  • Six curated NN_ corpus seeds for the quoted / dup-q cases.
  • Verified: 3.5M ASan+UBSan runs clean, no oracle divergence.

Tests

t/00-filter.t TEST 68 asserts the zstd_bypass identity arm (predicate fires → identity response that still carries Vary, the cache-poisoning case TEST 66 omitted).

Repo hygiene

Untrack t/servroot-static/ (Test::Nginx working dir committed despite the .gitignore rule).

CI

clang-tidy now gates on the low-FP security/cert checks (--warnings-as-errors) while keeping bugprone/unix advisory.

Both modules compile clean under -Wall -Wextra -Werror with and without -DZSTD_STATIC_LINKING_ONLY.

Second-pass audit follow-ups on the Accept-Encoding parser and its tests.

Correctness (medium): the element-skip in ngx_http_zstd_accept_encoding()
was not quoted-string aware, while eval_qvalue()'s value-skip (just made
quote-aware) was. A quoted comma inside a non-q parameter therefore split
the element and fabricated a phantom coding token, e.g.

    Accept-Encoding: gzip;x="a, zstd";q=1   ->   accepted zstd (wrong)

Both skip sites now route through a single ngx_http_zstd_skip_quoted()
helper (RFC 9110 §5.6.4 quoted-string, honoring quoted-pair escapes,
strictly length-bounded). eval_qvalue() also rejects a repeated "q"
parameter (RFC 9110 §12.4.2 permits at most one weight).

Fuzzing: fuzz/extract_parser.sh now also slices the new helper. The
reference oracle in fuzz_accept_encoding.c learns simple (no-escape)
quoted-string parameters so it is confident on the quoted-comma class it
previously skipped, and bails to "unsure" on stray quotes in name/token
positions so it never disagrees with the production parser on malformed
input. Six curated NN_ corpus seeds cover the quoted/dup-q cases.
Verified: 3.5M ASan+UBSan runs clean, no oracle divergence.

Tests: t/00-filter.t TEST 68 asserts the zstd_bypass identity arm (predicate
fires -> identity response that still carries Vary, the cache-poisoning case
TEST 66 omitted).

Repo hygiene: untrack t/servroot-static/ (Test::Nginx working dir that was
committed despite the .gitignore rule).

CI: clang-tidy now gates on the low-FP security/cert checks
(--warnings-as-errors) while keeping bugprone/unix advisory.

Both modules compile clean under -Wall -Wextra -Werror with and without
-DZSTD_STATIC_LINKING_ONLY.
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.

1 participant