326506 - [FIX]Implement a powerless non-principal

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See discussion in bug 326279 and the comment in the URL field.  Also see comments in bug 211999 (esp. bug 211999 comment 10 and bug 211999 comment 17).

In brief, we want to have a principal that indicates "no security information available".  Access to objects with such a principal would be disallowed to everyone (including chrome, possibly?).  Actions by actors with such a principal would be disallowed (more on this below, though).

My first question is whether this non-principal should be a singleton (like the system principal).  The way I see it, the main benefit of a singleton is that we can guarantee its existence (allocate during module init).  That means that we can ensure that documents and other nodes always have a principal -- on OOM, just use the non-principal.  That should let us clean up lots of null-checks, I think.

The drawback of a singleton is that use of pointer equality checks on principals  would need to be stamped out.  Which might cause issues, since Equals() on itself will return false for the non-principal.  This means that, for example, a document with the non-principal would not be able to access various parts of itself.  But maybe that's ok?  Also, are there performance considerations of replacing equality compares with a virtual call (which would hopefully do an equality compare first thing, but would still be slowed in cases like CheckSameOriginPrincipal where the principals may well be unequal, but same origin)?

If we do a non-singleton, then we continue to have to worry about OOM leading to null principals.  But then we can declare that the non-principal is Equals() to itself via pointer compare and various places where we pointer-compare on principals are OK.  Or we implement brendan's comment on nsIPrincipal that non-principals are not equal to themselves and then we need to eliminate the pointer compares in any case.

So I guess the real first question is whether a given document's non-principal should be equal to the non-principal of nodes in that document.  That is, should script in that document be able to touch the DOM in that same document?  If not, then we need to nuke pointer compares and we should just do a singleton.  And remove the secuirty-check-skipping optimizations in nsDOMClassInfo, possibly.  If it should, then we need to figure out exactly when two non-principals are equal.  For example, could we have them equal via pointer compare but still have Equals() return false?  Again, checks of all places where we do pointer compare would be needed.

Other thoughts?  Please cc anyone who should be cced and whom I missed, ok?

Comment 1

[Boris Zbarsky [:bzbarsky]]

We could also use this in nsJSThunk::EvaluateScript if there is no owner. 

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not sure the singleton approach would work. Wouldn't we get issues with being unable with document.appendChild(document.createElement(..)) failing because the document doesn't have access to the element? Or maybe that is ok that it is failing.

Another issue I can see with getting rid of pointer compares is that it might be a performance hit. But we could probably get rid of that by doing something like the following in the critical places:

if (princA != mNonPrincipal && princA == princB) return NS_OK;

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Wouldn't we get issues with being unable with
> document.appendChild(document.createElement(..)) failing

Yes, but the question is when documents would have the non-principal... As I see it, this would be a fallback case for cases when all else fails, so failing basic DOM operations there would be ok.  But yes, the question is what we want the behavior for the non-principal to be, basically.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So making the non-principal a singleton and deny it access to itself is going to limit its usefulness. It would make it so that a function couldn't access objects it had itself created. This is fine in OOM cases, but pretty much no-where else.

Instead I propose that we make the non-principal behave much like the about:blank principal, but allow noone except chrome access to it. OOM can be handled separatly.

Another argument for not using the singleton approach and disallowing pointer comparisons is that although we could remove existing pointer comparisons, i suspect that they would pop up again over time.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #1)
> We could also use this in nsJSThunk::EvaluateScript if there is no owner. 

Agreed, but that case really should not happen.  Does it still?  Can you remind of under what conditions?  Thanks.

Dirty hack to make == tests ok: add %{C++ goo to nsIPrincipal that defines operator== and operator!= -- wotta hack!  Beats romping through our code doing mindless and error-prone search and replaces.

/be 

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Dirty hack to make == tests ok: add %{C++ goo to nsIPrincipal that defines
> operator== and operator!= -- wotta hack!  Beats romping through our code doing
> mindless and error-prone search and replaces.

Assuming we use a singleton that's != itself, of course.

For the cases sicking mentioned, we would want a non-prinicpal (null principal? nonce principal?) for every distinct document that couldn't find a better one to use.  That'd work.

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Agreed, but that case really should not happen.  Does it still?

Sure.  javascript: uris in stylesheets, etc, etc.

> Dirty hack to make == tests ok: add %{C++ goo to nsIPrincipal 

That won't help with pointer (not class!) compares.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> > Agreed, but that case really should not happen.  Does it still?
> 
> Sure.  javascript: uris in stylesheets, etc, etc.

Whe shouldn't the principal be the principal of the document that loaded the stylesheet?

> > Dirty hack to make == tests ok: add %{C++ goo to nsIPrincipal 
> 
> That won't help with pointer (not class!) compares.

Duh, my C++ brain fell out.  Let's not do the singleton thing anyway, unless we really believe it's necessary.  My doc-comment in nsIPrincipal.idl was abstract to a fault.  It's more useful to have least-powerful non-principals that don't match for different documents that we want to isolate.

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

I was thinking some more...  How about the following:

1)  In Init() all documents get a non-principal (allocated and non-singleton, so
    can fail, but then Init() fails) from the security manager and store it in
    mDefaultPrincipal and set it as their principal.
2)  Any time the document fails to set the principal (or any time the principal 
    is set to null), the principal is reset to mDefaultPrincipal.
3)  All non-document nodeinfo managers (createDocumentType comes to mind) do
    something similar.
4)  We allow non-principals access to themselves but nothing else and allow no
    one (including chrome?) access to non-principals.
