nsTreeSanitizer::SanitizeStyleSheet creates an unexpected result that removes a comment closing tag.

Given this input:

<!--
/* Font Definitions */
@font-face
	{font-family:"Cambria Math";
	panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
	{font-family:"Yu Gothic";
	panose-1:2 11 4 0 0 0 0 0 0 0;}
@font-face
	{font-family:"Yu Gothic";
	panose-1:2 11 4 0 0 0 0 0 0 0;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
	{margin:0mm;
	text-align:justify;
	text-justify:inter-ideograph;
	font-size:10.5pt;
	font-family:"Yu Gothic";}
span.17
	{mso-style-type:personal-compose;
	font-family:"Yu Gothic";
	color:windowtext;}
.MsoChpDefault
	{mso-style-type:export-only;
	font-family:"Yu Gothic";}
/* Page Definitions */
@page WordSection1
	{size:612.0pt 792.0pt;
	margin:99.25pt 30.0mm 30.0mm 30.0mm;}
div.WordSection1
	{page:WordSection1;}
-->

we get this output:

<!--
/* Font Definitions */
@font-face
	{font-family:"Cambria Math";
	panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
	{font-family:"Yu Gothic";
	panose-1:2 11 4 0 0 0 0 0 0 0;}
@font-face
	{font-family:"Yu Gothic";
	panose-1:2 11 4 0 0 0 0 0 0 0;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
	{margin:0mm;
	text-align:justify;
	text-justify:inter-ideograph;
	font-size:10.5pt;
	font-family:"Yu Gothic";}mso-style-type:personal-compose;
	font-family:"Yu Gothic";
	color:windowtext;}
.MsoChpDefault
	{mso-style-type:export-only;
	font-family:"Yu Gothic";}size:612.0pt 792.0pt;
	margin:99.25pt 30.0mm 30.0mm 30.0mm;}
div.WordSection1
	{page:WordSection1;}

The biggest issue we're facing is the omission of the close comment tag.
That would be my highest priority in this bug report, it leads to bug 1659362.

However, there is some other data corruption going on, too.

The issue has been reported with Thunderbird 78 - but it looks like the bug is still present in latest m-c.

Correction: this bug causes bug 1675507 (which is worse than bug 1659362).

Let me poke... that's odd.

Hmm, I think this could be turned into a security bug since it can be trivially turned into a sanitizer bypass:

<style>@page Foo { @import 'bar.css'; } div { width: 10px; }</style>

Ends up being:

<style> @import 'bar.css'; } div { width: 10px; }</style>

Which is pretty terrible. Daniel, can I have a rating for this? I think it's in the sec-high range, probably, actually, as bug 1602843 was sec-moderate, but that relied on the page re-serializing the DOM, which this doesn't require.

Kai, I'm ~sure the patch above will fix the issue in TB as well, but if you could double-check it'd be great.

Do we want to split the test out into a separate patch to land later? Or do you feel the patch make it pretty obvious how to work around the sanitizer?

Comment on attachment 9191219 [details]
Bug 1680084 - Use a more precise rule start for sanitization. r=heycam,#style

Security Approval Request

• How easily could an exploit be constructed based on the patch?: not super-trivially, I guess. I'll remove the test before landing.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Should apply cleanly-ish.
• How likely is this patch to cause regressions; how much testing does it need?: not much, simple fix to the sanitizer to not include stuff that we ended up ignoring.

Comment on attachment 9191219 [details]
Bug 1680084 - Use a more precise rule start for sanitization. r=heycam,#style

sec-approved to land as we want this in 84

Comment on attachment 9191219 [details]
Bug 1680084 - Use a more precise rule start for sanitization. r=heycam,#style

Beta/Release Uplift Approval Request

• User impact if declined: Sanitizer bypass.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Will attach a test-case asap.
• List of other uplifts needed: bug 1680558
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Tried to make a very minimal patch.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: see above
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): see above
• String or UUID changes made by this patch: none

Emilio, what is supposed to happen with a style definition like this:

<style><!-- @whatever {} --> </style>

With your change, after sanitizing I see

