338451 - Cannot submit with replace=instance more than one time

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect, P1)

Description

[Charles Brunet]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

I use replace=instance submissions to prefill parts of my form with some data. The submission is triggered with a DOMActivate event on an input box. It works well the first time, but if I want to submit my form a second time, pressing 'enter' in an input box that has DOMActivate event associated to it, the form isn't resubmitted.

See attached file for more details.

Reproducible: Always




Here is the PHP script I use to proceed data:

<?php
	$post = DOMDocument::loadXML($HTTP_RAW_POST_DATA);
	$entree = $post->getElementsByTagName("entree")->item(0);
	$sortie = $post->getElementsByTagName("valeur")->item(0);
	
	$sortie->nodeValue = $entree->nodeValue;
	$entree->nodeValue = "";

	echo $post->saveXML();
?>

Comment 1

[Charles Brunet]

Attachment: Testcase

This form works with the little PHP script described before.

I forgot to tell that the second time the form is submited, the following exception is triggered:
Error: uncaught exception: Security Error: Content at
http://localhost/xftst/Submit/testxform.xhtml may not load
data from about:blank.

Comment 2

[Allan Beaufour]

I guess this is again some principal setup that is wrong?

This is a very good XForms usecase, so we should fix it asap.

Comment 3

[Doron Rosenberg (IBM)]

about:blank could point at us getting the wrong origin document, perhaps we are getting the instance document (which would have a url of about:blank I think).

Comment 4

[Doron Rosenberg (IBM)]

Attachment: patch

this fixes the problem on the 1.8 branch, not the trunk.  However, trunk should ge tthis too, since its probably part of the fix for trunk.

Comment 5

[Doron Rosenberg (IBM)]

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

So the problem was, without a URI, the document gets a about:blank base uri, which the security principal is based on (hence the about:blank error).

Comment 6

[Allan Beaufour]

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

Check the return values of the calls, and r=me

Comment 7

[aaronr]

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

with allan's comment fixed, r=me

Comment 8

[Allan Beaufour]

