342274 - implement alternative nsIXSLTProcessor interface without access to document loading

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

For better security when transforming local but not entirely trusted XSLT stylesheets, the XSLT processor should provide an interface which doesn't have access to document loading.

Comment 1

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

I suggest we add a SetFlags method that disables the processor from loading any external documents. You would then use the nomral methods on nsIXSLTProcessor to do the transformations.

Comment 2

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

Attachment: Patch to fix

Comment 3

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

Attachment: Complete patch to fix

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 227782 [details] [diff] [review]
Complete patch to fix

>Index: content/xslt/public/nsIXSLTProcessorPrivate.idl
>===================================================================

>+[scriptable, uuid(b8d727f7-67f4-4dc1-a318-ec0c87280816)]
>+interface nsIXSLTProcessorPrivate : nsISupports
>+{
>+	/**
>+	 * Disables all loading of external documents, such as from
>+	 * <xsl:import> and document()
>+	 * Defaults to off and is *not* reset by calls to reset()
>+	 */
>+	const unsigned long DISABLE_ALL_LOADS = 1;

TABS!!!!!!!!

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

> class txSyncCompileObserver : public txACompileObserver

> protected:
>+    txMozillaXSLTProcessor* mProcessor;

Make that an nsRefPtr.

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

>@@ -338,17 +339,17 @@ txMozillaXSLTProcessor::TransformDocumen

>-    nsresult rv = TX_CompileStylesheet(aStyleDOM, getter_AddRefs(mStylesheet));
>+    nsresult rv = TX_CompileStylesheet(aStyleDOM, this, getter_AddRefs(mStylesheet));

Long line.

>@@ -589,17 +590,17 @@ txMozillaXSLTProcessor::ImportStylesheet

>-    nsresult rv = TX_CompileStylesheet(aStyle, getter_AddRefs(mStylesheet));
>+    nsresult rv = TX_CompileStylesheet(aStyle, this, getter_AddRefs(mStylesheet));

Long line.

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

>+    PRBool DisableLoads()
>+    {
>+        return (mFlags & DISABLE_ALL_LOADS) != 0;
>+    }

IsLoadDisabled


Nice patch! Some of your better work!

Comment 5

[Myk Melez [:myk] [@mykmelez]]

In preliminary testing, it looks like I'm seeing the same behavior whether I set the flag or not.  For example, a microsummary generator for Merriam-Webster's word of the day page that tries to load a remote file via document() fails with the following error in both cases:

Security Error: Content at http://www.m-w.com/cgi-bin/mwwod.pl may not load data from http://people.mozilla.com/~myk/microsummaries/generators/fedex-track-package.xml

If that generator tries to include or import a file, the processor throws the following error in both cases:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80600004 [nsIXSLTProcessor.transformToFragment]"  nsresult: "0x80600004 (<unknown>)"  location: "JS frame :: file:///home/myk/Projects/minefield/mozilla/dist/bin/components/nsMicrosummaryService.js :: MSD__processTemplate :: line 1002"  data: no]
************************************************************

The document() error seems unrelated to the changes, but it seems like the import/include error shouldn't happen unless I set the flag.  I'll investigate further.

Comment 6

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

The document() error seems to be because we check the origin against the source document. So if you let http://example.com/index.html use a generator (installed or linked) that contains document('http://example.com/test.xml') you will probably not get an error.

Does the import/include error happen even without the patch?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> The document() error seems to be because we check the origin against the source
> document. So if you let http://example.com/index.html use a generator
> (installed or linked) that contains document('http://example.com/test.xml') you
> will probably not get an error.

Indeed, if I don't apply the patch, or if I apply the patch but don't set the flag, I don't get an error, and the document function works.


> Does the import/include error happen even without the patch?

It did, but I'm not convinced of my results.  Unfortunately I'm now having trouble reproducing them, as the following code throws an NS_NOINTERFACE exception:

     // XXX Should we just have one global instance of the processor?
     var processor = Cc["@mozilla.org/document-transformer;1?type=xslt"].
                     createInstance(Ci.nsIXSLTProcessor);
+
+    // Turn off document loading of all kinds (document(), <include>, <import>).
+    var processorPrivate = processor.QueryInterface(Ci.nsIXSLTProcessorPrivate);
+    processorPrivate.flags |= Ci.nsIXSLTProcessorPrivate.DISABLE_ALL_LOADS;
+
     processor.importStylesheet(this.template);
     var fragment = processor.transformToFragment(doc, doc);
 
I'll investigate further tomorrow.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: make component QIable to interface and update to trunk

I looked around for code that references nsIXSLTProcessorObsolete but doesn't reference nsIXSLTProcessorPrivate, thinking that perhaps the latter isn't declared somewhere it needs to be.  There are a few places like that in the code:

1. content/xslt/src/xslt/txMozillaXSLTProcessor.cpp, line 302:

http://lxr.mozilla.org/mozilla/source/content/xslt/src/xslt/txMozillaXSLTProcessor.cpp#302

2. dom/src/base/nsDOMClassInfo.cpp, lines 199 and 2982:

http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#199
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#2982

If I add equivalent code for nsIXSLTProcessorPrivate to those places and rebuild, I no longer get the NS_NOINTERFACE error (not sure they're all needed, but haven't tested each line individually, as I figure Jonas will know).  Here's a patch that includes those fixes and has also been updated to work with recent trunk changes.

Now that it works, I'll start testing its use in the microsummary service.

Comment 9

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

Attachment: With security checks

This is the same as myks patch, but adds security checks to prevent anyone but chrome from getting or setting flags.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

With the patch applied, if the microsummary service doesn't set the flag, document() works.  If it sets the flag, document() silently fails.  But import and include throw the same exception whether or not the flag is set: 0x8060001b, which Mozilla Error Lookup (http://twpol.dyndns.org/mozilla/misc/nserror) claims is NS_ERROR_XSLT_LOAD_BLOCKED_ERROR.  So it's not clear that the flag is working for those elements.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch for microsummary service to use new API

With help from Jonas, I corrected my testcase, and now I do see the correct behavior.  Without the flag set, a local generator can import/include a local file.  With the flag set, it cannot.

Here's a patch for the microsummary service that makes it use the new API when importing a stylesheet.

Comment 12

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

Comment on attachment 228229 [details] [diff] [review]
patch for microsummary service to use new API

>Index: browser/components/microsummaries/src/nsMicrosummaryService.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js.in,v
>retrieving revision 1.20
>diff -u -r1.20 nsMicrosummaryService.js.in
>--- browser/components/microsummaries/src/nsMicrosummaryService.js.in	5 Jul 2006 20:23:57 -0000	1.20
>+++ browser/components/microsummaries/src/nsMicrosummaryService.js.in	6 Jul 2006 00:17:51 -0000
>@@ -1234,6 +1234,12 @@
>     // XXX Should we just have one global instance of the processor?
>     var processor = Cc["@mozilla.org/document-transformer;1?type=xslt"].
>                     createInstance(Ci.nsIXSLTProcessor);
>+
>+    // Turn off document loading of all kinds (document(), <include>, <import>)
>+    // for security (otherwise local generators would be able to load local files).
>+    var processorPrivate = processor.QueryInterface(Ci.nsIXSLTProcessorPrivate);
>+    processorPrivate.flags |= Ci.nsIXSLTProcessorPrivate.DISABLE_ALL_LOADS;
>+

Actually, you don't need the QueryInterface call any more, with the way that we ended up doing security.

Comment 13

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

Attachment: With security checks

Opps, diffed wrong tree.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: use new API, no need to QI

Per Jonas, checked in the patch to the trunk without the unnecessary QI.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Both fixes checked in to trunk.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Could this be responsible for regressing Tp on btek?

Comment 17

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

There is no way that the code I checked in (which is when the Tp regression happened) could be run during the Tp tests. And even if it was there is no way it could affect performance. I suspect it's just a fluke or that the code in gklayout got ordered differently or some such.

Comment 18

[Darin Fisher]

fwiw: noticed tabs in the IDL file

Comment 19

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

Attachment: branch version

This is what i'm landing on the 1.8 branch

Comment 20

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

Attachment: branch version

Grr.. forgot to do a final diff

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 228362 [details] [diff] [review]
branch version

In testing, this patch works as advertised.  I should note that <include> and <import> fail silently on the branch, with or without the patch, but with the patch applied and the flag set, they throw an exception, as expected.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Both fixes checked in to branch.