Fix two heap buffer overflows (expires past 2037, secret random) - #85
Open
redz-tech wants to merge 1 commit into
Open
Fix two heap buffer overflows (expires past 2037, secret random)#85redz-tech wants to merge 1 commit into
redz-tech wants to merge 1 commit into
Conversation
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.
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.
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.
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.