5)  We make nsINode::NodePrincipal never return null and have the best of both
    worlds.

Thoughts?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Whe shouldn't the principal be the principal of the document that loaded the
> stylesheet?

No.  See XSS exploits and the recent LiveJournal mess.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #9)

sounds goo, except for:

> 4)  We allow non-principals access to themselves but nothing else and allow no
>     one (including chrome?) access to non-principals.

I don't see any harm in giving chrome access to non-principals. Additionally, it would be hard to deny chrome access since we have a lot of code that does

CheckAccess(nsIPrincipal* aSubject,....)
{
  if (aSubject == systemPrincipal)
    return NS_OK;
  ...

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #10)
> > Whe shouldn't the principal be the principal of the document that loaded the
> > stylesheet?
> 
> No.  See XSS exploits and the recent LiveJournal mess.

Yet script tags can src cross-domain and bring in code that runs, defines functions, etc.  This has been around for 9+ years (Netscape 3).  AJAX-y web apps and libraries use it to work around XMLHttpRequest's same-origin limitation.  It is a two-edged sword for sure, but so far more useful than harmful.

The LJ mess was manifold, and it showed problems on the content side (I'll avoid repeating the details here), as well as in the top two browsers.  It did not prove in general that linked resources that may contain script need to run that script under a non-principal.  Either the includer's origin or the resource's origin might be better than the non-principal.

Since <script src=> continues to subsume the included script in the includer's origin, something more, some new proposition or distinction, is needed for your argument here.  What is the missing ingredient?

I'm sincerely interested, because as things stand, we either have safety through crippling (XMLHttp same-origin; non-principal for too many hard cases; soon enough you can't do anything interesting), or scary but powerful features (<script src=>, JSONRequest or other new stuff requiring more analysis, or anything depending on users clicking OK to a scarily-worded dialog).

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Yet script tags can src cross-domain and bring in code that runs,

But sites know that.  So they can filter it out.  Sites don't expect background-image in CSS to run script with the site's security context.  Same for tons of other CSS properties.  Or for XUL trees (nor should they have to be aware of XUL trees to be secure!).  Oh, same for <xul:image>.  And <img src>.  And SVG patterns.  The list goes on and on.  We can't expect sites to defend against this stuff that we're adding all over, so we must not (and do not) run it with the page's principals.

If we implement some of Gerv's proposals for restricting what sort of script is allowed to run, maybe we can consider relaxing some of these restrictions for sites that indicate that it's ok to do it (opt-in, imho, if we don't want to make large chunks of the web exploitable in our browser).

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> > Yet script tags can src cross-domain and bring in code that runs,
> 
> But sites know that.  So they can filter it out.

Yes, although it's clear the list of IE or even cross-browser features that should be filtered from user-generated content is long and not perfectly known.

> Sites don't expect
> background-image in CSS to run script with the site's security context.

Why should we run any scripts from such CSS?

> Same for tons of other CSS properties.

Sounds like a problem, but why are non-principals the solution?

> Or for XUL trees (nor should they have to be
> aware of XUL trees to be secure!).  Oh, same for <xul:image>. 

I would hope XML would be stripped by sanitizers, but ok -- say someone uses remote XUL in HTML and uses JS to implement the tree view.  Why shouldn't that JS run as content of the same origin as the HTML?

> And <img src>. 
> And SVG patterns.  The list goes on and on.  We can't expect sites to defend
> against this stuff that we're adding all over, so we must not (and do not) run
> it with the page's principals.

Either it's included by the page, not injected through some bad path, why should we treat it differently?  I must be missing something.  How is this different from XBL or <script src=>?

> If we implement some of Gerv's proposals for restricting what sort of script is
> allowed to run, maybe we can consider relaxing some of these restrictions for
> sites that indicate that it's ok to do it (opt-in, imho, if we don't want to
> make large chunks of the web exploitable in our browser).

Our job is not to help user-generated content hosting sites sanitize content, although if we can make their lives easier and not hurt users, performance, footprint, etc., we should.

If we consider the well-hosted content model, where users cannot generate content that needs to be sanitized, I think we want something like the <script src=> model of inclusion (transclusion, whatever).  The MySpace or LJ situation, OTOH, leads to a different set of content policies.  We should try to separate these if we can (or else I am missing something).

/be 

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Why should we run any scripts from such CSS?

Good question.  We did not use to.  You argued that we should, since javascript: can return data.  So now we do.  ;)

