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

RESOLVED FIXED in Thunderbird 68.0

Status

enhancement
P3
normal
RESOLVED FIXED
Last year
28 days ago

People

(Reporter: arthur, Assigned: aceman)

Tracking

(Blocks 2 bugs)

Trunk
Thunderbird 68.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tor 13398][overhead:noted])

Attachments

(2 attachments, 2 obsolete attachments)

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

Last year
> 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
Product: Core → Toolkit
Reporter

Comment 2

Last year
Here's a patch putting it under the MOZ_THUNDERBIRD flag.
Attachment #8949294 - Flags: review?(dtownsend)
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?
Flags: needinfo?(arthuredelstein)
Reporter

Comment 5

Last year
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.
Flags: needinfo?(arthuredelstein)
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 :)
Attachment #8949294 - Flags: review?(dtownsend) → review-
Jorg, looks like the nsIUserInfo component is used in Thunderbird but I'd expect it to be easy to move into comm-central.
Flags: needinfo?(jorgk)

Comment 8

Last year
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.
Flags: needinfo?(acelists)
Assignee

Comment 9

Last year
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).
Flags: needinfo?(acelists)
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.
Flags: needinfo?(acelists)
Assignee

Comment 11

Last year
Posted patch remove uses in Thunderbird (obsolete) — Splinter Review
Flags: needinfo?(acelists)
Attachment #8953783 - Flags: review?(mkmelin+mozilla)
Thanks, Aceman.
Flags: needinfo?(jorgk)
Comment on attachment 8953783 [details] [diff] [review]
remove uses in Thunderbird

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

Such a shame, but r=mkmelin
Attachment #8953783 - Flags: review?(mkmelin+mozilla) → review+
Reporter

Comment 14

Last year
(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.
Assignee

Comment 15

Last year
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?
Reporter

Comment 16

Last year
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.
Priority: -- → P3

Updated

9 months ago
Whiteboard: [tor 13398] → [tor 13398][overhead:noted]

Updated

7 months ago
Blocks: meta_tor
No longer blocks: uplift_tor_fingerprinting

Comment 17

2 months ago

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.

Depends on: 1541958
Product: Toolkit → Thunderbird

Comment 18

2 months ago

Aceman, can you please rebase this.

Flags: needinfo?(acelists)

Comment 19

2 months ago
Posted patch 1433350.patchSplinter Review

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

Attachment #8949294 - Attachment is obsolete: true
Attachment #8953783 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Attachment #9056055 - Flags: review+

Comment 20

2 months ago

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(?).

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)

Comment 21

2 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/92d3924994f3
remove uses of nsIUserInfo from Thunderbird. r=mkmelin

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED

Updated

2 months ago
Target Milestone: --- → Thunderbird 68.0

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

Flags: needinfo?(mkmelin+mozilla)
Assignee

Comment 23

2 months ago

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.

Assignee: nobody → acelists
Flags: needinfo?(acelists)
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Assignee

Comment 24

2 months ago

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

Attachment #9056330 - Flags: review?(jorgk)

Comment 25

2 months ago
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.
Attachment #9056330 - Flags: review?(jorgk) → review+

Comment 26

2 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/516af70f7341
remove remaining use of nsIUserInfo. r=jorgk
Assignee

Updated

28 days ago
Blocks: 1552866
Regressions: 1552866
You need to log in before you can comment on or make changes to this bug.