search

nodejs / http-parser

http request/response parser for c
MIT License
6.33k stars 1.53k forks source link

http_parser: revert `memchr()` optimization #469

Closed indutny closed 5 years ago

indutny commented 5 years ago

An optimization was introduced in c6097e1d and 0097de58. The crux of optimization was to skip all characters in header value until either of CR or LF. Unfortunately, this optimization comes at cost of inconsistency in header value validation, which might lead to security issue due to violated expectations in the user code.

Partially revert the optimization, and add additional check to make general header value parsing consistent.

Fix: #468

indutny commented 5 years ago

cc @nodejs/http .

I do not believe that there is any security risk for Node.js applications. However, since this is causing a public security issue for envoyproxy, I'd suggest we act promptly on this.

Thank you!

indutny commented 5 years ago

Landed in 2a0d106. Thank you everyone!

@htuch I'd use Node.js security channels for this. https://nodejs.org/en/security/ Thanks again for reporting it!

sam-github commented 4 years ago

@indutny @nodejs/http Can anyone confirm that this does not affect Node.js? https://github.com/nodejs/http-parser/pull/469#issuecomment-480421822 --- why would it not affect Node.js?

Because it looks like it could affect Node.js, in which case we'd need a CVE for it and to backport the fix to LTS branches.

indutny commented 4 years ago

I believe it would not affect Node.js due to the way the header values are converted to V8 strings in Node. We use OneByteString(env->isolate(), str_, size_) and thus \0 have no special effects on our operation.