(In reply to comment #7)
> (From update of attachment 223188 [details] [diff] [review] [edit])
> with allan's comment fixed, r=me

I fixed the comments, and checked it in on trunk.

That means that it should be fixed on branch when we sync (bug 339087). On trunk, I believe it boils down to the same as bug 337688:
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file nsXFormsUtils.cpp, line 589
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file nsXFormsSubmissionElement.cpp, line 638

I'll keep this bug open, and depend it on bug 337688.

Comment 9

[Allan Beaufour]

Attachment: Patch that also fixes trunk

This patch sets the principal of any instance document to the same as the document owning the instance. That should make sense, because if we set the instance document we have loaded it succesfully (passing all CheckOrigin() checks).

It is second approach that sicking mentions in bug 337688 comment 7. It also fixes the problems with the flickr sample mentioned in bug 334937.

Unfortunately it does not fix bug 337688.

Comment 10

[Doron Rosenberg (IBM)]

Comment on attachment 223288 [details] [diff] [review]
Patch that also fixes trunk

Doing this is probably correct in that it will allow cross-domain access, which is fine, since we can only get the contents if the user whitelisted the domain in the first place.

I emailed jst and bz about the change in behavior here that broke us on trunk too, since a document should get the principal of its baseURI.

Comment 11

[Allan Beaufour]

Attachment: Patch v2

This also fixed bug 337688. It was just me not looking properly at the submission code. When the external instance document is loaded I change the principal for that one too.

If you are ok with it Doron, we should throw it after sicking or bz.

Comment 12

[Doron Rosenberg (IBM)]

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

Should be safe, and in line I believe with what we do in other parts of Mozilla.

Comment 13

[Allan Beaufour]

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

Sicking, could you please take a look at this security-wise?

Comment 14

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

Hmm.. I'm very nervous about this patch. I would much rather that the call to ReplacePrincipal be done much closer to where the XForms security checks are done.

Basically I'm worried that something happens (either in the current code, or in future code) between the point where XForms does its checks, and it changes the principal. For example, could a http-redirect happen? Or could script change any attributes in the XForms DOM?

Comment 15

[Allan Beaufour]

(In reply to comment #14)
> Hmm.. I'm very nervous about this patch. I would much rather that the call to
> ReplacePrincipal be done much closer to where the XForms security checks are
> done.

It cannot really happen before, as the security checks check whether a connection is allowed. This is, logically, before we actually have a document to set a principal for.

> Basically I'm worried that something happens (either in the current code, or in
> future code) between the point where XForms does its checks, and it changes the
> principal.

I follow your concern, but imho that is unrelated to the setting of the principal. These things should be handled/checked before we reach SetInstanceDocument().

> For example, could a http-redirect happen?

We check OnChannelRedirect() too

> Or could script change any attributes in the XForms DOM?

I fail to see how that would change the principal. It could influence the security checks for submissions, but we save the @replace attribute at the start of nsXFormsSubmissionElement::Submit() to safe-guard against that.

Overall, the principal setting should happen at a time where security checks have been made.

Comment 16

[Allan Beaufour]

Boris, do you have any comments on this?

We really need this working.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Frankly, I'm _very_ cautious of setting a principal to the loading document's principal.  If there is any way whatsoever for the instance document to cause something "interesting" to happen with its own permissions, this leads to privilege escalation.  I don't know nearly enough about XForms to say whether this is an issue; in particular, I have no idea how instance documents are used.  Is there a clear one-page explanation of that somewhere?

Comment 18

[aaronr]

I am soooo sorry that this took so long.  I wrote up a summary about instance documents on 06/06 and I swore that I posted it to this bug.  Somehow it didn't make it to the bug though and I can't find it on my machine.  So I'm rewriting it from scratch again.  Crap!

XForms instance documents are well-formed XML documents.  Instance documents are created in one of three ways:

1) XForms instance documents can be loaded from an external file (specified via the src attribute on the xforms:instance element).  
2) If no src attribute is present, we'll create an instance document using the contents of the xforms:instance element.
3) If no instance element is found yet controls are found that try to bind using xpath expressions, XForms processors will 'lazily create' an instance document that will contain the nodes used in these XPath expressions.

Once an instance document is created, it is maintained in memory by the XForms processor.  Controls bind to nodes in the instance documents by using binding attributes.  The value of these binding attributes are XPath expressions.  During the initialization of the xform, for each control, we'll evaluate the binding XPath expression and set the value of the control to the result, which the control will then represent to the user.  For example, a textarea will contain the resulting string, a select1 will show a combo box or a set of radio buttons with the appropriate item selected, etc.  Once the xform is loaded, initialized and ready to go, the instance document (maintained by the XForms processor) can be updated by:

1) User interaction with a control.  When the user updates the value of a control, the node in the instance document that it is bound to is updated.  Most controls just update the first text node of a bound instance node with strings of one format or another, but xforms:select and xforms:select1 controls can actually copy a whole subtree of instance nodes into the instance document under the bound node.  This subtree could come from the same instance document or another instance document.  But both instance documents are referenced from the same XForms document that is currently being used.
2) JavaScript.  Each instance document is accessible to javascript using XPath.  The script can update the document in any way that it wants to.  Once the instance document has been updated, the author will send an event to signal that he is done manipulating the document and that the XForms processor should now update the controls to reflect those changes.
3) Calculation.  Bound nodes can have their values change as a result of a combination of calculations based on other nodes.  If one node that the calculation depends on has changed, then the calculation is re-run and the result is stored in the bound node.
4) Event Handlers.  There are a variety of XForms actions that can be registered as event handlers using XML Events.  Some of these actions can update the instance document by inserting or deleting a node, setting a node's value, etc.
5) Reset.  As I said before, each instance document is maintained in memory.  A deep clone of each document is also maintained in memory that represents the document as it was when it was initially loaded.  If a xforms-reset is sent to a xforms:model, then every instance document maintained in that model is thrown away and replaced by a deep clone of the original instance document.

