Closed Bug 1262263 Opened 4 years ago Closed 3 years ago

Refactor functionality needed by HSTS Priming into separate methods

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1275402

People

(Reporter: kmckinley, Assigned: kmckinley)

Details

(Whiteboard: [hsts-priming])

Attachments

(1 file, 2 obsolete files)

HSTS priming should use the same logic as nsMixedContentBlocker and nsContentSecurityManager when performing the following actions:

1. determining if a request is active or passive mixed content
2. updating the ui as mixed content
3. determining the request context

Break these out into reusable methods.
Attachment #8738257 - Flags: review?(tanvi)
Attachment #8738257 - Flags: review?(mozilla)
Treeherder push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3d07704af0b
Attachment #8738257 - Attachment is obsolete: true
Attachment #8738257 - Flags: review?(tanvi)
Attachment #8738257 - Flags: review?(mozilla)
Attachment #8738707 - Flags: review?(tanvi)
Attachment #8738707 - Flags: review?(mozilla)
Comment on attachment 8738707 [details] [diff] [review]
Extract methods to be used by HSTS priming

>   int16_t shouldLoad = nsIContentPolicy::ACCEPT;
>   nsresult rv = NS_CheckContentLoadPolicy(internalContentPolicyType,
>-                                          aURI,
>-                                          aLoadInfo->LoadingPrincipal(),
>-                                          requestingContext,
>-                                          mimeTypeGuess,
>-                                          nullptr,        //extra,
>-                                          &shouldLoad,
>-                                          nsContentUtils::GetContentPolicy(),
>-                                          nsContentUtils::GetSecurityManager());
>+                                 aURI,
>+                                 aLoadInfo->LoadingPrincipal(),
>+                                 requestingContext,
>+                                 mimeTypeGuess,
>+                                 nullptr,
>+                                 &shouldLoad,
>+                                 nsContentUtils::GetContentPolicy(),
>+                                 nsContentUtils::GetSecurityManager());
>+

Indent and put back the "//extra" after nullptr.


>+nsresult
>+nsMixedContentBlocker::UpdateUIAsMixedContent(uint32_t aContentType,
>+                                              nsIURI* aContentLocation,
>+                                              nsISupports* aRequestingContext,
>+                                              int16_t* aDecision,
>+                                              bool aHadInsecureImageRedirect,
>+                                              bool hasHSTSPriming)
>+{
>+  MixedContentTypes classification = eMixedScript;
>+  ClassifyContentType(aContentType, classification);
>+
>+  if (classification == eTopLevelDocument || classification == eWebSocket) {
>+    // this should have been accepted in ShouldLoad
>+    NS_ASSERTION(false, "Mixed content can't happen with top-level documents or websockets");
>+  }
> 
Why are we doing the classification twice?  Once in ShouldLoad and again in here.


>@@ -1001,3 +1069,4 @@ nsMixedContentBlocker::AccumulateMixedContentHSTS(nsIURI* aURI, bool aActive)
>     }
>   }
> }
>+
Unnecessary new line.
(In reply to Tanvi Vyas [:tanvi] from comment #2)
> Comment on attachment 8738707 [details] [diff] [review]
> Extract methods to be used by HSTS priming
 
> 
> >+nsresult
> >+nsMixedContentBlocker::UpdateUIAsMixedContent(uint32_t aContentType,
> >+                                              nsIURI* aContentLocation,
> >+                                              nsISupports* aRequestingContext,
> >+                                              int16_t* aDecision,
> >+                                              bool aHadInsecureImageRedirect,
> >+                                              bool hasHSTSPriming)
> >+{
> >+  MixedContentTypes classification = eMixedScript;
> >+  ClassifyContentType(aContentType, classification);
> >+
> >+  if (classification == eTopLevelDocument || classification == eWebSocket) {
> >+    // this should have been accepted in ShouldLoad
> >+    NS_ASSERTION(false, "Mixed content can't happen with top-level documents or websockets");
> >+  }
> > 
> Why are we doing the classification twice?  Once in ShouldLoad and again in
> here.

When this is called from outside nsMixedContentBlocker::ShouldLoad (nsHttpClient::OnPrimingFailed), we need to determine whether it is active or passive mixed content.
Comment on attachment 8738707 [details] [diff] [review]
Extract methods to be used by HSTS priming

>diff --git a/dom/security/nsMixedContentBlocker.cpp b/dom/security/nsMixedContentBlocker.cpp
>index 475cdd7..fac79000 100644
>--- a/dom/security/nsMixedContentBlocker.cpp
>+++ b/dom/security/nsMixedContentBlocker.cpp
>@@ -763,6 +772,7 @@ nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
>     }
>   }
> 
>+  /*
>   // Get the root document from the sameTypeRoot
>   nsCOMPtr<nsIDocument> rootDoc = do_GetInterface(sameTypeRoot);
>   NS_ASSERTION(rootDoc, "No root document from document shell root tree item.");
>@@ -782,6 +792,26 @@ nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
>     return NS_OK;
>   }
>   nsresult stateRV = securityUI->GetState(&state);
>+  */
Remove commented part.

