Closed Bug 1188028 Opened 9 years ago Closed 9 years ago

Use channel->ascynOpen2 in dom/security/nsCSPContext.cpp

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox45 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

bug_1188028_asyncopen2_nscspcontext.patch
27.77 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
bug_1188028_queue_up_csp_messages.patch
27.58 KB, patch
sicking
: review+
Details | Diff | Splinter Review 
      No description provided.
Assignee: nobody → mozilla
Blocks: 1182535
Jonas, unfortunately we might not have a loadingNode in all cases. Since CSP reports are kind of special, we only consult CSP for the reportChannel and do not want to perform any other security checks for that channel. Hence I set the flag for SetInitialSecurityCheckDone() on the loadInfo.

We could of course eliminate the if(loadingNode) on the callsite and always use the nullPrincipal, but I think it's better to keep as much accurate information in the loadInfo as possible. Let me know how you would like to handle this, I am fine either way!
Attachment #8639432 - Flags: review?(jonas)
We *always* want to do at least a CheckLoadURI check. CSP reports are no different. Otherwise the website can trigger requests to URLs like local URLs that might have side effects.

Likewise, I don't see why we would want to allow websites to circumvent the nsIContentPolicy by using a CSP-report? So I would think we'd want to call that too here.


In fact, we should probably document that SetInitialSecurityCheckDone must never ever be called outside of the AsyncOpen2 implementation, and that doing so will lead to security bugs.


I take it we can't get a loadingNode for the cases when we are reporting a CSP violation caused by a channel which didn't have a loadingNode? Is that correct?

Can we get a loadingPrincipal in that case instead?
(In reply to Jonas Sicking (:sicking) from comment #2)
> I take it we can't get a loadingNode for the cases when we are reporting a
> CSP violation caused by a channel which didn't have a loadingNode? Is that
> correct?

That is one case and the other case is where we need to serialize a principal:
http://mxr.mozilla.org/mozilla-central/source/caps/nsPrincipal.cpp#428

Both of those cases should be rare though. In the most of all cases we do have a loadingNode available.
 
> Can we get a loadingPrincipal in that case instead?

For the times we do not have a loadingNode available I added some code to create a loadingPrincipal from selfURI, which always needs to be passed to setRequestContext. (Or if not passed exlicitly, then it's queried from the channel).

Doing so allows us to remove the security check from the callsite and add it to the security check within the contentSecurityManager.

Looks better now, I agree.
Attachment #8639432 - Attachment is obsolete: true
Attachment #8639432 - Flags: review?(jonas)
Attachment #8640644 - Flags: review?(jonas)
Jonas, not all callsites of csp->SetRequestContext() provide 'aChannel' argument [1]. If aChannel is not passed, then we can't query the loadingNode and hence we temporarily used the 'nullPrincipal' as the loadingPrincipal when creating the channel for sending out csp reports.

Instead of using the nullPrincipal we can create a principal from selfUri [2] in those cases. Please note that meta CSP is also going to land soonish which will also call csp->SetRequestContext() without providing 'aChannel' as an argument.

I think it makes sense to create CodeBasePrincipal using selfURI for those cases, agreed?

[1] http://mxr.mozilla.org/mozilla-central/search?string=-%3ESetRequestContext
[2] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#585
Flags: needinfo?(jonas)
Status: NEW → ASSIGNED
Comment on attachment 8640644 [details] [diff] [review]
bug_1188028_asyncopen2_nscspcontext.patch

Just clearing flags.
Flags: needinfo?(jonas)
Attachment #8640644 - Flags: review?(jonas)
Jonas, as discussed the other day, let's change the signature of SetRequestContext. All CSP tests are running, but we are leaking memory. I tried to use an nsWeakPtr for the principal, but that does not work because principals do not support weak references (nsISupportsWeakReference).

As of now I can't think of a good place where we can break the cycle between the principal and the csp. Any ideas?
Attachment #8640644 - Attachment is obsolete: true
Attachment #8679810 - Flags: review?(jonas)
Yeah, I thought that we decided that you couldn't make the CSP object hold on to the principal, because there is no reasonable way to make that not leak. So just hold a weakptr to the document instead and if you can't get to the document, just don't send a report.
Hmm... though that won't work for reports from workers...

Ideally for workers we would hold on to the WorkerPrivate or some such, but I suspect that would be hard to make work.

If there's no document, maybe you can get the loadingNode/loadingPrincipal from the channel which violated the policy?
Jonas, a couple changes to the patch:
1) SetRequestContext needs to take 3 arguments (doc, principal, selfURI) where
   either the context can be set using the 'doc' or a by passing 'principal' and 'selfURI'.
   CSP needs a way to resolve 'self', hence passing the principal is not enough, because the URI
   of the prinicpal might be different than the actual 'self' uri.

2) The CSP hangs off the principal and can not outlive the principal, hence we can store a raw
   pointer to the principal within the CSP - no more memory leaks and no weak pointer needed.