> Sounds like a problem, but why are non-principals the solution?

They're _a_ solution.  The one we use right now.  Other solutions are possible.

> Why shouldn't that JS run as content of the same origin as the HTML?

That JS should.  javascript: URIs as the return value of getImageSrc() on the other hand?  Not sure.

> why should we treat it differently?

Because we can't tell.

> How is this different from XBL

It's not, and XBL is causing sites problems.

> or <script src=>?

Sites know to watch out for it.

> although if we can make their lives easier

That's all I'm saying.

Once we have support for a better method than non-principal to de-fang injected JS, we should use it.  See comment 13.

In any case, I don't think any of this affects how the non-principal should be implemented, does it?  I still crave responses to comment 9.  If we agree on that, we should start hacking.

Comment 16

[Brendan Eich [:brendan]]

Oh, sorry -- I thought the CSS background image case was something else, some other way of injecting script -- if the CSS itself uses a javascript: URL, I still want that to work just as it would if used directly as an <img src=> or whatever.

We run the risk of crippling everything to make MySpace etc. have an easier time. Then, assuming something doesn't go wrong for us, we get to implementing Gerv's proposal or something like it, and then we go back and replace non-principals with appropriate principals?  I'd rather figure out what we should do and minimize changes if we can.

This may be metabug fodder; sorry, not sure where else to say it.

As for comment 9, I thought sicking nailed it with comment 11.  I'm in favor -- start patching!

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK.  So what branches are we targeting with the non-principal?  Trunk and 1.8 and 1.7?

For anything but trunk, what URI, domain, and origin should it have, if any?  Note that since JS can't access principals it tends to use URIs for security checks.  :(  So we'd really like something that the security manager will treat as "the URI of the non-principal" or something.

On trunk I think we should make nsIPrincipal scriptable, expose a way to get them, and remove as much as possible of the stuff on nsIScriptSecurityManager that uses a URI as the subject.  And fix the callers.

So I propose what we do in this bug is decide on the uri/domain/origin thing and just implement non-principals.  Then file followup bugs for using them in specific places.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 19

[Boris Zbarsky [:bzbarsky]]

Notes on the patch:

1)  The URI random generation should be better (either fix bug 327161) or hack my 
    own.
2)  I decided that getting null principals by contract outside caps was fine, but 
    if people object I can add an nsPIScriptSecurityManager.
3)  I didn't implement a protocol handler for moz-nullprincipal:, but I could do
    that if desired.  It'd just return simple URIs and always fail to create
    channels.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 211906 [details] [diff] [review]
Proposed patch

Couple of nits and minor comments:

* Nat Friedman promoted this: instead of XXXbz or other XXX comments, write FIXME: bug# or buglink and file the bugs.  Don't leave unfiled problems noted by XXX comments.  XXX considered harmful.

* Fix the "non-principal" comments I wrote last year in nsIPrincipal.idl to reflect current reality.

Thanks for doing this.

/be

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to brendan's comments

Good point on XXX comments.  But MAN is filing bugs a pain.  The two extra stupid pages I have to go through to get to the "file a core bug" screen really don't help.  I wish Bugzilla UI let me add random links to it....

This points to the new bugs I filed and updates nsIPrincipal comments.  Brendan, want to sanity-check those?

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #21)
> The two extra stupid pages I have to go through to get to the "file a core bug"
> screen really don't help.  I wish Bugzilla UI let me add random links to it....

Bug 322327 was recently fixed for b.m.o, so you can set a pref to always get the full page.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Minor tweak so it compiles again

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 211952 [details] [diff] [review]
Minor tweak so it compiles again

r=mrbkap

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 211952 [details] [diff] [review]
Minor tweak so it compiles again

>Index: caps/include/nsNullPrincipal.h
>===================================================================
>+/**
>+ * This is the principal that has no rights and can't be accessed by
>+ * anything other than itself and chrome.  Null principals are not
>+ * same-origin with anything but themselves.
>+ */

Please repeat this description in the .cpp file for LXR

>+  // will return false for us, since we're not about:blank and not Equals to
>+  // reasonably nsPrincipals.

ObNit: "reasonable"

>+    // Allow access to about:blank, except to null principals.  If SchemeIs
>+    // fails, just deny access -- better safe than sorry.

This confused me for a bit. At the very least "except FROM null principals",
perhaps expanding with "which never have access to anything but themselves".

sr=dveditz

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to trunk

Comment 27

[Boris Zbarsky [:bzbarsky]]

Checked in.  Thanks for the reviews!

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Comment change that somehow got lost with all the merging

I just checked this in too.

Comment 29

[jag (Peter Annema)]

This increased Tp on btek by 3 or 4 ms. Prolly not much that can be done about it, huh.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yeah, esp. because I don't see how this patch would have affected Tp unless our Tp tests create javascript: URIs in stylesheets or some such....  :(