Closed Bug 467035 Opened 11 years ago Closed 5 months ago

<!DOCTYPE> ignores contentaccessible, leaks DTD strings and therefore browser UI locale

Categories

(Core :: XML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: neil, Assigned: acat)

References

(Depends on 1 open bug, Blocks 1 open bug, )

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.
[sg:low] -- disclosure of information that is unlikely to be sensitive.  (But strings from a private extension could be confidential.)
Whiteboard: [sg:low]
Flags: wanted1.9.0.x+
(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.
Keywords: testcase
Summary: <!DOCTYPE> ignores contentaccessible, leaks information about installed extensions → <!DOCTYPE> ignores contentaccessible, leaks DTD strings and therefore browser UI locale
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
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
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.
Assignee: nobody → tihuang
Priority: -- → P2
Whiteboard: [sg:low] → [sg:low][fingerprinting][fp-triaged]
Whiteboard: [sg:low][fingerprinting][fp-triaged] → [sg:low][fingerprinting][fp-triaged][tor 30304]

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 :)

Flags: needinfo?(ckerschb)

(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 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?

Gijs is the go to person for those kinds of questions - 302 to him!

Flags: needinfo?(ckerschb) → needinfo?(gijskruitbosch+bugs)

(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 in nsChromeRegistry::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 load chrome:// 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 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.

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.

Flags: needinfo?(gijskruitbosch+bugs)

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?

Attachment #9062444 - Attachment is obsolete: true

(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 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.

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?

Flags: needinfo?(ckerschb)
Flags: needinfo?(bgrinstead)

(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 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.

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 videocontrols, perhaps we could do one of:

  1. Expose a [Func="IsChromeOrXBLOrUAWidget"] method on DOMParser called parseFromStringNonNullPrincipal or similar from videocontrols UA widget via: https://firefox-source-docs.mozilla.org/toolkit/content/toolkit_widgets/ua_widget.html#the-javascript-sandbox.
  2. 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.
  3. 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.
Flags: needinfo?(bgrinstead)

(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!

Flags: needinfo?(ckerschb)

(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...

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.

Flags: needinfo?(MattN+bmo)

(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.

Flags: needinfo?(MattN+bmo)
Keywords: checkin-needed

Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b5afc9aa309
Add new internal DTD content types r=ckerschb

Keywords: checkin-needed
Keywords: checkin-needed

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9276e88ea3fe
Avoid leaking browser language via DTD r=Gijs,bzbarsky

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Whiteboard: [sg:low][fingerprinting][fp-triaged][tor 30304] → [sg:low][fingerprinting][fp-triaged][tor 30304][adv-main70-]
You need to log in before you can comment on or make changes to this bug.