Skip to content

Fix two heap buffer overflows (expires past 2037, secret random) - #85

Open
redz-tech wants to merge 1 commit into
kyprizel:masterfrom
redz-tech:master
Open

Fix two heap buffer overflows (expires past 2037, secret random)#85
redz-tech wants to merge 1 commit into
kyprizel:masterfrom
redz-tech:master

Conversation

@redz-tech

@redz-tech redz-tech commented Aug 19, 2026

Copy link
Copy Markdown
  1. ngx_http_testcookie_set_uid() reserved sizeof(expires) - 1 == 37 bytes
    for the "; expires=..." part of Set-Cookie. That only fits the 2-digit
    year form. For any expiry past 2037 ngx_http_cookie_time() switches to
    a 4-digit year and writes 29 bytes instead of 27, overrunning the
    cookie buffer by 2 bytes on every request. Reserve the length the way
    ngx_http_userid_filter_module does, and mirror the branching already
    used where the value is written.

  2. The testcookie_secret post handler wrote MD5_DIGEST_LENGTH random
    bytes into secret->data, which still points at the configuration
    token "random" -- a 7 byte allocation made by ngx_conf_read_token().
    Allocate a buffer for the generated secret instead. While here,
    generate 32 bytes rather than 16: a secret supplied in the
    configuration is rejected below 32 bytes, so the random one should
    not be shorter. It is regenerated on every start, so nothing can
    depend on its previous length.

Both reproduced on nginx 1.31.3 built with
-fsanitize=address -DNGX_DEBUG_PALLOC=1:

testcookie_expires 15y, one request

ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 8 ... 0 bytes after 105-byte region
# 1 ngx_http_testcookie_set_uid ngx_http_testcookie_access_module.c:1515

testcookie_secret random, plain "nginx -t"

ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 16 ... 7-byte region

NGX_DEBUG_PALLOC is required to observe either one: without it both writes land inside a larger pool block and go unnoticed.

After the fix the same ASAN build is clean across testcookie_expires unset / 10d / 15y / max and testcookie_secret random, and the cookie challenge still passes over HTTP/1.1, HTTP/2 and HTTP/3 in a real browser.

1) ngx_http_testcookie_set_uid() reserved sizeof(expires) - 1 == 37 bytes
   for the "; expires=..." part of Set-Cookie. That only fits the 2-digit
   year form. For any expiry past 2037 ngx_http_cookie_time() switches to
   a 4-digit year and writes 29 bytes instead of 27, overrunning the
   cookie buffer by 2 bytes on every request. Reserve the length the way
   ngx_http_userid_filter_module does, and mirror the branching already
   used where the value is written.

2) The testcookie_secret post handler wrote MD5_DIGEST_LENGTH random
   bytes into secret->data, which still points at the configuration
   token "random" -- a 7 byte allocation made by ngx_conf_read_token().
   Allocate a buffer for the generated secret instead. While here,
   generate 32 bytes rather than 16: a secret supplied in the
   configuration is rejected below 32 bytes, so the random one should
   not be shorter. It is regenerated on every start, so nothing can
   depend on its previous length.

Both reproduced on nginx 1.31.3 built with
-fsanitize=address -DNGX_DEBUG_PALLOC=1:

  # testcookie_expires 15y, one request
  ERROR: AddressSanitizer: heap-buffer-overflow
  WRITE of size 8 ... 0 bytes after 105-byte region
    kyprizel#1 ngx_http_testcookie_set_uid ngx_http_testcookie_access_module.c:1515

  # testcookie_secret random, plain "nginx -t"
  ERROR: AddressSanitizer: heap-buffer-overflow
  WRITE of size 16 ... 7-byte region

NGX_DEBUG_PALLOC is required to observe either one: without it both
writes land inside a larger pool block and go unnoticed.

After the fix the same ASAN build is clean across testcookie_expires
unset / 10d / 15y / max and testcookie_secret random, and the cookie
challenge still passes over HTTP/1.1, HTTP/2 and HTTP/3 in a real
browser.
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.

2 participants