Bug 1332422 - CSP should not use 'aExtra' to indicate redirects within ContentPolicy, r=ckerschb
authorAndrea Marchesini <amarchesini@mozilla.com>
Thu, 19 Jul 2018 13:25:50 +0200
changeset 427270 8a66951dd403316aba45f36c3502df83cc5ed5f1
parent 427269 694089774911b78e06ab2f784278400a76155582
child 427271 68da215bdf87b5486a4f8c0a600ef77112f21ac4
push id105430
push useramarchesini@mozilla.com
push dateThu, 19 Jul 2018 11:26:13 +0000
treeherdermozilla-inbound@8a66951dd403 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckerschb
bugs1332422
milestone63.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 1332422 - CSP should not use 'aExtra' to indicate redirects within ContentPolicy, r=ckerschb Instead, let's pass a nsIURI object to indicate when we have to check a redirect CSP loading.
dom/interfaces/security/nsIContentSecurityPolicy.idl
dom/security/nsCSPContext.cpp
dom/security/nsCSPContext.h
dom/security/nsCSPService.cpp
dom/security/nsMixedContentBlocker.cpp
dom/security/nsMixedContentBlocker.h
image/imgLoader.cpp
--- a/dom/interfaces/security/nsIContentSecurityPolicy.idl
+++ b/dom/interfaces/security/nsIContentSecurityPolicy.idl
@@ -289,23 +289,27 @@ interface nsIContentSecurityPolicy : nsI
 
   /**
    * Delegate method called by the service when sub-elements of the protected
    * document are being loaded.  Given a bit of information about the request,
    * decides whether or not the policy is satisfied.
    *
    * Calls to this may trigger violation reports when queried, so
    * this value should not be cached.
+   *
+   * aOriginalURIIfRedirect must be passed only if this loading is the result
+   * of a redirect. In this case, aOriginalURIIfRedirect must be the original
+   * URL.
    */
   short shouldLoad(in nsContentPolicyType aContentType,
                    in nsIURI          aContentLocation,
                    in nsIURI          aRequestOrigin,
                    in nsISupports     aContext,
                    in ACString        aMimeTypeGuess,
-                   in nsISupports     aExtra);
+                   in nsIURI          aOriginalURIIfRedirect);
 
 %{ C++
 // nsIObserver topic to fire when the policy encounters a violation.
 #define CSP_VIOLATION_TOPIC "csp-on-violate-policy"
 %}
 
   /**
    * Returns the CSP in JSON notation.
--- a/dom/security/nsCSPContext.cpp
+++ b/dom/security/nsCSPContext.cpp
@@ -156,17 +156,17 @@ CreateCacheKey_Internal(nsIURI* aContent
 /* =====  nsIContentSecurityPolicy impl ====== */
 
 NS_IMETHODIMP
 nsCSPContext::ShouldLoad(nsContentPolicyType aContentType,
                          nsIURI*             aContentLocation,
                          nsIURI*             aRequestOrigin,
                          nsISupports*        aRequestContext,
                          const nsACString&   aMimeTypeGuess,
-                         nsISupports*        aExtra,
+                         nsIURI*             aOriginalURIIfRedirect,
                          int16_t*            outDecision)
 {
   if (CSPCONTEXTLOGENABLED()) {
     CSPCONTEXTLOG(("nsCSPContext::ShouldLoad, aContentLocation: %s",
                    aContentLocation->GetSpecOrDefault().get()));
     CSPCONTEXTLOG((">>>>                      aContentType: %d", aContentType));
   }
 
@@ -221,27 +221,21 @@ nsCSPContext::ShouldLoad(nsContentPolicy
     }
 
     nsCOMPtr<nsIScriptElement> script = do_QueryInterface(aRequestContext);
     if (script && script->GetParserCreated() != mozilla::dom::NOT_FROM_PARSER) {
       parserCreated = true;
     }
   }
 
