2000815 - Limit number of empty lines rendered at the end of a message

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, enhancement, P3)

Description

[Kai Engert [:KaiE:]]

We should limit the number of empty lines that are rendered at the end of a message.

Currently, if I have a message with 10 million newlines at the end, TB will happily render that.

The scenario I'm mostly worried about is related to the support for unobtrusive signatures.

The working group asks that we normalize the input, and simply ignore all additional newlines at the end of the message.

My worry is: An adversary could inject additional newlines into the message. TB would render the message with a valid signature status. But the scrollbar would be mostly unusable for the real content. And if the user sees there is so much empty space at the end, the user might think "this is a signed message, so this is really what the sender sent to me, let me scroll down whether there is anything of importance in addition".

But this is too hard to check manually.

For that reason, I suggest that we limit the number of newlines at the end of the message that we render, to maybe 100 or 1000, and cut off everything else, and not render it.

Comment 1

[Kai Engert [:KaiE:]]

After a discussion, it seems this applies only to email messages that contain of a single text/plain part.

If there are any other parts, those would be part of the whitespace of MIME containers, or of a HTML encoded part, which means the newlines aren't rendered.

So the potential to abuse seems to be almost none. It's just an annoyance.

Comment 2

[Kai Engert [:KaiE:]]

The stripping would also be useful when forwarding inline, or when replying to such a message.

Comment 3

[Kai Engert [:KaiE:]]

I think it would be worth fixing this bug. I hope it would be straightforward and not a lot of work.

It would be nice to get help with this bug. Testing how Thunderbird really behaves, when rendering, forwarding, replying.

Comment 4

[Kai Engert [:KaiE:]]

We have similar bugs that also wait for attention, and good ideas, like bug 1462488

Comment 5

[Max Emig [:maxe]]

