811 bytes, application/xhtml+xml
1.37 KB, patch
Allan Beaufour: review+
|Details | Diff | Splinter Review|
6.28 KB, patch
Doron Rosenberg (IBM): review+
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr; rv:188.8.131.52) Gecko/20060426 Firefox/184.108.40.206 Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr; rv:220.127.116.11) Gecko/20060426 Firefox/18.104.22.168 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(); ?>
Created attachment 222515 [details] 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.
I guess this is again some principal setup that is wrong? This is a very good XForms usecase, so we should fix it asap.
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).
Created attachment 223188 [details] [diff] [review] 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 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 on attachment 223188 [details] [diff] [review] patch Check the return values of the calls, and r=me
Comment on attachment 223188 [details] [diff] [review] patch with allan's comment fixed, r=me
(In reply to comment #7) > (From update of attachment 223188 [details] [diff] [review] ) > 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.
Created attachment 223288 [details] [diff] [review] 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 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.
Created attachment 223295 [details] [diff] [review] 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 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 on attachment 223295 [details] [diff] [review] Patch v2 Sicking, could you please take a look at this security-wise?
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?
(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.
Boris, do you have any comments on this? We really need this working.
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?
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?
(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 on attachment 223295 [details] [diff] [review] Patch v2 bz should be the one to review this. I can sr it after that though.
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 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 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.
Err... that first sentence should say "as long as it's _not_ part of the default build"
checked second patch into trunk
checked second patch into 1.8.1
checked second patch into 22.214.171.124
removing xf-to-branch keyword. Already in both branches.