1378552 - Audit use of NullPrincipal::Create() to see if we're missing origin attributes

Status: RESOLVED FIXED

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

Description

[Daniel Veditz [:dveditz]]

When reviewing the patch for bug 1377426 I noticed that documents with a CSP sandbox directive that doesn't have allow-same-origin would end up escaping its container because of the use of NullPrincipal::Create(). This will end up in the default container regardless of the docshell it was actually in.


Some of these might be fine if they're internal things and don't include remote web resources, but there are a couple dozen to go through and I bet there's at least one or two more that aren't correct.

https://searchfox.org/mozilla-central/search?q=NullPrincipal%3A%3ACreate()

Comment 1

[Daniel Veditz [:dveditz]]

Jonathan, is this something you could look into?

Comment 2

[Jonathan Kingston [:jkt] he/him]

I can take this yeah, it's a great catch.
I will need to check if I can work on this time wise though.

Comment 3

[Frederik Braun [:freddy]]

This sort of reminds me of the CSP bypass in bug 1073952.
While auditing, it would be great to look for both things:
a) that we inherit origin attributes
b) that we keep the CSP around

Comment 4

[Jonathan Kingston [:jkt] he/him]

Hey Tom,

I was asked by Tanvi to show this to you as this will impact Tor FP isolation and also PB mode too.

(It seems I can't add Tim to this bug who was also suggested).

I'm unlikely to be able to work on this in Q3 however I will be available if you have questions.

Thanks

Comment 5

[Tom Ritter [:tjr]]

This seems like something I should make time for.

Comment 6

[Tom Ritter [:tjr]]

I made good progress on this, I went through all the callsites: https://docs.google.com/spreadsheets/d/1tlRGY56BELAGxb64Ebo9eHoyi7ALRgpXwpyIFOAOMPc/edit?usp=sharing

I found the following of concern:

1) In parser/htmlparser/nsExpatDriver.cpp IF we process External Entties in XML documents, they will inherit a Null Principal. That seems like a definite bypass. But I also really really hope we don't do that, because it's a security problem in and of itself, nevermind OriginAttributes.

2) In layout/style/StyleSheet.cpp it seems like we will frequently (many callsites) make StyleSheets with a null principal? That seems... likely to be incorrect.

3) We use a null principal in dom/base/DOMParser.cpp : SetUpDocument and the callsites for this get pretty confusing.

Christoph, could you skim through my spreadsheet and confirm nothing else seems concerning to you? The yellow ones are "I think this is okay, but I am making an educated guess."  Any suggestions or info you have about 2 and 3 would be great.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tom Ritter [:tjr] from comment #6)
> I made good progress on this, I went through all the callsites:
> https://docs.google.com/spreadsheets/d/
> 1tlRGY56BELAGxb64Ebo9eHoyi7ALRgpXwpyIFOAOMPc/edit?usp=sharing

Going forward we should think about:
* Making the NullPrincipal constructor private or protected. I didn't find any callsite, but better worry than feeling sorry.
* NullPrincipal::Create(OA=default) doesn't seem good to me, maybe we should have two create methods, where one takes OA and on explicitly says, no OA needed. Probably an exercise we should do, which also would make the audit more sophisticated.
* The ScriptSecurityManager also allows to create a NullPrincipal with OA from JS, luckily I only found one callsite, so I guess there is no real audit needed, but probably we should keep that on the radar as well:
https://dxr.mozilla.org/mozilla-central/search?q=createNullPrincipal+ext%3Ajs&redirect=false

 
> 2) In layout/style/StyleSheet.cpp it seems like we will frequently (many
> callsites) make StyleSheets with a null principal? That seems... likely to
> be incorrect.

I am not sure if that is incorrect. To me that rather seems, start off with a NullPrincipal and open up if needed, but also not entirely sure.
 
> 3) We use a null principal in dom/base/DOMParser.cpp : SetUpDocument and the
> callsites for this get pretty confusing.

Probably we can have an assertion within Init() to make sure we pass a principal to init() and not just having the warning there. But I agree, that gets confusing.

> Christoph, could you skim through my spreadsheet and confirm nothing else
> seems concerning to you? The yellow ones are "I think this is okay, but I am
> making an educated guess."  Any suggestions or info you have about 2 and 3
> would be great.

