Bug 1629307 - prevent auth prompts if XFO checks fails. r=necko-reviewers,valentin,ckerschb

Differential Revision: https://phabricator.services.mozilla.com/D156292

dom/security/FramingChecker.cpp

@@ -141,21 +141,6 @@ static bool ShouldIgnoreFrameOptions(nsIChannel* aChannel,
     return false;
   }
 
-  // log warning to console that xfo is ignored because of CSP
-  nsCOMPtr<nsILoadInfo> loadInfo = aChannel->LoadInfo();
-  uint64_t innerWindowID = loadInfo->GetInnerWindowID();
-  bool privateWindow = !!loadInfo->GetOriginAttributes().mPrivateBrowsingId;
-  AutoTArray<nsString, 2> params = {u"x-frame-options"_ns,
-                                    u"frame-ancestors"_ns};
-  CSP_LogLocalizedStr("IgnoringSrcBecauseOfDirective", params,
-                      u""_ns,  // no sourcefile
-                      u""_ns,  // no scriptsample
-                      0,       // no linenumber
-                      0,       // no columnnumber
-                      nsIScriptError::warningFlag,
-                      "IgnoringSrcBecauseOfDirective"_ns, innerWindowID,
-                      privateWindow);
-
   return true;
 }
 
@@ -165,7 +150,8 @@ static bool ShouldIgnoreFrameOptions(nsIChannel* aChannel,
 // multiple headers, etc).
 /* static */
 bool FramingChecker::CheckFrameOptions(nsIChannel* aChannel,
-                                       nsIContentSecurityPolicy* aCsp) {
+                                       nsIContentSecurityPolicy* aCsp,
+                                       bool& outIsFrameCheckingSkipped) {
   if (!aChannel) {
     return true;
   }
@@ -218,6 +204,7 @@ bool FramingChecker::CheckFrameOptions(nsIChannel* aChannel,
   // xfo checks are ignored in case CSP frame-ancestors is present,
   // if so, there is nothing to do here.
   if (ShouldIgnoreFrameOptions(aChannel, aCsp)) {
+    outIsFrameCheckingSkipped = true;
     return true;
   }
 

dom/security/FramingChecker.h

@@ -25,7 +25,8 @@ class FramingChecker {
   // Determine if X-Frame-Options allows content to be framed
   // as a subdocument
   static bool CheckFrameOptions(nsIChannel* aChannel,
-                                nsIContentSecurityPolicy* aCSP);
+                                nsIContentSecurityPolicy* aCSP,
+                                bool& outIsFrameCheckingSkipped);
 
  protected:
   enum XFOHeader { eDENY, eSAMEORIGIN };

dom/security/nsContentSecurityUtils.cpp

@@ -952,8 +952,8 @@ nsresult nsContentSecurityUtils::GetHttpChannelFromPotentialMultiPart(
   return NS_OK;
 }
 
-nsresult ParseCSPAndEnforceFrameAncestorCheck(
-    nsIChannel* aChannel, nsIContentSecurityPolicy** aOutCSP) {
+nsresult CheckCSPFrameAncestorPolicy(nsIChannel* aChannel,
+                                     nsIContentSecurityPolicy** aOutCSP) {
   MOZ_ASSERT(aChannel);
 
   nsCOMPtr<nsILoadInfo> loadInfo = aChannel->LoadInfo();
@@ -1054,7 +1054,6 @@ nsresult ParseCSPAndEnforceFrameAncestorCheck(
 
   if (NS_FAILED(rv) || !safeAncestry) {
     // stop!  ERROR page!
-    aChannel->Cancel(NS_ERROR_CSP_FRAME_ANCESTOR_VIOLATION);
     return NS_ERROR_CSP_FRAME_ANCESTOR_VIOLATION;
   }
 
@@ -1064,22 +1063,51 @@ nsresult ParseCSPAndEnforceFrameAncestorCheck(
   return NS_OK;
 }
 
+void EnforceCSPFrameAncestorPolicy(nsIChannel* aChannel,
+                                   const nsresult& aError) {
+  if (aError == NS_ERROR_CSP_FRAME_ANCESTOR_VIOLATION) {
+    aChannel->Cancel(NS_ERROR_CSP_FRAME_ANCESTOR_VIOLATION);
+  }
+}
+
 void EnforceXFrameOptionsCheck(nsIChannel* aChannel,
                                nsIContentSecurityPolicy* aCsp) {
   MOZ_ASSERT(aChannel);
-  if (!FramingChecker::CheckFrameOptions(aChannel, aCsp)) {
+  bool isFrameOptionsIgnored = false;
+  // check for XFO options
+  // XFO checks can be skipped if there are frame ancestors
+  if (!FramingChecker::CheckFrameOptions(aChannel, aCsp,
+                                         isFrameOptionsIgnored)) {
     // stop!  ERROR page!
     aChannel->Cancel(NS_ERROR_XFO_VIOLATION);
   }
+
+  if (isFrameOptionsIgnored) {
+    // log warning to console that xfo is ignored because of CSP
+    nsCOMPtr<nsILoadInfo> loadInfo = aChannel->LoadInfo();
+    uint64_t innerWindowID = loadInfo->GetInnerWindowID();
+    bool privateWindow = !!loadInfo->GetOriginAttributes().mPrivateBrowsingId;
+    AutoTArray<nsString, 2> params = {u"x-frame-options"_ns,
+                                      u"frame-ancestors"_ns};
+    CSP_LogLocalizedStr("IgnoringSrcBecauseOfDirective", params,
+                        u""_ns,  // no sourcefile
+                        u""_ns,  // no scriptsample
+                        0,       // no linenumber
+                        0,       // no columnnumber
+                        nsIScriptError::warningFlag,
+                        "IgnoringSrcBecauseOfDirective"_ns, innerWindowID,
+                        privateWindow);
+  }
 }
 
 /* static */
 void nsContentSecurityUtils::PerformCSPFrameAncestorAndXFOCheck(
     nsIChannel* aChannel) {
   nsCOMPtr<nsIContentSecurityPolicy> csp;
-  nsresult rv =
-      ParseCSPAndEnforceFrameAncestorCheck(aChannel, getter_AddRefs(csp));
+  nsresult rv = CheckCSPFrameAncestorPolicy(aChannel, getter_AddRefs(csp));
+
   if (NS_FAILED(rv)) {
+    EnforceCSPFrameAncestorPolicy(aChannel, rv);
     return;
   }
 
@@ -1088,6 +1116,21 @@ void nsContentSecurityUtils::PerformCSPFrameAncestorAndXFOCheck(
   // will be discarded
   EnforceXFrameOptionsCheck(aChannel, csp);
 }
+/* static */
+bool nsContentSecurityUtils::CheckCSPFrameAncestorAndXFO(nsIChannel* aChannel) {
+  nsCOMPtr<nsIContentSecurityPolicy> csp;
+  nsresult rv = CheckCSPFrameAncestorPolicy(aChannel, getter_AddRefs(csp));
+
+  if (NS_FAILED(rv)) {
+    EnforceCSPFrameAncestorPolicy(aChannel, rv);
+    return false;
+  }
+
+  bool isFrameOptionsIgnored = false;
+
+  return FramingChecker::CheckFrameOptions(aChannel, csp,
+                                           isFrameOptionsIgnored);
+}
 
 #if defined(DEBUG)
 /* static */

dom/security/nsContentSecurityUtils.h

@@ -61,6 +61,11 @@ class nsContentSecurityUtils {
   // If any of the two disallows framing, the channel will be cancelled.
   static void PerformCSPFrameAncestorAndXFOCheck(nsIChannel* aChannel);
 
+  // Helper function which just checks if the channel violates any:
+  // 1. CSP frame-ancestors properties
+  // 2. x-frame-options
+  static bool CheckCSPFrameAncestorAndXFO(nsIChannel* aChannel);
+
   // Helper function to Check if a Download is allowed;
   static long ClassifyDownload(nsIChannel* aChannel,
                                const nsAutoCString& aMimeTypeGuess);

netwerk/protocol/http/nsHttpChannel.cpp

@@ -17,6 +17,7 @@
 #include "mozilla/dom/nsCSPService.h"
 #include "mozilla/StoragePrincipalHelper.h"
 
+#include "nsContentSecurityUtils.h"
 #include "nsHttp.h"
 #include "nsHttpChannel.h"
 #include "nsHttpChannelAuthProvider.h"
@@ -2359,6 +2360,9 @@ nsresult nsHttpChannel::ContinueProcessResponse3(nsresult rv) {
         // It's up to the consumer to re-try w/o setting a custom
         // auth header if cached credentials should be attempted.
         rv = NS_ERROR_FAILURE;
+      } else if (!nsContentSecurityUtils::CheckCSPFrameAncestorAndXFO(this)) {
+        // CSP Frame Ancestor and X-Frame-Options check has failed
+        rv = NS_ERROR_FAILURE;
       } else {
         rv = mAuthProvider->ProcessAuthentication(
             httpStatus, mConnectionInfo->EndToEndSSL() && mTransaction &&

netwerk/test/browser/browser.ini

@@ -62,6 +62,9 @@ support-files =
   cookie_filtering_secure_resource_org.html^headers^
   cookie_filtering_square.png
   cookie_filtering_square.png^headers^
+  x_frame_options.html
+  x_frame_options.html^headers^
+  test_1629307.html
 
 [browser_about_cache.js]
 [browser_bug1535877.js]
@@ -139,3 +142,4 @@ support-files =
   early_hint_preload_test_helper.jsm
 skip-if =
   os == 'linux' && bits == 64 && !debug # Bug 1744028 and Bug 1746324
+[browser_bug1629307.js]

netwerk/test/browser/browser_bug1629307.js

@@ -0,0 +1,47 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+// Load a web page containing an iframe that requires authentication but includes the X-Frame-Options: SAMEORIGIN header.
+// Make sure that we don't needlessly show an authentication prompt for it.
+
+const { PromptTestUtils } = ChromeUtils.import(
+  "resource://testing-common/PromptTestUtils.jsm"
+);
+
+const { BrowserTestUtils } = ChromeUtils.import(
+  "resource://testing-common/BrowserTestUtils.jsm"
+);
+
+add_task(async function() {
+  let URL =
+    "https://example.com/browser/netwerk/test/browser/test_1629307.html";
+
+  let hasPrompt = false;
+
+  PromptTestUtils.handleNextPrompt(
+    window,
+    {
+      modalType: Services.prefs.getIntPref("prompts.modalType.httpAuth"),
+      promptType: "promptUserAndPass",
+    },
+    { buttonNumClick: 1 }
+  )
+    .then(function() {
+      hasPrompt = true;
+    })
+    .catch(function() {});
+
+  BrowserTestUtils.loadURI(gBrowser.selectedBrowser, URL);
+
+  // wait until the page and its iframe page is loaded
+  await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, true, URL);
+
+  Assert.equal(
+    hasPrompt,
+    false,
+    "no prompt when loading page via iframe with x-auth options"
+  );
+});

netwerk/test/browser/test_1629307.html

@@ -0,0 +1,9 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+</head>
+<body>
+  <iframe
+    src="https://example.org/browser/netwerk/test/browser/x_frame_options.html"></iframe>
+</body>
+</html>

netwerk/test/browser/x_frame_options.html^headers^

@@ -0,0 +1,3 @@
+HTTP 401 UNAUTHORIZED
+X-Frame-Options: SAMEORIGIN
+WWW-Authenticate: basic realm="login required"