332840 - [FIX]DOMParser gets wrong principal if no JS on stack (reload all live bookmarks produces security error in console)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Henrik Gemal]

When I reload all live bookmarks I see:

Security Error: Content at moz-nullprincipal:{28d3a532-b53f-49be-a030-f1894af42d9d} may not load or link to http://gemal.dk/blog/feeds/index.xml.

not sure what it means. I just reloaded my love bookmark

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060404 Firefox/1.6a1

Comment 1

[Mike Connor [:mconnor]]

I doubt this is specific to places...

Comment 2

[Boris Zbarsky [:bzbarsky]]

I doubt it too, but that's about all I can say without actual steps to reproduce.  I mean step-by-step ones for someone who doesn't use Firefox, starting with a clean Firefox install an a clean profile.

I'm also kinda curious about whether you run into this problem in builds from before "2006-04-02 13:58" (when we didn't have null principals and used about:blank in various cases instead).

Comment 3

[Henrik Gemal]

(In reply to comment #2)
> I'm also kinda curious about whether you run into this problem in builds from
> before "2006-04-02 13:58" (when we didn't have null principals and used
> about:blank in various cases instead).

The build from 2006-04-02 dont have the reload all live bookmarks function. So I cant really test

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK. When was that button added?  And I still need steps to reproduce...

Comment 5

[Phil Ringnalda (:philor)]

Reload was bug 329634, so 20060403 17:42. As for STR (guessing, since I haven't been able to reproduce it myself), maybe create a live bookmark through Bookmarks > Manage Bookmarks > File > New Live Bookmark in a non-Places build, so you get one without a siteURI when you then import that profile into a Places build, making this another case of bug 332613? Dunno, I won't be able to test that guess until tonight at the earliest.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, so I reproduced this error on startup, presumably for the same reasons.

The problem is that nsBookmarksFeedHandler.cpp uses DOMParser from C++ without any JS on the stack.  DOMParser really doesn't deal well with this use case, since it relies on the JS context stack to tell it who's calling it and hence what security context it should give the document it's creating.  See in particular http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/xmlextras/base/src/nsDOMParser.cpp&rev=1.47#280

So we create a document with a null principal but try to set a base URI on it, which is not kosher.  And security manager blocks it.

We used to just use the baseURI for the principal in this case, and we can probably change DOMParser to do so, but that seems pretty bogus... we should really have a version of DOMParser for C++ use that explicitly takes a principal to work with, imo.  Or an easy way for callers to push a principal on the stack.  Or something.

Comment 7

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

Could we possibly give nsIDOMParser an init method that takes an nsIPrincipal and then make nsDOMClassInfo call that?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm... That would work, yes...  And I guess make the method noscript?

Comment 9

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

Yeah. Are there any other objects we should do the same thing with? XMLHttpRequest?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Mmm....  I'd rather leave XMLHttpRequest separate.  I do think we want to do something similar to it, but it's so tied to script contexts in general, that I simply consider it unusable from C++ right now.  It'll need a heck of a lot more work to be usable.  :(

Comment 11

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

Ok with me.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Per IRC suggestion from shaver, the current plan is the following:

1)  Require init(nsIPrincipal) to be called on a DOMParser to have it be usable.
2)  Make init() noscript.
3)  Allow new DOMParser(principal) from trusted script to allow parsing into a
    downgraded-privilege document.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Very incomplete patch

I've run into a slight hitch.  :(  Sadly, I don't think JS components can use "new DOMParser()".  Which means they all need to start calling init().  Or maybe I'm wrong and we just need to switch them from createInstance to the constructor?  But I doubt they'd be using createInstance then...

If people think that's acceptable, I'll go ahead and make that change.  Just attaching the patch here because it doesn't compile/run nicely yet and I need that tree for something else.

Comment 14

[Bill Gianopoulos [:WG9s]]

Changing component as this has absolutely nothing to do with places.  I get the same error on a non-places build.

Comment 15

[Mike Shaver (:shaver emeritus)]

I'd be more than happy to see the DOMParser constructor exposed to JS components, and I think there's already a bug on file to do that for XMLHttpRequest.  Doing it without making xpconnect depend on DOM or whatever requires either some mildly fancy hook addition, or some mildly ugly #ifdefing and exporting of some utility functions from the parser and XHR components.  (Specifically: utility functions that create the appropriate JS constructors given a context and global.)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Another possibility is to just use a null principal (and no base uri) by default (so if nothing is specified).  And make init() optional.  Thay may be the most compatible way to go...

Comment 17

[Boris Zbarsky [:bzbarsky]]