>+
>+  return UpdateUIAsMixedContent(aContentType, innerContentLocation, aRequestingContext, aDecision, aHadInsecureImageRedirect, false);
This method updates the UI and calls run.  So I'm not sure "UpdateUIAsMixedContent" quite captures this.  It might be better to keep the call to run() in ShouldLoad - depends on if you need it for HSTS priming.

>+}
>+
>+nsresult
>+nsMixedContentBlocker::UpdateUIAsMixedContent(uint32_t aContentType,
>+                                              nsIURI* aContentLocation,
>+                                              nsISupports* aRequestingContext,
>+                                              int16_t* aDecision,
>+                                              bool aHadInsecureImageRedirect,
>+                                              bool hasHSTSPriming)
>+{
>+  MixedContentTypes classification = eMixedScript;
>+  ClassifyContentType(aContentType, classification);
>+
>+  if (classification == eTopLevelDocument || classification == eWebSocket) {
>+    // this should have been accepted in ShouldLoad
>+    NS_ASSERTION(false, "Mixed content can't happen with top-level documents or websockets");
>+  }

Why not just pass in the classification and remove the duplicate code?


>   // At this point we know that the request is mixed content, and the only
>   // question is whether we block it.  Record telemetry at this point as to
>@@ -798,23 +828,61 @@ nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
>   bool active = (classification == eMixedScript);
>   if (!aHadInsecureImageRedirect) {
>     if (XRE_IsParentProcess()) {
>-      AccumulateMixedContentHSTS(innerContentLocation, active);
>+      AccumulateMixedContentHSTS(aContentLocation, active);
>     } else {
>       // Ask the parent process to do the same call
>       mozilla::dom::ContentChild* cc = mozilla::dom::ContentChild::GetSingleton();
>       if (cc) {
>         mozilla::ipc::URIParams uri;
>-        SerializeURI(innerContentLocation, uri);
>+        SerializeURI(aContentLocation, uri);
>         cc->SendAccumulateMixedContentHSTS(uri, active);
>       }
>     }
>   }
The HSTS telemetry code doesn't have to do with Updating the security UI, so you may want to put that back in ShouldLoad.
Also, the data collected might change with this patch since before we were returning early when there is !securityUI and now we are not.

> 
>+  nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(aRequestingContext);
>+  NS_ENSURE_TRUE(docShell, NS_OK);
>+

Can we avoid the next 10 lines of duplicate code?  Would it require passing in all 3 booleans?
>+  // Determine if the rootDoc is https and if the user decided to allow Mixed Content
>+  bool rootHasSecureConnection = false;
>+  bool allowMixedContent = false;
>+  bool isRootDocShell = false;
>+  nsresult rv = docShell->GetAllowMixedContentAndConnectionData(&rootHasSecureConnection,
>+                                                                &allowMixedContent,
>+                                                                &isRootDocShell);
>+  if (NS_FAILED(rv)) {
>+    *aDecision = REJECT_REQUEST;
>+    return rv;
>+  }
>+
>+  // Get the sameTypeRoot tree item from the docshell
>+  nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
>+  docShell->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
>+  NS_ASSERTION(sameTypeRoot, "No root tree item from docshell!");

Add a new line plus a comment before the next part:
// Get the current security state from the docShell
>+  nsCOMPtr<nsIDocShell> rootShell = do_GetInterface(sameTypeRoot);
>+  NS_ASSERTION(rootShell, "No root docshell from document shell root tree item.");
>+  uint32_t state = nsIWebProgressListener::STATE_IS_BROKEN;
>+  nsCOMPtr<nsISecureBrowserUI> securityUI;
>+  rootShell->GetSecurityUI(getter_AddRefs(securityUI));

This below comment no longer applies, since you have done an ensure success on the security state.  Make sure this doesn't break try.
>+  // If there is no securityUI, document doesn't have a security state.
>+  // Allow load and return early.
>+  NS_ENSURE_STATE(securityUI);
>+  nsresult stateRV = securityUI->GetState(&state);
>+  NS_ENSURE_SUCCESS(stateRV, stateRV);
>+
>+  // Get the root document from the sameTypeRoot
>+  nsCOMPtr<nsIDocument> rootDoc = do_GetInterface(sameTypeRoot);
>+  NS_ASSERTION(rootDoc, "No root document from document shell root tree item.");
>+
>   // set hasMixedContentObjectSubrequest on this object if necessary
>   if (aContentType == TYPE_OBJECT_SUBREQUEST) {
>     rootDoc->SetHasMixedContentObjectSubrequest(true);
>   }
> 
>+  // Get eventSink and the current security state from the docShell
>+  nsCOMPtr<nsISecurityEventSink> eventSink = do_QueryInterface(docShell);
>+  NS_ASSERTION(eventSink, "No eventSink from docShell.");
>+

The next whole if/else part is like a method to "BlockOrAllowMixedContentBasedOnBrowserAndUserPrefs".  Maybe this will help in renaming UpdateUIAsMixedContent, or maybe we can break this whole part out.  Would require a lot of parameter passing though.
>   // If the content is display content, and the pref says display content should be blocked, block it.
>   if (sBlockMixedDisplay && classification == eMixedDisplay) {
>     if (allowMixedContent) {


>@@ -1001,3 +1069,4 @@ nsMixedContentBlocker::AccumulateMixedContentHSTS(nsIURI* aURI, bool aActive)
As mentioned above, the else { nsContentUtils::AddScriptRunner() } part may fit well back in ShouldLoad.  Depends if you need it for HSTS priming.

>     }
>   }
> }
>+

