<!DOCTYPE> ignores contentaccessible, leaks DTD strings and therefore browser UI locale
Categories
(Core :: XML, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox70 | --- | fixed |
People
(Reporter: neil, Assigned: acat)
References
(Blocks 2 open bugs, )
Details
(Keywords: sec-low, testcase, Whiteboard: [sg:low][fingerprinting][fp-triaged][tor 30304][adv-main70-])
Attachments
(4 files, 1 obsolete file)
A webpage can include an invisible <iframe> whose source tries to read an entity from an extension's locale. This could be used to determine whether a particular extension is installed. The chrome package does not need to be marked as contentaccessible for this information leak to occur. The URL demonstrates reading an entity from the mozapps package which should not be contentaccessible.
Comment 1•16 years ago
|
||
[sg:low] -- disclosure of information that is unlikely to be sensitive. (But strings from a private extension could be confidential.)
Updated•15 years ago
|
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment 4•6 years ago
|
||
(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #2) > Does this still work? We've added a bunch of restrictions to chrome URLs > since 2008 and the attached URL renders blank for me. Yes, the testcase was supposed to render blank, the <title> is what was important. (In reply to RNicoletto from comment #3) > Is there any testcase URL in order to verify this? It's in the URL field but didn't explain what was expected. I'll attach a better test case.
Updated•6 years ago
|
Comment 5•6 years ago
|
||
I confirmed that I get the French string in a French build even with privacy.resistFingerprinting=true:
> A string from your language: Visiter la page web
Comment 6•6 years ago
|
||
At first glance, knowing nothing about this code, I would guess this can be fixed in nsExpatDriver::OpenInputStreamFromExternalDTD to not allow web content to use DTDs. Some resource URIs do want at least global.dtd though so I think we would need an exception for some internal pages depending on flags… https://dxr.mozilla.org/mozilla-central/rev/6ff60a083701d08c52702daf50f28e8f46ae3a1c/parser/htmlparser/nsExpatDriver.cpp#624,665
Comment 7•6 years ago
|
||
Looks like we tried to close this hole in bug 1182546 but then had to revert it in bug 1228116 due to addon-compat. Looks like we would start with reverting https://hg.mozilla.org/mozilla-central/rev/7ace0805c2d3 and the test commit but would also want to plug the leak for even contentaccessible DTD to get rid of all locale fingerprinting options.
Updated•5 years ago
|
Comment 8•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
We are trying to fix this in Tor Browser. Even though initially this bug was concerned about non-contentaccessible, we actually want to forbid reading locale, no matter if it's contentaccessible or not.
I'm attaching a patch which does not seem to break much for ESR60 based current Tor Browser (at least I could not find breakage so far, although I did not try exhaustively). However, the same patch for central breaks quite a lot of stuff.
But ignoring breakage for now, there's a couple of assumptions I would like to check. First, I'm assuming the only way to load locale DTDs is via chrome://*/locale/*
(so that the check in nsChromeRegistry::AllowContentToAccess
will get all of these). And second, I'm assuming enabling all about:*
pages to load chrome://
resources does not have sec. issues (there is no way for web content to trigger loads as about:*
). Do you think this is this correct?
Now w.r.t. to breakage, in central I see several chrome://*/locale/*
resources being loaded from UI as moz-nullprincipal
, which break with this patch. Not sure if there is a way currently to whitelist these loads that does not allow moz-nullprincipal
loads also from web content (e.g. from data-uri inside iframe). But hopefully it would not be difficult to fix it so that the loads in UI come from something more privileged than the null principal. Then there is also breakage in web content, like video controls, etc. In principle this is because chrome://*/locale/*
resources are needed for localization, would it be possible to do the localization needed for video controls, etc. in a different way (Fluent?) that does not leak locale?
Perhaps I'm missing a simpler way to fix this with less breakage, would be good to know too :)
Assignee | ||
Comment 10•5 years ago
|
||
Comment 11•5 years ago
|
||
(In reply to Alex Catarineu from comment #9)
But ignoring breakage for now, there's a couple of assumptions I would like to check. First, I'm assuming the only way to load locale DTDs is via
chrome://*/locale/*
(so that the check innsChromeRegistry::AllowContentToAccess
will get all of these). And second, I'm assuming enabling allabout:*
pages to loadchrome://
resources does not have sec. issues (there is no way for web content to trigger loads asabout:*
). Do you think this is this correct?Now w.r.t. to breakage, in central I see several
chrome://*/locale/*
resources being loaded from UI asmoz-nullprincipal
, which break with this patch. Not sure if there is a way currently to whitelist these loads that does not allowmoz-nullprincipal
loads also from web content (e.g. from data-uri inside iframe). But hopefully it would not be difficult to fix it so that the loads in UI come from something more privileged than the null principal. Then there is also breakage in web content, like video controls, etc. In principle this is becausechrome://*/locale/*
resources are needed for localization, would it be possible to do the localization needed for video controls, etc. in a different way (Fluent?) that does not leak locale?
Gijs is the go to person for those kinds of questions - 302 to him!
Comment 12•5 years ago
|
||
(In reply to Alex Catarineu from comment #9)
We are trying to fix this in Tor Browser. Even though initially this bug was concerned about non-contentaccessible, we actually want to forbid reading locale, no matter if it's contentaccessible or not.
I'm attaching a patch which does not seem to break much for ESR60 based current Tor Browser (at least I could not find breakage so far, although I did not try exhaustively). However, the same patch for central breaks quite a lot of stuff.
But ignoring breakage for now, there's a couple of assumptions I would like to check. First, I'm assuming the only way to load locale DTDs is via
chrome://*/locale/*
(so that the check innsChromeRegistry::AllowContentToAccess
will get all of these).
In some cases there may be alternative resource: URLs that also work, and jar:file: or file: URIs (but the latter wouldn't be accessible from web content. For resource:, it depends on the contentaccessible stuff there, I believe).
And second, I'm assuming enabling all
about:*
pages to loadchrome://
resources does not have sec. issues
Note that this is a very broad claim.
(there is no way for web content to trigger loads as
about:*
).
This is much narrower. It is not strictly true. Trivial proof is that web content can obviously access about:blank.
The more elaborate explanation is that there are 3 levels of security for about: content today:
- system privileged (e.g. about:preferences ) that isn't linkable from web content
- non-system privileged (e.g. about:mozilla) that isn't linkable from web content
- non-system privileged (e.g. about:blank, about:srcdoc, (unfortunately) a few others) that is linkable from web content
So the scheme check in the patch is probably too simplistic. You can find all the web-linkable about: URIs by searching the codebase for the MAKE_LINKABLE about: URI flag.
Now w.r.t. to breakage, in central I see several
chrome://*/locale/*
resources being loaded from UI asmoz-nullprincipal
, which break with this patch.
Do you have a link to a trypush or which loads these are? Without more details it is almost impossible to say something specific here.
Then there is also breakage in web content, like video controls, etc. In principle this is because
chrome://*/locale/*
resources are needed for localization, would it be possible to do the localization needed for video controls, etc. in a different way (Fluent?) that does not leak locale?
I don't understand this question. If web content can access the video control DOM and check its localized content somehow, this information leaks irrespective of the l10n mechanism we use to put the localized content there, no? So there's no point in switching to fluent...
Is it actually possible for web content to access this localized data inside a video control's DOM? (I haven't read the rest of this bug)
Or are you saying that the requests for the locale resources in the video controls also happen with a null principal, and that that is what needs fixing? That seems more easily fixable.
Long-term we want to switch the in-content stuff to use fluent but there are some open questions about the exact specifics, and nobody with time to do/prioritize the implementation.
Assignee | ||
Comment 13•5 years ago
|
||
I took a look at the current Fluent checks to prevent localization in web content (https://hg.mozilla.org/mozilla-central/file/c6d806b496845985516cc04342c04988aa1817dd/dom/base/Document.cpp#l1995). It seemed reasonable to me to reuse that logic and apply it also for DTD loads. This way there's no need to add more exceptions for about:
pages (URI_DANGEROUS_TO_LOAD
takes care of that) and also no need to think whether locale can also be accessed via resource://
as in original patch. Still not sure if there would be a better place to do the PrincipalAllowsL10n
check and if NS_ERROR_DOM_BAD_URI
is the right error code to return in case the check fails. I updated my patch with this, but the same breakage still applies (not checked Firefox tests, just using the browser):
Now w.r.t. to breakage, in central I see several chrome:///locale/ resources being loaded from UI as moz-nullprincipal, which break with this patch.
Do you have a link to a trypush or which loads these are? Without more details it is almost impossible to say something specific here.
I checked, and these seem to come from parseXULToFragment
calls done in UI like https://hg.mozilla.org/mozilla-central/file/c6d806b496845985516cc04342c04988aa1817dd/toolkit/content/widgets/wizard.js#l40. These end up calling DomParser::ParseFromString
which sets the principal to null in the case of system principal. So the XML gets parsed and the DTDs are loaded with null principal, and therefore blocked after this patch.
Then there is also breakage in web content, like video controls, etc. In principle this is because chrome:///locale/ resources are needed for localization, would it be possible to do the localization needed for video controls, etc. in a different way (Fluent?) that does not leak locale?
I don't understand this question. If web content can access the video control DOM and check its localized content somehow, this information leaks irrespective of the l10n mechanism we use to put the localized content there, no? So there's no point in switching to fluent...
I did not find a way to read the localized DOM: the video-control issue was just about breakage. The question was whether there would be a way of doing the localization (still) without leaking locale and not breaking because of DTD not accessible to web content. In this case, the problem seems to come from https://hg.mozilla.org/mozilla-central/file/c6d806b496845985516cc04342c04988aa1817dd/toolkit/content/widgets/videocontrols.js#l2264, again because of parseFromString
. Here it takes the principal of the web content, it's not null principal but still cannot load the DTD after the patch.
Is there a simple way of doing these parseFromString
calls with privileged principals so that the DTD loads don't fail? Or would there be a better alternative?
Assignee | ||
Comment 14•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 15•5 years ago
|
||
(In reply to Alex Catarineu from comment #13)
I checked, and these seem to come from
parseXULToFragment
calls done in UI like https://hg.mozilla.org/mozilla-central/file/c6d806b496845985516cc04342c04988aa1817dd/toolkit/content/widgets/wizard.js#l40. These end up callingDomParser::ParseFromString
which sets the principal to null in the case of system principal. So the XML gets parsed and the DTDs are loaded with null principal, and therefore blocked after this patch.
I expect that our best bet is forcing the DOMParser in question to be using a different principal that passes the checks - perhaps a codebase principal for an about: URI that we know is allowed access and unprivileged (about:mozilla would be fitting, perhaps?). Either that or building some other special case. Brian/Christoph, thoughts?
Comment 16•5 years ago
•
|
||
(In reply to :Gijs (he/him) from comment #15)
(In reply to Alex Catarineu from comment #13)
I checked, and these seem to come from
parseXULToFragment
calls done in UI like https://hg.mozilla.org/mozilla-central/file/c6d806b496845985516cc04342c04988aa1817dd/toolkit/content/widgets/wizard.js#l40. These end up callingDomParser::ParseFromString
which sets the principal to null in the case of system principal. So the XML gets parsed and the DTDs are loaded with null principal, and therefore blocked after this patch.I expect that our best bet is forcing the DOMParser in question to be using a different principal that passes the checks - perhaps a codebase principal for an about: URI that we know is allowed access and unprivileged (about:mozilla would be fitting, perhaps?). Either that or building some other special case. Brian/Christoph, thoughts?
Assuming Christoph thinks this it's OK to allow non-null principal for parseFromString in these cases, I will add that:
- For the customElements.js case this is only ever happening with system principal docs (injected here https://searchfox.org/mozilla-central/rev/e7d9a8749303b39dadcc0e18ea0d60a570a68145/toolkit/components/processsingleton/CustomElementsListener.jsm#14).
- We are also calling forceEnableXULXBL() on the DOMParser so perhaps that is a signal to allow for a non-null principal: https://searchfox.org/mozilla-central/rev/e7d9a8749303b39dadcc0e18ea0d60a570a68145/toolkit/content/customElements.js#202-203. Or we could add a new option to allow it.
For videocontrols, perhaps we could do one of:
- Expose a
[Func="IsChromeOrXBLOrUAWidget"]
method on DOMParser calledparseFromStringNonNullPrincipal
or similar from videocontrols UA widget via: https://firefox-source-docs.mozilla.org/toolkit/content/toolkit_widgets/ua_widget.html#the-javascript-sandbox. - As mentioned above, figuring out how to convert this to Fluent may be a better solution - I think it was done with DTD just because it was the closest available option.
- Store the strings in a properties file, read them from chrome code and pass them into the widget constructor similar to how we are now passing prefs for picture in picture: (https://searchfox.org/mozilla-central/rev/e7d9a8749303b39dadcc0e18ea0d60a570a68145/toolkit/actors/UAWidgetsChild.jsm#99). Then take those strings as variables into the widget and set the textContent, etc appropriately instead of from the DTD. If you wanted to test that out without moving strings into a properties file, you could use DOMParser from UAWidgetsChild.jsm, create a document fragment that includes those entities into nodes, then read the textContent from the nodes and pass them into the widget constructor that way.
Comment 17•5 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #16)
Assuming Christoph thinks this it's OK to allow non-null principal for parseFromString in these cases, I will add that:
Yeah, I think that makes sense - thanks!
Assignee | ||
Comment 18•5 years ago
|
||
Comment 19•5 years ago
|
||
Updated•5 years ago
|
Comment 20•5 years ago
|
||
Comment 21•5 years ago
|
||
(In reply to Tom Ritter [:tjr] from comment #20)
A few test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=730249db0383ef31a1f1fb8359c6592ac16337ef
This patch is trying to use about:mozilla as a content-privileged about: page to use for parsing the XUL fragments that the various callsites use, but because we're creating a document object for it it trips the CSP checks that require a CSP. We obviously don't really want to manually add a CSP meta tag into every fragment string we pass to DOMParser.parseFromString
at the callsite. Perhaps the setMozPrincipal
thing can set a default CSP... I'd ask Christoph about this but he's not accepting needinfos...
Assignee | ||
Comment 22•5 years ago
|
||
Updated the patch with a different approach that does not set about:mozilla as the principal of the DOMParser document.
Still, some failing tests. One is browser/components/payments/test/mochitest/formautofill/test_editCreditCard.html, which loads an iframe with an xhtml document that uses some chrome:// DTDs, which fail to load. One way to fix the test would be to use a DOMParser to parse the xhtml (with SpecialPowers setting the forceDTDAllowed flag on the DOMParser). But not sure if this would stop testing something important.
Something related that breaks after the patch is the local debug server for payments: ./mach python browser/components/payments/server.py
and the corresponding http://localhost:8000/paymentRequest.xhtml?debug=1 page. The same issue, tries to load some chrome:// DTDs. I am not sure how to deal with this one, since in theory I think this should not work after the patch.
Assignee | ||
Comment 23•5 years ago
|
||
Comment 24•5 years ago
|
||
(In reply to Alex Catarineu from comment #22)
Still, some failing tests. One is browser/components/payments/test/mochitest/formautofill/test_editCreditCard.html, which loads an iframe with an xhtml document that uses some chrome:// DTDs, which fail to load. One way to fix the test would be to use a DOMParser to parse the xhtml (with SpecialPowers setting the forceDTDAllowed flag on the DOMParser). But not sure if this would stop testing something important.
That should be solved by Fluent so I'm fine with adding skip-if = true # Bug 1446164
to the manifest.
Something related that breaks after the patch is the local debug server for payments:
./mach python browser/components/payments/server.py
and the corresponding http://localhost:8000/paymentRequest.xhtml?debug=1 page. The same issue, tries to load some chrome:// DTDs. I am not sure how to deal with this one, since in theory I think this should not work after the patch.
Don't worry about that one since we're not working on that code now and Fluent should address that issue.
Assignee | ||
Comment 25•5 years ago
|
||
Then this should be fine to land now, thanks!
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=661dc1eda53c0c1feef2f7dc83ed798856b4c4cd
Assignee | ||
Updated•5 years ago
|
Comment 26•5 years ago
|
||
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b5afc9aa309
Add new internal DTD content types r=ckerschb
Assignee | ||
Comment 27•5 years ago
|
||
https://phabricator.services.mozilla.com/D34187 would also need to be pushed
Assignee | ||
Updated•5 years ago
|
Comment 28•5 years ago
|
||
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9276e88ea3fe
Avoid leaking browser language via DTD r=Gijs,bzbarsky
Comment 29•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3b5afc9aa309
https://hg.mozilla.org/mozilla-central/rev/9276e88ea3fe
Updated•5 years ago
|
Description
•