(In reply to Kai Engert [:KaiE:] from comment #3)

I think it would be worth fixing this bug. I hope it would be straightforward and not a lot of work.

It would be nice to get help with this bug. Testing how Thunderbird really behaves, when rendering, forwarding, replying.

That's pretty close to my core work, should be fairly straightforward.

Which cut off value do you suggest? How do other clients do it in practice?

Comment 6

[Kai Engert [:KaiE:]]

Great, thanks for your interest!

I haven't checked yet what other clients do. Part of the work is probably do play around with it.

I think for rendering purposes it's probably fine to cut off all of them. But the scanning couldn't simply stop after e.g. 1000 lines, we'd have to be certain that no non-whitespace following later is suppressed.

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to Kai Engert [:KaiE:] from comment #0)

My worry is: An adversary could inject additional newlines into the message. TB would render the message with a valid signature status. But the scrollbar would be mostly unusable for the real content. And if the user sees there is so much empty space at the end, the user might think "this is a signed message, so this is really what the sender sent to me, let me scroll down whether there is anything of importance in addition".

Let's not conflate signing with "message contains truth". Signing ONLY says the message was sent by an entity with a certain key. They may well have sent nonsense or a lie.

Comment 8

[Magnus Melin [:mkmelin]]

I'd like to point out a huge scroll can be achieved in many, many ways. What about a 20000px high empty div? What about messages containing spaces mixed with invisible characters or similar things? A blank huge image?

Comment 9

[Justin Tracey [:jtracey]]

(In reply to Magnus Melin [:mkmelin] from comment #8)

I'd like to point out a huge scroll can be achieved in many, many ways. What about a 20000px high empty div? What about messages containing spaces mixed with invisible characters or similar things? A blank huge image?

Those things would be detected by signature validation though (i.e., if those things are in the message, then the sender who signed really sent them). The point here is that an adversary, who does not have access to the signer's key, can add/remove trailing newlines without modifying what is signed, because

The working group asks that we normalize the input, and simply ignore all additional newlines at the end of the message.

The question is whether that could actually be used to convey anything malicious in practice. I suspect the best they could do is mildly annoy the recipient. E.g., a malicious mail provider could make it look like the sender sent the recipient a message gigabytes in size. Which isn't nothing, but not an especially high priority problem.

Comment 10

[Max Emig [:maxe]]

If you decide that you actually want this, I would implement the following part of the RFC 6376 spec, intended for accurate DKIM validation:

3.4.3.  The "simple" Body Canonicalization Algorithm

   The "simple" body canonicalization algorithm ignores all empty lines
   at the end of the message body.  An empty line is a line of zero
   length after removal of the line terminator.  If there is no body or
   no trailing CRLF on the message body, a CRLF is added.  It makes no
   other changes to the message body.  In more formal terms, the
   "simple" body canonicalization algorithm converts "*CRLF" at the end
   of the body to a single "CRLF".

   Note that a completely empty or missing body is canonicalized as a
   single "CRLF"; that is, the canonicalized length will be 2 octets.

   The SHA-1 value (in base64) for an empty body (canonicalized to a
   "CRLF") is:

   uoq1oCgLlTqpdDX/iUbLy7J1Wic=

   The SHA-256 value is:

   frcCV1k9oG9oKj3dpUqdJg1PxRT2RSN/XKdLCPjaYaY=

I can build this, but I can't take away the decision whether you actually want this. I would also suggest adding a way to opt-out of this via about:config.

Comment 11

[Max Emig [:maxe]]

Apple Mail strips away all trailing empty lines, yet also gives up on rendering entirely with too many empty lines; I haven't bothered getting the exact cutoff, though.

I think this is a good idea actually, yet more like UI/UX-wise, not necessarily with security as the main reason behind.

Comment 12

[Magnus Melin [:mkmelin]]

(In reply to Max Emig from comment #10)
That sounds like a good idea to me.
If we don't do it already, we should probably perform such normalization at compose time as well.

Comment 13

[Kai Engert [:KaiE:]]

(In reply to Max Emig from comment #10)

If you decide that you actually want this, I would implement the following part of the RFC 6367 spec, intended for accurate DKIM validation:

Note you have a typo, the correct RFC number is 6376:
https://www.rfc-editor.org/info/rfc6376/#section-3.4.3

And FYI, we're proposing the same in the unobtrusive specification:
https://www.ietf.org/archive/id/draft-ietf-mailmaint-unobtrusive-signatures-02.html#name-canonicalization

And it's implemented already for the signature calculation and verification part when creating and processing messages with that structure:
https://hg-edge.mozilla.org/comm-central/rev/7f9b00b0122a#l3.232
https://hg-edge.mozilla.org/comm-central/rev/dbee5a3fd41e#l9.55

I can build this, but I can't take away the decision whether you actually want this. I would also suggest adding a way to opt-out of this via about:config.

Great!

I think it's a good idea to be able to control that using a hidden preference while testing, but it feels like the pref could get removed after a few months of testing. We'll see.

Comment 14

[Max Emig [:maxe]]

Attachment: Bug 2000815 - Stop rendering trailing empty lines in plain text message bodies. r=#thunderbird-back-end-reviewers

Default unchanged, gated behind mail.collapse_trailing_blank_lines.

Comment 15

[Max Emig [:maxe]]

And FYI, we're proposing the same in the unobtrusive specification:
https://www.ietf.org/archive/id/draft-ietf-mailmaint-unobtrusive-signatures-02.html#name-canonicalization

I genuinely did not reference this draft, this was just the closest I could find to your proposal in the bug report.

Comment 16

[Kai Engert [:KaiE:]]

Attachment: test message 1, text/plain

Comment 17

[Kai Engert [:KaiE:]]

Attachment: test message 2, text/plain inside unobtrusive signature

Comment 18

[Kai Engert [:KaiE:]]

Attachment: test message 3, text/html inside unobtrusive signature

Comment 19

[Kai Engert [:KaiE:]]

I think test 1 and test 2 must be covered by the change.

I guess test 3 is optional, you could decide to also not render the html encoded linefeeds, I think that's up for discussion.

Comment 20

[Max Emig [:maxe]]

I think test 1 and test 2 must be covered by the change.

They are

Comment 21

[Kai Engert [:KaiE:]]

This patch already covers replies, right?
(It did when I tested earlier)

If "forward inline" isn't done yet, could you please file a follow-up bug to get that done?
Or would you like to do it as part of this bug immediately?

Comment 22

[Max Emig [:maxe]]

This patch already covers replies, right?

Yes.

If "forward inline" isn't done yet, could you please file a follow-up bug to get that done?

I will look into that, but would prefer a follow-up patch.

Comment 23

[Kai Engert [:KaiE:]]

(In reply to Max Emig from comment #22)

I will look into that, but would prefer a follow-up patch.

Thanks, that's fine with me, just want to make sure it's tracked. If there's a chance it might land in a future release, we'll need a separate tracking bug anyway.

Comment 24

[Kai Engert [:KaiE:]]

Max, could you please add the comment that I suggested in the phab revision? With that, the patch would be ready for checkin.

Comment 25

[Max Emig [:maxe]]

(In reply to Kai Engert [:KaiE:] from comment #24)

Max, could you please add the comment that I suggested in the phab revision? With that, the patch would be ready for checkin.

There was some discussion on Phabricator whether we should have that about:config gate, I am reconsidering that right now.

Comment 26

[Kai Engert [:KaiE:]]

(In reply to Max Emig [:maxe] from comment #25)

There was some discussion on Phabricator whether we should have that about:config gate, I am reconsidering that right now.

I think we shouldn't block this bug based on that discussion.

I suggest to land, and consider to remove the pref later.

Comment 27

[Max Emig [:maxe]]

ACK

Comment 28

[Kai Engert [:KaiE:]]

Max, the bug is still blocked from landing, because there is a blocking reviewer (John) who has not yet reviewed.

Also, I had requested one comment line, because I didn't understand what that line does, could you please add that?

Comment 29

[Max Emig [:maxe]]

We should document mail.body_sanitize_trailing_blank_lines somewhere and decide when to remove it, I'd say before next year's ESR if no-one needs it.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0153becc0c43
Stop rendering trailing empty lines in plain text message bodies. r=kaie

Comment 31

[Francesco (no longer active, use PM for contact)]

(In reply to Max Emig [:maxe] from comment #29)

We should document mail.body_sanitize_trailing_blank_lines somewhere and decide when to remove it, I'd say before next year's ESR if no-one needs it.

Please file a new bug for this.