Closed Bug 1339004 Opened 7 years ago Closed 7 years ago

DocGroup labeling in dom/security

Categories

(Core :: DOM: Security, defect, P2)

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ethan, Assigned: tnguyen)

References

Details

(Whiteboard: [domsecurity-active][QDL][TDC-MVP][SECENG])

Attachments

(1 file)

Bug 1339004 - Do DocGroup labeling in dom/security.
59 bytes, text/x-review-board-request
ckerschb
: review+
Details 
This bug is to apply labeling APIs (https://wiki.mozilla.org/Quantum/DOM) to DOM Security component.
Blocks: Labeling
Priority: -- → P2
Whiteboard: [domsecurity-backlog1]
There's one place will call NS_DispatchToMainThread in dom/security
and this line will be run in both parent and child process.

http://searchfox.org/mozilla-central/source/dom/security/nsCSPContext.cpp#1166
Assignee: nobody → allstars.chh
Assignee: allstars.chh → nobody
Assignee: nobody → tnguyen
Bill,
I guess there're some corner cases that we may temporarily except unlabeled runnables [1]
[1] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#2082
It seems to be happened in this bug. Because I don't see how to get tabgorup or docgroup to do labelling if the csp context comes from addon [2]
[2] http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/AddonContentPolicy.cpp#434
We may have null document, no InnerWindowID, null nsILoadGroup and only have a valid principal in this case.
SystemGroup would not be applicable here, do you have any concern if I leave them unlabeled?
Flags: needinfo?(wmccloskey)
I don't think the add-on content policy should every be used in a content process, so we don't need to worry about labeling anything there.

In general, though, if runnables are expected to be very rare, it's okay not to label them.
Flags: needinfo?(wmccloskey)
Hi Christoph
Could you please take a look at this patch first?
Basically, there's CSPReportSenderRunnable in nsCSPContext need to be labeled and we have to find a appropriate document docgroup or window tab group to do labeling. In most of the cases, nsCSPContext is tied to a document so we could use the document's DocGroup to do labeling.

We are handling the case if nsCSPContext is tied to a nsIPrincipal:
- In worker, will try to get the top level document then use it to label csp runnable
- In Session Restores, the principal is read from history content serialized principal, so when we try to restore a tab we could docshell of contentviewer to do labeling if necessary.
We would ignore corner cases in which we may not be able to find a good docgroup or tab group (add-on or maybe service worker)
Comment on attachment 8847969 [details]
Bug 1339004 - Do DocGroup labeling in dom/security.

https://reviewboard.mozilla.org/r/120508/#review122950

That's already a great start, thanks for taking on that work. I think we need to put a little more attention on some of those cases. As a first step, please incorporate/answer my comments and we can take it from there. Even though I know our tests are not exhaustive, please run all the CSP tests with an assertion so we can list all the tests that don't have a valid loadGroup before dispatching (those within dom/security/test/csp and also the csp worker tests). We can than closely evaluate those cases and figure if we need to call setReportDispatcher() in more places (even though I don't think so at the moment).

I am really not sure about the history entry case. Once we are done with our internal reviews we should consult mdeboer for his opinion.

::: docshell/base/nsDocShell.cpp:9968
(Diff revision 1)
> +    if (csp && !csp->GetLoadingContext() && docShell) {
> +      nsIDocument* document = docShell->GetDocument();
> +      if (document) {
> +        csp->SetReportDispatcher(document);
> +      }
> +    }

If I am not mistaken, then the CSP at this point should already have a document from some SetRequestContext() call on the CSP. It's possible that I am missing a corner case here, if so, please let me know.

Also, JFYI, that ContentPolicy check here underneath will go away once once we have fixed Bug 1331740.

::: docshell/shistory/nsSHEntry.cpp:537
(Diff revision 1)
> +
> +    nsCOMPtr<nsIDocument> doc = contentViewer->GetDocument();
> +    if (doc) {
> +      csp->SetReportDispatcher(doc);
> +    }
> +  }

Ultimately we need a a review from a peer here because I am not sure that calling GetContentViewer will provide the right context for the history entry.

I suppose we also need to update nsSHEntry::Create() which is used to create a history entry the most, right?

::: dom/interfaces/security/nsIContentSecurityPolicy.idl:234
(Diff revision 1)
> +
> +  /**
> +   * Set dispatcher for NS_DispatchToMainThread(CSPReportSenderRunnable)
> +   * so that runnalbes will be dispatched to the TabGroup or DocGroup
> +   */
> +  [noscript] void setReportDispatcher(in nsIDocument aDocument);

Should that probably pass an argument for type dispatcher? Alternatively we could update the function name to setReportDispatcherForDoc(...);

::: dom/workers/WorkerPrivate.cpp:2667
(Diff revision 1)
> +    // query the top document if there's any
> +    nsCOMPtr<nsIDocument> doc = GetDocument();
> +    if (doc) {
> +      csp->SetReportDispatcher(doc);
> +    }
> +  }

