1760328 - XHR request performed with system principal for XML/HTML docs may force-enable XUL/XBL

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

When ReaderMode loads a file using XMLHttpRequest, it will load it using the system principal as the XMLHttpRequest is occuring in a jsm scope. While I was looking into a different issue, I noticed that XMLHttpRequest force-enables XUL and XBL when parsing the body if the principal which created the request is the system principal here: https://searchfox.org/mozilla-central/rev/478ecccb085b7efd3c4773edb0480a7b2b16754c/dom/xhr/XMLHttpRequestMainThread.cpp#2045-2047.

When I mentioned this to :gijs, he noticed that we also use XMLHttpRequest from a system privileged scope to perform captive portal detection (https://searchfox.org/mozilla-central/rev/478ecccb085b7efd3c4773edb0480a7b2b16754c/toolkit/components/captivedetect/CaptiveDetect.jsm#28). I haven't looked into this enough yet to determine if we would decide to parse the response body in this situation (https://searchfox.org/mozilla-central/rev/478ecccb085b7efd3c4773edb0480a7b2b16754c/dom/xhr/XMLHttpRequestMainThread.cpp#1945-1997), so I don't know if it would be impacted.

This is somewhat likely to not be a serious security issue, as the document is loaded as a data document and should therefore be inert, but it might increase the surface area for parsed documents, so I've marked it as a security bug out of an abundance of caution.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

It may be worth removing this check entirely and seeing if it causes us any issues. It seems somewhat unlikely to me that any of our system consumers of XMLHttpRequest actually expect XUL/XBL to be force-enabled when parsing the response.

Comment 2

[:Gijs (he/him)]

Basically I think there are just a ton of these: https://searchfox.org/mozilla-central/search?q=new+XMLHttpRequest&path=&case=false&regexp=false shows plenty of .jsm hits, including captive portal (as comment 0) mentions, but also network error page fallback code, a bunch of newtab-related stuff, search suggestions, pdfjs, and some other tidbits. I checked the captive portal case, that does not appear to hit the requisite conditions for the initial request on a non-captive-portal network but I don't know if that would be true irrespective of the response.

Coverage suggests some 145k hits to the mFlagParseBody check that surrounds all of this code, and then 8k hits on the condition for enabling XUL/XBL, and then 6k hits that enter that condition. We run coverage on multiple platforms and so on but I somewhat doubt that that's 100% reader mode, because we don't actually have that many tests for reader mode, and most won't use XHR (they'll load the to-be-reader-mode'd page first, and then we just reuse the DOM from that).

Combined with the fact that I don't want to audit / wait to find out which of them are potentially at risk here, or update all the consumers, I think we should just remove this method entirely. Nobody should be using XUL/XBL anymore from XHR, I would have thought... At the very least, we should see what breaks, and if that list is non-empty we can think about adding an opt-in for code that really needs it. We could call it xhr.imFromThe90sLeaveMeAlone() or something.

Comment 3

[:Gijs (he/him)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #1)

It may be worth removing this check entirely and seeing if it causes us any issues. It seems somewhat unlikely to me that any of our system consumers of XMLHttpRequest actually expect XUL/XBL to be force-enabled when parsing the response.

(mid-aired but I think we're agreeing :-) )

Comment 4

[:Gijs (he/him)]

I just happened to have my debugger-attached nightly still open while writing the comment and found that add-on updates hit this code-path, for instance. :-(

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1760328, r?nika

Comment 6

[Frederik Braun [:freddy]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #0)

This is somewhat likely to not be a serious security issue, as the document is loaded as a data document and should therefore be inert, but it might increase the surface area for parsed documents, so I've marked it as a security bug out of an abundance of caution.

Yeah, I think there are some risks that highly depend on the code that works on the resulting document.

(In reply to :Gijs (he/him) from comment #2)

Combined with the fact that I don't want to audit / wait to find out which of them are potentially at risk here, or update all the consumers (...), I think we should just remove this method entirely. Nobody should be using XUL/XBL anymore from XHR

Let's flag this as sec-want. I don't think it's worth digging too deep if it's easily removed (we can always revisit if it's not.)

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

r=nika,smaug
https://hg.mozilla.org/integration/autoland/rev/581b6b46df00ac1e107fa660258eaf5d114b736d
https://hg.mozilla.org/mozilla-central/rev/581b6b46df00