1539758 - Convert consumers using inline HTML markup in browser.dtd to fluent and/or to not use inline markup

Status: RESOLVED FIXED

(Firefox :: General, task)

Description

[:Gijs (he/him)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1538007#c26 and https://bugzilla.mozilla.org/show_bug.cgi?id=1538007#c30 .

Comment 1

[:Gijs (he/him)]

bug 1523757 deals with the addon post install notification, at least.

Comment 2

[:Gijs (he/him)]

Brian: this bug is the reason for my comment at https://phabricator.services.mozilla.com/D24914#753802 .

Comment 3

[Brian Grinstead [:bgrins]]

I think https://phabricator.services.mozilla.com/D24914 will be ready to land after review.

Comment 4

[Daniel Veditz [:dveditz]]

Is this bug limited to browser.dtd specifically, or does it mean ".dtd used by Firefox" generically? If the former we need to file another bug about the general case.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Daniel Veditz [:dveditz] from comment #4)

Is this bug limited to browser.dtd specifically, or does it mean ".dtd used by Firefox" generically? If the former we need to file another bug about the general case.

It means any DTD used in a chrome-privileged document.

Comment 6

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #5)

(In reply to Daniel Veditz [:dveditz] from comment #4)

Is this bug limited to browser.dtd specifically, or does it mean ".dtd used by Firefox" generically? If the former we need to file another bug about the general case.

It means any DTD used in a chrome-privileged document.

Yeah, this. bug 1523757 will deal with some of the ones in browser.dtd, Brian's patch on bug 1495861 deals with the other ones, I think, and bug 1523741 will hopefully deal with the about telemetry ones this week.

Between those bugs, I'm hopeful that we can do this for 68. Because we're touching l10n, I'm not sure about uplifting to 67. In principle I believe we could because we have migration scripts to convert existing l10n stuff to fluent, but we'd need to check that with flod/gandalf. I doubt we can realistically uplift any of this work to 60esr, but given this is sec-want that doesn't seem too bad?

Comment 7

[:Gijs (he/him)]

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

(In reply to Kris Maglione [:kmag] from comment #5)

(In reply to Daniel Veditz [:dveditz] from comment #4)

Is this bug limited to browser.dtd specifically, or does it mean ".dtd used by Firefox" generically? If the former we need to file another bug about the general case.

It means any DTD used in a chrome-privileged document.

Yeah, this. bug 1523757 will deal with some of the ones in browser.dtd, Brian's patch on bug 1495861 deals with the other ones, I think, and bug 1523741 will hopefully deal with the about telemetry ones this week.

To be clear, I think that's everything.

https://searchfox.org/mozilla-central/search?q=%3C%5Ba-z%5D&case=true&regexp=true&path=.dtd

has 9 hits in browser.dtd, the first 3 in comments, then 3 for the panic panel that Brian's patch deals with, then 2 more comments, then an add-on install message bug 1523757 should deal with.

We don't (shouldn't) load the network/cert/phishing error page DTDs (from any directory) in chrome privileged docs.

about:mozilla and the video controls are unprivileged.

Comment 8

[:Gijs (he/him)]

Fixed in 68 by the various deps.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

IIUC, we're not intending to try to backport this work to 67 at this point, but feel free to correct me if that's wrong.

Comment 10

[Kris Maglione [:kmag]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

IIUC, we're not intending to try to backport this work to 67 at this point, but feel free to correct me if that's wrong.

It's quite a large set of changes, mostly to locale strings. I don't think we could ask localizers to deal with that in an uplift. So, yeah, a backport doesn't seem likely.

Comment 11

[Daniel Veditz [:dveditz]]

but given this is sec-want that doesn't seem too bad?

keyword fail/limitation. This is a "task" to break a sec-high sandbox escape rather than a vulnerability in its own right. We "want" it really really badly. It didn't seem fair or accurate to pile on a dozen "sec-high" sub-tasks for a single sandbox escape, but maybe that would have helped set the right level of urgency.