994466 - CSP in C++: Fix memory leak in CSPContext

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

In the current implementation the Principal holds a reference to CSPContext and vice versa. We need to find a way to break that cycle. Probably best in nsDocument destructor

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_994466_fix_memory_leak.patch

In an ideal world I would like to define nsDocument as a |friend| of nsCSPContext and not change the *.idl. Something like this:

> class nsCSPContext : public nsIContentSecurityPolicy
> {
>  friend class nsDocument;
>  friend void DropPrincipal() {
>    mPrincipal = nullptr;
>  }
>  // class methods  
> }

Unfortunately, the nsPrincipal holds a pointer to nsIContentSecurityPolicy which leaves no other option than extending the interface with |dropPrincipal|.
I haven't tested this closely, but this patch should fix the circular reference problem and therefore avoid the encountered memory leak.

A similar solution would drop the csp in the principal. But "Safety first"; worst case scenario with this the current patch is that we someone drops the Principal too early in nsIContentPolicy which would 'deny' reports to be sent in |SendReports|. So I guess dropping the principal in the contentPolicy is the better option.

Sid, what do you think?

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

I just spoke with :mccr8 and we should exhaust all possibility of using a weak reference from the CSP to Principal first.  If that doesn't work, we might be able to use a mutation observer to detect events on the document (document destruction, for example) and break the cycle there, but that's not good if the document simply gets a new principal and is not destroyed.

So... lets try weak references to the principal first.  Where will that not work, and can we get by without a principal (copy in relevant info) in those cases where the principal is destroyed before we use it?

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Looks like the only place the principal is used by CSP is in the report sending logic.  It's passed to NS_CheckContentLoadPolicy, and we might be able to get by without a principal, not sure.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> So... lets try weak references to the principal first.  Where will that not
> work, and can we get by without a principal (copy in relevant info) in those
> cases where the principal is destroyed before we use it?

I am going to figure out in what cases the weakReference does not work. If the weak reference to the principal does not work, why not modify the current patch to use the following code in the destructor of nsDocument?

> if (principal) {
>     nsCOMPtr<nsIContentSecurityPolicy> csp;
>    if (NS_SUCCEEDED(principal->GetCsp(getter_AddRefs(csp))) && csp) {
>      nsRefPtr<nsCSPContext> cspContext(do_QueryObject(csp));
>      if (cspContext) {
>        cspContext->DropPrincipal(); 
>      }
>     }
>   }

No need to change the idl, and we would be sure that the document is destroyed, meaning the CSP is definitely not going to be needed again.
But in general, I agree, the weak reference to principal would be the best solution.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #3)
> Looks like the only place the principal is used by CSP is in the report
> sending logic.  It's passed to NS_CheckContentLoadPolicy, and we might be
> able to get by without a principal, not sure.

Test test/csp/test_csp_report.html times out in case the principal is null. Reason: NS_CheckContentLoadPolicy returns REJECT if no principal is provided.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Looks like we can call ShouldLoad directly instead of using the detour of calling NS_CheckContentLoadPolicy. Therefore we can use the selfURI as aRequestOrigin and can disregard the principal completely. Sid just pushed the fix to our csp patch queue.
Can we close this bug, or should we upload the patch here? Sid?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_994466_change_call_to_shouldload.patch

Instead of calling NS_CheckContentLoadPolicy we call ShouldLoad directly which allows us to skip the principal and use the originalURI directly. Awesome, easy fix for a memory leak after all.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8404883 [details] [diff] [review]
bug_994466_change_call_to_shouldload.patch

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

yay for leak fixes!  r=me if you fix the minors, and with garrett's review.

::: content/base/src/nsCSPContext.cpp
@@ +634,5 @@
>  
>      // refuse to load if we can't do a security check
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      if (shouldLoad != nsIContentPolicy::ACCEPT) {

Please use NS_CP_REJECTED(shouldLoad) instead of !=.

@@ +637,5 @@
>  
>      if (shouldLoad != nsIContentPolicy::ACCEPT) {
>        // skip unauthorized URIs
>        // probably should report error
>        return NS_OK;

This should probably be a "continue" because there may be other report URIs.  And we should post a warning to the web console.

Comment 9

[Garrett Robinson [:grobinson]]

The attached patch has some issues, but it appears to have been merged (and the issues fixed) in the latest commits from the patch queue. I'm clearing the review flag, if a review is still needed post an updated patch and re-flag me.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

The patch here still needs an update to address the minors in comment 8 before it lands.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Waiting till other patches landed to avoid unnecessary exporting. Will address the nits in Comment 8 ASAP. Thanks for reminding!

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_994466.patch

Carrying over r+ from sstamm; also added him as a reviewer.
Please note that all the nits from comment 8 got incorporated into the patch in bug 994322 which has to land the at same time (but before) this patch.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

I had this patch on try [1] together with the patch from bug 994322. Those two patches are ready to land and need to land together. First apply patch from bug 994322 and then this one on top. Thanks!

[1] https://tbpl.mozilla.org/?tree=Try&rev=955de4d3dda2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eca5faae807d

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eca5faae807d