So I've run into a bit of a dilemma.  I can certainly fix this bug, by either giving the null principal and no base URI (and about:blank as document URI?) to DOMParser unless told otherwise or giving it about:blank (as both URI and principal).

The problem with doing the former (and to some extent the latter) is that if someone parses a DOM that contains, say, and image pointing to some URI we'll get that security error (because the security check is done before the data document content policy check).  For the null principal this will happen no matter what the image URI is; for about:blank, HTTP URIs won't trigger it (because about:blank can load HTTP stuff), but others could.

Are we ok with these spurious errors?  I'm really not happy with calling content policies before calling CheckLoadURI, and short of doing that I see no way to avoid them.  :(

Comment 18

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

It might be ok, or even a good idea, to require that JS components call .init(). But if we do, I think that we should throw unless init is called, not fallback to 'about:blank' or null-principal.

Comment 19

[Boris Zbarsky [:bzbarsky]]

One problem is that for a JS component it's not clear where they would get a principal...  What principal should they use, exactly?  The obvious ones are the null principal, a principal for some codebase (which one?), and the system principal.  Which should they pick?  C++ code has a similar issue, really.

And the real issue with doing that is that it'll break all current component consumers; we have 4 or 5 in the tree right now, and probably some in various extensions and so forth.  I'd really not break them all if we can avoid it.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

For what it's worth, the C++ WebDAV (and therefore CalDAV) code triggers this bug too.  

Comment 21

[Mike Shaver (:shaver emeritus)]

