[FIX]Implement a powerless non-principal

RESOLVED FIXED in mozilla1.9alpha1

Status

()

P1
normal
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

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?
We could also use this in nsJSThunk::EvaluateScript if there is no owner. 
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;
> 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.
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.
(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 

(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
> 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.
(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
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?
> Whe shouldn't the principal be the principal of the document that loaded the
> stylesheet?

No.  See XSS exploits and the recent LiveJournal mess.
(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;
  ...
(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
> 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).
(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 
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
> 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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

Updated

13 years ago
Blocks: 301375
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.
Created attachment 211906 [details] [diff] [review]
Proposed patch
Assignee: dveditz → bzbarsky
Status: REOPENED → ASSIGNED
Attachment #211906 - Flags: superreview?(dveditz)
Attachment #211906 - Flags: review?(mrbkap)
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.
Priority: -- → P1
Summary: Implement a powerless non-principal → [FIX]Implement a powerless non-principal
Target Milestone: --- → mozilla1.9alpha
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
Created attachment 211949 [details] [diff] [review]
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?
Attachment #211906 - Attachment is obsolete: true
Attachment #211949 - Flags: superreview?(dveditz)
Attachment #211949 - Flags: review?(mrbkap)
Attachment #211906 - Flags: superreview?(dveditz)
Attachment #211906 - Flags: review?(mrbkap)
(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.


Created attachment 211952 [details] [diff] [review]
Minor tweak so it compiles again
Attachment #211949 - Attachment is obsolete: true
Attachment #211952 - Flags: superreview?(dveditz)
Attachment #211952 - Flags: review?(mrbkap)
Attachment #211949 - Flags: superreview?(dveditz)
Attachment #211949 - Flags: review?(mrbkap)
Comment on attachment 211952 [details] [diff] [review]
Minor tweak so it compiles again

r=mrbkap
Attachment #211952 - Flags: review?(mrbkap) → review+
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
Attachment #211952 - Flags: superreview?(dveditz) → superreview+
Created attachment 216984 [details] [diff] [review]
Merged to trunk
Attachment #211952 - Attachment is obsolete: true
Checked in.  Thanks for the reviews!
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
Created attachment 216985 [details] [diff] [review]
Comment change that somehow got lost with all the merging

I just checked this in too.

Comment 29

13 years ago
This increased Tp on btek by 3 or 4 ms. Prolly not much that can be done about it, huh.
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....  :(

Updated

13 years ago
Depends on: 335549
You need to log in before you can comment on or make changes to this bug.