Issue #2070 - When multiple HSTS headers are received, only consider the first.

This implements a plain interpretations of RFC 6797, which says to only consider
the first HSTS header.
This slightly conflicts with RFC 7230, which says that sending multiple headers
which can't be merged is illegal (except for a specific whitelist which HSTS isn't in),
so this situation should never occur in the first place (and would therefore not need
the explicit entry in RFC 6797).

It improves HSTS robustness dealing with non-compliant servers.

Resolves #2070

5 files changed, 51 insertions, 3 deletions

diff --git a/netwerk/protocol/http/nsHttpAtomList.h b/netwerk/protocol/http/nsHttpAtomList.h
index e4b22e8da3..c8a01f9ccb 100644
--- a/netwerk/protocol/http/nsHttpAtomList.h
+++ b/netwerk/protocol/http/nsHttpAtomList.h
@@ -79,6 +79,7 @@ HTTP_ATOM(Service_Worker_Allowed,    "Service-Worker-Allowed")
 HTTP_ATOM(Set_Cookie,                "Set-Cookie")
 HTTP_ATOM(Set_Cookie2,               "Set-Cookie2")
 HTTP_ATOM(Status_URI,                "Status-URI")
+HTTP_ATOM(Strict_Transport_Security, "Strict-Transport-Security")
 HTTP_ATOM(TE,                        "TE")
 HTTP_ATOM(Title,                     "Title")
 HTTP_ATOM(Timeout,                   "Timeout")
diff --git a/netwerk/protocol/http/nsHttpHeaderArray.cpp b/netwerk/protocol/http/nsHttpHeaderArray.cpp
index 1030bc91ee..2b779da359 100644
--- a/netwerk/protocol/http/nsHttpHeaderArray.cpp
+++ b/netwerk/protocol/http/nsHttpHeaderArray.cpp
@@ -79,7 +79,7 @@ nsHttpHeaderArray::SetHeader(nsHttpAtom header,
         return SetHeader_internal(header, headerName, value, variety);
     } else if (merge && !IsSingletonHeader(header)) {
         return MergeHeader(header, entry, value, variety);
-    } else {
+    } else if (!IsIgnoreMultipleHeader(header)) {
         // Replace the existing string with the new value
         if (entry->variety == eVarietyResponseNetOriginalAndResponse) {
             MOZ_ASSERT(variety == eVarietyResponse);
@@ -190,7 +190,7 @@ nsHttpHeaderArray::SetHeaderFromNet(nsHttpAtom header,
                                     eVarietyResponseNetOriginal);
         }
         return rv;
-    } else {
+    } else if (!IsIgnoreMultipleHeader(header)) {
         // Multiple instances of non-mergeable header received from network
         // - ignore if same value
         if (!entry->value.Equals(value)) {
diff --git a/netwerk/protocol/http/nsHttpHeaderArray.h b/netwerk/protocol/http/nsHttpHeaderArray.h
index cfa7fc6a81..b65b36fcdd 100644
--- a/netwerk/protocol/http/nsHttpHeaderArray.h
+++ b/netwerk/protocol/http/nsHttpHeaderArray.h
@@ -160,6 +160,8 @@ private:
 
     // Header cannot be merged: only one value possible
     bool    IsSingletonHeader(nsHttpAtom header);
+    // Header cannot be merged, and subsequent values should be ignored
+    bool    IsIgnoreMultipleHeader(nsHttpAtom header);
     // For some headers we want to track empty values to prevent them being
     // combined with non-empty ones as a CRLF attack vector
     bool    TrackEmptyHeader(nsHttpAtom header);
@@ -231,7 +233,22 @@ nsHttpHeaderArray::IsSingletonHeader(nsHttpAtom header)
            header == nsHttp::If_Unmodified_Since         ||
            header == nsHttp::From                        ||
            header == nsHttp::Location                    ||
-           header == nsHttp::Max_Forwards;
+           header == nsHttp::Max_Forwards                ||
+           // Ignore-multiple-headers are singletons in the sense that they
+           // shouldn't be merged.
+           IsIgnoreMultipleHeader(header);
+}
+
+// These are headers for which, in the presence of multiple values, we only
+// consider the first.
+inline bool nsHttpHeaderArray::IsIgnoreMultipleHeader(nsHttpAtom header)
+{
+    // https://tools.ietf.org/html/rfc6797#section-8:
+    //
+    //     If a UA receives more than one STS header field in an HTTP
+    //     response message over secure transport, then the UA MUST process
+    //     only the first such header field.
+    return header == nsHttp::Strict_Transport_Security;
 }
 
 inline bool
diff --git a/netwerk/test/gtest/TestHeaders.cpp b/netwerk/test/gtest/TestHeaders.cpp
new file mode 100644
index 0000000000..ead56ec9ab
--- /dev/null
+++ b/netwerk/test/gtest/TestHeaders.cpp
@@ -0,0 +1,29 @@
+#include "gtest/gtest.h"
+
+#include "nsHttpHeaderArray.h"
+
+
+TEST(TestHeaders, DuplicateHSTS) {
+    // When the Strict-Transport-Security header is sent multiple times, its
+    // effective value is the value of the first item. It is not coalesced like
+    // other headers are.
+    mozilla::net::nsHttpHeaderArray headers;
+    nsresult rv = headers.SetHeaderFromNet(
+        mozilla::net::nsHttp::Strict_Transport_Security, NS_LITERAL_CSTRING("max-age=360"), true
+    );
+    ASSERT_EQ(rv, NS_OK);
+
+    nsAutoCString h;
+    rv = headers.GetHeader(mozilla::net::nsHttp::Strict_Transport_Security, h);
+    ASSERT_EQ(rv, NS_OK);
+    ASSERT_EQ(h.get(), "max-age=360");
+
+    rv = headers.SetHeaderFromNet(
+        mozilla::net::nsHttp::Strict_Transport_Security, NS_LITERAL_CSTRING("max-age=720"), true
+    );
+    ASSERT_EQ(rv, NS_OK);
+
+    rv = headers.GetHeader(mozilla::net::nsHttp::Strict_Transport_Security, h);
+    ASSERT_EQ(rv, NS_OK);
+    ASSERT_EQ(h.get(), "max-age=360");
+}
diff --git a/netwerk/test/gtest/moz.build b/netwerk/test/gtest/moz.build
index 588c1275d8..7b2782ca8a 100644
--- a/netwerk/test/gtest/moz.build
+++ b/netwerk/test/gtest/moz.build
@@ -5,6 +5,7 @@
 
 UNIFIED_SOURCES += [
     'TestBase64Stream.cpp',
+    'TestHeaders.cpp',
     'TestProtocolProxyService.cpp',
     'TestStandardURL.cpp',
 ]