-  // aExtra holds the original URI of the channel if the
-  // channel got redirected (until we fix Bug 1332422).
-  nsCOMPtr<nsIURI> originalURI = do_QueryInterface(aExtra);
-  bool wasRedirected = originalURI;
-
   bool permitted = permitsInternal(dir,
                                    nullptr, // aTriggeringElement
                                    aContentLocation,
-                                   originalURI,
+                                   aOriginalURIIfRedirect,
                                    nonce,
-                                   wasRedirected,
                                    isPreload,
                                    false,     // allow fallback to default-src
                                    true,      // send violation reports
                                    true,     // send blocked URI in violation reports
                                    parserCreated);
 
   *outDecision = permitted ? nsIContentPolicy::ACCEPT
                            : nsIContentPolicy::REJECT_SERVER;
@@ -259,33 +253,32 @@ nsCSPContext::ShouldLoad(nsContentPolicy
   }
   return NS_OK;
 }
 
 bool
 nsCSPContext::permitsInternal(CSPDirective aDir,
                               Element* aTriggeringElement,
                               nsIURI* aContentLocation,
-                              nsIURI* aOriginalURI,
+                              nsIURI* aOriginalURIIfRedirect,
                               const nsAString& aNonce,
-                              bool aWasRedirected,
                               bool aIsPreload,
                               bool aSpecific,
                               bool aSendViolationReports,
                               bool aSendContentLocationInViolationReports,
                               bool aParserCreated)
 {
   bool permits = true;
 
   nsAutoString violatedDirective;
   for (uint32_t p = 0; p < mPolicies.Length(); p++) {
     if (!mPolicies[p]->permits(aDir,
                                aContentLocation,
                                aNonce,
-                               aWasRedirected,
+                               !!aOriginalURIIfRedirect,
                                aSpecific,
                                aParserCreated,
                                violatedDirective)) {
       // If the policy is violated and not report-only, reject the load and
       // report to the console
       if (!mPolicies[p]->getReportOnlyFlag()) {
         CSPCONTEXTLOG(("nsCSPContext::permitsInternal, false"));
         permits = false;
@@ -294,17 +287,17 @@ nsCSPContext::permitsInternal(CSPDirecti
       // Do not send a report or notify observers if this is a preload - the
       // decision may be wrong due to the inability to get the nonce, and will
       // incorrectly fail the unit tests.
       if (!aIsPreload && aSendViolationReports) {
         AsyncReportViolation(aTriggeringElement,
                              (aSendContentLocationInViolationReports ?
                               aContentLocation : nullptr),
                              BlockedContentSource::eUnknown, /* a BlockedContentSource */
-                             aOriginalURI,  /* in case of redirect originalURI is not null */
+                             aOriginalURIIfRedirect,  /* in case of redirect originalURI is not null */
                              violatedDirective,
                              p,             /* policy index        */
                              EmptyString(), /* no observer subject */
                              EmptyString(), /* no source file      */
                              EmptyString(), /* no script sample    */
                              0,             /* no line number      */
                              0);            /* no column number    */
       }
@@ -1563,17 +1556,16 @@ nsCSPContext::PermitsAncestry(nsIDocShel
     bool okToSendAncestor = NS_SecurityCompareURIs(ancestorsArray[a], mSelfURI, true);
 
 
     bool permits = permitsInternal(nsIContentSecurityPolicy::FRAME_ANCESTORS_DIRECTIVE,
                                    nullptr,
                                    ancestorsArray[a],
                                    nullptr, // no redirect here.
                                    EmptyString(), // no nonce
-                                   false,   // no redirect here.
                                    false,   // not a preload.
                                    true,    // specific, do not use default-src
                                    true,    // send violation reports
                                    okToSendAncestor,
                                    false);  // not parser created
     if (!permits) {
       *outPermitsAncestry = false;
     }
@@ -1593,17 +1585,16 @@ nsCSPContext::Permits(Element* aTriggeri
     return NS_ERROR_FAILURE;
   }
 
   *outPermits = permitsInternal(aDir,
                                 aTriggeringElement,
                                 aURI,
                                 nullptr,  // no original (pre-redirect) URI
                                 EmptyString(),  // no nonce
-                                false,    // not redirected.
                                 false,    // not a preload.
                                 aSpecific,
                                 true,     // send violation reports
                                 true,     // send blocked URI in violation reports
                                 false);   // not parser created
 
   if (CSPCONTEXTLOGENABLED()) {
       CSPCONTEXTLOG(("nsCSPContext::Permits, aUri: %s, aDir: %d, isAllowed: %s",
--- a/dom/security/nsCSPContext.h
+++ b/dom/security/nsCSPContext.h
@@ -142,19 +142,18 @@ class nsCSPContext : public nsIContentSe
     {
       return std::max(sScriptSampleMaxLength, 0);
     }
 
   private:
     bool permitsInternal(CSPDirective aDir,
                          mozilla::dom::Element* aTriggeringElement,
                          nsIURI* aContentLocation,
-                         nsIURI* aOriginalURI,
+                         nsIURI* aOriginalURIIfRedirect,
                          const nsAString& aNonce,
-                         bool aWasRedirected,
                          bool aIsPreload,
                          bool aSpecific,
                          bool aSendViolationReports,
                          bool aSendContentLocationInViolationReports,
                          bool aParserCreated);
 
     // helper to report inline script/style violations
     void reportInlineViolation(nsContentPolicyType aContentType,
--- a/dom/security/nsCSPService.cpp
+++ b/dom/security/nsCSPService.cpp
@@ -179,23 +179,22 @@ CSPService::ShouldLoad(nsIURI *aContentL
 
   if (isPreload) {
     nsCOMPtr<nsIContentSecurityPolicy> preloadCsp;
     rv = principal->GetPreloadCsp(getter_AddRefs(preloadCsp));
     NS_ENSURE_SUCCESS(rv, rv);
 
     if (preloadCsp) {
       // obtain the enforcement decision
-      // (don't pass aExtra, we use that slot for redirects)
       rv = preloadCsp->ShouldLoad(contentType,
                                   aContentLocation,
                                   requestOrigin,
                                   requestContext,
                                   aMimeTypeGuess,
-                                  nullptr, // aExtra
+                                  nullptr, // no redirect, aOriginal URL is null.
                                   aDecision);
       NS_ENSURE_SUCCESS(rv, rv);
 
       // if the preload policy already denied the load, then there
       // is no point in checking the real policy
       if (NS_CP_REJECTED(*aDecision)) {
         return NS_OK;
       }
@@ -204,23 +203,22 @@ CSPService::ShouldLoad(nsIURI *aContentL
 
   // 2) Apply actual CSP to all loads
   nsCOMPtr<nsIContentSecurityPolicy> csp;
   rv = principal->GetCsp(getter_AddRefs(csp));
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (csp) {
     // obtain the enforcement decision
-    // (don't pass aExtra, we use that slot for redirects)
     rv = csp->ShouldLoad(contentType,
                          aContentLocation,
                          requestOrigin,
                          requestContext,
                          aMimeTypeGuess,
-                         nullptr,
+                         nullptr, // no redirect, aOriginal URL is null.
                          aDecision);
     NS_ENSURE_SUCCESS(rv, rv);
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 CSPService::ShouldProcess(nsIURI           *aContentLocation,
@@ -315,23 +313,23 @@ CSPService::AsyncOnChannelRedirect(nsICh
   int16_t aDecision = nsIContentPolicy::ACCEPT;
   nsCOMPtr<nsISupports> requestContext = loadInfo->GetLoadingContext();
   // 1) Apply speculative CSP for preloads
   if (isPreload) {
     nsCOMPtr<nsIContentSecurityPolicy> preloadCsp;
     loadInfo->LoadingPrincipal()->GetPreloadCsp(getter_AddRefs(preloadCsp));
 
     if (preloadCsp) {
-      // Pass  originalURI as aExtra to indicate the redirect
+      // Pass  originalURI to indicate the redirect
       preloadCsp->ShouldLoad(policyType,     // load type per nsIContentPolicy (uint32_t)
                              newUri,         // nsIURI
                              nullptr,        // nsIURI
                              requestContext, // nsISupports
                              EmptyCString(), // ACString - MIME guess
-                             originalUri,    // aExtra
+                             originalUri,    // Original nsIURI
                              &aDecision);
 
       // if the preload policy already denied the load, then there
       // is no point in checking the real policy
       if (NS_CP_REJECTED(aDecision)) {
         autoCallback.DontCallback();
         oldChannel->Cancel(NS_ERROR_DOM_BAD_URI);
         return NS_BINDING_FAILED;
@@ -339,23 +337,23 @@ CSPService::AsyncOnChannelRedirect(nsICh
     }
   }
 
   // 2) Apply actual CSP to all loads
   nsCOMPtr<nsIContentSecurityPolicy> csp;
   loadInfo->LoadingPrincipal()->GetCsp(getter_AddRefs(csp));
 
   if (csp) {
-    // Pass  originalURI as aExtra to indicate the redirect
+    // Pass  originalURI to indicate the redirect
     csp->ShouldLoad(policyType,     // load type per nsIContentPolicy (uint32_t)
                     newUri,         // nsIURI
                     nullptr,        // nsIURI
                     requestContext, // nsISupports
                     EmptyCString(), // ACString - MIME guess
-                    originalUri,    // aExtra
+                    originalUri,    // Original nsIURI
                     &aDecision);
   }
 
   // if ShouldLoad doesn't accept the load, cancel the request
   if (!NS_CP_ACCEPTED(aDecision)) {
     autoCallback.DontCallback();
     oldChannel->Cancel(NS_ERROR_DOM_BAD_URI);
     return NS_BINDING_FAILED;
--- a/dom/security/nsMixedContentBlocker.cpp
+++ b/dom/security/nsMixedContentBlocker.cpp
@@ -373,17 +373,16 @@ nsMixedContentBlocker::ShouldLoad(nsIURI
   // image redirects.  This is handled by direct callers of the static
   // ShouldLoad.
   nsresult rv = ShouldLoad(false,   // aHadInsecureImageRedirect
                            contentType,
                            aContentLocation,
                            requestingLocation,
                            requestingContext,
                            aMimeGuess,
-                           nullptr, // aExtra,
                            requestPrincipal,
                            aDecision);
   return rv;
 }
 
 bool
 nsMixedContentBlocker::IsPotentiallyTrustworthyLoopbackURL(nsIURI* aURL) {
   nsAutoCString host;
@@ -423,17 +422,16 @@ nsMixedContentBlocker::IsPotentiallyTrus
  */
 nsresult
 nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
                                   uint32_t aContentType,
                                   nsIURI* aContentLocation,
                                   nsIURI* aRequestingLocation,
                                   nsISupports* aRequestingContext,
                                   const nsACString& aMimeGuess,
-                                  nsISupports* aExtra,
                                   nsIPrincipal* aRequestPrincipal,
                                   int16_t* aDecision)
 {
   // Asserting that we are on the main thread here and hence do not have to lock
   // and unlock sBlockMixedScript and sBlockMixedDisplay before reading/writing
   // to them.
   MOZ_ASSERT(NS_IsMainThread());
 
--- a/dom/security/nsMixedContentBlocker.h
+++ b/dom/security/nsMixedContentBlocker.h
@@ -60,17 +60,16 @@ public:
    * Remaining parameters are from nsIContentPolicy::ShouldLoad().
    */
   static nsresult ShouldLoad(bool aHadInsecureImageRedirect,
                              uint32_t aContentType,
                              nsIURI* aContentLocation,
                              nsIURI* aRequestingLocation,
                              nsISupports* aRequestingContext,
                              const nsACString& aMimeGuess,
-                             nsISupports* aExtra,
                              nsIPrincipal* aRequestPrincipal,
                              int16_t* aDecision);
   static void AccumulateMixedContentHSTS(nsIURI* aURI,
                                          bool aActive,
                                          const OriginAttributes& aOriginAttributes);
 
   static bool ShouldUpgradeMixedDisplayContent();
 
--- a/image/imgLoader.cpp
+++ b/image/imgLoader.cpp
@@ -687,17 +687,16 @@ ShouldLoadCachedImage(imgRequest* aImgRe
       // reset the decision for mixed content blocker check
       decision = nsIContentPolicy::REJECT_REQUEST;
       rv = nsMixedContentBlocker::ShouldLoad(insecureRedirect,
                                              aPolicyType,
                                              contentLocation,
                                              requestingLocation,
                                              aLoadingContext,
                                              EmptyCString(), //mime guess
-                                             nullptr,
                                              aTriggeringPrincipal,
                                              &decision);
       if (NS_FAILED(rv) || !NS_CP_ACCEPTED(decision)) {
         return false;
       }
     }
   }