1603359 - Option to prevent local time disclosure in Date header

Status: RESOLVED FIXED

(Thunderbird :: Preferences, enhancement)

Description

[segfault]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Comment 1

[segfault]

This is about the same feature that was requested in bug 902573, which was closed because the original authors found a way to solve this in the TorBirdy extension. Because TorBirdy is not compatible with current Thunderbird anymore, and the new MailExtensions API doesn't allow extensions to do this kind of modification, Thunderbird should provide an option for that.

Comment 2

[segfault]

Attachment: Avoid-local-timestamp-disclosure-in-Date-header.patch

This is another patch created by Tails to replace part of the functionality previously provided by TorBirdy, in this case to sanitize the Date header to prevent fingerprinting.

Comment 3

[segfault]

This patch will conflict with the one of bug 1370217, because both add preferences to comm/mailnews/mailnews.js. I can update the other patch once one of them was accepted.

Comment 4

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9115396 [details] [diff] [review]
Avoid-local-timestamp-disclosure-in-Date-header.patch

Review of attachment 9115396 [details] [diff] [review]:
-----------------------------------------------------------------

This is in the wrong place. jsmime.js shouldn't read prefs using Services. It's meant to be a general purpose library... We have jsmime.jsm for intgrations, but I'm not sure that's right either. Could be better to fix this at the caller, but creating a tidied up date there, instead of trying to correct it in the emitting phase. 

Also, please add a test.

Comment 5

[segfault]

Could be better to fix this at the caller, but creating a tidied up date there, instead of trying to correct it in the emitting phase.

I now have a patch ready to sanitize the date in nsMsgCompUtils.cpp. But the date string is parsed to a JS date object and converted back to a string in jsmime.js, which means that the timezone information is lost, and the final date string is in the local timezone again. So we do have to strip off the timezone information in jsmime.js or jsmime.jsm - and I don't know how to do that in jsmime.jsm. Should I somehow override the HeaderEmitter.prototype.addDate function there? Could you give me a hint how I can do that?

Comment 6

[Magnus Melin [:mkmelin] (away until Aug 10)]

Maybe you can rely on this: https://searchfox.org/comm-central/rev/31a8ac5c20c4ed20d064ea5b51c72956d1944235/mailnews/mime/jsmime/jsmime.js#1402,1449-1453 - that is, in nsMsgCompUtils.cpp produce a suitable date string which would set timezone to 0?

Comment 7

[segfault]

Maybe you can rely on this: https://searchfox.org/comm-central/rev/31a8ac5c20c4ed20d064ea5b51c72956d1944235/mailnews/mime/jsmime/jsmime.js#1402,1449-1453 - that is, in nsMsgCompUtils.cpp produce a suitable date string which would set timezone to 0?

Unfortunately, that doesn't work, because in any case the date object is created in the local timezone, and, as stated in the function's comment, JS date objects cannot retain the timezone information: https://searchfox.org/comm-central/rev/31a8ac5c20c4ed20d064ea5b51c72956d1944235/mailnews/mime/jsmime/jsmime.js#1383-1387,1459-1465

Comment 8

[Magnus Melin [:mkmelin] (away until Aug 10)]

I don't think I understand. If you in cpp create and convert the date string such that it's the UTC time with 00 timezone indicated (or missing). Don't you get the thing you want? The date is created in the local timezone, but the code adjusts for the local timezone, so the "time" would be right no?

Comment 9

[segfault]

Sure, the time would be right, but it does not contain information that it's in UTC (because JS date objects don't store that information). So when that date is finally added to the header, the local timezone is added again: https://searchfox.org/comm-central/rev/31a8ac5c20c4ed20d064ea5b51c72956d1944235/mailnews/mime/jsmime/jsmime.js#3463

The only way I see to change that is by changing the code in the addDate function, which I did in my first patch.

Comment 10

[segfault]

Attachment: 0001-Bug-1603359-Add-pref-mail.sanitize_date_header.patch

How about this? I extracted the code which encodes the date object to a string to a new function in jsmime.jsm.

I didn't add a test yet, because I would first like to know if you're ok with that solution.

Comment 11

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9121760 [details] [diff] [review]
0001-Bug-1603359-Add-pref-mail.sanitize_date_header.patch

Review of attachment 9121760 [details] [diff] [review]:
-----------------------------------------------------------------

