169982 - XMLSerializer.serializeToStream needs same origin check

Status: VERIFIED FIXED

(Core :: XML, defect, P1)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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)]

Attachment: Proposed fix

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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)]

Attachment: 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)]

Attachment: Same as above w/o an alert that's not needed.

Comment 5

[georgi - hopefully not receiving bugspam]

Sometimes the alert shows trailing garbage when using cross domain documents -
reading from uninitialized memory?

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

There is another bug in the stream code, it does not null terminate the stream.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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]

a little more pointers to the code would be helpful... what stream doesn't null
terminate?  thx!  --darin

Comment 9

[Johnny Stenback (:jst)]

I filed bug 170416 on the null-termination issues related to
nsIOutputStream::Write().

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 100037 [details] [diff] [review]
Proposed fix

sr=jst

Comment 11

[Mitchell Stoltz (not reading bugmail)]

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)]

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.]

petersen: can you pls verify the fix on the trunk? thanks!

Comment 14

[chris hofmann]

Comment on attachment 100037 [details] [diff] [review]
Proposed fix

a=chofmann for 1.0.2

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Landed the fix on the branch as well.

Comment 16

[Jaime Rodriguez, Jr.]

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]

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]]

adt1.0.2+ approval for landing on the branch.

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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]

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]

Johnny's testcase (attachment (id=100043)) is also not working on
2002-09-30-08-1.0 branch build.

Comment 22

[Gayatri Rimola]

If this needs to be re-opened on the branch, please remove the fixed1.0.2 keyword.

Comment 23

[ji]

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)]

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]

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)]

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)]

v

Comment 28

[Jaime Rodriguez, Jr.]

adding john, so we can get a private test build tonight. :-)

Comment 29

[John Keiser (jkeiser)]

With an 8:30PM pull, I get a security exception on this testcase.  All seems in
good order.

Comment 30

[Sivakiran Tummala]

verified fixed on 10-01-08-branch.

Comment 31

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Changing the the keyword for verified1.0.2.