Closed Bug 1603359 Opened 8 months ago Closed 5 months ago

Option to prevent local time disclosure in Date header

Categories

(Thunderbird :: Preferences, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 76.0

People

(Reporter: segfault, Assigned: segfault)

Details

Attachments

(1 file, 5 obsolete files)

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

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.

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.

Attachment #9115396 - Flags: review?(mkmelin+mozilla)

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.

Assignee: nobody → segfault
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #9115396 - Flags: review?(mkmelin+mozilla) → review-

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?

Flags: needinfo?(mkmelin+mozilla)

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?

Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(mkmelin+mozilla)

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?

Flags: needinfo?(mkmelin+mozilla)

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.

Flags: needinfo?(mkmelin+mozilla)

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.

Attachment #9115396 - Attachment is obsolete: true
Attachment #9121760 - Flags: review?(mkmelin+mozilla)
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
Attachment #9121760 - Flags: review?(mkmelin+mozilla) → review-

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.

Flags: needinfo?(mkmelin+mozilla)

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.

Attachment #9121760 - Attachment is obsolete: true
Attachment #9123227 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9123227 - Flags: review?(mkmelin+mozilla)

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.

Attachment #9123227 - Attachment is obsolete: true
Attachment #9131340 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9131340 - Flags: review?(mkmelin+mozilla) → feedback+
Attachment #9131340 - Attachment is obsolete: true
Attachment #9133024 - Flags: review?(mkmelin+mozilla)
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
Attachment #9133024 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Thunderbird 76.0

Thanks!

Pushed by geoff@darktrojan.net:
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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.