Last Comment Bug 284225 - XSLT uses wrong security context for security checks
: XSLT uses wrong security context for security checks
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal with 1 vote (vote)
: ---
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
: Keith Visco
Mentors:
Depends on: 342487
Blocks: 391298
  Show dependency treegraph
 
Reported: 2005-03-01 02:05 PST by Raoul Nakhmanson-Kulish
Modified: 2007-09-27 15:30 PDT (History)
4 users (show)
jonas: blocking1.9+
jonas: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix (23.97 KB, patch)
2007-08-28 17:34 PDT, Jonas Sicking (:sicking) PTO Until July 5th
peterv: review-
Details | Diff | Splinter Review
Patch v2 (24.26 KB, patch)
2007-09-12 15:54 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Splinter Review
Patch v2 -w (21.84 KB, patch)
2007-09-12 15:55 PDT, Jonas Sicking (:sicking) PTO Until July 5th
peterv: review+
peterv: superreview+
Details | Diff | Splinter Review

Description Raoul Nakhmanson-Kulish 2005-03-01 02:05:32 PST
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 [:bz] 2005-03-03 10:38:58 PST
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] 2005-09-27 01:59:20 PDT
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] 2005-10-13 10:29:58 PDT
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 2005-10-13 22:09:02 PDT
Bug still persists.
Comment 5 Boris Zbarsky [:bz] 2005-10-15 06:40:11 PDT
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 [:bz] 2005-10-15 06:40:52 PDT
Sicking, peterv, see comment 5.
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2007-08-01 15:56:41 PDT
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) PTO Until July 5th 2007-08-28 17:34:34 PDT
Created attachment 278684 [details] [diff] [review]
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] - away till Aug 1st 2007-09-02 06:32:38 PDT
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) PTO Until July 5th 2007-09-12 15:54:53 PDT
Created attachment 280671 [details] [diff] [review]
Patch v2

Addresses comments. I'm still working on a testcase for this.
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2007-09-12 15:55:44 PDT
Created attachment 280672 [details] [diff] [review]
Patch v2 -w

Same as above, but with -w
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2007-09-27 15:30:51 PDT
Checked in

Note You need to log in before you can comment on or make changes to this bug.