::: comm/mailnews/mime/jsmime/jsmime.js
@@ +3434,4 @@
>       * @param {Date} date The date to be added to the output string.
>       */
>      HeaderEmitter.prototype.addDate = function(date) {
> +      let dateStr = EncodeDate(date, mimeutils);

this would mean jsmime couldn't be used as is, which it needs to be

Comment 12

[Magnus Melin [:mkmelin] (away until Aug 10)]

Sorry I don't know what the best approach here would be. Maybe the headers should be just parsed and reprocessed at a later stage if that pref is set.

Comment 13

[segfault]

Attachment: 0001-Bug-1603359-Add-pref-mail.sanitize_date_header.patch

Maybe the headers should be just parsed and reprocessed at a later stage if that pref is set.

I think that would require assuming the header format on the receiver site and rewriting the parsing function in C++, and that does not seem like a great solution to me.

How about this instead? I added an optional argument to the JS function which encodes the headers and is called by nsMsgSend.cpp.

Comment 14

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9123227 [details] [diff] [review]
0001-Bug-1603359-Add-pref-mail.sanitize_date_header.patch

Review of attachment 9123227 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like a way forwards. Please add a test, maybe in https://searchfox.org/comm-central/source/mailnews/mime/jsmime/test/unit/test_header_emitter.js

::: comm/mailnews/mailnews.js
@@ +28,5 @@
>  pref("mail.suppress_content_language", false);
> +// hidden pref for controlling if the Date header is sanitized, by:
> +// 1. Converting the date to UTC, to prevent leaking the local time zone.
> +// 2. Rounding the date down to the most recent whole minute, to prevent
> +//    fingerprinting of small clock offsets.

Hmm, are you sure you want that? Because *that* would surely stand out if someone always has 0 seconds on their messages.

Comment 15

[segfault]

Attachment: 0001-Bug-1603359-Add-pref-mail.sanitize_date_header.patch

Seems like a way forwards. Please add a test, maybe in https://searchfox.org/comm-central/source/mailnews/mime/jsmime/test/unit/test_header_emitter.js

Sorry for the late reply, I had quite a lot of trouble to build Thunderbird with tests.

I now added a test to comm/mailnews/mime/test/unit/test_structured_headers.js, is that good enough?

::: comm/mailnews/mailnews.js
@@ +28,5 @@

pref("mail.suppress_content_language", false);
+// hidden pref for controlling if the Date header is sanitized, by:
+// 1. Converting the date to UTC, to prevent leaking the local time zone.
+// 2. Rounding the date down to the most recent whole minute, to prevent
+// fingerprinting of small clock offsets.

Hmm, are you sure you want that? Because that would surely stand out if someone always has 0 seconds on their messages.

Yes, I'm sure that's what we want. You are right that it will be obvious from the headers that the user has this option activated, but we assume that the anonymity set of all users who have this option activated is still bigger than that of users with a specific clock offset. And it's the behavior TorBirdy had for years, which we want to preserve.

Comment 16

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9131340 [details] [diff] [review]
0001-Bug-1603359-Add-pref-mail.sanitize_date_header.patch

Review of attachment 9131340 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good, just some minor things left. Please for the commit message make the first line describe what it's for.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +720,5 @@
>      nsAutoCString headers;
> +    bool bSanitizeDate = false;
> +    nsCOMPtr<nsIPrefBranch> prefs(
> +        do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
> +    prefs->GetBoolPref("mail.sanitize_date_header", &bSanitizeDate);

this would need some error checking, but better to just use

bool sanitizeDate = Preferences::GetBool("mail.sanitize_date_header", false));

::: mailnews/mailnews.js
@@ +23,4 @@
>  // hidden pref for controlling if the Content-Language header
>  // should be set.
>  pref("mail.suppress_content_language", false);
> +// hidden pref for controlling if the Date header is sanitized, by:

We can just say:
// Pref for ....

::: mailnews/mime/public/msgIStructuredHeaders.idl
@@ +107,2 @@
>     * interface, since the JSMime headeremitter functionality allows more
>     * fine-grained customization of the results.

please here in the idl document what the param does
@param sanitizeDate - If true convert the date to UTC and round to closest minute.

Comment 17

[segfault]

Attachment: 0001-Bug-1603359-Add-pref-mail.sanitize_date_header.patch

Comment 18

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9133024 [details] [diff] [review]
0001-Bug-1603359-Add-pref-mail.sanitize_date_header.patch

Review of attachment 9133024 [details] [diff] [review]:
-----------------------------------------------------------------

Fixed the compile errors and run ../mach eslint --fix mailnews

After that, looks good thx, r=mkmelin

::: mailnews/compose/src/nsMsgSend.cpp
@@ +719,4 @@
>      // Convert the blocks of headers into a single string for emission.
>      nsAutoCString headers;
> +    bool sanitizeDate = Preferences::GetBool("mail.sanitize_date_header", false));
> +    outputHeaders->BuildMimeText(bSanitizeDate, headers);

some compile errors here

Comment 19

[Magnus Melin [:mkmelin] (away until Aug 10)]

Attachment: 0001-Bug-1603359-Add-pref-mail.sanitize_date_header.patch

Comment 20

[segfault]

Thanks!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/98aa0bf2e719
Add pref mail.sanitize_date_header that if set converts date to UTC and rounds to closest minute. r=mkmelin