396443 - SVG documents only documents that don't do an Enumerate security check

Status: RESOLVED FIXED

(Core :: SVG, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

All of our document classes use "DOCUMENT_SCRIPTABLE_FLAGS|nsIXPCScriptable::WANT_ENUMERATE" as the scriptable flags in nsDOMClassInfo.cpp, except for SVG which uses plain DOCUMENT_SCRIPTABLE_FLAGS.  Maybe that's ok; if so this bug should be marked invalid.

If it's not ok, I would suggest just rolling nsIXPCScriptable::WANT_ENUMERATE into DOCUMENT_SCRIPTABLE_FLAGS.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Does nsDOMClassInfo::Enumerate actually have to do a security check anymore? I think I simply missed that when I was cleaning nsDOMClassInfo up for XOWs. In fact, I see that I missed a bunch of access checks that I think XOWs subsume.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Good question.  If we can remove these checks, so much the better!  I just think SVGDocument should be doing the same thing as other documents, whatever that thing is.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We should really either be doing these checks, or remove them.  One or the other...

Comment 4

[Jonathan Watt [:jwatt]]

If this is to block 1.9 then we really need someone assigned to this, and I have no clue about this stuff.

Comment 5

[Jonathan Watt [:jwatt]]

Re-setting schrep's original blocking1.9+ and marking P1 in case any fix needs the extra testing of a beta release.

Comment 6

[Brendan Eich [:brendan]]

jst and mrbkap should advise.

/be

Comment 7

[Johnny Stenback (:jst)]

Attachment: Fix.

Yeah, I see no reason why we wouldn't want this, especially now that window.document is no longer allAccess which means that other origins shouldn't be able to reach the document, though if they do they certainly shouldn't be able to enumerate it.

Comment 8

[Johnny Stenback (:jst)]

Oh, I missed earlier comments about whether we need this at all any more. Probably not, XOWs and no more allAccess should make us pretty safe here even w/o this check. I could go either way. Same patch w/o the first hunk if we don't want this any more, unless we go even further and remove the security checking enumerate hook alltogether.

Comment 9

[Brendan Eich [:brendan]]

Can we assert for DEBUG builds that a check is unnecessary? Do that and remove release-build code and everyone wins, I think we are pretty confident that XOWs and stuff work.

/be

Comment 10

[Johnny Stenback (:jst)]

Attachment: Make checks debug only.

Same thing, but makes the security checks in the enumerate hook debug only, and make them only assert.

Comment 11

[Johnny Stenback (:jst)]

Fix landed.