Is that the right document? If so, I suppose we could add the doc argument to mPrincipal->EnsureCSP(doc, ...); above, right?
Attachment #8847969 - Flags: review?(ckerschb) → review-
Comment on attachment 8847969 [details]
Bug 1339004 - Do DocGroup labeling in dom/security.

https://reviewboard.mozilla.org/r/120508/#review122966

::: docshell/base/nsDocShell.cpp:9968
(Diff revision 1)
> +    if (csp && !csp->GetLoadingContext() && docShell) {
> +      nsIDocument* document = docShell->GetDocument();
> +      if (document) {
> +        csp->SetReportDispatcher(document);
> +      }
> +    }

http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/toolkit/content/browser-child.js#333
That case we delivery a principal from serialized principal and then pass it to docshel to load an URI from history (session restore).
So the principal may have null document context. I am not sure during loading uri, we may override or use another principal (principaltoinherit or something), but we may want to pass a doc to do labeling later.

::: dom/workers/WorkerPrivate.cpp:2667
(Diff revision 1)
> +    // query the top document if there's any
> +    nsCOMPtr<nsIDocument> doc = GetDocument();
> +    if (doc) {
> +      csp->SetReportDispatcher(doc);
> +    }
> +  }

Probably not, in the case csp is not inherited from parent (embedded resource with data: and blob: if I remember correctly :)). 
In that case, we delivery a new CSP with context the principal of . mPrincipal->EnsureCSP(doc, ...) would cause the wrong context.
In term of labeling, we dont have to care about that, a top level doc is good enough for us.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Comment on attachment 8847969 [details]
> Bug 1339004 - Do DocGroup labeling in dom/security
> 
> https://reviewboard.mozilla.org/r/120508/#review122950
> 
> That's already a great start, thanks for taking on that work. I think we
> need to put a little more attention on some of those cases. As a first step,
> please incorporate/answer my comments and we can take it from there. Even
> though I know our tests are not exhaustive, please run all the CSP tests
> with an assertion so we can list all the tests that don't have a valid
> loadGroup before dispatching (those within dom/security/test/csp and also
> the csp worker tests). We can than closely evaluate those cases and figure
> if we need to call setReportDispatcher() in more places (even though I don't
> think so at the moment).
> 

Thanks Christoph for your review. That's a great idea, I will try to add an assertion to find more places that we may miss.
I've just changed the way we label runnable:
- Workers and nested worker have already support mainthread eventtarget, just pass it to csp context
- Using tabgroup window (node window) which we could query from requesting context in ContentPolicy check.

I added an assetion and ran on try and it looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a5bcf52e88afb881bd60b80468a61a4b54df9d9&selectedJob=86109728
Updated a little, move the same code blocks into 1 function: EnsureEventTarget
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1][QDL][TDC-MVP][SECENG]
Comment on attachment 8847969 [details]
Bug 1339004 - Do DocGroup labeling in dom/security.

https://reviewboard.mozilla.org/r/120508/#review126884

This is pretty close, i would like to see it one more time though, mostly because I would like to understand why we need the null check within EnsureEventTarget(). I think we should also run this by a worker peer. I suggest you ask bkelly for those bits - thanks!

