1780361 - Consider dropping conditional CSS sanitization

Status: RESOLVED FIXED

(Thunderbird :: Security, task)

Description

[Emilio Cobos Álvarez (:emilio)]

It's imperfect in a way that doesn't seem trivially fixable at all:

• It doesn't strip @import "url" <media>; (no big deal, easy to fix)
• It doesn't strip viewport units.
• It doesn't strip calc() / min() / max() with percentages or viewport units / font-relative, which seems a lot harder to fix and creates the same effect as a viewport media query.

Additionally, it adds a lot of complexity to the existing sanitizer (it has a bunch of checks spread around to avoid doing things when in this mode).

Given that, I think we should remove it, unless there are any strong objections.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1780361 - Split conditional CSS sanitization into its own function. r=evilpie!

This was also completely untested, seems like.

Comment 2

[Magnus Melin [:mkmelin]]

I agree, it's not worth keeping.

Comment 3

[Tom Schuster (MoCo)]

Thank you Emilio for already writing this patch. I think this definitely makes the code easier to read. When I first started reading this code this feature definitely jumped out to me as not fitting very well into the rest of the code.

We are probably going to add complexity when fully implementing the HTML Sanitizer API anyway, so this will give us a bit of an offset.

Comment 4

[Frederik Braun [:freddy]]

There are use cases I have heard of, but not been involved with. If this isn't required for copy & paste or email or what not. Sure. Thanks!

Comment 5

[Kai Engert (:KaiE:)]

I'm not comfortable with simply allowing all conditional CSS, based on the reported scenarios to trick users.

If we're required to allowing it, then I think we need to change how Thunderbird displays digital signatures for html/css emails.

Comment 6

[Ben Bucksch (:BenB)]

(For reference: I'm the original author of the original sanitizer code in Gecko.)

Kai is saying we need this for displaying encrypted emails.

I understand that you're saying "We're not keeping our promise completely, so there's no point in even trying", but I think it's still better to do 80% of the work. The problem is inherent and doesn't go away just because we remove the flag. It just means that users who need it will be far more exposed than before.

I'd rather suggest to fix the cases that are fixable, and add a warning (in the API doc) for those cases that cannot reasonably be fixed.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

We do have stricter CSS sanitization as part of the tree sanitizer that is stricter and I'm not proposing to remove this. But having this weird "keep markup as-is, completely unsanitized, but remove only conditional rules from CSS" is quite unfortunate. Also, it's useless, in the sense that we still allow @import and that can load conditional CSS itself.

I had a proposed patch in bug 1603299 which would sanitize email more consistently across all Thunderbird, but that went unnoticed / unreviewed / something.

Anyways, I think this sanitizer option in particular is just bad. It prevents legitimate usecases (dark mode, responsive emails) from working, and doesn't protect against the problem it's supposed to protect.

Comment 8

[Ben Bucksch (:BenB)]

Bug 1603299

The bug is marked as security hole, so not even I can see it (even though I am in the TB council and wrote the sanitizer). That might be why it went unnoticed. But thanks for the patch (whatever it is).

The problem that Kai wants to fix is that we want to keep most presentational formatting as intended, but without the ability for an author to selectively hide/show parts of the email for specific kinds of recipients, but that all recipients see the same content. You seem to have an idea how to solve that. What do you propose?

Comment 9

[Kai Engert (:KaiE:)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

I had a proposed patch in bug 1603299 which would sanitize email more consistently across all Thunderbird, but that went unnoticed / unreviewed / something.

We did comment on that somewhat, but it seemed like an unfortunate solution.

We have a conflict of interest. One group of people wants thing to be as secure as possible (including me), and I'd very much appreciate full sanitization. But in my understanding, the consequence of that is that the content will lose all its beauty, it will look ugly. For me that's acceptable, But for other people at Thunderbird, that was unacceptable at the time we discussed it.

The current production code, even if it's incomplete, is a kind of middle ground. It removes the things that were considered worst, while still keeping most visual appealing styling.

I don't have a perfect solution, and I agree with Ben that the change requested in this bug is worse than what we have today.

I have attempted to come up with a new idea for this problem, I've described the idea in bug 1780423.

Comment 10

[Kai Engert (:KaiE:)]

Ben, I've cc'ed you on bug 1603299

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Ok, I kept the behavior for now, bout outside of nsIParserUtils.sanitize() / nsTreeSanitizer.

Comment 12

[Ben Bucksch (:BenB)]

Thanks, Emilio! I just skimmed superficially through the patch, but the new code looks far more sane than the current code. It makes sense that it's a separate function. This looks like a reasonable approach. Thank you.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc8b6c28edc7
Split conditional CSS sanitization into its own function. r=evilpie

Comment 14

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/bc8b6c28edc7