1433350 - As defense in depth, don't load user's name etc. into memory

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement, P3)

Description

[Arthur Edelstein [:arthur]]

In Tor Browser, we have a defense-in-depth privacy patch to prevent some code from loading the user's real name, username, windows domain and email address from being loaded into memory. Here's the original ticket and patch:
https://trac.torproject.org/13398
https://torpat.ch/13398

Perhaps we could turn off this code by default in Firefox as well? Or put it behind privacy.resistFingerprinting?

Comment 1

[Thorin [:thorin]]

> Perhaps we could turn off this code by default in Firefox as well? Or put it
> behind privacy.resistFingerprinting?

Disable it completely. There's no need for this information to be collected IMO. If if is needed for some technical reason (thunderbird compat?) then behind the RFP pref

Comment 2

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1433350-Don-t-load-full-name-email-etc.-except-i.patch

Here's a patch putting it under the MOZ_THUNDERBIRD flag.

Comment 3

[Arthur Edelstein [:arthur]]

try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df37254f7b079b18d68075fccdfa1b206ac32375

Comment 4

[Dave Townsend [:mossop]]

I'm not seeing any evidence that this component is ever instantiated or queried by Firefox code, can you explain more about what you're seeing here?

Comment 5

[Arthur Edelstein [:arthur]]

I don't think it is instantiated by Firefox code. After discussing with Richard (the patch's author) I think you're right that this patch is not as important now that extensions can't instantiate it anymore. The only value would be defense against a future internal use of nsIUserInfo.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8949294 [details] [diff] [review]
0001-Bug-1433350-Don-t-load-full-name-email-etc.-except-i.patch

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

So given that this component is unused in Firefox I think we should actually just remove it entirely from mozilla-central, that removes the possibility of a leak and makes our code smaller :)

Comment 7

[Dave Townsend [:mossop]]

Jorg, looks like the nsIUserInfo component is used in Thunderbird but I'd expect it to be easy to move into comm-central.

Comment 8

[Jorg K (CEST = GMT+2)]

We might be able to stop using it altogether given comments like:
  // nsIUserInfo may not be implemented on all platforms ...
Aceman, can you take a look, this is used in account creation. Let's also file a C-C bug for it.

Comment 9

[:aceman]

In m-c this seems to be used at least in https://dxr.mozilla.org/comm-central/source/mozilla/testing/modules/tests/xpcshell/test_mockRegistrar.js .

In Thunderbird we use the nsIUserInfo information to pre-fill a full name and email address when creating a new account.
This seems to be a convenience feature and there is always code (and comment) coping if getting the info from nsIUserInfo fails.

Note there is no use in Seamonkey.

While this interface is scriptable and accessible to addons, in Thunderbird addons (old-style, non-webextensions) can still get the information.
So I think we would survive if this interface got dropped. User will just need to fill it in manually if at all needed (he may want to put something else into the name).

Comment 10

[Jorg K (CEST = GMT+2)]

Aceman, could to prepare a patch to rip it all out and attach it here unless M-C people want us to file a separate bug.

Comment 11

[:aceman]

Attachment: remove uses in Thunderbird

Comment 12

[Jorg K (CEST = GMT+2)]

Thanks, Aceman.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 8953783 [details] [diff] [review]
remove uses in Thunderbird

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

Such a shame, but r=mkmelin

Comment 14

[Arthur Edelstein [:arthur]]

(In reply to :aceman from comment #9)

> So I think we would survive if this interface got dropped. User will just
> need to fill it in manually if at all needed (he may want to put something
> else into the name).

Instead of dropping nsIUserInfo altogether, could we move the interface and implementation to comm-central? mossop was only proposing to remove the code from mozilla-central because it is not used at all in Firefox. I agree with mkmelin that it's a shame to remove the pre-fill feature from Thunderbird.

Comment 15

[:aceman]

It we moved it to TB (if that is possible, as the implementation is in platform-specific C++ files), it wouldn't solve the original privacy problem why you filed this bug. It is actually Thunderbird where the code and also addons would still be able to silently access this data (in Firefox addons no longer can). Or you would then make your patch apply to Thunderbird? Is there a Tor version of Thunderbird?

Comment 16

[Arthur Edelstein [:arthur]]

My thinking is that the usability/privacy tradeoff is different for Firefox vs Thunderbird. In Firefox, there is no downside to removing the code, and a modest reduction of risk to privacy by ensuring no future mozilla-central code calls these functions. In Thunderbird, there would be potentially an improvement in privacy but a reduction in usability.

In any case, my focus is on the browser -- I defer to the Thunderbird team to decide if they want to keep the auto-fill feature. :)

There is a Tor version of Thunderbird planned, based around torbirdy. I am informed that the wizard that uses this code is already disabled by torbirdy.

Comment 17

[:Gijs (he/him)]

I just noticed this code lying around in m-c and realized it was unused, and filed bug 1541958 to remove it.

I intend to fix that bug there. I'm moving this one to Thunderbird so the TB team can decide whether they want to resurrect the code into c-c or do something else.

Comment 18

[Jorg K (CEST = GMT+2)]

Aceman, can you please rebase this.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1433350.patch

Rebased. The hunk in test-mail-account-setup-wizard.js was already removed, see:
https://hg.mozilla.org/comm-central/rev/367d7c0fa00e#l2.31

Comment 20

[Jorg K (CEST = GMT+2)]

Looking at https://hg.mozilla.org/integration/autoland/rev/cb5bd749696e, we could move that code to common/ without problem. A bit of a maintenance burden to maintain that for three platforms. So guys, should we restore this in a new bug? But then, the bug here wanted to remove it in the first place(?).

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/92d3924994f3
remove uses of nsIUserInfo from Thunderbird. r=mkmelin

Comment 22

[Magnus Melin [:mkmelin]]

I guess we could keep it, but I don't feel strongly about it.

Comment 23

[:aceman]

I'm also not a fan of the app/addons having unrestricted access to this data. Addons can request permissions in some way, so if someone can do this and hide access to this object behind permissions, we could accept it. I don't know how to do it and do not intend to. So I'm fine with this staying removed. The uses in TB were trivial and just a convenience.
One could say you do not need to have real data in these OS fields. Yes, maybe on Linux. But on Windows where MS does all sorts of stunts to force you into binding the PC to an MS account (and hiding the fact it is not really needed YET), in which you may have your real data registered (e.g. due to Office licenses), as putting fake data there may have other consequences.

Comment 24

[:aceman]

Attachment: 1433350-2.patch

Somehow this hunk got lost from the refreshed patch, so this still errors in the console when creating a news account.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 9056330 [details] [diff] [review]
1433350-2.patch

Grrr, every single hunk needed rebasing here and I missed that the almost identical code got removed twice :-( - Thanks for following up.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/516af70f7341
remove remaining use of nsIUserInfo. r=jorgk