Closed Bug 1378552 Opened 3 years ago Closed 2 years ago

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

Categories

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

55 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: dveditz, Assigned: tjr)

References

Details

(Keywords: sec-audit, Whiteboard: [domsecurity-active][adv-main61-][post-critsmash-triage])

Attachments

(1 file, 2 obsolete files)

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()
Jonathan, is this something you could look into?
Flags: needinfo?(jkt)
I can take this yeah, it's a great catch.
I will need to check if I can work on this time wise though.
Flags: needinfo?(jkt)
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
Assignee: nobody → jkt
Status: NEW → ASSIGNED
Priority: P1 → P2
Whiteboard: [domsecurity-active]
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
Flags: needinfo?(tom)
This seems like something I should make time for.
Assignee: jkt → tom
Flags: needinfo?(tom)
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.
Flags: needinfo?(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().
Flags: needinfo?(ckerschb)
(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)
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...
Attached patch nullprincipal.patch (obsolete) — Splinter Review
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.
Attachment #8961755 - Flags: review?(emilio)
Attachment #8961755 - Flags: review?(ckerschb)
Is there any way we can make the NullPrincipal ctor private while supporting deserialization? https://searchfox.org/mozilla-central/source/caps/NullPrincipal.h#3
Flags: needinfo?(mrbkap)
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!
Attachment #8961755 - Flags: review?(emilio) → review+
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.
Attachment #8961755 - Flags: review?(ckerschb) → review+
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.
Attachment #8962773 - Flags: review?(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!
Attachment #8962773 - Flags: review?(ckerschb) → review+
Attachment #8962773 - Attachment is obsolete: true
Flags: needinfo?(mrbkap)
Attachment #8962883 - Flags: review?(ckerschb)
(In reply to Tom Ritter [:tjr] from comment #18)
> Final version that passes try.

What was the problem?
Flags: needinfo?(tom)
(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.
Flags: needinfo?(tom)
Comment on attachment 8962883 [details] [diff] [review]
nullprincipal.patch

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

Ship it!
Attachment #8962883 - Flags: review?(ckerschb) → review+
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 :)
https://hg.mozilla.org/mozilla-central/rev/1fe83d3f7d4a4b5dab1b6b71cf9a3df2cbfbaa93
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(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!
Group: dom-core-security → core-security-release
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main61-]
Flags: qe-verify-
Whiteboard: [domsecurity-active][adv-main61-] → [domsecurity-active][adv-main61-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.