Summing it up, I think we should get rid of the default arguments within NullPrincipal::Create().

Comment 8

[Frederik Braun [:freddy]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> (In reply to Tom Ritter [:tjr] from comment #6)
> > 2) In layout/style/StyleSheet.cpp it seems like we will frequently (many
> > callsites) make StyleSheets with a null principal? That seems... likely to
> > be incorrect.
> 
> I am not sure if that is incorrect. To me that rather seems, start off with
> a NullPrincipal and open up if needed, but also not entirely sure.
>  

Note how they intend to update it using SubjectSubsumesInnerPrincipal afterwards[1]. I'm not saying it's comprehensive. Just that it's there :-)


(FWIW, the spreadsheet is read-only, otherwise ckerschb would have likely added the JS callers)

Comment 9

[Tom Ritter [:tjr]]

I attached a patch; but it breaks on try as Style apparently uses the constructor in some way:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=feb87754734f775e36c931a40261e989a8a84fe0&selectedJob=169746904

Running out for the day so I haven't dug in...

Comment 10

[Tom Ritter [:tjr]]

Attachment: nullprincipal.patch

Will do a full try run before landing, but I think this is good.

I messed with the Style system a little, so asking Emilio for review too.

Comment 11

[Tom Ritter [:tjr]]

Is there any way we can make the NullPrincipal ctor private while supporting deserialization? https://searchfox.org/mozilla-central/source/caps/NullPrincipal.h#3

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8961755 [details] [diff] [review]
nullprincipal.patch

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

All the layout/style changes are effectively just removing the Create() which use to get the default OriginAttributes, and using CreateWithoutOriginAttributes(), which does the same thing, but explicitly, right?

If so, looks good to me, thanks!

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8961755 [details] [diff] [review]
nullprincipal.patch

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

Tom, thanks for incorporating that change. Code looks great, hopefully we can resolve the deserialization issue.

::: caps/NullPrincipal.h
@@ +85,5 @@
> +  // methods.
> +  NullPrincipal()
> +    : BasePrincipal(eNullPrincipal)
> +  {
> +  }

Where you able to resolve that problem for the deserialization we chatted about? Maybe we can mark the deserialization class as a friend, hence being more explicit about our change that no one outside the deserialization and this class itself should touch the constructor.

Comment 14

[Tom Ritter [:tjr]]

Attachment: nullprincipal.patch

Comment 15

[Tom Ritter [:tjr]]

Comment on attachment 8962773 [details] [diff] [review]
nullprincipal.patch

Per the discussion in #developers with :mystor - it's not possible to make the ctor private without significantly refactoring a whole lot of IPC and Serialization stuff. So I undid that.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8962773 [details] [diff] [review]
nullprincipal.patch

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

Ok, we leave that for another day. Anyway, eliminating default arguments and being more explicit about OA is definitely great progress here. Thanks for doing this Tom!

Comment 17

[Tom Ritter [:tjr]]

Attachment: nullprincipal.patch

Comment 18

[Tom Ritter [:tjr]]

Final version that passes try.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tom Ritter [:tjr] from comment #18)
> Final version that passes try.

What was the problem?

Comment 20

[Tom Ritter [:tjr]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19)
> (In reply to Tom Ritter [:tjr] from comment #18)
> > Final version that passes try.
> 
> What was the problem?

Oh, it was restoring the bits in the style class I told you about at the end of your day yesterday; I just hadn't put the patch up before you left.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8962883 [details] [diff] [review]
nullprincipal.patch

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

Ship it!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe83d3f7d4a4b5dab1b6b71cf9a3df2cbfbaa93

FYI, I've already confirmed that this grafts cleanly to Beta in case you're thinking this would be a good uplift for the next ESR :)

Comment 23

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/mozilla-central/rev/1fe83d3f7d4a4b5dab1b6b71cf9a3df2cbfbaa93

Comment 24

[Tom Ritter [:tjr]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 1fe83d3f7d4a4b5dab1b6b71cf9a3df2cbfbaa93
> 
> FYI, I've already confirmed that this grafts cleanly to Beta in case you're
> thinking this would be a good uplift for the next ESR :)

Eh; this is really about preventing future misuse, so I don't think it needs an uplift. Thanks though!