325814 - Instance document loads don't do security or content policy checks

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Boris Zbarsky [:bzbarsky]]

nsXFormsInstanceElement::LoadExternalInstance doesn't do security or content policy checks.  This means it can be used for things ranging from hanging the user's browser to crashing the user's OS to reading data off the user's hard drive.

This should really be fixed, trunk and branches...  Was this code on the 1.7 branch?

Comment 1

[Doron Rosenberg (IBM)]

Note that XForms never shipped as part of Mozilla or Firefox.  And no, this wasn't on 1.7.

Comment 2

[Doron Rosenberg (IBM)]

We actually do have a security check, http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsUtils.cpp#1110.

But:

bz	so that still needs to do a content policy check
bz	but it doesn't need to checkloaduri, I guess
bz	fwiw, on trunk if this did a content policy check and after this one patch sicking and I are planning you wouldn't have to worry about instance documents loading other instances... ;)
bz	no, content policy check being nsIContentPolicy
bz	ShouldLoad
doron	right now we use IsCapabilityEnabled because we have some crazu russian compay using chrome xforms/xul apps. So the nsIPrincipal stuff can be replaced with the contentpoilicy?
bz	no
bz	the two are orthogonal
bz	content policy allows extensions to register to disallow loads
bz	eg we do our image disabling that way
bz	and some other stuff
bz	various whitelists
bz	so somewhat similar to the TestPermission thing
doron	so the core security cross-domain check is still CheckSameOriginPrincipal
bz	So you'd have 4 different checks, I guess. IsCapabilityEnabled, CheckSameOriginPrincipal, TestPermission, and content policy
bz	yeah

Comment 3

[Daniel Veditz [:dveditz]]

Since this is doing a same-origin check the remaining missing content-policy calls don't constitute a vulnerability. Can I clear the security bit or am I missing something?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah this is probably not a security issue.  Bug 325005 doesn't really apply here.  I'd missed the same-origin check when I was reading the function.  :(

Comment 5

[Allan Beaufour]

Attachment: Patch

Adds NS_CheckContentLoadPolicy() checks to XForms loading of content.

I just use nsIContentPolicy::TYPE_OTHER all the time. Do we need to distinguish between the different types, and use f.x. TYPE_SUBDOCUMENT for submissions?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 223689 [details] [diff] [review]
Patch

>Index: nsXFormsUtils.cpp

>+#include "nsContentUtils.h"
>+#include "nsContentPolicyUtils.h"

Why can you use these outside the layout module?  And can you?  I'm not convinced. 

>+nsXFormsUtils::CheckSameOrigin(nsIDocument   *aBaseDocument,
>+  nsIPrincipal* basePrincipal = aBaseDocument->NodePrincipal();

So basePrincipal is guaranteed non-null...

>-    if (NS_SUCCEEDED(rv)) {
>+    if (basePrincipal && NS_SUCCEEDED(rv)) {

... and I don't understand this change.

>+                                          aDoc,            // context

You really don't know what XForms element is trying to do the load?

Comment 7

[Allan Beaufour]

(In reply to comment #6)
> (From update of attachment 223689 [details] [diff] [review] [edit])
> >Index: nsXFormsUtils.cpp
> 
> >+#include "nsContentUtils.h"
> >+#include "nsContentPolicyUtils.h"
> 
> Why can you use these outside the layout module?  And can you?  I'm not
> convinced. 

My compiler is not complaining :-). It is used in modules/plugin too?

> >+                                          aDoc,            // context
> 
> You really don't know what XForms element is trying to do the load?

I do, I just did not check the parameters well enough. I assumed it wanted a document.

Comment 8

[Allan Beaufour]

Attachment: Patch v2

I now pass on the actual XForms (DOM) element.

I am using GetOwnerDocument() though, I am not sure whether I should use GetCurrentDoc() (ie bug 303625)?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 223751 [details] [diff] [review]
Patch v2

>Index: nsXFormsInstanceElement.cpp
>@@ -563,17 +556,17 @@ nsXFormsInstanceElement::LoadExternalIns

Unrelated to this bug, but should this really use the document's URI instead of the node's base URI?  This applies to several places in this code... Might want to file a separate bug on this.

>Index: nsXFormsUtils.cpp
> /* static */ PRBool
>+nsXFormsUtils::CheckConnectionAllowed(nsIDOMElement *aElement,

>+  NS_ENSURE_STATE(doc);

That returns NS_ERROR_UNEXPECTED if !doc.  But this method returns a PRBool.  Please fix to return a boolean.

Using GetOwnerDoc is correct -- for security check purposes that's what you want.

>+  nsIPrincipal *basePrincipal = doc->NodePrincipal();

For what it's worth, you could use the NodePrincipal() of the nsINode that got passed in here...  And also in CheckSameOrigin.  Unless there are other callers of CheckSameOrigin?  Not a big deal either way.

>+nsXFormsUtils::CheckSameOrigin(nsIDocument   *aBaseDocument,

This documents that aBaseDocument must be non-null, right?

>+nsXFormsUtils::CheckContentPolicy(nsIDOMElement *aElement,
>+                                  nsIDocument   *aDoc,

This should assert that aElement is non-null.  And document that both that and aDoc must be non-null.

r=bzbarsky with the NS_ENSURE_STATE thing fixed and the comments added.

Comment 10

[Allan Beaufour]

(In reply to comment #9)
> (From update of attachment 223751 [details] [diff] [review] [edit])
> >Index: nsXFormsInstanceElement.cpp
> >@@ -563,17 +556,17 @@ nsXFormsInstanceElement::LoadExternalIns
> 
> Unrelated to this bug, but should this really use the document's URI instead of
> the node's base URI?  This applies to several places in this code... Might want
> to file a separate bug on this.

Filed bug 339777.

> >Index: nsXFormsUtils.cpp
> > /* static */ PRBool
> >+nsXFormsUtils::CheckConnectionAllowed(nsIDOMElement *aElement,
> 
> >+  NS_ENSURE_STATE(doc);
> 
> That returns NS_ERROR_UNEXPECTED if !doc.  But this method returns a PRBool. 
> Please fix to return a boolean.

Doh! Classic error.

Comment 11

[Allan Beaufour]

Attachment: Patch v3

Comment 12

[Allan Beaufour]

Fixed on trunk.