Skip to content

fix: sign more headers safely, and order prefix names correctly - #9

Merged
bonifaido merged 1 commit into
mainfrom
fix/sign-all-x-amz-headers
Aug 3, 2026
Merged

fix: sign more headers safely, and order prefix names correctly#9
bonifaido merged 1 commit into
mainfrom
fix/sign-all-x-amz-headers

Conversation

@bonifaido

Copy link
Copy Markdown
Member

AWS rejects a request when any x-amz-* header it carries is missing from SignedHeaders ("There were headers present in the request which were not signed"), so callers have to pass all of them in. Doing that hit three problems here:

  • Header limit / stack overflow. AWS_SIGV4_MAX_NUM_HEADERS was 10 and the local arrays in get_signed_headers / get_canonical_headers were sized to it, while host, x-amz-date and x-amz-content-sha256 are appended on top of the caller's headers — 8 caller headers already wrote past both arrays. Limit is now 24 and the arrays are sized for the appended entries.
  • Prefix name ordering. The comparators did strncmp over the shorter name, so names sharing a prefix (x-amz-server-side-encryption and x-amz-server-side-encryption-aws-kms-key-id on SSE-KMS requests) compared equal and could be emitted in either order — a canonical form AWS won't accept. Ties now break on length, shorter first.
  • Unbounded writes. Canonical request, string-to-sign and the Authorization value were written with aws_sigv4_sprintf (no limit) into fixed buffers and only length-checked afterwards, and aws_sigv4_sign dropped get_canonical_request's overflow return. All writes are clamped to the buffer end now and the error is propagated, so a too-large request fails instead of corrupting the caller's stack.

Buffers: canonical request 2048 → 4096 (a session token alone can be 1–2 KB), string-to-sign 2048 → 1024 (it holds a fixed ~140 bytes).

get_signed_headers, get_canonical_headers, get_canonical_request, get_credential_scope and get_string_to_sign take a last limit; the latter now returns a status. aws_sigv4_sign is unchanged for callers.

Testing

make test — 4/4 pass. New cases: an x-amz-copy-source (CopyObject) signature, SSE-KMS prefix-name ordering, and the overflow path returning AWS_SIGV4_BUFFER_OVERFLOW_ERROR. The existing test vector is byte-identical. All signatures were cross-checked against an independent SigV4 implementation written from the AWS spec, and the suite was also run under ASan/UBSan with no reports.

AWS rejects a request when any x-amz-* header it carries is missing from
SignedHeaders, so callers need to pass all of them through. Three things got in
the way:

- the header limit was 10, and the internal arrays were sized to that same limit
  while host, x-amz-date and x-amz-content-sha256 are appended on top of the
  caller's headers -- 8 caller headers already wrote past both stack arrays.
  Limit is now 24, arrays are sized for the appended ones.
- the comparators used strncmp over the shorter name, so names that share a
  prefix (x-amz-server-side-encryption and its -aws-kms-key-id variant on
  SSE-KMS requests) compared equal and could be emitted in either order,
  producing a canonical form AWS does not accept. Ties now break on length.
- canonical request, string to sign and the Authorization value were written
  with unbounded sprintf into fixed stack buffers, checked for overflow only
  after the fact, and aws_sigv4_sign discarded the overflow return entirely.
  All writes are now clamped and the error is propagated.

The canonical request buffer goes 2048 -> 4096, since a session token alone can
take 1-2 KB, and string to sign 2048 -> 1024, which is far more than the fixed
~140 bytes it holds.

Tests cover an x-amz-copy-source (CopyObject) signature, prefix-name ordering
and the overflow path; the existing vector is unchanged.
@bonifaido
bonifaido merged commit e204ea6 into main Aug 3, 2026
1 check passed
@bonifaido
bonifaido deleted the fix/sign-all-x-amz-headers branch August 3, 2026 17:30
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.

4 participants