Repository navigation
Reqest headers validation - #1111
Conversation
970258a to
2fbb3d0
Compare
Previously, a property name used to store a response header was copied into the header list as is, without checking it against the token grammar, so a name containing control characters produced a malformed response. The name is now validated in the two places where it is resolved for a store: ngx_http_js_ext_header_out() and its QuickJS counterpart, which also covers Object.defineProperty(). An invalid name is rejected with a TypeError. Previously, values assigned to r.headersOut were stored as is, without checking them for control characters, while ngx_js_check_header_value() was applied only on the Fetch Headers path. The conversion of a JS value into a header value is now done by ngx_js_header_value() and ngx_qjs_header_value(), which validate the result. All the r.headersOut store paths, including the special headers and Content-Type, use them and reject an invalid value with a TypeError. Reported by vladimirelitokarev@gmail.com.
Previously, for a redirect status ngx_http_send_response() stores the text passed to r.return() as the Location header value, but the text was only checked when it was used as a response body, where control characters are valid. The text is now validated as a header value for the statuses that turn it into Location, and is left as is otherwise.
2fbb3d0 to
023a4dd
Compare
VadimZhestikov
left a comment
VadimZhestikov
left a comment
There was a problem hiding this comment.
Looks good.
Reviewed and tested -- this supersedes #1109, which is now closed.
It catches two things #1109 missed:
- Header names. #1109 validated values only, so
r.headersOut['X\r\nInjected: 1'] = 'v'
still split the response. Validating where the name is resolved for a store
coversObject.defineProperty()too. - The
r.return()redirect target, which reaches Location without ever being
checked as a header value -- it was only checked as a body, where control
characters are legal.
The ngx_js_header_value() / ngx_qjs_header_value() wrappers are also a better
shape than what #1109 did. Converting and validating in one call means a store
path cannot pick up the conversion without the check, which is what went wrong
here in the first place.
Coverage looks right: ten call sites across both engines and both nginx version
branches. The one remaining raw ngx_js_string(vm, setval, ...) is in
ngx_http_js_request_variables(), which is not a header path -- correctly left
alone.
Tested on both engines, js_engine qjs and the default:
value CRLF -> invalid header value
name CRLF -> invalid header name
Object.defineProperty -> invalid header name
r.return(302, ...) -> invalid header value
with no injected header appearing in any response, and benign traffic unchanged:
plain values, Location, and Set-Cookie arrays all still work.
One thing that is not yours, but worth knowing: js_shared_dict.t tests 35 and
37 ("foo keys after expire", "no of items in foo after expire") failed in one of
two full-suite runs on this branch. They pass 3 out of 3 when the file is run
alone, and this branch does not touch shared dict code, so it looks like an
expiry test that is timing-sensitive under full-suite load -- the extra tests
here shift the timing. Not attributable to this change, just flagging it.
github-project-automation
Bot
moved this from New
to In Review
in NGINX OSS Unified Workspace
github-project-automation
Bot
moved this from In Review
to Done
in NGINX OSS Unified Workspace
2 participants
This is more extended fix for problem described in #1109.