(In reply to comment #17)
> I'm really not happy with calling
> content policies before calling CheckLoadURI, and short of doing that I see no
> way to avoid them.  :( 

Forgive my naivete: why don't we want to call content policies before CLU?  Doesn't seem like the content policies are allowed to care (the later CLU is like a later content policy), and it seems harmless to ask a CP about a load that CLU will later veto.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> why don't we want to call content policies before CLU? 

I'm paranoid about content policies that cancel that load and then load the URI themselves or some such sick thing....  In any case, fixing all consumers of CheckLoadURI to call content policy first is a major pain and hard to maintain/enforce.

Comment 23

[Phil Ringnalda (:philor)]

*** Bug 338657 has been marked as a duplicate of this bug. ***

Comment 24

[Phil Ringnalda (:philor)]

-> default QA for component

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Another incomplete patch using classinfo in case I need it

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

I tried to avoid the second interface, but it was just too painful to get right...

Comment 27

[Doron Rosenberg (IBM)]

bz: when one does parser->Init(nsnull, uri, nsnull); - which principal does the parser get?  The one for the uri?

Comment 28

[Boris Zbarsky [:bzbarsky]]

From the patch:

+   * @param principal The principal to use for documents we create.
+   *                  If this is null, a codebase principal will be created
+   *                  based on documentURI; in that case the documentURI must
+   *                  be non-null.

Comment 29

[aaronr]

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

>Index: extensions/xforms/nsXFormsSubmissionElement.cpp
>===================================================================
>RCS file: /home/bzbarsky/mozilla/cvs-mirror/mozilla/extensions/xforms/nsXFormsSubmissionElement.cpp,v
>retrieving revision 1.69
>diff -u -p -d -8 -r1.69 nsXFormsSubmissionElement.cpp
>--- extensions/xforms/nsXFormsSubmissionElement.cpp	26 Jul 2006 23:25:20 -0000	1.69
>+++ extensions/xforms/nsXFormsSubmissionElement.cpp	23 Aug 2006 03:25:29 -0000
>@@ -495,17 +495,19 @@ nsXFormsSubmissionElement::LoadReplaceIn
>   mPipeIn->Available(&contentLength);
> 
>   // set the base uri so that the document can get the correct security
>   // principal (this has to be here to work on 1.8.0)
>   // @see https://bugzilla.mozilla.org/show_bug.cgi?id=338451
>   nsCOMPtr<nsIURI> uri;
>   nsresult rv = channel->GetURI(getter_AddRefs(uri));
>   NS_ENSURE_SUCCESS(rv, rv);
>-  rv = parser->SetBaseURI(uri);
>+
>+  // XXXbz is this the right principal?
>+  rv = parser->Init(nsnull, uri, nsnull);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsCOMPtr<nsIDOMDocument> newDoc;
>   parser->ParseFromStream(mPipeIn, contentCharset.get(), contentLength,
>                           "application/xml", getter_AddRefs(newDoc));
>   // XXX Add URI, etc?
>   if (!newDoc) {
>     nsXFormsUtils::ReportError(NS_LITERAL_STRING("instanceParseError"),

I think that you are using the right uri and I tested XForms with this patch and we work the same as we do without the patch (i.e. if domain is whitelisted we still cross domain submit, if it isn't whitelisted, we don't cross domain submit).

Comment 30

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

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

>Index: content/base/src/nsDOMParser.cpp
>@@ -182,61 +185,45 @@ nsDOMParser::ParseFromStream(nsIInputStr
>   *aResult = nsnull;
> 
>   // For now, we can only create XML documents.
>   if ((nsCRT::strcmp(contentType, "text/xml") != 0) &&
>       (nsCRT::strcmp(contentType, "application/xml") != 0) &&
>       (nsCRT::strcmp(contentType, "application/xhtml+xml") != 0))
>     return NS_ERROR_NOT_IMPLEMENTED;
> 
>-  // Put the nsCOMPtr out here so we hold a ref to the stream as needed
>   nsresult rv;
>+  if (!mPrincipal) {
>+    NS_ENSURE_TRUE(!mAttemptedInit, NS_ERROR_NOT_INITIALIZED);
>+    mAttemptedInit = PR_TRUE;

Won't this cause Init to bail early?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Er, yes.  I have that problem in a few places.  I'll fix.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: With that issue fixed

Comment 33

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

This version still has the same problem at the top of nsDOMParser::Init().

Why do we need a no-argument init() at all? Is it only in contexts where you can't do |new DOMParser|?

Comment 34

[Boris Zbarsky [:bzbarsky]]

> This version still has the same problem at the top of nsDOMParser::Init().

Indeed.  I'll fix.

> Why do we need a no-argument init() at all?  Is it only in contexts where you
> can't do |new DOMParser|?

Yes.  Like any JS component.  Well, that and it behaves differently from new DOMParser() in that it does NOT fall back to the caller principal (which would be chrome in this case anyway).

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the issue sicking pointed out, and actually do the security check we should have been doing to make sure our args are not fake or anything.

Comment 36

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

Comment on attachment 236487 [details] [diff] [review]
Fix the issue sicking pointed out, and actually do the security check we should have been doing to make sure our args are not fake or anything.

>Index: content/base/src/nsDOMParser.cpp
>+NS_IMETHODIMP
>+nsDOMParser::Init(nsIPrincipal* principal, nsIURI* documentURI,
>+                  nsIURI* baseURI)

Prefix arguments with 'a'.

>+GetInitArgs(JSContext *cx, PRUint32 argc, jsval *argv,
>+            nsIPrincipal** aPrincipal, nsIURI** aDocumentURI,
>+            nsIURI** aBaseURI)
> {
>-  mBaseURI = aBaseURI;
>+  NS_ASSERTION(!*aPrincipal && !*aDocumentURI && !*aBaseURI,
>+               "swap() will not work");

It doesn't really seem worth it to use swap here just to save cycles

r=sicking with that

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Prefix arguments with 'a'.

I could, but I actually followed file style here... (I had them prefixed first, then undid it to match the rest of the file.)  Let me know if you still want the prefixing.

> It doesn't really seem worth it to use swap here just to save cycles

It saves codesize too...  And there's really no reason not to use it, more importantly.  Again, let me know if you still want me to stop using swap() here.

Comment 38

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

I'd rather not use swap, but it's not a big deal.

Good point about the arguments. Up to you.

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 236487 [details] [diff] [review]
Fix the issue sicking pointed out, and actually do the security check we should have been doing to make sure our args are not fake or anything.

+  class AttempedInitMarker {

That's missing a 't', Attemp_t_edInitMaker, right?

sr=jst

Comment 40

[Johnny Stenback (:jst)]

I'm not going to argue either way regarding the use of swap() here, but I will point out that it's potentially not a safe thing to use unless you *know* that what you're swap'ing into the nsCOMPtr is either null or a strong reference needing a release (in the case of an in-out param). The uses here look safe tho.

Comment 41

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

The only reason i have for not using swap is that it's safer as far as future code goes. But as i said, it's no biggie.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Without swap(), with fixed typo

This is what I checked in.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 44

[u60234]

Building with disable-places is now broken.

nsBookmarksFeedHandler.cpp
d:/Cvs/L10n/mozilla/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp(628) : error C2039: 'SetBaseURI' : is not a member of 'nsDerivedSafe<T>'
        with
        [
            T=nsIDOMParser
        ]

Comment 45

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix for --disable-places

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 237634 [details] [diff] [review]
Fix for --disable-places

Checked this in

Comment 47

[Eric Shepherd [:sheppy]]

Looking over this patch, it looks like what needs documenting are the nsIDOMParser and nsIDOMParserJS interfaces, which had some changes in this patch (but aren't documented at all yet).  Correct?

Comment 48

[Boris Zbarsky [:bzbarsky]]

The API changes would be good, but what really needs documenting are the changes to how callers can (or must) instantiate DOMParsers and initialize them before they become usable.  I guess there was never a clear description of the new behavior in this bug.  Here's an attempt:

* When a DOMParser is instantiated by calling |new DOMParser()| on some
  window, the DOMParser gets the principal of the calling code and the
  documentURI and baseURI of the window the constructor came from.
* If the caller has UniversalXPConnect privileges, it can pass arguments to
  |new DOMParser()|.  The first argument is the principal to use, the second
  is the document URI, the third is the base URI.  The caller may pass only
  one or two arguments, in which case the remaining ones will be defaulted
  to null.
* In both of the above cases, DOMParser will handle calling init() on itself
  with the values described above.  The behavior of that is the API
  documentation for init().
* If instantiating a DOMParser via the contract (createInstance, etc), the
  caller must call init().  The behavior is as documented in the API.  This
  affects JS components and C++ callers.

The requirement that init() be called somehow and the ability for UniversalXPConnect code to easily downgrade its principal by passing a different one to |new DOMParser()| are the big changes in this bug.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Actually, correction.  We got rid of the requirement that init() be called.  If it's never called, then attempts to parse will create a null-principal and init with that and null pointers for the documentURI and baseURI.

Comment 50

[Eric Shepherd [:sheppy]]

This article has been updated.  Please re-mark as doc needed if this doesn't satisfactorily address the issues.

http://developer.mozilla.org/en/docs/DOMParser

Comment 51

[Boris Zbarsky [:bzbarsky]]

That looks great.  Thanks, Eric!

Comment 52

[R Pruitt]

Since I'm not sure if this is by design, I'm raising it here.  When called from chrome, the following

var xmlBody = new DOMParser().parseFromString("<p>hello world</p>","text/xml");

gives the error

Security Error: Content at moz-nullprincipal:{61852779-dde6-4443-beac-95e801b1bd80} may not load or link to chrome://newsfox/content/newsfox.xul.

From the discussion here, this can be fixed by

var iosvc = Components.classes["@mozilla.org/network/io-service;1"]
     .getService(Components.interfaces.nsIIOService);
var artURI = iosvc.newURI("about:blank", null, null);
var xmlBody = new DOMParser(null,artURI,null)
     .parseFromString("<p>hello world</p>","text/xml");

But is this really what you wanted(the error that is)?  Chrome can be trusted to put content into chrome directly from javascript, but not from parseFromString().

Comment 53

[Boris Zbarsky [:bzbarsky]]

R Pruitt, which build are you seeing that in?  That should not be happening.

Comment 54

[R Pruitt]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041206 Minefield/3.0pre

Remember, I'm just trying to make sure my extension works, not trying to keep up with the bleeding edge :)  I'll download the latest and see what happens.  It says security error, but it is just an information warning: it still does the parsing.

Unnecessary information: I'm glad it got in there temporarily, since now I use a documentURI based on the page(not "hello world"/about:blank) I'm testing for XHTMLness, and potential XML errors(that don't get suppressed in try-catch) will show the offending URI rather than my chrome address.  I never would have figured out how to do that.

Comment 55

[R Pruitt]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041807 Minefield/3.0pre

Still happens when parsing "<p>hello world</p>"



Offtopic?? problem in 080418 build and not 080412, but folks here will have a better idea what to do with this (probably more widespread problem).  Otherwise I'm tempted to just put the data in a data: URI and avoid the issue rather than trying to figure out if it is at this late date my problem or a bug and where to submit it.

Setup: an XUL <iframe> subelement of an XUL <window> (chrome) where the iframe content is built with createElement and appendChild.  "doc" in the following is iframe.contentDocument.

  var img = doc.createElement("img");
  img.setAttribute("src","chrome://newsfox/skin/images/weblink.png");

gives the following in console and "src" is not set:

Security Error: Content at about:blank may not load or link to chrome://newsfox/skin/images/weblink.png.

Comment 56

[Boris Zbarsky [:bzbarsky]]

The second half of comment 55 is covered by http://starkravingfinkle.org/blog/2008/04/extension-developers-chrome-uri-changes-you/

The error described in comment 52 is a recent regression from bug 421228.  I've filed bug 429785 on it.  Thanks for bringing it up!

Comment 57

[R Pruitt]

Tons of thanks for the response in comment 56.  These things are hard to find for extension developers, and of course I can't even see bug 421228.  Personally, I'm much more likely to file a bug if I can figure out a proper place to put it where it will get looked at.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Makes sense.  It's hard to tell with security stuff sometimes what's really a bug and what's a necessary security measure.  :(