Last Comment Bug 169982 - XMLSerializer.serializeToStream needs same origin check
: XMLSerializer.serializeToStream needs same origin check
Status: VERIFIED FIXED
[ADT1 RTM] [fixinhand] [ETA 09/27] [N...
: topembed+
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.0.2
Assigned To: Heikki Toivonen (remove -bugzilla when emailing directly)
: Rakesh Mishra
: Andrew Overholt [:overholt]
Mentors:
http://green.nscp.aoltw.net/heikki/16...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-09-20 16:27 PDT by Heikki Toivonen (remove -bugzilla when emailing directly)
Modified: 2011-08-05 21:29 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (2.64 KB, patch)
2002-09-20 16:29 PDT, Heikki Toivonen (remove -bugzilla when emailing directly)
security-bugs: review+
jst: superreview+
chofmann: approval+
Details | Diff | Splinter Review
Testcase that shows how to create an nsIInputStream in JS (484 bytes, text/html)
2002-09-20 16:51 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details
Same as above w/o an alert that's not needed. (477 bytes, text/html)
2002-09-20 16:52 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details

Description Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-20 16:27:24 PDT
Can be done by factoring the code for serializeToString, patch coming up. jst,
can you provide a testcase?
Comment 1 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-20 16:29:18 PDT
Created attachment 100037 [details] [diff] [review]
Proposed fix
Comment 2 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-20 16:31:26 PDT
We already fixed this for serializeToString, but jst found how serializeToStream
could also be abused from content, although it is a little bit more complicated.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2002-09-20 16:51:28 PDT
Created attachment 100042 [details]
Testcase that shows how to create an nsIInputStream in JS

This testcase does not show this exploit, but lays the foundation for a
testcase that does show the exploit.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2002-09-20 16:52:49 PDT
Created attachment 100043 [details]
Same as above w/o an alert that's not needed.
Comment 5 georgi - hopefully not receiving bugspam 2002-09-23 09:39:33 PDT
Sometimes the alert shows trailing garbage when using cross domain documents -
reading from uninitialized memory?
Comment 6 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-23 10:58:57 PDT
There is another bug in the stream code, it does not null terminate the stream.
Comment 7 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-23 13:26:07 PDT
I don't think we filed the stream bug yet... Darin, could you look at these
testcases here, we often get garbage and jst mentioned that the stream does not
null terminate when it probably should.

I am also looking for reviews for the patch.
Comment 8 Darin Fisher 2002-09-23 13:32:09 PDT
a little more pointers to the code would be helpful... what stream doesn't null
terminate?  thx!  --darin
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2002-09-23 17:36:07 PDT
I filed bug 170416 on the null-termination issues related to
nsIOutputStream::Write().
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2002-09-23 17:37:33 PDT
Comment on attachment 100037 [details] [diff] [review]
Proposed fix

sr=jst
Comment 11 Mitchell Stoltz (not reading bugmail) 2002-09-26 20:46:19 PDT
Comment on attachment 100037 [details] [diff] [review]
Proposed fix

r=mstoltz.

One nit - you might want to change the name of your new function from
CheckSameOrigin to something like CheckSameOriginForSerializer, just so it's
really obvious you're not calling the CheckSameOrigin function on
nsIScriptSecurityManager directly.
Comment 12 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-27 16:40:36 PDT
I think the name is ok, especially since the file is so small that you can
almost see it all in one glance.

Checked in.
Comment 13 Jaime Rodriguez, Jr. 2002-09-27 18:35:27 PDT
petersen: can you pls verify the fix on the trunk? thanks!
Comment 14 chris hofmann 2002-09-27 20:16:23 PDT
Comment on attachment 100037 [details] [diff] [review]
Proposed fix

a=chofmann for 1.0.2
Comment 15 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-27 22:07:06 PDT
Landed the fix on the branch as well.
Comment 16 Jaime Rodriguez, Jr. 2002-09-27 23:21:33 PDT
Thanks Heikki! Much Appreciated.

petersen/ji: pls verify this as fixed on the 1.0 branch in tomorrow's builds,
then replace "fixed1.0.2" with "verified1.0.2".
Comment 17 Frank Tang 2002-09-28 13:31:02 PDT
can you write down a clear test procedure for this? I read this bug, I have no
clue how to reproduce the problem or how to verify it. 
Comment 18 Daniel Veditz [:dveditz] 2002-09-30 10:19:46 PDT
adt1.0.2+ approval for landing on the branch.
Comment 19 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-30 14:06:55 PDT
To test, open URL. Hit "Attack!" button. If this bug is not fixed, you will get
the other frame's document serialized into alert boxes. If this is fixed,
nothing seems to happen (you get a security violation error in the JS console).
Comment 20 ji 2002-09-30 14:55:35 PDT
Using 2002-09-30-10-trunk build, clicking on the Attack! button, nothing happens
while on 2002-09-30-08-1.0 branch build I saw some documents on the alert boxes.
It's fixed on the trunk.
Comment 21 ji 2002-09-30 15:22:18 PDT
Johnny's testcase (attachment (id=100043)) is also not working on
2002-09-30-08-1.0 branch build.
Comment 22 Gayatri Rimola 2002-09-30 15:27:28 PDT
If this needs to be re-opened on the branch, please remove the fixed1.0.2 keyword.
Comment 23 ji 2002-09-30 15:35:01 PDT
Johnny's testcase (attachment (id=100043)) doesn't seem to be working on the
trunk either.
below is my test summary:
1. Johnny's testcase (attachment (id=100043)): both on the trunk and branch,
clicking on the "Click me!" button shows something in the alert box. Same behaviour.
2. Testcase mention in comments #19: On the trunk build, clicking on "Attack!"
button doesn't do anything. On the branch build, clicking on the button, it
shows some documents on the alert boxes. Different behaviour.
Comment 24 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-30 15:40:08 PDT
This is totally bizarre, but it looks like only part of the patch made it to the
branch :( My guess would be that the patch failed, I did not notice, checked in
partial fix and then tested with wrong build :/

Anyway, I just checked in the missing part on the branch.

Also according to ji verified on trunk.

Please ignore jst's testcases, they just demonstrate how to create the testcase
and are not correct testcases themselves (they output current document which is
always allowed, not a document in another frame as in the real testcase URL).
Comment 25 Sivakiran Tummala 2002-09-30 17:58:01 PDT
Looks good on 09-30-10-trunk
failed on 09-30-06-branch 

Reopening bug and removing the "fixed1.0.2" keyword
Comment 26 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-30 19:42:38 PDT
stummala: Please read the comments. This bug's status should be verified fixed
for the trunk, and fixed for the branch.
Comment 27 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-09-30 19:43:40 PDT
v
Comment 28 Jaime Rodriguez, Jr. 2002-09-30 20:16:33 PDT
adding john, so we can get a private test build tonight. :-)
Comment 29 John Keiser (jkeiser) 2002-09-30 21:48:56 PDT
With an 8:30PM pull, I get a security exception on this testcase.  All seems in
good order.
Comment 30 Sivakiran Tummala 2002-10-01 13:19:14 PDT
verified fixed on 10-01-08-branch.
Comment 31 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-10-01 13:31:52 PDT
Changing the the keyword for verified1.0.2.

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