<style> @whatever {} </style>

You're removing the comment tags, but you are keeping the rules inside the comment.
Is that intended?

Is @whatever the right thing? Or you mean something like @font-face? With @whatever I see an empty stylesheet getting returned. With @font-face { ... } I see just the font-face rule which is the intended behavior.

Also, those rules do apply to the page, so should be kept unless I'm missing something. That comment is just there for backwards compat with UAs that don't support CSS (so the CSS isn't rendered)

I was using @whatever as a general placeholder for the general question.

Thanks for clarifying that it's intended to hide the rules from UAs that don't understand CSS.

It's probably not important for the security, so that's just another general question: If the comment tags were intentionally added by the document author - why not keep them?

Well, because we don't keep unknown stuff when sanitizing rules, the same reason why we don't keep the CSS comments or what not. We keep only what we know to be allowed.

(Those tokens are just invalid from the CSS parser's point of view, so we don't reason about them and drop them)

https://hg.mozilla.org/integration/autoland/rev/1a65440b05307d39d46e24ef856a1f3eabf5f8b6

I confirm this fix works for Thunderbird 78.

With the reproducer attached to bug 1675507, and the fixes from bug 1680558 and this bug applied to mozilla-esr78, then Thunderbird 78.x will correctly remove the <!-- comment opening tag, too.

(Previously, it kept <!-- and removed -->. Now it removes both.)

The base bug will need approvals and uplifting, too.

Well, the number of MUAs affected by incorrect removal of closing comment tag shows, that the presence of comment tags (albeit they're invalid from CSS perspective) affects message display especially in webmail applications.

Thus when TB removes both comment tags, it might affect the message display as well - i.e. in some MUAs, the original message might be shown differently than the forwarded message.

Is this really what we want ?

Removing malicious tags from CSS for security reasons is one thing, but removing otherwise innocent tags that were placed into the message by its author for backwards compatibility is completely different thing.

So instead, I'll suggest to keep both <!-- and --> instead of removing them.

https://hg.mozilla.org/integration/autoland/rev/1a65440b05307d39d46e24ef856a1f3eabf5f8b6
https://hg.mozilla.org/mozilla-central/rev/1a65440b0530

(In reply to Petr Hroudný from comment #22)

Removing malicious tags from CSS for security reasons is one thing, but removing otherwise innocent tags that were placed into the message by its author for backwards compatibility is completely different thing.

How does it matter? My understanding is that TB sanitizes received mail before displaying it, but TB doesn't care about the comments because it understands <style>. Similarly, we remove unknown rules, which other MUAs may understand, and I think that's ok.

It's not about displaying mail in TB - there was no problem even before this fix.

Bug 1675507 describes situation, when TB user forwards mail to 3-rd person, and this person uses some Webmail client to read mail.

So by removing comment tags, TB is altering mail mesages in transit which might negatively impact 3-rd parties.

Anyhow, I'm not too concerned about removing comments, that's also the behavior before bug 1602843. (And I don't know any MUA that would need the comments nowadays if it's rendering HTML emails).

Comment on attachment 9191219 [details]
Bug 1680084 - Use a more precise rule start for sanitization. r=heycam,#style

Approved for 84.0rc1 and 78.6esr.

https://hg.mozilla.org/releases/mozilla-beta/rev/5e2894744e33
https://hg.mozilla.org/releases/mozilla-esr78/rev/d4ff6884df02

We keep only what we know to be allowed.

+1

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Done

Reproduced the issue here using the testcase attached and old Nightly build from 2020-12-01, verified that this is fixed using Firefox 84.0.1, Firefox 85.0b4 and Firefox 78.6.0esr across platforms (Windows 10, Ubuntu 18.04 and macOS 11).

Tom, do you know when should I land the tests here? Thanks

Sorry for not giving a date when I sec-approved, you can land them now.

Tests. r=freddyb
https://hg.mozilla.org/integration/autoland/rev/d5f3b4467116c4d579e8b86b815faa6436c12a18
https://hg.mozilla.org/mozilla-central/rev/d5f3b4467116