Bug 1531344 - Be strict about incorrect chunked encoding. r=mayhemer
authorDragana Damjanovic <dd.mozilla@gmail.com>
Wed, 08 May 2019 19:29:16 +0000
changeset 531965 5b5baa11e448f70893bc096aa518b4f58d07cc2f
parent 531964 dae0588357c3d47f3550fc4df8b6fbf19eac7f0e
child 531966 244b0ee2c89066b418c7088acf880f5f4eb6fa3d
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1531344
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1531344 - Be strict about incorrect chunked encoding. r=mayhemer Differential Revision: https://phabricator.services.mozilla.com/D28811
modules/libpref/init/all.js
netwerk/protocol/http/nsHttpChunkedDecoder.h
netwerk/protocol/http/nsHttpHandler.cpp
netwerk/protocol/http/nsHttpHandler.h
netwerk/protocol/http/nsHttpTransaction.cpp
netwerk/test/unit/test_content_length_underrun.js
netwerk/test/unit/test_partial_response_entry_size_smart_shrink.js
--- a/modules/libpref/init/all.js
+++ b/modules/libpref/init/all.js
@@ -1875,16 +1875,17 @@ pref("network.http.tcp_keepalive.short_l
 // Idle time of TCP connection until first keepalive probe sent.
 pref("network.http.tcp_keepalive.short_lived_idle_time", 10);
 
 pref("network.http.tcp_keepalive.long_lived_connections", true);
 pref("network.http.tcp_keepalive.long_lived_idle_time", 600);
 
 pref("network.http.enforce-framing.http1", false); // should be named "strict"
 pref("network.http.enforce-framing.soft", true);
+pref("network.http.enforce-framing.strict_chunked_encoding", true);
 
 // Max size, in bytes, for received HTTP response header.
 pref("network.http.max_response_header_size", 393216);
 
 // If we should attempt to race the cache and network
 pref("network.http.rcwn.enabled", true);
 pref("network.http.rcwn.cache_queue_normal_threshold", 8);
 pref("network.http.rcwn.cache_queue_priority_threshold", 2);
--- a/netwerk/protocol/http/nsHttpChunkedDecoder.h
+++ b/netwerk/protocol/http/nsHttpChunkedDecoder.h
@@ -29,16 +29,17 @@ class nsHttpChunkedDecoder {
   MOZ_MUST_USE nsresult HandleChunkedContent(char* buf, uint32_t count,
                                              uint32_t* contentRead,
                                              uint32_t* contentRemaining);
 
   nsHttpHeaderArray* Trailers() { return mTrailers.get(); }
 
   nsHttpHeaderArray* TakeTrailers() { return mTrailers.forget(); }
 
+  // How mush data is still missing(needs to arrive) from the current chunk.
   uint32_t GetChunkRemaining() { return mChunkRemaining; }
 
  private:
   MOZ_MUST_USE nsresult ParseChunkRemaining(char* buf, uint32_t count,
                                             uint32_t* countRead);
 
  private:
   nsAutoPtr<nsHttpHeaderArray> mTrailers;
--- a/netwerk/protocol/http/nsHttpHandler.cpp
+++ b/netwerk/protocol/http/nsHttpHandler.cpp
@@ -1821,26 +1821,33 @@ void nsHttpHandler::PrefsChanged(const c
   if (PREF_CHANGED(HTTP_PREF("tcp_keepalive.long_lived_idle_time"))) {
     rv = Preferences::GetInt(HTTP_PREF("tcp_keepalive.long_lived_idle_time"),
                              &val);
     if (NS_SUCCEEDED(rv) && val > 0)
       mTCPKeepaliveLongLivedIdleTimeS = clamped(val, 1, kMaxTCPKeepIdle);
   }
 
   if (PREF_CHANGED(HTTP_PREF("enforce-framing.http1")) ||
-      PREF_CHANGED(HTTP_PREF("enforce-framing.soft"))) {
+      PREF_CHANGED(HTTP_PREF("enforce-framing.soft")) ||
+      PREF_CHANGED(HTTP_PREF("enforce-framing.strict_chunked_encoding"))) {
     rv = Preferences::GetBool(HTTP_PREF("enforce-framing.http1"), &cVar);
     if (NS_SUCCEEDED(rv) && cVar) {
       mEnforceH1Framing = FRAMECHECK_STRICT;
     } else {
-      rv = Preferences::GetBool(HTTP_PREF("enforce-framing.soft"), &cVar);
+      rv = Preferences::GetBool(
+          HTTP_PREF("enforce-framing.strict_chunked_encoding"), &cVar);
       if (NS_SUCCEEDED(rv) && cVar) {
-        mEnforceH1Framing = FRAMECHECK_BARELY;
+        mEnforceH1Framing = FRAMECHECK_STRICT_CHUNKED;
       } else {
-        mEnforceH1Framing = FRAMECHECK_LAX;
+        rv = Preferences::GetBool(HTTP_PREF("enforce-framing.soft"), &cVar);
+        if (NS_SUCCEEDED(rv) && cVar) {
+          mEnforceH1Framing = FRAMECHECK_BARELY;
+        } else {
+          mEnforceH1Framing = FRAMECHECK_LAX;
+        }
       }
     }
   }
 
   if (PREF_CHANGED(TCP_FAST_OPEN_ENABLE)) {
     rv = Preferences::GetBool(TCP_FAST_OPEN_ENABLE, &cVar);
     if (NS_SUCCEEDED(rv)) {
       mUseFastOpen = cVar;
--- a/netwerk/protocol/http/nsHttpHandler.h
+++ b/netwerk/protocol/http/nsHttpHandler.h
@@ -42,17 +42,46 @@ bool OnSocketThread();
 class ATokenBucketEvent;
 class EventTokenBucket;
 class Tickler;
 class nsHttpConnection;
 class nsHttpConnectionInfo;
 class nsHttpTransaction;
 class AltSvcMapping;
 
-enum FrameCheckLevel { FRAMECHECK_LAX, FRAMECHECK_BARELY, FRAMECHECK_STRICT };
+/*
+ * FRAMECHECK_LAX - no check
+ * FRAMECHECK_BARELY - allows:
+ *                     1) that chunk-encoding does not have the last 0-size
+ *                     chunk. So, if a chunked-encoded transfer ends on exactly
+ *                     a chunk boundary we consider that fine. This will allows
+ *                     us to accept buggy servers that do not send the last
+ *                     chunk. It will make us not detect a certain amount of
+ *                     cut-offs.
+ *                     2) When receiving a gzipped response, we consider a
+ *                     gzip stream that doesn't end fine according to the gzip
+ *                     decompressing state machine to be a partial transfer.
+ *                     If a gzipped transfer ends fine according to the
+ *                     decompressor, we do not check for size unalignments.
+ *                     This allows to allow HTTP gzipped responses where the
+ *                     Content-Length is not the same as the actual contents.
+ *                     3) When receiving HTTP that isn't
+ *                     content-encoded/compressed (like in case 2) and not
+ *                     chunked (like in case 1), perform the size comparison
+ *                     between Content-Length: and the actual size received
+ *                     and consider a mismatch to mean a
+ *                     NS_ERROR_NET_PARTIAL_TRANSFER error.
+ * FRAMECHECK_STRICT_CHUNKED - This is the same as FRAMECHECK_BARELY only we
+ *                             enforce that the last 0-size chunk is received
+ *                             in case 1).
+ * FRAMECHECK_STRICT - we also do not allow case 2) and 3) from
+ *                     FRAMECHECK_BARELY.
+ */
+enum FrameCheckLevel { FRAMECHECK_LAX, FRAMECHECK_BARELY,
+                       FRAMECHECK_STRICT_CHUNKED, FRAMECHECK_STRICT };
 
 //-----------------------------------------------------------------------------
 // nsHttpHandler - protocol handler for HTTP and HTTPS
 //-----------------------------------------------------------------------------
 
 class nsHttpHandler final : public nsIHttpProtocolHandler,
                             public nsIObserver,
                             public nsSupportsWeakReference,
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -1124,18 +1124,33 @@ void nsHttpTransaction::Close(nsresult r
 
   if ((mChunkedDecoder || (mContentLength >= int64_t(0))) &&
       (NS_SUCCEEDED(reason) && !mResponseIsComplete)) {
     NS_WARNING("Partial transfer, incomplete HTTP response received");
 
     if ((mHttpResponseCode / 100 == 2) && (mHttpVersion >= HttpVersion::v1_1)) {
       FrameCheckLevel clevel = gHttpHandler->GetEnforceH1Framing();
       if (clevel >= FRAMECHECK_BARELY) {
+        // If clevel == FRAMECHECK_STRICT mark any incomplete response as
+        // partial.
+        // if clevel == FRAMECHECK_BARELY: 1) mark a chunked-encoded response
+        // that do not ends on exactly a chunk boundary as partial; We are not
+        // strict about the last 0-size chunk and do not mark as parial
+        // responses that do not have the last 0-size chunk but do end on a
+        // chunk boundary. (check mChunkedDecoder->GetChunkRemaining() != 0)
+        // 2) mark a transfer that is partial and it is not chunk-encoded or
+        // gzip-encoded or other content-encoding as partial. (check
+        // !mChunkedDecoder && !mContentDecoding && mContentDecodingCheck))
+        // if clevel == FRAMECHECK_STRICT_CHUNKED mark a chunked-encoded
+        // response that ends on exactly a chunk boundary also as partial.
+        // Here a response must have the last 0-size chunk.
         if ((clevel == FRAMECHECK_STRICT) ||
-            (mChunkedDecoder && mChunkedDecoder->GetChunkRemaining()) ||
+            (mChunkedDecoder &&
+             (mChunkedDecoder->GetChunkRemaining() ||
+              (clevel == FRAMECHECK_STRICT_CHUNKED))) ||
             (!mChunkedDecoder && !mContentDecoding && mContentDecodingCheck)) {
           reason = NS_ERROR_NET_PARTIAL_TRANSFER;
           LOG(("Partial transfer, incomplete HTTP response received: %s",
                mChunkedDecoder ? "broken chunk" : "c-l underrun"));
         }
       }
     }
 
--- a/netwerk/test/unit/test_content_length_underrun.js
+++ b/netwerk/test/unit/test_content_length_underrun.js
@@ -14,27 +14,31 @@ XPCOMUtils.defineLazyGetter(this, "URL",
 var httpserver = new HttpServer();
 var index = 0;
 var test_flags = new Array();
 var testPathBase = "/cl_hdrs";
 
 var prefs;
 var enforcePrefStrict;
 var enforcePrefSoft;
+var enforcePrefStrictChunked;
 
 Services.prefs.setBoolPref("security.allow_eval_with_system_principal", true);
 registerCleanupFunction(() => {
   Services.prefs.clearUserPref("security.allow_eval_with_system_principal");
 });
 
 function run_test()
 {
   prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
   enforcePrefStrict = prefs.getBoolPref("network.http.enforce-framing.http1");
   enforcePrefSoft = prefs.getBoolPref("network.http.enforce-framing.soft");
+  enforcePrefStrictChunked = prefs.getBoolPref(
+      "network.http.enforce-framing.strict_chunked_encoding");
+
   prefs.setBoolPref("network.http.enforce-framing.http1", true);
 
   httpserver.start(-1);
 
   do_test_pending();
   run_test_number(1);
 }
 
@@ -95,16 +99,18 @@ function setupChannel(url)
   return httpChan;
 }
 
 function endTests()
 {
   // restore the prefs to pre-test values
   prefs.setBoolPref("network.http.enforce-framing.http1", enforcePrefStrict);
   prefs.setBoolPref("network.http.enforce-framing.soft", enforcePrefSoft);
+  prefs.setBoolPref("network.http.enforce-framing.strict_chunked_encoding",
+                    enforcePrefStrictChunked);
   httpserver.stop(do_test_finished);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Test 1: FAIL because of Content-Length underrun with HTTP 1.1
 test_flags[1] = CL_EXPECT_LATE_FAILURE;
 
 function handler1(metadata, response)
@@ -170,16 +176,18 @@ function handler2(metadata, response)
 
 function completeTest2(request, data, ctx)
 {
   Assert.equal(request.status, Cr.NS_OK);
 
   // test 3 requires the enforce-framing prefs to be false
   prefs.setBoolPref("network.http.enforce-framing.http1", false);
   prefs.setBoolPref("network.http.enforce-framing.soft", false);
+  prefs.setBoolPref("network.http.enforce-framing.strict_chunked_encoding",
+                    false);
   run_test_number(3);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Test 3: SUCCEED with bad Content-Length because pref allows it
 test_flags[3] = CL_IGNORE_CL;
 
 function handler3(metadata, response)
--- a/netwerk/test/unit/test_partial_response_entry_size_smart_shrink.js
+++ b/netwerk/test/unit/test_partial_response_entry_size_smart_shrink.js
@@ -48,19 +48,22 @@ function contentHandler(metadata, respon
     response.bodyOutputStream.write(slice, slice.length);
   }
 }
 
 var enforcePref;
 
 function run_test()
 {
-  enforcePref = Services.prefs.getBoolPref("network.http.enforce-framing.soft");
+  enforceSoftPref = Services.prefs.getBoolPref("network.http.enforce-framing.soft");
   Services.prefs.setBoolPref("network.http.enforce-framing.soft", false);
 
+  enforceStrictChunkedPref = Services.prefs.getBoolPref("network.http.enforce-framing.strict_chunked_encoding");
+  Services.prefs.setBoolPref("network.http.enforce-framing.strict_chunked_encoding", false);
+
   httpServer = new HttpServer();
   httpServer.registerPathHandler("/content", contentHandler);
   httpServer.start(-1);
 
   var chan = make_channel(URL + "/content");
   chan.asyncOpen(new ChannelListener(firstTimeThrough, null, CL_IGNORE_CL));
   do_test_pending();
 }
@@ -72,11 +75,12 @@ function firstTimeThrough(request, buffe
 
   var chan = make_channel(URL + "/content");
   chan.asyncOpen(new ChannelListener(finish_test, null));
 }
 
 function finish_test(request, buffer)
 {
   Assert.equal(buffer, responseBody);
-  Services.prefs.setBoolPref("network.http.enforce-framing.soft", enforcePref);
+  Services.prefs.setBoolPref("network.http.enforce-framing.soft", enforceSoftPref);
+  Services.prefs.setBoolPref("network.http.enforce-framing.strict_chunked_encoding", enforceStrictChunkedPref);
   httpServer.stop(do_test_finished);
 }