Vulnerability in http-parser & embedded NULL header handling

Due to a snafu in how [email protected] is setup to forward (see https://github.com/envoyproxy/envoy/issues/5155), the following bug report was not made available prior to disclosure. For completeness, I'm providing the original e-mail below. Please note that this has been fixed in http-parser since disclosures. I understand that Node has moved away from http-parser, but this might affect Node.JS LTS for earlier versions. See https://github.com/nodejs/http-parser/issues/468 for the fix. Rather than file a full report, I would like to share with Node.JS security WG the following resources: * Envoy CVE: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-9900 * Envoy GH issue with CVE details: https://github.com/envoyproxy/envoy/issues/6434 * http-parser GH issue: https://github.com/nodejs/http-parser/issues/468 * Other discussion of the handling of this issue: https://github.com/envoyproxy/envoy/issues/5155#issuecomment-481854258 Original e-mail ------------------------ MIME-Version: 1.0 Date: Thu, 14 Mar 2019 16:35:20 -0400 Message-ID: <CAA4W8ZmaBzTMFU8VdpJzVDM7LXo0o5-WPTdYisGJUF9qsXiPnQ@mail.gmail.com> Subject: Vulnerability in http-parser & embedded NULL header handling From: Harvey Tuch <[email protected]> To: [email protected], [email protected] Cc: [email protected] Content-Type: multipart/alternative; boundary="0000000000006b3f64058413dc0b" --0000000000006b3f64058413dc0b Content-Type: text/plain; charset="UTF-8" Hi Node.js Security WG, Ryan, We (Envoy security team) have discovered a potential security vulnerability related to our use of http-parser that we are working to fix, patch and issue a security update for Envoy-side following https://github.com/envoyproxy/envoy/blob/master/SECURITY_RELEASE_PROCESS.md. We would like to give you advanced notice of this under embargo, check in with you to see if this might affect Node.js or other http-parser users, and potentially coordinate on an http-parser side fix. Envoy makes use of http-parser as its HTTP/1 codec on its data plane. Envoy has baked into it today the assumption that its HTTP codecs (http-parser, nghttp2) enforce RFC constraints on valid header values ( https://tools.ietf.org/html/rfc7230#section-3.2.6). In particular, we expect that there are no embedded NULLs in header values or keys that are placed in Envoy's HeaderStrings and HeaderMapImpls objects. This is particularly important because we allow two views of a HeaderString, via c_str() <https://github.com/envoyproxy/envoy/blob/b41ba5925a4e93d22a86c6501d63314ccf0d79f3/include/envoy/http/header_map.h#L115> and getStringView() <https://github.com/envoyproxy/envoy/blob/b41ba5925a4e93d22a86c6501d63314ccf0d79f3/include/envoy/http/header_map.h#L120>; embedded NULLS cause inconsistent views and lengths through these accessors. We use a mixture of these in header matching and routing. Our fuzzers and some recently introduced ASSERTs indicated that embedded NULLs were making their way into header values received from http-parser. Digging deeper, the errant behavior is due to a bug in how validation of header values is performed by http-parser. You can see this in the validation logic at https://github.com/nodejs/http-parser/blob/0d0a24e19eb5ba232d2ea8859aba2a7cc6c42bc4/http_parser.c#L1469 . In particular, only the first character of the header value is validate at line 1490. Then the entire header value is accepted via a memchr scan at https://github.com/nodejs/http-parser/blob/0d0a24e19eb5ba232d2ea8859aba2a7cc6c42bc4/http_parser.c#L1506 . This means that we can have arbitrary embedded NULLs in any HTTP/1.1 header value today. There are places in Envoy where we use one view of these strings for matching (e.g. route table lookup, authorization) and another view for routing and sending to our upstreams. We have scored this as 6.5 using CVSS and will work on an Envoy patch to reject any header values that contain NULL and issue a point release and public disclosure following https://github.com/envoyproxy/envoy/blob/master/SECURITY_RELEASE_PROCESS.md ASAP. This issue can/should also be resolved in http-parser by ensuring the entire header value is validated as per RFC. We do not plan on doing the fix for this prior to our release, but will coordinate with you folks if you are interested in doing so. Please advise on how you would like to proceed with this. We are planning to get our disclosure/release happening either end-of-week or early next week, but we would like to provide you an opportunity to provide some input here, since this has not been publicly disclosed yet we can provide some additional time if needed. Thanks, Harvey (on behalf of Envoy security team) --0000000000006b3f64058413dc0b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><div dir=3D"ltr">Hi Node.js Security WG, = Ryan,<div><br></div><div>We (Envoy security team) have discovered a potenti= al security vulnerability related to our use of http-parser that we are wor= king to fix, patch and issue a security update for Envoy-side following=C2= =A0<a href=3D"https://github.com/envoyproxy/envoy/blob/master/SECURITY_RELE= ASE_PROCESS.md">https://github.com/envoyproxy/envoy/blob/master/SECURITY_RE= LEASE_PROCESS.md</a>.</div><div><br></div><div>We would like to give you ad= vanced notice of this under embargo, check in with you to see if this might= affect Node.js or other http-parser users, and potentially coordinate on a= n http-parser side fix.</div><div><br></div><div>Envoy makes use of http-pa= rser as its HTTP/1 codec on its data plane. Envoy has baked into it today t= he assumption that its HTTP codecs (http-parser, nghttp2) enforce RFC const= raints on valid header values (<a href=3D"https://tools.ietf.org/html/rfc72= 30#section-3.2.6" target=3D"_blank">https://tools.ietf.org/html/rfc7230#sec= tion-3.2.6</a>). In particular, we expect that there are no embedded NULLs = in header values or keys that are placed in Envoy&#39;s HeaderStrings and H= eaderMapImpls objects. This is particularly important because we allow two = views of a HeaderString, via=C2=A0<a href=3D"https://github.com/envoyproxy/= envoy/blob/b41ba5925a4e93d22a86c6501d63314ccf0d79f3/include/envoy/http/head= er_map.h#L115" target=3D"_blank">c_str()</a>=C2=A0and=C2=A0<a href=3D"https= ://github.com/envoyproxy/envoy/blob/b41ba5925a4e93d22a86c6501d63314ccf0d79f= 3/include/envoy/http/header_map.h#L120" target=3D"_blank">getStringView()</= a>; embedded NULLS cause inconsistent views and lengths through these acces= sors. We use a mixture of these in header matching and routing.</div></div>= <div><br></div><div>Our fuzzers and some recently introduced ASSERTs indica= ted that embedded NULLs were making their way into header values received f= rom http-parser. Digging deeper, the errant behavior is due to a bug in how= validation of header values is performed by http-parser. You can see this = in the validation logic at=C2=A0<a href=3D"https://github.com/nodejs/http-p= arser/blob/0d0a24e19eb5ba232d2ea8859aba2a7cc6c42bc4/http_parser.c#L1469" ta= rget=3D"_blank">https://github.com/nodejs/http-parser/blob/0d0a24e19eb5ba23= 2d2ea8859aba2a7cc6c42bc4/http_parser.c#L1469</a>.=C2=A0</div><div><br></div= ><div>In particular, only the first character of the header value is valida= te at line 1490. Then the entire header value is accepted via a memchr scan= at=C2=A0<a href=3D"https://github.com/nodejs/http-parser/blob/0d0a24e19eb5= ba232d2ea8859aba2a7cc6c42bc4/http_parser.c#L1506" target=3D"_blank">https:/= /github.com/nodejs/http-parser/blob/0d0a24e19eb5ba232d2ea8859aba2a7cc6c42bc= 4/http_parser.c#L1506</a>.</div><div><br></div><div>This means that we can = have arbitrary embedded NULLs in any HTTP/1.1 header value today. There are= places in Envoy where we use one view of these strings for matching (e.g. = route table lookup, authorization) and another view for routing and sending= to our upstreams.<br></div><div><br></div><div>We have scored this as 6.5 = using CVSS and will work on an Envoy patch to reject any header values that= contain NULL and issue a point release and public disclosure following=C2= =A0<a href=3D"https://github.com/envoyproxy/envoy/blob/master/SECURITY_RELE= ASE_PROCESS.md">https://github.com/envoyproxy/envoy/blob/master/SECURITY_RE= LEASE_PROCESS.md</a> ASAP.</div><div><br></div><div>This issue can/should a= lso be resolved in http-parser by ensuring the entire header value is valid= ated as per RFC. We do not plan on doing the fix for this prior to our rele= ase, but will coordinate with you folks if you are interested in doing so.<= /div><div><br></div><div>Please advise on how you would like to proceed wit= h this. We are planning to get our disclosure/release happening either end-= of-week or early next week, but we would like to provide you an opportunity= to provide some input here, since this has not been publicly disclosed yet= we can provide some additional time if needed.</div><div><br></div><div>Th= anks,</div><div>Harvey (on behalf of Envoy security team)</div><div><br></d= iv></div></div> --0000000000006b3f64058413dc0b-- ## Impact This has a CVSS score of 8.3 for Envoy as a project consuming http-parser. The impact on Node.JS is unclear to me.