165110 - Make textnodes and XUL elements work properly with IsNativeAnonymous

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[John Keiser (jkeiser)]

Neither textnodes nor XUL elements store information about whether they are
anonymous, and this makes it possible for people to access them when they should
not be able to.  Related to bug 164086, which created this check in the first place.

Comment 1

[Bradley Baetz (:bbaetz)]

jkeiser says this is security sensitive

Comment 2

[John Keiser (jkeiser)]

Eww is one word to describe this.  TextNode and GenericDOMDataNode are saturated
 to the max with bits already.  We are even reusing the lower two bits. 
DOMSlots seems like a bad idea but maybe that's where we have to go.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You still have two bits in nsIDocument* mDocument that you can steal :-). Let's
do it.

Wow, some of these elements must be burning a lot of space on vtable pointers.
So many interfaces...

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also, offtopic question: why don't we remove the nsIDocument* mDocument pointer
from all the content nodes, and move it to DOMSlots? For those nodes that don't
have an mDocument pointer, GetDocument can walk the parent chain to until we
find a node that has the pointer. Plus of course we'd force all nodes without a
parent to hold the mDocument pointer in their slots. This would force all
content nodes to support DOMSlots, I guess.

Comment 5

[John Keiser (jkeiser)]

Would work, except that we do GetDocument *constantly*.  However, we should do
some tests to see if we can remove it from text nodes, since those would be able
to just get the document from their parent and return that.

Comment 6

[John Keiser (jkeiser)]

CC'ing roc, whose comments seem to make him an interested party

Comment 7

[Mitchell Stoltz (not reading bugmail)]

Is it possible to get this fixed for 1.3a, next Tuesday?

Comment 8

[Mitchell Stoltz (not reading bugmail)]

John, I get the impression that there's no security problem here that we know
about, and that making this change would be difficult. Is this accurate? If so,
please remove the sg:mustfix from the status whiteboard. If not, we need this
fixed by 1.4b.

Comment 9

[John Keiser (jkeiser)]

There is no known security vulnerability and the fix is not perfectly trivial. 
The fix could be done with a couple days' work, though; and is probably worth it
in terms of future-proofing.  I plan to do it asap after I get my current high
priority item off my plate.

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Great, thanks John.

Comment 11

[chris hofmann]

jkeiser, any progress on the fix for landing early 1.5 alpha?

Comment 12

[chris hofmann]

added jst and caillon to the cc list...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Could we just reopen this so people who run into comments in the code about this
can at least see what the bug is about and try to fix it?  I see no reason for
keeping this closed....

Comment 14

[Daniel Veditz [:dveditz]]

removing confidential flag per discussion by the security group.

Comment 15

[[e-mail]]

Attachment: Fix for XUL elements

This does not completely address bug 251197 comment 11; although it does not
regress the test case Modern scrollbars still do not follow Mac preferences.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 171457 [details] [diff] [review]
Fix for XUL elements

sr=bzbarsky

Comment 17

[[e-mail]]

XUL element fix checked in.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Note, that patch caused bug 280541

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

This should be fixed now. (Bug 334075, Bug 411054, Bug 410119)