From 5c887984a2ccd6c9981497b389f6edbfded96f2c Mon Sep 17 00:00:00 2001 From: Thomas Wisniewski Date: Thu, 17 Aug 2023 14:50:15 +0000 Subject: [PATCH] Bug 1687903 - Enable stricter HTTP response status line parsing; r=valentin,necko-reviewers Differential Revision: https://phabricator.services.mozilla.com/D186229 --- .../test/browser_net_http3_request_details.js | 4 +- dom/xhr/tests/mochitest.ini | 4 +- modules/libpref/init/StaticPrefList.yaml | 6 +++ .../protocol/http/InterceptedHttpChannel.cpp | 3 +- netwerk/protocol/http/nsHttpResponseHead.cpp | 40 ++++++++++++++++--- netwerk/protocol/http/nsHttpResponseHead.h | 4 +- netwerk/protocol/http/nsHttpTransaction.cpp | 10 +++-- netwerk/test/gtest/TestHttpResponseHead.cpp | 32 +++++++++++++-- .../meta/fetch/h1-parsing/__dir__.ini | 1 + .../fetch/h1-parsing/lone-cr.window.js.ini | 4 -- .../h1-parsing/status-code.window.js.ini | 32 --------------- .../meta/wasm/webapi/status.any.js.ini | 23 ----------- 12 files changed, 86 insertions(+), 77 deletions(-) diff --git a/devtools/client/netmonitor/test/browser_net_http3_request_details.js b/devtools/client/netmonitor/test/browser_net_http3_request_details.js index 8ec7aa1b03edc..9ceb9dba88137 100644 --- a/devtools/client/netmonitor/test/browser_net_http3_request_details.js +++ b/devtools/client/netmonitor/test/browser_net_http3_request_details.js @@ -50,7 +50,7 @@ add_task(async function () { ); is( tabpanel.querySelector(".status").childNodes[1].textContent, - "OK", + "", // HTTP/2 and 3 send no status text, only a code "The status summary value is incorrect." ); // Version @@ -139,7 +139,7 @@ add_task(async function () { const responseHeadersText = rawHeadersElements[0].textContent; const rawResponseHeaderFirstLine = responseHeadersText.split(/\r\n|\n|\r/)[0]; - is(rawResponseHeaderFirstLine, "HTTP/3 200 OK"); + is(rawResponseHeaderFirstLine, "HTTP/3 200 "); // H2/3 send no status text info("Assert the content of the protocol column"); const target = document.querySelectorAll(".request-list-item")[0]; diff --git a/dom/xhr/tests/mochitest.ini b/dom/xhr/tests/mochitest.ini index e0bbd3ac86fbd..4aed4db02b9a0 100644 --- a/dom/xhr/tests/mochitest.ini +++ b/dom/xhr/tests/mochitest.ini @@ -115,7 +115,7 @@ skip-if = [test_worker_xhrAbort.html] skip-if = (os == "win") || (os == "mac") [test_XHR.html] -skip-if = http2 +skip-if = http2 || http3 [test_xhr_abort_after_load.html] [test_XHR_anon.html] skip-if = @@ -127,7 +127,7 @@ skip-if = http3 http2 [test_XHR_http2.html] -skip-if = !http2 +skip-if = !http2 && !http3 [test_XHR_onuploadprogress.html] [test_xhr_overridemimetype_throws_on_invalid_state.html] [test_XHR_parameters.html] diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 7960db8798b2d..195abf1c17ac4 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -12613,6 +12613,12 @@ value: true mirror: always + # If true, will be more strict with status code parsing +- name: network.http.strict_response_status_line_parsing + type: RelaxedAtomicBool + value: true + mirror: always + # If true, remove the resumption token when 0RTT failed. - name: network.http.remove_resumption_token_when_early_data_failed type: RelaxedAtomicBool diff --git a/netwerk/protocol/http/InterceptedHttpChannel.cpp b/netwerk/protocol/http/InterceptedHttpChannel.cpp index c493259905d8b..6e733cc360135 100644 --- a/netwerk/protocol/http/InterceptedHttpChannel.cpp +++ b/netwerk/protocol/http/InterceptedHttpChannel.cpp @@ -848,7 +848,8 @@ InterceptedHttpChannel::SynthesizeStatus(uint16_t aStatus, statusLine.AppendLiteral(" "); statusLine.Append(aReason); - mSynthesizedResponseHead->ParseStatusLine(statusLine); + NS_ENSURE_SUCCESS(mSynthesizedResponseHead->ParseStatusLine(statusLine), + NS_ERROR_FAILURE); return NS_OK; } diff --git a/netwerk/protocol/http/nsHttpResponseHead.cpp b/netwerk/protocol/http/nsHttpResponseHead.cpp index 68693faabe3e4..71dd803292cfd 100644 --- a/netwerk/protocol/http/nsHttpResponseHead.cpp +++ b/netwerk/protocol/http/nsHttpResponseHead.cpp @@ -8,6 +8,7 @@ #include "HttpLog.h" #include "mozilla/StaticPrefs_network.h" +#include "mozilla/TextUtils.h" #include "mozilla/Unused.h" #include "nsHttpResponseHead.h" #include "nsIHttpHeaderVisitor.h" @@ -351,19 +352,18 @@ void nsHttpResponseHead::AssignDefaultStatusText() { net_GetDefaultStatusTextForCode(mStatus, mStatusText); } -void nsHttpResponseHead::ParseStatusLine(const nsACString& line) { +nsresult nsHttpResponseHead::ParseStatusLine(const nsACString& line) { RecursiveMutexAutoLock monitor(mRecursiveMutex); - ParseStatusLine_locked(line); + return ParseStatusLine_locked(line); } -void nsHttpResponseHead::ParseStatusLine_locked(const nsACString& line) { +nsresult nsHttpResponseHead::ParseStatusLine_locked(const nsACString& line) { // // Parse Status-Line:: HTTP-Version SP Status-Code SP Reason-Phrase CRLF // const char* start = line.BeginReading(); const char* end = line.EndReading(); - const char* p = start; // HTTP-Version ParseVersion(start); @@ -373,9 +373,38 @@ void nsHttpResponseHead::ParseStatusLine_locked(const nsACString& line) { if ((mVersion == HttpVersion::v0_9) || (index == -1)) { mStatus = 200; AssignDefaultStatusText(); + } else if (StaticPrefs::network_http_strict_response_status_line_parsing()) { + // Status-Code: all ASCII digits after any whitespace + const char* p = start + index + 1; + while (p < end && NS_IsHTTPWhitespace(*p)) ++p; + if (p == end || !mozilla::IsAsciiDigit(*p)) { + return NS_ERROR_FAILURE; + } + const char* codeStart = p; + while (p < end && mozilla::IsAsciiDigit(*p)) ++p; + + // Only accept 3 digits (including all leading zeros) + // Also if next char isn't whitespace, fail (ie, code is 0x2) + if (p - codeStart > 3 || (p < end && !NS_IsHTTPWhitespace(*p))) { + return NS_ERROR_FAILURE; + } + + // At this point the code is between 0 and 999 inclusive + nsDependentCSubstring strCode(codeStart, p - codeStart); + nsresult rv; + mStatus = strCode.ToInteger(&rv); + if (NS_FAILED(rv)) { + return NS_ERROR_FAILURE; + } + + // Reason-Phrase: whatever remains after any whitespace (even empty) + while (p < end && NS_IsHTTPWhitespace(*p)) ++p; + if (p != end) { + mStatusText = nsDependentCSubstring(p, end - p); + } } else { // Status-Code - p += index + 1; + const char* p = start + index + 1; mStatus = (uint16_t)atoi(p); if (mStatus == 0) { LOG(("mal-formed response status; assuming status = 200\n")); @@ -394,6 +423,7 @@ void nsHttpResponseHead::ParseStatusLine_locked(const nsACString& line) { LOG1(("Have status line [version=%u status=%u statusText=%s]\n", unsigned(mVersion), unsigned(mStatus), mStatusText.get())); + return NS_OK; } nsresult nsHttpResponseHead::ParseHeaderLine(const nsACString& line) { diff --git a/netwerk/protocol/http/nsHttpResponseHead.h b/netwerk/protocol/http/nsHttpResponseHead.h index dd007d3a7566e..9caf92b3e12e0 100644 --- a/netwerk/protocol/http/nsHttpResponseHead.h +++ b/netwerk/protocol/http/nsHttpResponseHead.h @@ -101,7 +101,7 @@ class nsHttpResponseHead { [[nodiscard]] nsresult ParseCachedOriginalHeaders(char* block); // parse the status line. - void ParseStatusLine(const nsACString& line); + nsresult ParseStatusLine(const nsACString& line); // parse a header line. [[nodiscard]] nsresult ParseHeaderLine(const nsACString& line); @@ -160,7 +160,7 @@ class nsHttpResponseHead { nsresult ParseResponseContentLength(const nsACString& aHeaderStr) MOZ_REQUIRES(mRecursiveMutex); - void ParseStatusLine_locked(const nsACString& line) + nsresult ParseStatusLine_locked(const nsACString& line) MOZ_REQUIRES(mRecursiveMutex); [[nodiscard]] nsresult ParseHeaderLine_locked(const nsACString& line, bool originalFromNetHeaders) diff --git a/netwerk/protocol/http/nsHttpTransaction.cpp b/netwerk/protocol/http/nsHttpTransaction.cpp index caddb2dd950a4..999e1de7fb772 100644 --- a/netwerk/protocol/http/nsHttpTransaction.cpp +++ b/netwerk/protocol/http/nsHttpTransaction.cpp @@ -1961,8 +1961,10 @@ nsresult nsHttpTransaction::ParseLine(nsACString& line) { nsresult rv = NS_OK; if (!mHaveStatusLine) { - mResponseHead->ParseStatusLine(line); - mHaveStatusLine = true; + rv = mResponseHead->ParseStatusLine(line); + if (NS_SUCCEEDED(rv)) { + mHaveStatusLine = true; + } // XXX this should probably never happen if (mResponseHead->Version() == HttpVersion::v0_9) mHaveAllHeaders = true; } else { @@ -2086,7 +2088,9 @@ nsresult nsHttpTransaction::ParseHead(char* buf, uint32_t count, // Treat any 0.9 style response of a put as a failure. if (mRequestHead->IsPut()) return NS_ERROR_ABORT; - mResponseHead->ParseStatusLine(""_ns); + if (NS_FAILED(mResponseHead->ParseStatusLine(""_ns))) { + return NS_ERROR_FAILURE; + } mHaveStatusLine = true; mHaveAllHeaders = true; return NS_OK; diff --git a/netwerk/test/gtest/TestHttpResponseHead.cpp b/netwerk/test/gtest/TestHttpResponseHead.cpp index 19ec022997b41..bb10f3e9ff6c2 100644 --- a/netwerk/test/gtest/TestHttpResponseHead.cpp +++ b/netwerk/test/gtest/TestHttpResponseHead.cpp @@ -4,6 +4,8 @@ #include "mozilla/net/PHttpChannelParams.h" #include "mozilla/Unused.h" #include "nsHttp.h" +#include "nsIPrefBranch.h" +#include "nsIPrefService.h" namespace mozilla { namespace net { @@ -44,7 +46,7 @@ TEST(TestHttpResponseHead, Bug1636930) { nsHttpResponseHead head; - head.ParseStatusLine("HTTP/1.1 200 OK"_ns); + Unused << head.ParseStatusLine("HTTP/1.1 200 OK"_ns); Unused << head.ParseHeaderLine("content-type: text/plain"_ns); Unused << head.ParseHeaderLine("etag: Just testing"_ns); Unused << head.ParseHeaderLine("cache-control: max-age=99999"_ns); @@ -61,7 +63,7 @@ TEST(TestHttpResponseHead, bug1649807) { nsHttpResponseHead head; - head.ParseStatusLine("HTTP/1.1 200 OK"_ns); + Unused << head.ParseStatusLine("HTTP/1.1 200 OK"_ns); Unused << head.ParseHeaderLine("content-type: text/plain"_ns); Unused << head.ParseHeaderLine("etag: Just testing"_ns); Unused << head.ParseHeaderLine("cache-control: age=99999"_ns); @@ -81,7 +83,7 @@ TEST(TestHttpResponseHead, bug1660200) { nsHttpResponseHead head; - head.ParseStatusLine("HTTP/1.1 200 OK"_ns); + Unused << head.ParseStatusLine("HTTP/1.1 200 OK"_ns); Unused << head.ParseHeaderLine("content-type: text/plain"_ns); Unused << head.ParseHeaderLine("etag: Just testing"_ns); Unused << head.ParseHeaderLine("cache-control: no-cache"_ns); @@ -94,6 +96,30 @@ TEST(TestHttpResponseHead, bug1660200) AssertRoundTrips(head); } +TEST(TestHttpResponseHead, bug1687903) +{ + nsHttpResponseHead head; + + bool usingStrictParsing = false; + nsCOMPtr prefs = do_GetService(NS_PREFSERVICE_CONTRACTID); + if (prefs) { + prefs->GetBoolPref("network.http.strict_response_status_line_parsing", + &usingStrictParsing); + } + + nsresult expectation = usingStrictParsing ? NS_ERROR_FAILURE : NS_OK; + + ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 "_ns)); + ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 BLAH"_ns)); + ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 1000 BOO"_ns)); + ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 0200 BOO"_ns)); + ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 60200 200"_ns)); + ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 131072 HIOK"_ns)); + ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 -200 OK"_ns)); + ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 0x9 OK"_ns)); + ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 C8 OK"_ns)); +} + TEST(TestHttpResponseHead, atoms) { // Test that the resolving the content-type atom returns the initial static diff --git a/testing/web-platform/meta/fetch/h1-parsing/__dir__.ini b/testing/web-platform/meta/fetch/h1-parsing/__dir__.ini index 20fc98f0efe17..27fe8837e3b47 100644 --- a/testing/web-platform/meta/fetch/h1-parsing/__dir__.ini +++ b/testing/web-platform/meta/fetch/h1-parsing/__dir__.ini @@ -1 +1,2 @@ leak-threshold: [default:3020800] +prefs: [network.http.strict_response_status_line_parsing:true] diff --git a/testing/web-platform/meta/fetch/h1-parsing/lone-cr.window.js.ini b/testing/web-platform/meta/fetch/h1-parsing/lone-cr.window.js.ini index 1516da5e07f58..9f27700910836 100644 --- a/testing/web-platform/meta/fetch/h1-parsing/lone-cr.window.js.ini +++ b/testing/web-platform/meta/fetch/h1-parsing/lone-cr.window.js.ini @@ -1,7 +1,4 @@ [lone-cr.window.html] - [Parsing response with a lone CR before message-body (HTTP/1.1\r200 OK\n\nBODY)] - expected: FAIL - [Parsing response with a lone CR before message-body (HTTP/1.1 200\rOK\n\nBODY)] expected: FAIL @@ -28,4 +25,3 @@ [Parsing response with a lone CR before message-body (HTTP/1.1 200 OK\nHeader: Value\n\r)] expected: FAIL - diff --git a/testing/web-platform/meta/fetch/h1-parsing/status-code.window.js.ini b/testing/web-platform/meta/fetch/h1-parsing/status-code.window.js.ini index 43b780fa7e534..af190b81d1174 100644 --- a/testing/web-platform/meta/fetch/h1-parsing/status-code.window.js.ini +++ b/testing/web-platform/meta/fetch/h1-parsing/status-code.window.js.ini @@ -2,35 +2,3 @@ expected: if (os == "android") and debug and fission: [TIMEOUT, OK] if (os == "android") and debug and not fission: [OK, TIMEOUT] - [HTTP/1.1 (network error)] - expected: FAIL - - [HTTP/1.1 BLAH (network error)] - expected: FAIL - - [HTTP/1.1 0 OK ] - expected: FAIL - - [HTTP/1.1 200 ] - expected: FAIL - - [HTTP/1.1 1000 BOO (network error)] - expected: FAIL - - [HTTP/1.1 0200 BOO (network error)] - expected: FAIL - - [HTTP/1.1 65736 NOT 200 OR SOME SUCH (network error)] - expected: FAIL - - [HTTP/1.1 131072 HI (network error)] - expected: FAIL - - [HTTP/1.1 -200 TEST (network error)] - expected: FAIL - - [HTTP/1.1 0xA (network error)] - expected: FAIL - - [HTTP/1.1 C8 (network error)] - expected: FAIL diff --git a/testing/web-platform/meta/wasm/webapi/status.any.js.ini b/testing/web-platform/meta/wasm/webapi/status.any.js.ini index d0b1737170acc..a824ff713acd7 100644 --- a/testing/web-platform/meta/wasm/webapi/status.any.js.ini +++ b/testing/web-platform/meta/wasm/webapi/status.any.js.ini @@ -1,38 +1,15 @@ [status.any.html] expected: if (os == "android") and fission: [OK, TIMEOUT] - [Response with status 0: compileStreaming] - expected: FAIL - - [Response with status 0: instantiateStreaming] - expected: FAIL - [status.any.serviceworker.html] expected: if (os == "android") and fission: [OK, TIMEOUT] - [Response with status 0: compileStreaming] - expected: FAIL - - [Response with status 0: instantiateStreaming] - expected: FAIL - [status.any.sharedworker.html] expected: if (os == "android") and fission: [OK, TIMEOUT] - [Response with status 0: compileStreaming] - expected: FAIL - - [Response with status 0: instantiateStreaming] - expected: FAIL - [status.any.worker.html] expected: if (os == "android") and fission: [OK, TIMEOUT] - [Response with status 0: compileStreaming] - expected: FAIL - - [Response with status 0: instantiateStreaming] - expected: FAIL