284225 - XSLT uses wrong security context for security checks

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Raoul Nakhmanson-Kulish]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041217
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041217

If I create XML document from string via DOMParser.parseFromString, then attempt
to transform it via XSLT which contains document() function to refer to XML
placed on site I see security errors on JS console:
Security Error: Content at about:blank may not load data from
http://myserver/user_types.xml.

Also I see security error if I try serializeToString with XML document created
from string:
Error: uncaught exception: [Exception... "Access to restricted URI denied" 
code: "1012" nsresult: "0x805303f4 (NS_ERROR_DOM_BAD_URI)"  location:
"http://myserver/js/parser.js Line: 76"]

Seems that XML document created from string is being interpreted in
"about:blank" context. I guess, this isn't right.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Boris Zbarsky [:bzbarsky]]

Please attach a testcase showing the problem?  Attach the xslt first, then point
your other code to the bugzilla XSLT attachment and attach it?

Comment 2

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 3

[Gervase Markham [:gerv]]

This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.

Comment 4

[Raoul Nakhmanson-Kulish]

Bug still persists.

Comment 5

[Boris Zbarsky [:bzbarsky]]

See comment 1 -- please attach a testcase showing the bug.  It's very hard to
test a fix otherwise.

That said, the problem is that XSLT is using CheckLoadURI on the document URI
(which is correctly about:blank for DOMParser-created stuff, imo) instead of
using CheckLoadURIWithPrincipal on the document principal (which is the
creator's principal, or should be -- we set it as the owner on the channel).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sicking, peterv, see comment 5.

Comment 7

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

Raoul: We're still missing a testcase from you. It is not going to be possible to properly test a fix without it.

Comment 8

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

Attachment: Patch to fix

This also fixes bug 391298, as well as converts some code from using nsIDOMNode to using nsINode

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 278684 [details] [diff] [review]
Patch to fix

>Index: content/xslt/src/xslt/txMozillaStylesheetCompiler.cpp
>===================================================================

>+CheckLoadURI(nsIURI *aUri, nsIPrincipal *aReferrerPrincipal,
>+             nsISupports *aContext)

Could we just replace CheckLoadURI with nsContentUtils::CheckSecurityBeforeLoad?

>@@ -759,26 +743,33 @@ txSyncCompileObserver::loadURI(const nsA

>-    rv = nsSyncLoadService::LoadDocument(uri, referrerUri, nsnull, PR_FALSE,
>-                                         getter_AddRefs(document));
>+    rv = nsSyncLoadService::LoadDocument(uri, referrerUri, nsnull,
>+                                         PR_FALSE, getter_AddRefs(document));

Pointless wrapping change?


>+TX_CompileStylesheet(nsINode* aNode, txMozillaXSLTProcessor* aProcessor,
>                      nsIPrincipal* aCallerPrincipal,
>                      txStylesheet** aStylesheet)

>+    if (aNode->IsNodeOfType(nsINode::eCONTENT)) {
>+      uri = static_cast<nsIContent*>(aNode)->GetBaseURI();
>     }
>-    else {
>-        doc = do_QueryInterface(aNode);
>-        NS_ASSERTION(doc, "aNode should be a doc or an element by now");
>-
>-        uri = doc->GetBaseURI();
>+    else if (aNode->IsNodeOfType(nsINode::eDOCUMENT)) {

I think a simple else with NS_ASSERTION(aNode->IsNodeOfType(nsINode::eDOCUMENT), ...) would be ok here.

>Index: content/xslt/src/xslt/txMozillaXSLTProcessor.cpp
>===================================================================

>+    nsCOMPtr<nsINode> styleNode = do_QueryInterface(styleNode);

Did this even compile?

Please also attach a diff -w next time.

We should have tests for this stuff too.

Comment 10

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

Attachment: Patch v2

Addresses comments. I'm still working on a testcase for this.

Comment 11

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

Attachment: Patch v2 -w

Same as above, but with -w

Comment 12

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

Checked in