302685 - XForms submission allows cross domain instance replacement

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Doron Rosenberg (IBM)]

javier foud a bug in the submission security code.

Comment 1

[jhp (no longer active)]

The bug is in |nsXFormsSubmissionElement::CheckSameOrigin()|.  If the
"submission" element has a "replace" attribute (and therefore
|mIsReplaceInstance| is set to TRUE), then the function returns without having
run the security check.

The quick fix would be to change the starting value of |result| to PR_FALSE.

But I still have some issues with the code.  The middle part of the function
does not check the permissions manager if |mIsReplaceInstance| is set to TRUE. 
This means that any "submission" element with the "replace" attr set cannot
submit to a different domain no matter what is in the permissions manager.  Is
this by design?

Comment 2

[Doron Rosenberg (IBM)]

Attachment: patch

We need to reset the default to false if we are going to do a same origin
check.

Comment 3

[Doron Rosenberg (IBM)]

testcase is https://bugzilla.mozilla.org/show_bug.cgi?id=302496's last testcase
- that shouldn't be allowed by default.

Comment 4

[Doron Rosenberg (IBM)]

(In reply to comment #1)
> The bug is in |nsXFormsSubmissionElement::CheckSameOrigin()|.  If the
> "submission" element has a "replace" attribute (and therefore
> |mIsReplaceInstance| is set to TRUE), then the function returns without having
> run the security check.
> 
> The quick fix would be to change the starting value of |result| to PR_FALSE.
> 
> But I still have some issues with the code.  The middle part of the function
> does not check the permissions manager if |mIsReplaceInstance| is set to TRUE. 
> This means that any "submission" element with the "replace" attr set cannot
> submit to a different domain no matter what is in the permissions manager.  Is
> this by design?

Somewhat.  Someone needs to talk to darin and see if he is ok with whitelisting
instance replacement.  Not me though, too much on my plate ;)

Comment 5

[jhp (no longer active)]

Darin, take a look at comment #4.

Comment 6

[aaronr]

Comment on attachment 190988 [details] [diff] [review]
patch

>Index: extensions/xforms/nsXFormsSubmissionElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSubmissionElement.cpp,v
>retrieving revision 1.36
>diff -u -r1.36 nsXFormsSubmissionElement.cpp
>--- extensions/xforms/nsXFormsSubmissionElement.cpp	13 Jul 2005 17:07:42 -0000	1.36
>+++ extensions/xforms/nsXFormsSubmissionElement.cpp	29 Jul 2005 
>@@ -851,11 +852,15 @@
> PRBool
> nsXFormsSubmissionElement::CheckSameOrigin(nsIURI *aBaseURI, nsIURI *aTestURI)
> {
>+  // we default to true to allow regular posts to work like html forms.
>   PRBool result = PR_TRUE;
> 
>   // We require same-origin for replace="instance" or XML submission
>   if (mFormat & (ENCODING_XML | ENCODING_MULTIPART_RELATED) || mIsReplaceInstance) {

nit: Could you change the above comment, please?  Since we don't require same
origin for XML submission if it is whitelisted.  And please clearly spell out
somewhere that whitelisting won't work with replace instance as it stands now
and the reasoning behind the limitation.  Thanks.

with that, r=me

Comment 7

[Darin Fisher]

I think you should make the whitelisting apply uniformly to XForms.  If you go
with the patch as is, then I'd open a follow-up bug to fully implement
whitelisting.  Or, just rev the patch here :-)

Comment 8

[Doron Rosenberg (IBM)]

Attachment: patch, with whitelisting working on replace instance

Comment 9

[jhp (no longer active)]

Comment on attachment 191386 [details] [diff] [review]
patch, with whitelisting working on replace instance

+	- if we are not replacing instance, then file:// urls can submit
anywhere

If I am reading bug 296231 correctly, it seems that this restriction is in
place for |replace="instance"| because file:// urls may not always be local,
and therefore trustworthy.  Correct?  Could you add to the comment, such that a
reader would know why we limit file:// urls in such a way?

+    // lets check the permission manager

-> "let's"

Comment 10

[jhp (no longer active)]

Comment on attachment 191386 [details] [diff] [review]
patch, with whitelisting working on replace instance

If you fix the stuff I commented on above, r=jhpedemonte.

Comment 11

[aaronr]

Comment on attachment 191386 [details] [diff] [review]
patch, with whitelisting working on replace instance

If you fix Javier's comments, then r=me.

Comment 12

[Doron Rosenberg (IBM)]

checked in.