XMLSerializer.serializeToStream needs same origin check

VERIFIED FIXED in mozilla1.0.2

Status

()

Core
XML
P1
normal
VERIFIED FIXED
15 years ago
6 years ago

People

(Reporter: Heikki Toivonen (remove -bugzilla when emailing directly), Assigned: Heikki Toivonen (remove -bugzilla when emailing directly))

Tracking

({topembed+})

Trunk
mozilla1.0.2
topembed+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ADT1 RTM] [fixinhand] [ETA 09/27] [Needs Approvals], URL)

Attachments

(1 attachment, 2 obsolete attachments)

Can be done by factoring the code for serializeToString, patch coming up. jst,
can you provide a testcase?
Created attachment 100037 [details] [diff] [review]
Proposed fix
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.
Keywords: nsbeta1
Priority: -- → P1
Target Milestone: --- → mozilla1.2beta
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.
Created attachment 100043 [details]
Same as above w/o an alert that's not needed.
Attachment #100042 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #100043 - Attachment is obsolete: true
Sometimes the alert shows trailing garbage when using cross domain documents -
reading from uninitialized memory?
There is another bug in the stream code, it does not null terminate the stream.
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.
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [ADT1 RTM][fixinhand]

Comment 8

15 years ago
a little more pointers to the code would be helpful... what stream doesn't null
terminate?  thx!  --darin
I filed bug 170416 on the null-termination issues related to
nsIOutputStream::Write().
Comment on attachment 100037 [details] [diff] [review]
Proposed fix

sr=jst
Attachment #100037 - Flags: superreview+
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.
Attachment #100037 - Flags: review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Keywords: adt1.0.2, mozilla1.0.2

Updated

15 years ago
Keywords: approval
Whiteboard: [ADT1 RTM][fixinhand] → [ADT1 RTM] [fixinhand] [ETA ]

Updated

15 years ago
Whiteboard: [ADT1 RTM] [fixinhand] [ETA ] → [ADT1 RTM] [fixinhand] [ETA 09/27]
Target Milestone: mozilla1.2beta → mozilla1.0.2

Comment 13

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

Updated

15 years ago
Whiteboard: [ADT1 RTM] [fixinhand] [ETA 09/27] → [ADT1 RTM] [fixinhand] [ETA 09/27] [Needs Approvals]

Updated

15 years ago
Keywords: topembed+

Comment 14

15 years ago
Comment on attachment 100037 [details] [diff] [review]
Proposed fix

a=chofmann for 1.0.2
Attachment #100037 - Flags: approval+

Updated

15 years ago
Keywords: mozilla1.0.2 → mozilla1.0.2+
Landed the fix on the branch as well.
Keywords: fixed1.0.2

Comment 16

15 years ago
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

15 years ago
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. 
adt1.0.2+ approval for landing on the branch.
Keywords: adt1.0.2 → adt1.0.2+
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

15 years ago
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

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

Comment 22

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

Comment 23

15 years ago
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.
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).
Status: RESOLVED → VERIFIED

Comment 25

15 years ago
Looks good on 09-30-10-trunk
failed on 09-30-06-branch 

Reopening bug and removing the "fixed1.0.2" keyword
Status: VERIFIED → REOPENED
Keywords: fixed1.0.2
Resolution: FIXED → ---
stummala: Please read the comments. This bug's status should be verified fixed
for the trunk, and fixed for the branch.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Keywords: fixed1.0.2
Resolution: --- → FIXED
v
Status: RESOLVED → VERIFIED

Comment 28

15 years ago
adding john, so we can get a private test build tonight. :-)
With an 8:30PM pull, I get a security exception on this testcase.  All seems in
good order.

Comment 30

15 years ago
verified fixed on 10-01-08-branch.
Changing the the keyword for verified1.0.2.
Keywords: fixed1.0.2 → verified1.0.2

Updated

15 years ago
QA Contact: petersen → rakeshmishra
Group: security
You need to log in before you can comment on or make changes to this bug.