I think this is the change we want, especially because we can also send violation reports after deserialization of a principal.
Attachment #8679810 - Attachment is obsolete: true
Attachment #8679810 - Flags: review?(jonas)
Attachment #8680437 - Flags: review?(jonas)
Comment on attachment 8680437 [details] [diff] [review]
bug_1188028_asyncopen2_nscspcontext.patch

Review of attachment 8680437 [details] [diff] [review]:
-----------------------------------------------------------------

I think I'd like to see an updated patch.

::: caps/BasePrincipal.cpp
@@ +209,5 @@
>  
>  BasePrincipal::~BasePrincipal()
> +{
> +  // break the cycle between the CSP and the principal
> +  mCSP = nullptr;

This doesn't do anything useful since the mCSP pointer is just about to be deleted anyway.

However you should null out the mLoadingPrincipal member in the CSP object though since the CSP object might outlive the principal.

::: dom/security/nsCSPContext.cpp
@@ +586,5 @@
>  
>  #undef CASE_CHECK_AND_REPORT
>  
>  NS_IMETHODIMP
> +nsCSPContext::SetRequestContext(nsIDOMDocument* aDOMDocument,

Use an nsIDocument here. They are generally more useful from C++.

@@ +597,3 @@
>  
> +  if (aDOMDocument) {
> +    nsCOMPtr<nsINode> node = do_QueryInterface(aDOMDocument);

That way you also don't need this QI

@@ +597,4 @@
>  
> +  if (aDOMDocument) {
> +    nsCOMPtr<nsINode> node = do_QueryInterface(aDOMDocument);
> +    nsCOMPtr<nsIDocument> doc = node->OwnerDoc();

Or to do this

@@ +600,5 @@
> +    nsCOMPtr<nsIDocument> doc = node->OwnerDoc();
> +    mLoadingContext = do_GetWeakReference(doc);
> +    mSelfURI = doc->GetDocumentURI();
> +    nsCOMPtr<nsIChannel> channel = doc->GetChannel();
> +    mInnerWindowID = nsContentUtils::GetInnerWindowID(channel);

Use doc->InnerWindowID() instead

@@ +601,5 @@
> +    mLoadingContext = do_GetWeakReference(doc);
> +    mSelfURI = doc->GetDocumentURI();
> +    nsCOMPtr<nsIChannel> channel = doc->GetChannel();
> +    mInnerWindowID = nsContentUtils::GetInnerWindowID(channel);
> +    channel->GetLoadGroup(getter_AddRefs(mCallingChannelLoadGroup));

Use doc->GetDocumentLoadGroup() instead

@@ +604,5 @@
> +    mInnerWindowID = nsContentUtils::GetInnerWindowID(channel);
> +    channel->GetLoadGroup(getter_AddRefs(mCallingChannelLoadGroup));
> +    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(channel));
> +    if (httpChannel) {
> +      httpChannel->GetReferrer(getter_AddRefs(mReferrer));

It's probably better to get this off of the document when a report is about to be created. That way you'll get a more up-to-date document URI (which matters for pages that use pushState)
Attachment #8680437 - Flags: review?(jonas) → review-
(In reply to Jonas Sicking (:sicking) from comment #10)

Jonas, incorporated most of your suggestions, but there are some open questions:

> However you should null out the mLoadingPrincipal member in the CSP object
> though since the CSP object might outlive the principal.

When would that be the case that the CSP outlives the principal it's hanging off?
 
 > >  NS_IMETHODIMP
> > +nsCSPContext::SetRequestContext(nsIDOMDocument* aDOMDocument,
> 
> Use an nsIDocument here. They are generally more useful from C++.

I think we can't, since the CSP itself is scriptable. We have to use an nsIDOMDocument within the idl, no?
Attachment #8680437 - Attachment is obsolete: true
Attachment #8680716 - Flags: review?(jonas)
Comment on attachment 8680716 [details] [diff] [review]
bug_1188028_asyncopen2_nscspcontext.patch

Review of attachment 8680716 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/nsCSPContext.cpp
@@ +597,4 @@
>  
> +  if (aDOMDocument) {
> +    nsCOMPtr<nsINode> node = do_QueryInterface(aDOMDocument);
> +    nsCOMPtr<nsIDocument> doc = node->OwnerDoc();

QI directly to nsIDocument

@@ +604,5 @@
> +    mCallingChannelLoadGroup = doc->GetDocumentLoadGroup();
> +  }
> +  else {
> +    mLoadingPrincipal = aPrincipal;
> +    mSelfURI = aSelfURI;

Can you use aPrincipal->GetURI here instead? And remove the selfURI argument?

@@ +697,5 @@
> +  nsCOMPtr<nsIDOMNode> loadingContext = do_QueryReferent(mLoadingContext);
> +  nsCOMPtr<nsINode> loadingNode = do_QueryInterface(loadingContext);
> +  if (loadingNode) {
> +    nsAutoString referrerURI;
> +    loadingNode->OwnerDoc()->GetReferrer(referrerURI);

You realize that you are literally getting the same node 3 times here, right?

But anyway, the referrer you are getting here is not the referrer you are looking for. What document->GetReferrer returns is the referrer that was used to fetch the document.

What you probably want here is something like:
if (doc) ref = doc->GetDocumentURI();
else ref = mPrincipal->GetURI();

or simply

ref = mSelfURI;

(Generally, it might be better to get rid of mSelfURI and always get an updated URI from the document/principal. This makes us report a more correct URI if pushState has been used)
Attachment #8680716 - Flags: review?(jonas)
As discussed over IRC, we can simplify the interface of setRequestContext() and remove selfURI as an argument. Callers have to pass a principal where principal->GetURI == selfURI.

> But anyway, the referrer you are getting here is not the referrer you are
> looking for. What document->GetReferrer returns is the referrer that was
> used to fetch the document.

And that is the referrer that should be set in the csp report JSON.
Attachment #8680716 - Attachment is obsolete: true
Attachment #8680968 - Flags: review?(jonas)
Comment on attachment 8680968 [details] [diff] [review]
bug_1188028_asyncopen2_nscspcontext.patch

Review of attachment 8680968 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/nsCSPContext.cpp
@@ +693,5 @@
>  
>    // referrer
> +  nsCOMPtr<nsIDocument> doc = do_QueryReferent(mLoadingContext);
> +  if (doc) {
> +    doc->GetReferrer(report.mCsp_report.mReferrer);

This doesn't match the description here:

http://www.w3.org/TR/CSP11/#violation-report-referrer

Are you sure this is what we want?
(In reply to Jonas Sicking (:sicking) from comment #14)
> Comment on attachment 8680968 [details] [diff] [review]
> bug_1188028_asyncopen2_nscspcontext.patch
> 
> Review of attachment 8680968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/security/nsCSPContext.cpp
> @@ +693,5 @@
> >  
> >    // referrer
> > +  nsCOMPtr<nsIDocument> doc = do_QueryReferent(mLoadingContext);
> > +  if (doc) {
> > +    doc->GetReferrer(report.mCsp_report.mReferrer);
> 
> This doesn't match the description here:
> 
> http://www.w3.org/TR/CSP11/#violation-report-referrer
> 
> Are you sure this is what we want?

As discussed over IRC, that is what we want, just adding a reference for the 'protected resource' here that was misleading:
http://www.w3.org/TR/CSP11/#protected-resource
Comment on attachment 8680968 [details] [diff] [review]
bug_1188028_asyncopen2_nscspcontext.patch

Review of attachment 8680968 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsPrincipal.cpp
@@ +79,5 @@
>  nsPrincipal::~nsPrincipal()
> +{ 
> +  // let's nullify the principal within the csp to avoid a tangling pointer
> +  if (mCSP) {
> +    static_cast<nsCSPContext*>(mCSP.get())->nullifyLoadingPrincipal();

s/nullify/clear/

::: dom/security/nsCSPContext.cpp
@@ +693,5 @@
>  
>    // referrer
> +  nsCOMPtr<nsIDocument> doc = do_QueryReferent(mLoadingContext);
> +  if (doc) {
> +    doc->GetReferrer(report.mCsp_report.mReferrer);

Given that the referrer doesn't change for the lifetime of the document, we might as well keep mReferrer and grab it when we're grabbing the other information from the document in SetRequestContext. Up to you.
Attachment #8680968 - Flags: review?(jonas) → review+
Arrh, now we are facing the problem that the innerWindowID is not available when calling ::SetRequestContext [1]. We somehow have to queue up those console messages and flush that queue once the innerwindow id is available.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3c15b5f65f0


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=970790#c14
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> Arrh, now we are facing the problem that the innerWindowID is not available
> when calling ::SetRequestContext [1]. We somehow have to queue up those
> console messages and flush that queue once the innerwindow id is available.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3c15b5f65f0

Hey Blake, (Jonas said it's worth consulting you because you might be able to help us out). Is there anything we could do to make the innerWindowId to be available whenever we call ::InitCSP/SetRequestContext()? Or is the only option we have to queue up those console messages and log them once the document is fully initialized?
Flags: needinfo?(mrbkap)
I'm not entirely sure if there's a good answer here. One thing I did notice (from bug 970790 comment 14) is that the period of time between the call to nsDocument::InitCSP and nsGlobalWindow::SetNewDocument is very short, so it might not be possible to generate a CSP report in that section of code. So, it could be fine to throw out (or assert) that we don't generate any reports and update the inner window ID once we have it (maybe in nsDocument::SetScriptGlobalObject)?
Flags: needinfo?(mrbkap)
Incorporated Jonas nits; carrying over r+.
Attachment #8680968 - Attachment is obsolete: true
Attachment #8683983 - Flags: review+
Jonas, let's have one central point within nsCSPContext where we can queue up console messages in case the window id is not ready (please note that the windowID is already available once SetRequestContext is called for meta csp). nsCSPParser should also forward it's console messages to that central point within nsCSPContext. Once the windowID becomes available, we can flush the queue and from that point on there is no need to queue up messages anymore.

When browsing through the code I found 'FlushCSPWebConsoleErrorQueue' which dates back to ancient times and has been unused for quite some time. Anyway, it provides a nice point in the code from where we can call back to CSP to flush the queued up messages.
Attachment #8683986 - Flags: review?(jonas)
Blocks: 970790
All the devtools tests expecting a web console message from CSP are working again :-)
Looks good:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2da903a78cbe
Comment on attachment 8683986 [details] [diff] [review]
bug_1188028_queue_up_csp_messages.patch

Review of attachment 8683986 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/security/nsCSPContext.cpp
@@ +605,5 @@
>      mInnerWindowID = doc->InnerWindowID();
> +    // the innerWindowID is not available for CSPs delivered through the
> +    // header at the time setReqeustContext is called - let's queue up
> +    // console messages until it becomes available, see flushConsoleMessages
> +    mQueueUpMessages = mInnerWindowID ? false : true;

mQueueUpMessages = !mInnerWindowID;

@@ +621,5 @@
>    NS_ASSERTION(mSelfURI, "mSelfURI not available, can not translate 'self' into actual URI");
>    return NS_OK;
>  }
>  
> +struct consoleMsgQueueElem {

ConsoleMsgQueueElem. Gecko uses InitCase for classnames generally.

@@ +679,5 @@
> +    nsXPIDLString msg;
> +    CSP_GetLocalizedStr(aName, aParams, aParamsLength, getter_Copies(msg));
> +    consoleMsgQueueElem elem(msg, PromiseFlatString(aSourceName), PromiseFlatString(aSourceLine),
> +                             aLineNumber, aColumnNumber, aSeverityFlag);
> +    mConsoleMsgQueue.AppendElement(elem);

This ends up doing some extra copying.

You could instead remove the consoleMsgQueueElem ctor and do

consoleMsgQueueElem& elem = *mConsoleMsgQueue.AppendElement();
elem.mMsg = ...;
elem.mSourceName = ...;

It's more lines here, but fewer lines in the struct. Should work out to be the same.

Up to you
Attachment #8683986 - Flags: review?(jonas) → review+
https://hg.mozilla.org/mozilla-central/rev/acc983ca0dec
https://hg.mozilla.org/mozilla-central/rev/573a41d3890e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: