Backed out changeset 3fc04d8eb32b (bug 1659763) for frame checker crashes. a=backout

dom/security/FramingChecker.cpp

@@ -5,26 +5,26 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "FramingChecker.h"
-
+#include "nsCharSeparatedTokenizer.h"
-#include <stdint.h>  // uint32_t
-
-#include "nsCOMPtr.h"
 #include "nsContentUtils.h"
-#include "nsDebug.h"
+#include "nsCSPUtils.h"
-#include "nsError.h"
+#include "nsDocShell.h"
 #include "nsHttpChannel.h"
 #include "nsContentSecurityUtils.h"
-#include "nsIContentPolicy.h"
+#include "nsGlobalWindowOuter.h"
+#include "nsIChannel.h"
+#include "nsIConsoleReportCollector.h"
+#include "nsIContentSecurityPolicy.h"
 #include "nsIScriptError.h"
-#include "nsLiteralString.h"
+#include "nsNetUtil.h"
+#include "nsQueryObject.h"
 #include "nsTArray.h"
-#include "nsStringFwd.h"
+#include "mozilla/BasePrincipal.h"
-#include "mozilla/Assertions.h"
+#include "mozilla/dom/nsCSPUtils.h"
+#include "mozilla/dom/LoadURIOptionsBinding.h"
 #include "mozilla/dom/WindowGlobalParent.h"
+#include "mozilla/NullPrincipal.h"
 #include "mozilla/net/HttpBaseChannel.h"
-#include "mozilla/RefPtr.h"
-#include "mozilla/Services.h"
-#include "mozilla/Unused.h"
 
 #include "nsIObserverService.h"
 
@@ -57,13 +57,72 @@ void FramingChecker::ReportError(const char* aMessageTag,
   httpChannel->AddConsoleReport(nsIScriptError::errorFlag, "X-Frame-Options"_ns,
                                 nsContentUtils::eSECURITY_PROPERTIES, spec, 0,
                                 0, nsDependentCString(aMessageTag), params);
+}
 
-  // we are notifying observers for testing purposes because there is no event
+/* static */
-  // to gather that an iframe load was blocked or not.
+bool FramingChecker::CheckOneFrameOptionsPolicy(nsIHttpChannel* aHttpChannel,
-  nsCOMPtr<nsIObserverService> observerService =
+                                                const nsAString& aPolicy) {
-      mozilla::services::GetObserverService();
+  nsCOMPtr<nsIURI> uri;
-  nsAutoString policy(aPolicy);
+  aHttpChannel->GetURI(getter_AddRefs(uri));
-  observerService->NotifyObservers(aURI, "xfo-on-violate-policy", policy.get());
+
+  // return early if header does not have one of the values with meaning
+  if (!aPolicy.LowerCaseEqualsLiteral("deny") &&
+      !aPolicy.LowerCaseEqualsLiteral("sameorigin")) {
+    ReportError("XFrameOptionsInvalid", aHttpChannel, uri, aPolicy);
+    return true;
+  }
+
+  // If the X-Frame-Options value is SAMEORIGIN, then the top frame in the
+  // parent chain must be from the same origin as this document.
+  bool checkSameOrigin = aPolicy.LowerCaseEqualsLiteral("sameorigin");
+
+  nsCOMPtr<nsILoadInfo> loadInfo = aHttpChannel->LoadInfo();
+  RefPtr<mozilla::dom::BrowsingContext> ctx;
+  loadInfo->GetBrowsingContext(getter_AddRefs(ctx));
+
+  while (ctx) {
+    nsCOMPtr<nsIPrincipal> principal;
+    // Generally CheckOneFrameOptionsPolicy is consulted from within the
+    // DocumentLoadListener in the parent process. For loads of type object
+    // and embed it's called from the Document in the content process.
+    // After Bug 1646899 we should be able to remove that branching code for
+    // querying the principal.
+    if (XRE_IsParentProcess()) {
+      WindowGlobalParent* window = ctx->Canonical()->GetCurrentWindowGlobal();
+      if (window) {
+        // Using the URI of the Principal and not the document because e.g.
+        // window.open inherits the principal and hence the URI of the
+        // opening context needed for same origin checks.
+        principal = window->DocumentPrincipal();
+      }
+    } else if (nsPIDOMWindowOuter* windowOuter = ctx->GetDOMWindow()) {
+      principal = nsGlobalWindowOuter::Cast(windowOuter)->GetPrincipal();
+    }
+
+    if (principal && principal->IsSystemPrincipal()) {
+      return true;
+    }
+
+    if (checkSameOrigin) {
+      bool isSameOrigin = principal && principal->IsSameOrigin(uri);
+      // one of the ancestors is not same origin as this document
+      if (!isSameOrigin) {
+        ReportError("XFrameOptionsDeny", aHttpChannel, uri, aPolicy);
+        return false;
+      }
+    }
+    ctx = ctx->GetParent();
+  }
+
+  // If the value of the header is DENY, and the previous condition is
+  // not met (current docshell is not the top docshell), prohibit the
+  // load.
+  if (aPolicy.LowerCaseEqualsLiteral("deny")) {
+    ReportError("XFrameOptionsDeny", aHttpChannel, uri, aPolicy);
+    return false;
+  }
+
+  return true;
 }
 
 // Ignore x-frame-options if CSP with frame-ancestors exists
@@ -89,34 +148,33 @@ static bool ShouldIgnoreFrameOptions(nsIChannel* aChannel,
 // subdocument. This will iterate through and check any number of
 // X-Frame-Options policies in the request (comma-separated in a header,
 // multiple headers, etc).
-// This is based on:
-// https://html.spec.whatwg.org/multipage/document-lifecycle.html#the-x-frame-options-header
 /* static */
 bool FramingChecker::CheckFrameOptions(nsIChannel* aChannel,
                                        nsIContentSecurityPolicy* aCsp,
                                        bool& outIsFrameCheckingSkipped) {
-  // Step 1. If navigable is not a child navigable return true
   if (!aChannel) {
     return true;
   }
 
-  // xfo check only makes sense for subdocument and object loads, if this is
-  // not a load of such type, there is nothing to do here.
   nsCOMPtr<nsILoadInfo> loadInfo = aChannel->LoadInfo();
   ExtContentPolicyType contentType = loadInfo->GetExternalContentPolicyType();
+
+  // xfo check only makes sense for subdocument and object loads, if this is
+  // not a load of such type, there is nothing to do here.
   if (contentType != ExtContentPolicy::TYPE_SUBDOCUMENT &&
       contentType != ExtContentPolicy::TYPE_OBJECT) {
     return true;
   }
 
-  // xfo can only hang off an httpchannel, if this is not an httpChannel
-  // then there is nothing to do here.
   nsCOMPtr<nsIHttpChannel> httpChannel;
   nsresult rv = nsContentSecurityUtils::GetHttpChannelFromPotentialMultiPart(
       aChannel, getter_AddRefs(httpChannel));
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return true;
   }
+
+  // xfo can only hang off an httpchannel, if this is not an httpChannel
+  // then there is nothing to do here.
   if (!httpChannel) {
     return true;
   }
@@ -124,100 +182,50 @@ bool FramingChecker::CheckFrameOptions(nsIChannel* aChannel,
   // ignore XFO checks on channels that will be redirected
   uint32_t responseStatus;
   rv = httpChannel->GetResponseStatus(&responseStatus);
+  // GetResponseStatus returning failure is expected in several situations, so
+  // do not warn if it fails.
   if (NS_FAILED(rv)) {
-    // GetResponseStatus returning failure is expected in several situations, so
-    // do not warn if it fails.
     return true;
   }
   if (mozilla::net::nsHttpChannel::IsRedirectStatus(responseStatus)) {
     return true;
   }
 
-  nsAutoCString xfoHeaderValue;
+  nsAutoCString xfoHeaderCValue;
   Unused << httpChannel->GetResponseHeader("X-Frame-Options"_ns,
-                                           xfoHeaderValue);
+                                           xfoHeaderCValue);
+  NS_ConvertUTF8toUTF16 xfoHeaderValue(xfoHeaderCValue);
 
-  // Step 10. (paritally) if the only header we received was empty, then we
+  // if no header value, there's nothing to do.
-  // process it as if it wasn't sent at all.
   if (xfoHeaderValue.IsEmpty()) {
     return true;
   }
 
-  // Step 2. xfo checks are ignored in the case where CSP frame-ancestors is
+  // xfo checks are ignored in case CSP frame-ancestors is present,
-  // present, if so, there is nothing to do here.
+  // if so, there is nothing to do here.
   if (ShouldIgnoreFrameOptions(aChannel, aCsp)) {
     outIsFrameCheckingSkipped = true;
     return true;
   }
 
-  // Step 3-4. reduce the header options to a unique set and count how many
+  // iterate through all the header values (usually there's only one, but can
-  // unique values (that we track) are encountered. this avoids using a set to
+  // be many.  If any want to deny the load, deny the load.
-  // stop attackers from inheriting arbitrary values in memory and reduce the
+  nsCharSeparatedTokenizer tokenizer(xfoHeaderValue, ',');
-  // complexity of the code.
+  while (tokenizer.hasMoreTokens()) {
-  XFOHeader xfoOptions;
+    const nsAString& tok = tokenizer.nextToken();
-  for (const nsACString& next : xfoHeaderValue.Split(',')) {
+    if (!CheckOneFrameOptionsPolicy(httpChannel, tok)) {
-    nsAutoCString option(next);
+      // if xfo blocks the load we are notifying observers for
-    option.StripWhitespace();
+      // testing purposes because there is no event to gather
-
+      // what an iframe load was blocked or not.
-    if (option.LowerCaseEqualsLiteral("allowall")) {
+      nsCOMPtr<nsIURI> uri;
-      xfoOptions.ALLOWALL = true;
+      httpChannel->GetURI(getter_AddRefs(uri));
-    } else if (option.LowerCaseEqualsLiteral("sameorigin")) {
+      nsCOMPtr<nsIObserverService> observerService =
-      xfoOptions.SAMEORIGIN = true;
+          mozilla::services::GetObserverService();
-    } else if (option.LowerCaseEqualsLiteral("deny")) {
+      nsAutoString policy(tok);
-      xfoOptions.DENY = true;
+      observerService->NotifyObservers(uri, "xfo-on-violate-policy",
-    } else {
+                                       policy.get());
-      xfoOptions.INVALID = true;
-    }
-  }
-
-  nsCOMPtr<nsIURI> uri;
-  httpChannel->GetURI(getter_AddRefs(uri));
-
-  // Step 6. if header has multiple contradicting directives return early and
-  // prohibit the load. ALLOWALL is considered here for legacy reasons.
-  uint32_t xfoUniqueOptions = xfoOptions.DENY + xfoOptions.ALLOWALL +
-                              xfoOptions.SAMEORIGIN + xfoOptions.INVALID;
-  if (xfoUniqueOptions > 1 &&
-      (xfoOptions.DENY || xfoOptions.ALLOWALL || xfoOptions.SAMEORIGIN)) {
-    ReportError("XFrameOptionsInvalid", httpChannel, uri, u"invalid"_ns);
-    return false;
-  }
-
-  // Step 7 (multiple INVALID values) and partially Step 10 (single INVALID
-  // value). if header has any invalid options, but no valid directives (DENY,
-  // ALLOWALL, SAMEORIGIN) then allow the load.
-  if (xfoOptions.INVALID) {
-    ReportError("XFrameOptionsInvalid", httpChannel, uri, u"invalid"_ns);
-    return true;
-  }
-
-  // Step 8. if the value of the header is DENY prohibit the load.
-  if (xfoOptions.DENY) {
-    ReportError("XFrameOptionsDeny", httpChannel, uri, u"deny"_ns);
-    return false;
-  }
-
-  // Step 9. If the X-Frame-Options value is SAMEORIGIN, then the top frame in
-  // the parent chain must be from the same origin as this document.
-  RefPtr<mozilla::dom::BrowsingContext> ctx;
-  loadInfo->GetBrowsingContext(getter_AddRefs(ctx));
-
-  while (ctx && xfoOptions.SAMEORIGIN) {
-    nsCOMPtr<nsIPrincipal> principal =
-        ctx->Canonical()->GetCurrentWindowGlobal()->DocumentPrincipal();
-
-    if (principal && principal->IsSystemPrincipal()) {
-      return true;
-    }
-
-    // one of the ancestors is not same origin as this document
-    if (!principal || !principal->IsSameOrigin(uri)) {
-      ReportError("XFrameOptionsDeny", httpChannel, uri, u"sameorigin"_ns);
       return false;
     }
-    ctx = ctx->GetParent();
   }
-
-  // Step 10.
   return true;
 }

dom/security/FramingChecker.h

@@ -29,12 +29,7 @@ class FramingChecker {
                                 bool& outIsFrameCheckingSkipped);
 
  protected:
-  struct XFOHeader {
+  enum XFOHeader { eDENY, eSAMEORIGIN };
-    bool ALLOWALL = false;
-    bool SAMEORIGIN = false;
-    bool DENY = false;
-    bool INVALID = false;
-  };
 
   /**
    * Logs to the window about a X-Frame-Options error.
@@ -46,6 +41,9 @@ class FramingChecker {
    */
   static void ReportError(const char* aMessageTag, nsIHttpChannel* aChannel,
                           nsIURI* aURI, const nsAString& aPolicy);
+
+  static bool CheckOneFrameOptionsPolicy(nsIHttpChannel* aHttpChannel,
+                                         const nsAString& aPolicy);
 };
 
 #endif /* mozilla_dom_FramingChecker_h */

netwerk/protocol/http/nsHttpAtomList.h

@@ -103,6 +103,5 @@ HTTP_ATOM(X_Firefox_Spdy, "X-Firefox-Spdy")
 HTTP_ATOM(X_Firefox_Spdy_Proxy, "X-Firefox-Spdy-Proxy")
 HTTP_ATOM(X_Firefox_Early_Data, "X-Firefox-Early-Data")
 HTTP_ATOM(X_Firefox_Http3, "X-Firefox-Http3")
-HTTP_ATOM(X_Frame_Options, "X-Frame-Options")
 
 // methods are case sensitive and do not use atom table

netwerk/protocol/http/nsHttpHeaderArray.cpp

@@ -46,10 +46,13 @@ nsresult nsHttpHeaderArray::SetHeader(
       "Net original headers can only be set using SetHeader_internal().");
 
   nsEntry* entry = nullptr;
-  int32_t index = LookupEntry(header, &entry);
+  int32_t index;
 
-  // If an empty value is received and we aren't merging headers discard it
+  index = LookupEntry(header, &entry);
-  if (value.IsEmpty() && header != nsHttp::X_Frame_Options) {
+
+  // If an empty value is passed in, then delete the header entry...
+  // unless we are merging, in which case this function becomes a NOP.
+  if (value.IsEmpty()) {
     if (!merge && entry) {
       if (entry->variety == eVarietyResponseNetOriginalAndResponse) {
         MOZ_ASSERT(variety == eVarietyResponse);

netwerk/protocol/http/nsHttpHeaderArray.h

@@ -246,15 +246,10 @@ inline bool nsHttpHeaderArray::IsIgnoreMultipleHeader(
 [[nodiscard]] inline nsresult nsHttpHeaderArray::MergeHeader(
     const nsHttpAtom& header, nsEntry* entry, const nsACString& value,
     nsHttpHeaderArray::HeaderVariety variety) {
-  // merge of empty header = no-op
+  if (value.IsEmpty()) return NS_OK;  // merge of empty header = no-op
-  if (value.IsEmpty() && header != nsHttp::X_Frame_Options) {
-    return NS_OK;
-  }
 
-  // x-frame-options having an empty header value still has an effect so we make
-  // sure that we retain encountering it
   nsCString newValue = entry->value;
-  if (!newValue.IsEmpty() || header == nsHttp::X_Frame_Options) {
+  if (!newValue.IsEmpty()) {
     // Append the new value to the existing value
     if (header == nsHttp::Set_Cookie || header == nsHttp::WWW_Authenticate ||
         header == nsHttp::Proxy_Authenticate) {

testing/web-platform/meta/x-frame-options/multiple.html.ini

@@ -5,6 +5,90 @@
     if debug and (os == "linux") and fission and swgl: [OK, TIMEOUT]
     if debug and (os == "linux") and not fission: [OK, TIMEOUT]
     if debug and (os == "android") and fission: [OK, TIMEOUT]
+  [`ALLOWALL;INVALID` blocks same-origin framing]
+    expected: FAIL
+
+  [`(the empty string),SAMEORIGIN` blocks same-origin framing]
+    expected: FAIL
+
+  [`SAMEORIGIN,INVALID` blocks same-origin framing]
+    expected: FAIL
+
+  [`INVALID;allowAll` blocks same-origin framing]
+    expected: FAIL
+
+  [`INVALID,SAMEORIGIN` blocks same-origin framing]
+    expected: FAIL
+
+  [`ALLOWALL,INVALID` blocks same-origin framing]
+    expected: FAIL
+
+  [`INVALID,allowAll` blocks same-origin framing]
+    expected: FAIL
+
+  [`SAMEORIGIN;` blocks same-origin framing]
+    expected: FAIL
+
+  [`ALLOWALL,(the empty string)` blocks same-origin framing]
+    expected: FAIL
+
+  [`INVALID,ALLOWALL` blocks same-origin framing]
+    expected: FAIL
+
+  [`INVALID;SAMEORIGIN` blocks same-origin framing]
+    expected: FAIL
+
+  [`(the empty string),ALLOWALL` blocks same-origin framing]
+    expected: FAIL
+
+  [`ALLOWALL,SAMEORIGIN` blocks same-origin framing]
+    expected: FAIL
+
+  [`ALLOWALL;` blocks same-origin framing]
+    expected: FAIL
+
+  [`(the empty string);SAMEORIGIN` blocks same-origin framing]
+    expected: FAIL
+
+  [`allowAll,INVALID` blocks same-origin framing]
+    expected: FAIL
+
+  [`SAMEORIGIN,ALLOWALL` blocks same-origin framing]
+    expected: FAIL
+
+  [`ALLOWALL;SAMEORIGIN` blocks same-origin framing]
+    expected: FAIL
+
+  [`"DENY",SAMEORIGIN` blocks same-origin framing]
+    expected: FAIL
+
+  [`"DENY";SAMEORIGIN` blocks same-origin framing]
+    expected: FAIL
+
+  [`SAMEORIGIN,(the empty string)` blocks same-origin framing]
+    expected: FAIL
+
+  [`SAMEORIGIN;INVALID` blocks same-origin framing]
+    expected: FAIL
+
+  [`(the empty string);ALLOWALL` blocks same-origin framing]
+    expected: FAIL
+
+  [`SAMEORIGIN;ALLOWALL` blocks same-origin framing]
+    expected: FAIL
+
+  [`INVALID;ALLOWALL` blocks same-origin framing]
+    expected: FAIL
+
+  [`SAMEORIGIN;"DENY"` blocks same-origin framing]
+    expected: FAIL
+
+  [`allowAll;INVALID` blocks same-origin framing]
+    expected: FAIL
+
+  [`SAMEORIGIN,"DENY"` blocks same-origin framing]
+    expected: FAIL
+
   [`INVALID;` allows same-origin framing]
     expected:
       if (os == "linux") and debug and fission and swgl: [PASS, TIMEOUT]