The user can choose to submit an instance document or a subtree of the instance document to a URL.  The submission will be validated against any known schema registered against the model and then if it is valid (and fulfills other XForms rules for submission), it is sent to that URL.  If a xforms:submission element has a 'replace' attribute with the value of 'instance', then the specified instance document (which could be a different instance document) will be replaced by the reply from the server.  We parse the reply as application/xml and keep the resulting document in memory just as we did the previous instance document.  If replace="instance" or replace="none" is not used, then the whole XForms document is replaced by the reply just like HTML forms.

Mozilla's XForms processor is currently allowing users to 'whitelist' domains that they trust and say whether they trust XForms from that domain to:

1) load instance documents from other domains
2) submit instance documents to other domains
3) both (needed for 'replacement' of instance document)

If a domain is not specifically whitelisted, we allow neither cross-domain instance loading nor cross-domain submission.  By default, we allow local XForms files to submit anywhere but we don't allow local files to load or submit with replace instance.

If there is any concern that I can address, please let me know.  Allan, Olli or Doron, please let me know if I got any of the above wrong or if I missed something.  Thanks.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK.  So nothing in the instance document ever "acts", right?  It's loaded as data?  And essentially all instance documents are considered as parts of the same trust domain, together with the page that loaded them?

> 'whitelist' domains that they trust and say whether they trust XForms from that
> domain to:
> 1) load instance documents from other domains

Right.  The worry I have is that if I whitelist domain A and domain A loads an instance document from domain B that allows domain B to execute a cross-site scripting attack on domain A.  I never said I trusted domain B; I guess domain A is trusting them, eh?

Do we show a broken lock icon if an insecure instance document is loaded from a secure page, btw?

Comment 20

[aaronr]

(In reply to comment #19)
> OK.  So nothing in the instance document ever "acts", right?  It's loaded as
> data?  And essentially all instance documents are considered as parts of the
> same trust domain, together with the page that loaded them?
> 

that is the idea, its just treated as data.  You can see in the two places where we read in instance data: http://lxr.mozilla.org/mozilla/source/extensions/xforms/nsXFormsSubmissionElement.cpp#481
http://lxr.mozilla.org/mozilla/source/extensions/xforms/nsXFormsInstanceElement.cpp#527
I can't think of a way that the loaded instance could execute anything.

> > 'whitelist' domains that they trust and say whether they trust XForms from that
> > domain to:
> > 1) load instance documents from other domains
> 
> Right.  The worry I have is that if I whitelist domain A and domain A loads an
> instance document from domain B that allows domain B to execute a cross-site
> scripting attack on domain A.  I never said I trusted domain B; I guess domain
> A is trusting them, eh?
> 

right.  As far as I know, nothing can execute in the instance.  It is treated as application/xml.

> Do we show a broken lock icon if an insecure instance document is loaded from a
> secure page, btw?
> 

We don't do anything specifically with that icon, no.

Comment 21

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

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

bz should be the one to review this. I can sr it after that though.

Comment 22

[aaronr]

I guess if a xml processing instruction can do anything damaging, that might be a problem.  I don't know the bits and pieces of xml well enough to come up with anything, though.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

OK.  So the key is that nsXFormsInstanceElement::LoadExternalInstance uses kLoadAsData and nsXFormsSubmissionElement::OnStopRequest uses nsDOMParser...

That should make things safe enough, yeah.

Comment 24

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

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

I'm not very exited about this solution, but i guess it's good enough as long as it's part of the default build.

However if we ever make this part of core gecko, we should look into finding a better solution.

Comment 25

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

Err... that first sentence should say "as long as it's _not_ part of the default build"

Comment 26

[aaronr]

checked second patch into trunk

Comment 27

[aaronr]

checked second patch into 1.8.1

Comment 28

[aaronr]

checked second patch into 1.8.0.5

Comment 29

[aaronr]

removing xf-to-branch keyword.  Already in both branches.