Last Comment Bug 325814 - Instance document loads don't do security or content policy checks
: Instance document loads don't do security or content policy checks
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Allan Beaufour
: Stephen Pride
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-03 17:22 PST by Boris Zbarsky [:bz] (TPAC)
Modified: 2016-07-15 14:46 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (14.68 KB, patch)
2006-05-29 05:50 PDT, Allan Beaufour
no flags Details | Diff | Splinter Review
Patch v2 (15.89 KB, patch)
2006-05-30 01:39 PDT, Allan Beaufour
bzbarsky: review+
Details | Diff | Splinter Review
Patch v3 (16.23 KB, patch)
2006-05-31 01:39 PDT, Allan Beaufour
bugs: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (TPAC) 2006-02-03 17:22:32 PST
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) 2006-02-03 17:30:27 PST
Note that XForms never shipped as part of Mozilla or Firefox.  And no, this wasn't on 1.7.
Comment 2 Doron Rosenberg (IBM) 2006-02-03 17:48:56 PST
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] 2006-02-03 18:54:44 PST
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 [:bz] (TPAC) 2006-02-03 19:37:00 PST
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 2006-05-29 05:50:39 PDT
Created attachment 223689 [details] [diff] [review]
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 [:bz] (TPAC) 2006-05-29 14:56:41 PDT
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 2006-05-30 01:31:42 PDT
(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 2006-05-30 01:39:00 PDT
Created attachment 223751 [details] [diff] [review]
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 [:bz] (TPAC) 2006-05-30 08:13:57 PDT
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 2006-05-31 01:29:30 PDT
(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 2006-05-31 01:39:14 PDT
Created attachment 223893 [details] [diff] [review]
Patch v3
Comment 12 Allan Beaufour 2006-05-31 02:00:34 PDT
Fixed on trunk.

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