r- for now.  Please make these changes if they will still allow you to meet your needs for HSTS priming.  Happy to sit down and chat about the next patch over vidyo if you would like.  Thanks Kate!
Attachment #8738707 - Flags: review?(tanvi) → review-
(In reply to Kate McKinley [:kmckinley] from comment #3)
> (In reply to Tanvi Vyas [:tanvi] from comment #2)
> > >+  ClassifyContentType(aContentType, classification);
> > >+
> > >+  if (classification == eTopLevelDocument || classification == eWebSocket) {
> > >+    // this should have been accepted in ShouldLoad
> > >+    NS_ASSERTION(false, "Mixed content can't happen with top-level documents or websockets");
> > >+  }
> > > 
> > Why are we doing the classification twice?  Once in ShouldLoad and again in
> > here.
> 
> When this is called from outside nsMixedContentBlocker::ShouldLoad
> (nsHttpClient::OnPrimingFailed), we need to determine whether it is active
> or passive mixed content.

Can we call ClassifyContentType directly in OnPrimingFailed?
Attachment #8738707 - Flags: review?(ckerschb)
I removed some of the code duplication as requested, and this makes the method signature very long for nsMixedContentBlocker::BlockOrAllowMixedContent. It also means more duplicate code in nsHttpChannel::OnPrimingFailed, which I am not excited about.

Essentially, this patch splits nsMixedContentBlocker::ShouldLoad into an "upper half", which is determining whether or not a load is mixed-content, and a "lower half", which determines whether or not to block the load, and updates the UI accordingly.

The reason it is split here is that the HSTS priming check will happen right before ShouldLoad calls BlockOrAllowMixedContent. What happens next depends on the HSTS cache.
  1) If we have STS status cached for this host and we should upgrade the URI, nsHttpClient::Connect will handle the upgrade for us, similar to upgrade-insecure-requests.
  2) If it is cached, and no upgrade will be performed, we will call BlockOrAllowMixedContent immediately.
  3) If we don't have it cached, we will mark the channel for priming.
    a) if the result is positive (STS allows the upgrade), we will redirect to HTTPS
    b) if we have no answer, or no STS header in the priming reply, then call BlockOrAllowMixedContent.
Attachment #8738707 - Attachment is obsolete: true
Attachment #8741992 - Flags: feedback?(tanvi)
Attachment #8741992 - Flags: feedback?(ckerschb)
Attachment #8741992 - Flags: feedback?(tanvi)
Attachment #8741992 - Flags: feedback?(ckerschb)
Whiteboard: [domsecurity-active] → [domsecurity-active] [hsts-priming]
Kate: This was listed as "domsecurity-active" but doesn't seem to be so active. Is this something you're working on in the next quarter or should we put it in the backlog?
Flags: needinfo?(kmckinley)
Whiteboard: [domsecurity-active] [hsts-priming] → [hsts-priming]
Marking as WONTFIX as the work for this will end up happening in bug 1275402.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(kmckinley)
Resolution: --- → DUPLICATE
Duplicate of bug: 1275402
You need to log in before you can comment on or make changes to this bug.