::: dom/base/nsContentPolicy.cpp:28
(Diff revision 3)
>  #include "nsIContent.h"
>  #include "nsILoadContext.h"
>  #include "nsCOMArray.h"
>  #include "nsContentUtils.h"
>  #include "mozilla/dom/nsMixedContentBlocker.h"
> +#include "mozilla/Unused.h"

Not entirely sure but I think you can remove that inclusion, right?

::: dom/base/nsContentPolicy.cpp:146
(Diff revision 3)
> +    }
> +
> +    if (requestPrincipal) {
> +        nsCOMPtr<nsIContentSecurityPolicy> csp;
> +        requestPrincipal->GetCsp(getter_AddRefs(csp));
> +

nit: remove empty line

::: dom/interfaces/security/nsIContentSecurityPolicy.idl:11
(Diff revision 3)
>  #include "nsIContentPolicy.idl"
>  
>  interface nsIURI;
>  interface nsIChannel;
>  interface nsIDocShell;
> +interface nsIDocument;

I think you can remove nsIDocument from here, right?

::: dom/security/nsCSPContext.cpp:689
(Diff revision 3)
>  }
>  
> +NS_IMETHODIMP
> +nsCSPContext::EnsureEventTarget(nsIEventTarget* aEventTarget) {
> +  NS_ENSURE_ARG(aEventTarget);
> +  if (!mEventTarget) {

Why do we need that nullcheck? If needed, please add a comment why we need it.

::: dom/security/nsCSPContext.cpp:1197
(Diff revision 3)
> +  if (XRE_IsContentProcess()) {
> +    if (mEventTarget) {
> +      if (nsCOMPtr<nsINamed> named = do_QueryInterface(task)) {
> +        named->SetName("CSPReportSenderRunnable");
> +      }
> +

nit: remove empty line

::: dom/workers/WorkerPrivate.cpp:2648
(Diff revision 3)
>  template <class Derived>
> +void
> +WorkerPrivateParent<Derived>::SetCSP(nsIContentSecurityPolicy* aCSP)
> +{
> +  AssertIsOnMainThread();
> +  if (aCSP) {

wouldn't it make more sense to use
if (!aCSP) {
  return;
}
Attachment #8847969 - Flags: review?(ckerschb)
Thanks Christoph for your review.
I could not set bkelly as a reviewer at the moment (not reviewing due to deadline][:bkelly]). Hi baku, could you please take a look at this? Or do you know who would be good to take a look at this patch?
Attachment #8847969 - Flags: review?(amarchesini)
Comment on attachment 8847969 [details]
Bug 1339004 - Do DocGroup labeling in dom/security.

Better smaug. I just submitted my first labelling patches for dom/filesystem. I want to see if I have covered all before reviewing somebody else's patches.
Attachment #8847969 - Flags: review?(amarchesini) → review?(bugs)
Whiteboard: [domsecurity-backlog1][QDL][TDC-MVP][SECENG] → [domsecurity-active][QDL][TDC-MVP][SECENG]
Status: NEW → ASSIGNED
Comment on attachment 8847969 [details]
Bug 1339004 - Do DocGroup labeling in dom/security.

https://reviewboard.mozilla.org/r/120508/#review127578

thanks for addressing the concerns - looks good now!
Attachment #8847969 - Flags: review?(ckerschb) → review+
Comment on attachment 8847969 [details]
Bug 1339004 - Do DocGroup labeling in dom/security.

https://reviewboard.mozilla.org/r/120508/#review128204

::: dom/security/nsCSPContext.cpp:687
(Diff revision 4)
>    NS_ASSERTION(mSelfURI, "mSelfURI not available, can not translate 'self' into actual URI");
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsCSPContext::EnsureEventTarget(nsIEventTarget* aEventTarget) {

Nit, { should be on its own line
Attachment #8847969 - Flags: review?(bugs) → review+
Attachment #8847969 - Flags: review?(amarchesini)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/648099cae850
Do DocGroup labeling in dom/security. r=ckerschb,smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/648099cae850
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thomas, thanks for your diligent work to get this done!
Glad to see this was resolved.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: