Closed Bug 169982 Opened 18 years ago Closed 18 years ago

XMLSerializer.serializeToStream needs same origin check

Categories

(Core :: XML, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.0.2

People

(Reporter: hjtoi-bugzilla, Assigned: hjtoi-bugzilla)

References

()

Details

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

Attachments

(1 file, 2 obsolete files)

Can be done by factoring the code for serializeToString, patch coming up. jst,
can you provide a testcase?
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
This testcase does not show this exploit, but lays the foundation for a
testcase that does show the exploit.
Attachment #100042 - Attachment is obsolete: true
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: nsbeta1nsbeta1+
Whiteboard: [ADT1 RTM][fixinhand]
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
Closed: 18 years ago
Resolution: --- → FIXED
Keywords: approval
Whiteboard: [ADT1 RTM][fixinhand] → [ADT1 RTM] [fixinhand] [ETA ]
Whiteboard: [ADT1 RTM] [fixinhand] [ETA ] → [ADT1 RTM] [fixinhand] [ETA 09/27]
Target Milestone: mozilla1.2beta → mozilla1.0.2
petersen: can you pls verify the fix on the trunk? thanks!
Whiteboard: [ADT1 RTM] [fixinhand] [ETA 09/27] → [ADT1 RTM] [fixinhand] [ETA 09/27] [Needs Approvals]
Keywords: topembed+
Comment on attachment 100037 [details] [diff] [review]
Proposed fix

a=chofmann for 1.0.2
Attachment #100037 - Flags: approval+
Landed the fix on the branch as well.
Keywords: fixed1.0.2
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".
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.2adt1.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).
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.
Johnny's testcase (attachment (id=100043)) is also not working on
2002-09-30-08-1.0 branch build.
If this needs to be re-opened on the branch, please remove the fixed1.0.2 keyword.
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
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
Closed: 18 years ago18 years ago
Keywords: fixed1.0.2
Resolution: --- → FIXED
v
Status: RESOLVED → VERIFIED
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.
verified fixed on 10-01-08-branch.
Changing the the keyword for verified1.0.2.
QA Contact: petersen → rakeshmishra
Group: security
You need to log in before you can comment on or make changes to this bug.