902580 - Privacy enhancement: Create option for local time disclosure via Message-ID header

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Jacob Appelbaum]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36

Steps to reproduce:

Please see #776397 for details on previous work on this topic. We wanted to break the older bug into two different bugs - this should help with some confusion and to ensure that the discussion stays focused.

For this bug, we wish to add an option that allows a user to change the construction of the Message-ID header in outgoing email. This header discloses the local clock/timing information and when Thunderbird is composed with The Tor Network ala TorBirdy, we find that this is one of two major issues leaking information about a user's computer. Please see bug #902573 for more details on the related Date header information disclosure issue.

We think that it is reasonable to offer two possible states - there may be other states:

  Keep the Message-ID header as it is now (eg: leak timing information to the network)
  Randomize the time related Message-ID header construction bytes (eg: don't leak clock in every message)
  Remove the Message-ID header entirely (and let SMTP or NNTP sort it out - eg: what already happens without a valid hostname)

Keeping the Message-ID header would be the default state. It requires little discussion, we think.

Randomizing the bits in the Message-ID header that are time dependent requires some discussion. Generally, we support a design where we replace the clock with random data taken from the system entropy pool.

Removing the Message-ID entirely requires more discussion - what happens in this case?

We'll attach a patch that allows for configuration of the Message-ID header state shortly.

Comment 1

[Sukhbir Singh]

Attachment: Patch for preventing local timestamp disclosure via message-ID header

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 787176 [details] [diff] [review]
Patch for preventing local timestamp disclosure via message-ID header

Just to re-summarise my comments on the other bug for easy reference here:

Setting a preference to set a value in an outgoing message isn't acceptable. This needs to be a proper extension hook with an explicit call to get the value.

The issue is that you might get (as in the case of Lightning), emails being sent in the background or simply just being saved to folders, and you need to catch all of those. An explicit call makes that easy, listening to various notifications and hoping you've got the right ones when you set the preference doesn't.

Comment 3

[Sukhbir Singh]

How would you like this patch to be designed? The reason I ask is so that we can work on it accordingly, based on what you think is an acceptable change to the Message-ID header. 

Just to reiterate: "Generally, we support a design where we replace the clock with random data taken from the system entropy pool." Will such a patch that changes Thunderbird's message-ID format be acceptable or should we work on a TorBirdy-specific solution instead?

Comment 4

[Mark Banner (:standard8)]

I have already explained how I would like it designed in comment 2.

Comment 5

[Wayne Mery (:wsmwk)]

sukhbir, are you still interested in working on this?

Comment 6

[Magnus Melin [:mkmelin]]

I'm dont' think comment 2 applies to this patch. It changes msg_generate_message_id which is a direct change, not listening to a notification.

Comment 7

[Arthur Edelstein [:arthur]]

Attachment: bug902580.diff

In this new patch, when the "privacy.resistFingerprinting" pref is enabled, the two non-host parts of Message-ID are set to "high entropy" randomized hex strings. No local date-time is leaked. For example:

Message-ID: <[e-mail]>

Also, for extra credit, the MIME boundaries also get high-entropy random hex strings.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8669372 [details] [diff] [review]
bug902580.diff

Sorry, I'm no longer active on the Thunderbird project. Passing to someone who is.

Comment 9

[Magnus Melin [:mkmelin]]

I don't know if this actually needs to be behind a pref.

xref bug 765897.

Comment 10

[Arthur Edelstein [:arthur]]

Attachment: bug902580.diff

Here is a new version of the patch that uses securely random hexadecimal strings by default. No pref is involved.

Comment 11

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8682974 [details] [diff] [review]
bug902580.diff

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

Some notes on style:
In C++ use code, braces go on their own line, and we use Foo *bar; instead of Foo* bar;

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +516,5 @@
>  static void
>  GenerateGlobalRandomBytes(unsigned char *buf, int32_t len)
>  {
> +  nsCOMPtr<nsIRandomGenerator> randomGenerator(do_GetService("@mozilla.org/security/random-generator;1"));
> +  if (randomGenerator) {

Rather than making this an if statement if it exists, assert the existence of the object using MOZ_ASSERT.

@@ +518,5 @@
>  {
> +  nsCOMPtr<nsIRandomGenerator> randomGenerator(do_GetService("@mozilla.org/security/random-generator;1"));
> +  if (randomGenerator) {
> +    uint8_t* tempBuffer;
> +    randomGenerator->GenerateRandomBytes(len, (uint8_t**) &tempBuffer);

I'm nervous about this code since it turns randomness from a LCPRG seeded with a weak seed but is guaranteed to always work to something that isn't.

@@ +926,2 @@
>    return PR_smprintf("<%lX.%lX@%s>",
> +           (unsigned long) salt1, (unsigned long) salt2, host);

It's more efficient to generate a single 64-bit integer rather than two 32-bit integers.

In any case, I think the message-ID needs to be longer than just 16 bytes in the local part. UUID generation does seem to be a popular way of generating the localpart.

@@ +1423,5 @@
> +        for (int32_t i = 0; i < 8; i++) {
> +          char* hex = PR_smprintf("%02x", filePrefix[i]);
> +          filename.Append(hex);
> +          PR_smprintf_free(hex);
> +        }

So the problem here is that you're changing code that produces an alphabetical filename to one that produces a hexadecimal filename, and I don't think I want that change.

Comment 12

[Arthur Edelstein [:arthur]]

Attachment: bug902580.diff

Hi Joshua -- thanks for the review. I've made revisions to the patch to try to address all of your comments. Now if the high-entropy random number generator fails, we fall back to the low-entropy PRNG.

Comment 13

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8685096 [details] [diff] [review]
bug902580.diff

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

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +934,5 @@
>    /* If we couldn't find a valid host name to use, we can't generate a
>       valid message ID, so bail, and let NNTP and SMTP generate them. */
>      return 0;
>  
> +  // Generate 128-bit UUID for the local part (using the high-entropy GenerateGlobalRandomBytes).

There is an nsIUUIDGenerator that you can use to generate the string. See for example <https://dxr.mozilla.org/comm-central/source/mozilla/dom/datastore/DataStoreService.cpp#1418>. (Although you can probably just say "return id.ToString()" instead of using ToProvidedString).

Comment 14

[Arthur Edelstein [:arthur]]

Attachment: bug902580.diff

(In reply to Joshua Cranmer [:jcranmer] from comment #13)
> Comment on attachment 8685096 [details] [diff] [review]
> bug902580.diff
> 
> Review of attachment 8685096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/compose/src/nsMsgCompUtils.cpp
> @@ +934,5 @@
> >    /* If we couldn't find a valid host name to use, we can't generate a
> >       valid message ID, so bail, and let NNTP and SMTP generate them. */
> >      return 0;
> >  
> > +  // Generate 128-bit UUID for the local part (using the high-entropy GenerateGlobalRandomBytes).
> 
> There is an nsIUUIDGenerator that you can use to generate the string. See
> for example
> <https://dxr.mozilla.org/comm-central/source/mozilla/dom/datastore/
> DataStoreService.cpp#1418>. (Although you can probably just say "return
> id.ToString()" instead of using ToProvidedString).

Thanks again for the review. Here's a new version that uses the nsID to generate the UUID string. I'm still using GenerateGlobalRandomBytes because nsUUIDGenerator is not guaranteed to be high-entropy.

Comment 15

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8687635 [details] [diff] [review]
bug902580.diff

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

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +520,5 @@
> +  // Attempt to generate bytes from system entropy-based RNG.
> +  nsCOMPtr<nsIRandomGenerator> randomGenerator(do_GetService("@mozilla.org/security/random-generator;1"));
> +  MOZ_ASSERT(randomGenerator, "nsIRandomGenerator service not retrievable");
> +  uint8_t *tempBuffer;
> +  nsresult rv = randomGenerator->GenerateRandomBytes(len, (uint8_t**) &tempBuffer);

You shouldn't need a cast here.

Comment 16

[Arthur Edelstein [:arthur]]

Attachment: bug902580.diff

(In reply to Joshua Cranmer [:jcranmer] from comment #15)
> Comment on attachment 8687635 [details] [diff] [review]
> bug902580.diff
> 
> Review of attachment 8687635 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/compose/src/nsMsgCompUtils.cpp
> @@ +520,5 @@
> > +  // Attempt to generate bytes from system entropy-based RNG.
> > +  nsCOMPtr<nsIRandomGenerator> randomGenerator(do_GetService("@mozilla.org/security/random-generator;1"));
> > +  MOZ_ASSERT(randomGenerator, "nsIRandomGenerator service not retrievable");
> > +  uint8_t *tempBuffer;
> > +  nsresult rv = randomGenerator->GenerateRandomBytes(len, (uint8_t**) &tempBuffer);
> 
> You shouldn't need a cast here.

Thank you for the review. Here is the revised patch, with the cast removed.

Comment 17

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/a8573d4c67292962f9dd9b8f51496e9f62bbedb7
Bug 902580 - Use securely random Message-ID to avoid fingerprinting. r=jcranmer

Comment 18

[Rich Wales]

Is there a way for users (like me) who do not require or want a securely random Message-ID value to continue using the previous Message-ID format based on the clock?

Comment 19

[Magnus Melin [:mkmelin]]

No. What would be the use case for something like that? 
Note also that what we were using was too short (bug 765897)