Opened 4 years ago

Closed 14 months ago

#13398 closed defect (fixed)

at startup, browser gleans user FULL NAME (real name, given name) from O/S

Reported by: zinc Owned by: pospeselr
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201710R
Cc: arthuredelstein, brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by mcs)

(Reporting against Tor Browser 3.6.6, but this is a longstanding issue which affects all versions of the browser.)

At each startup, code within nsUserInfoWin.cpp
(see also: nsUserInfoUnix.cpp, nsUserInfoOS2.cpp, nsUserInfoMac.mm)
scrapes user's FULL NAME (real name, given name) from the operating system
and retains this in memory, stored to a constant, throughout the browser session.

Additionally, the browser scrapes user's windows login username (and windows domain) along with his/her email address (if present, filled in within user's windows user account details). These personal details are similarly stored by the browser throughout the life of each browsing session.

This privacy-infringing behavior is unconditional — no user_pref is available to prevent it.

In researching "How dare they?!?" I gathered that this behavior exists because Firefox shares a codebase with Thunderbird, and back in the day someone thought it would be "kewl" for a Thunderbird user to find that the system magically knows his/her details when setting up a new TB account...

If challenged to prove/demonstrate where these details are ever "leaked" by the browser, I cannot. However, these personal details are accessible to any extension (or out-of-band Mozilla update) and therefore are subject to exfiltration.

Child Tickets

Attachments (9)

13398-pospeselr.patch (12.2 KB) - added by pospeselr 16 months ago.
userinfotest.xpi (3.3 KB) - added by pospeselr 16 months ago.
13398-pospeselr2.patch (9.8 KB) - added by pospeselr 16 months ago.
13398-pospeselr3.patch (10.2 KB) - added by pospeselr 16 months ago.
13398-pospeselr4.patch (8.0 KB) - added by pospeselr 14 months ago.
13398-pospeselr5.patch (6.1 KB) - added by pospeselr 14 months ago.
13398-pospeselr6.patch (6.3 KB) - added by pospeselr 14 months ago.
#fdef ESR59 -> #ifndef TOR_BROWSER_VERSION
13398-pospeselr7.patch (6.3 KB) - added by pospeselr 14 months ago.
fixed #if -> #ifndef issue in nsUserInfoMac.mm
0001-Bug-13398-at-startup-browser-gleans-user-FULL-NAME-r.patch (7.0 KB) - added by pospeselr 14 months ago.
Properly formatted patch, no code changes

Download all attachments as: .zip

Change History (48)

comment:1 Changed 4 years ago by zinc

I have no idea what caused the "strikethrough" marks within my report. They appeared immediately upn posting.

comment:2 Changed 4 years ago by cypherpunks

I have no idea what caused the "strikethrough" marks within my report. They appeared immediately upn posting.

You wrote:

~~ no user_pref is available to prevent it.

WikiFormatting

comment:3 Changed 4 years ago by arthuredelstein

Cc: arthuredelstein added

comment:4 Changed 3 years ago by mcs

Cc: brade mcs added

comment:5 Changed 17 months ago by mcs

Description: modified (diff)
Severity: Normal

comment:6 Changed 16 months ago by pospeselr

Owner: changed from tbb-team to pospeselr
Status: newaccepted

Changed 16 months ago by pospeselr

Attachment: 13398-pospeselr.patch added

Changed 16 months ago by pospeselr

Attachment: userinfotest.xpi added

comment:7 Changed 16 months ago by pospeselr

I've attached a patch addressing this issue as well as the modified 'hello-world' add-on I used to manually verify the change (on Linux). Here's a description of the patch:

Change Summary:

  • added new 'privacy.hide_user_info' setting which defaults to 'false' in firefox, but is set to 'true' in tor-browser via the 000-tor-browser.js script
  • added static bool sHideUserInfo bool to the nsUserInfo class
    • bool is inited and registered with the mozilla::Preferences system in static method nsUserInfo::InitializeStatics which is called from the nsAppStartup::Init method on firefox boot
  • refactored the various nsUserInfo objects such that the old platform specific Get$PROPERTY methods are now named Get$PROPERTYImpl
    • Single implementation of nsIUserInfo interface methods added to new nsUserInfo.cpp file
    • Each Get$PROPERTY method checks against newly added sHideUserInfo static bool before either returning an empty string, or calling the Get$PROPERTYImpl method
    • Now calling the new Get$PROPERTYImp methods where appropriate (in the UNIX implementation of GetEmailAddress for instance)
  • refactor to OSX nsUserInfo implementation
    • changed GetPrimaryEmailAddress to be static function in nsUserInfoMac.mm, rather than a 'secret' class method (since it is not dependent on any object specific data or private/protected methods)
    • removed the OSX specific nsUserInfoMac.h header which included the 'secret' GetPrimaryEmailAddress method

Caveats:

  • haven't verified this change works (or even builds) on Windows or OSX
    • seems like something is borked with my Linux install regarding virtualization, so I'm going to wipe and re-install fresh
    • will resend an updated patch if I get to that this weekend and find any build or runtime errors on these platforms
  • an automated test also needs to be added verifying this parameter works as intended, if I have time this weekend will try to get that going too

Here's some screenshots of the userinfotest.xpi manual verification of the new privacy.hide_user_info setting: https://imgur.com/a/7stMu

Last edited 16 months ago by pospeselr (previous) (diff)

comment:8 Changed 16 months ago by gk

Keywords: TorBrowserTeam201707R added

Thanks!

comment:9 Changed 16 months ago by gk

Status: acceptedneeds_review

comment:10 Changed 16 months ago by mcs

Status: needs_reviewneeds_revision

I was unable to apply the 13398-pospeselr.patch; it looks like trailing whitespace has been removed from the patch. In any case, we typically avoid touching lines that we are not otherwise changing because larger patches are more work to maintain (but hopefully we can convince Mozilla to accept this patch upstream).

After I manually applied the patch, I then encountered a linker error while building on OSX (I attempted a non-gitian build). To fix the problem, I had to add an empty destructor to toolkit/components/startup/nsUserInfoMac.mm:

nsUserInfo::~nsUserInfo() {}

Using userinfotest.xpi I verified that the fix is effective. Good work!

I have mixed feelings about refactoring the OSX nsUserInfo implementation. This seems like a good clean up, but this also may make patch maintenance more difficult.

Please match the style of most other Mozilla code for the if statements (include a space after if).

In the code that calls ToNewUnicode() it would be slightly more compact to directly assign to the output parameter, e.g.,

NS_IMETHODIMP
nsUserInfo::GetFullname(char16_t** aFullname)
{
  NS_ENSURE_ARG_POINTER(aFullname);

  if (sHideUserInfo) {
    *aFullname = ToNewUnicode(NS_LITERAL_STRING(""));
    if (aFullname) {
      return NS_OK;
    }

    return NS_ERROR_FAILURE;
  }

  return this->GetFullnameImpl(aFullname);
}

comment:11 Changed 16 months ago by pospeselr

Thanks for the feedback!

In regards to the ToNewUnicode() call, it looks like you are completely correct! Not only is it less C++ code but the generated machine code is shorter too: https://godbolt.org/g/hyurar

Generated code for when that link eventually dies (gcc version 7.1 with -03):

; int func_local(void** aPtr)
; {
;         if(!aPtr)
;         {
;                 return FAILURE;
;         }
;         auto ptr = get_ptr();
;         if(ptr)
;         {
;                 *aPtr = ptr;
;                 return SUCCESS;
;         }
;         *aPtr = nullptr;
;         return FAILURE;
; }        
func_local(void**):
        test    rdi, rdi
        je      .L7
        push    rbx
        mov     rbx, rdi
        call    get_ptr()
        test    rax, rax
        je      .L6
        mov     QWORD PTR [rbx], rax
        xor     eax, eax
        pop     rbx
        ret
.L7:
        mov     eax, -1
        ret
.L6:
        mov     QWORD PTR [rbx], 0
        mov     eax, -1
        pop     rbx
        ret

; int func_ptr(void** aPtr)
; {
;         if(!aPtr)
;         {
;                 return FAILURE;
;         }
;         *aPtr = get_ptr();
;         if(*aPtr)
;         {
;                 return SUCCESS;
;         }
;         return FAILURE;
; }        
func_ptr(void**):
        test    rdi, rdi
        je      .L14
        push    rbx
        mov     rbx, rdi
        call    get_ptr()
        test    rax, rax
        mov     QWORD PTR [rbx], rax
        sete    al
        movzx   eax, al
        neg     eax
        pop     rbx
        ret
.L14:
        mov     eax, -1
        ret

I can go through another round, fix these issues, and upload a new patch if you like (while disabling sublime's 'trim trailing white-space' option).

comment:12 in reply to:  11 ; Changed 16 months ago by mcs

Replying to pospeselr:

I can go through another round, fix these issues, and upload a new patch if you like (while disabling sublime's 'trim trailing white-space' option).

If you can quickly produce a new patch, please proceed. But maybe it makes sense to wait for feedback from another member of our team. gk, please let pospeselr know if you want a revised patch right away.

comment:13 in reply to:  12 Changed 16 months ago by gk

Replying to mcs:

Replying to pospeselr:

I can go through another round, fix these issues, and upload a new patch if you like (while disabling sublime's 'trim trailing white-space' option).

If you can quickly produce a new patch, please proceed. But maybe it makes sense to wait for feedback from another member of our team. gk, please let pospeselr know if you want a revised patch right away.

I think proposing a new patch addressing your review comments is a good idea.

comment:14 Changed 16 months ago by gk

Keywords: TorBrowserTeam201707R removed

Changed 16 months ago by pospeselr

Attachment: 13398-pospeselr2.patch added

comment:15 Changed 16 months ago by pospeselr

Ok uploaded new patch without white-space issues and with adjusted if spacing and mcs's suggested code changes. Also verified patch behaves as expected at runtime.

Rest of the night goes to seeing if I can build OSX flavor using rbm.

Last edited 16 months ago by pospeselr (previous) (diff)

comment:16 Changed 16 months ago by cypherpunks

Keywords: TorBrowserTeam201707R added
Status: needs_revisionneeds_review

comment:17 Changed 16 months ago by gk

Keywords: TorBrowserTeam201708R added; TorBrowserTeam201707R removed

Moving review tickets to August.

comment:18 in reply to:  15 Changed 16 months ago by gk

Keywords: TorBrowserTeam201708R removed
Status: needs_reviewneeds_revision

Replying to pospeselr:

Ok uploaded new patch without white-space issues and with adjusted if spacing and mcs's suggested code changes. Also verified patch behaves as expected at runtime.

Rest of the night goes to seeing if I can build OSX flavor using rbm.

I have not tested an OS X build yet but the Windows one (done with mingw-w64) is failing with:

/home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfoWin.cpp:29:45: error: ambiguating new declaration of 'nsresult nsUserInfo::GetUsernameImpl(char**)'
 nsUserInfo::GetUsernameImpl(char **aUsername)
                                             ^
In file included from /home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfoWin.cpp:6:0:
/home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfo.h:27:12: note: old declaration 'nsresult nsUserInfo::GetUsernameImpl(char**)'
   nsresult GetUsernameImpl(char** aUsername);
            ^
/home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfoWin.cpp:45:49: error: ambiguating new declaration of 'nsresult nsUserInfo::GetFullnameImpl(char16_t**)'
 nsUserInfo::GetFullnameImpl(char16_t **aFullname)
                                                 ^
In file included from /home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfoWin.cpp:6:0:
/home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfo.h:26:12: note: old declaration 'nsresult nsUserInfo::GetFullnameImpl(char16_t**)'
   nsresult GetFullnameImpl(char16_t** aFullname);
            ^
/home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfoWin.cpp:97:41: error: ambiguating new declaration of 'nsresult nsUserInfo::GetDomainImpl(char**)'
 nsUserInfo::GetDomainImpl(char **aDomain)
                                         ^
In file included from /home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfoWin.cpp:6:0:
/home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfo.h:29:12: note: old declaration 'nsresult nsUserInfo::GetDomainImpl(char**)'
   nsresult GetDomainImpl(char** aDomain);
            ^
/home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfoWin.cpp:116:53: error: ambiguating new declaration of 'nsresult nsUserInfo::GetEmailAddressImpl(char**)'
 nsUserInfo::GetEmailAddressImpl(char **aEmailAddress)
                                                     ^
In file included from /home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfoWin.cpp:6:0:
/home/ubuntu/build/tor-browser/toolkit/components/startup/nsUserInfo.h:28:12: note: old declaration 'nsresult nsUserInfo::GetEmailAddressImpl(char**)'
   nsresult GetEmailAddressImpl(char** aEmailAddress);
            ^
make[5]: Leaving directory `/home/ubuntu/build/tor-browser/obj-mingw/toolkit/components/startup'
make[5]: *** [nsUserInfoWin.o] Error 1

comment:19 Changed 16 months ago by pospeselr

Hmm, if I had to guess it's probably the usage of the NS_METHODIMP macro rather than just specifying an nsresult return type. See if I can repro tonight and update the patch.

Changed 16 months ago by pospeselr

Attachment: 13398-pospeselr3.patch added

comment:20 Changed 16 months ago by pospeselr

Ok the latest patch is confirmed building on Win32, Linux and OSX using RBM. Also verified the behaviour of the Win32 build. Win32 Firefox totally runs on Linux via wine :)

comment:21 Changed 16 months ago by gk

Keywords: TorBrowserTeam201708R added
Status: needs_revisionneeds_review

comment:22 Changed 15 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201708R removed
Status: needs_reviewneeds_information

The patch looks okay to me, just a small nit: there is trailing whitespace on a bunch os

+nsresult 

lines that needs to get removed.

However, after thinking more about this patch I have a bigger concern. What is it defending against? I mean, what prevents a rogue extension from flipping our pref and just read the values we tried to hide? (I know I suggested the pref approach first and should probably have thought more about it and not just have recommended the "standard thing" when Firefox patches are concerned).

One could argue that's not possible with the new WebExtensions-based add-ons (which is correct) but then I bet those extensions are not allowed to extract the info we want to hide in the first place either (but I could be wrong about that). So, should we just say this will be fixed when we switch to Firefox 59? And, if we really want to defend against that in the ESR 52 cycle we would just rip out the offending code (not bothering about upstreaming the patch)?

mcs: What about your refactoring concerns?

comment:23 in reply to:  22 ; Changed 15 months ago by mcs

Replying to gk:

However, after thinking more about this patch I have a bigger concern. What is it defending against? I mean, what prevents a rogue extension from flipping our pref and just read the values we tried to hide? (I know I suggested the pref approach first and should probably have thought more about it and not just have recommended the "standard thing" when Firefox patches are concerned).

One could argue that's not possible with the new WebExtensions-based add-ons (which is correct) but then I bet those extensions are not allowed to extract the info we want to hide in the first place either (but I could be wrong about that). So, should we just say this will be fixed when we switch to Firefox 59? And, if we really want to defend against that in the ESR 52 cycle we would just rip out the offending code (not bothering about upstreaming the patch)?

So maybe just add #ifdefs for ESR52 to remove the code? I'd still feel better if the info was never read (and thus present in memory) in ESR 59 and later, but in theory the info should not be accessible to Webextensions.

mcs: What about your refactoring concerns?

That concern is fairly minor; we could just wait and see what Mozilla says if or when we try to upstream the patch. And we can change our approach later if upstreaming does not happen (and therefore we would need to maintain a patch forever).

comment:24 in reply to:  23 Changed 15 months ago by gk

Status: needs_informationneeds_revision

Replying to mcs:

Replying to gk:

However, after thinking more about this patch I have a bigger concern. What is it defending against? I mean, what prevents a rogue extension from flipping our pref and just read the values we tried to hide? (I know I suggested the pref approach first and should probably have thought more about it and not just have recommended the "standard thing" when Firefox patches are concerned).

One could argue that's not possible with the new WebExtensions-based add-ons (which is correct) but then I bet those extensions are not allowed to extract the info we want to hide in the first place either (but I could be wrong about that). So, should we just say this will be fixed when we switch to Firefox 59? And, if we really want to defend against that in the ESR 52 cycle we would just rip out the offending code (not bothering about upstreaming the patch)?

So maybe just add #ifdefs for ESR52 to remove the code? I'd still feel better if the info was never read (and thus present in memory) in ESR 59 and later, but in theory the info should not be accessible to Webextensions.

Yes, I think I agree. We could keep the #ifdefs for ESR59 and talk to Mozilla folks about it.

Sorry again, Richard, but we should revise this patch making it more straightforward and ignoring upstreaming concerns for now.

comment:25 Changed 14 months ago by pospeselr

So is the consensus here that we should just #ifdef out the offending code and instead return empty-string for the requested data, and undo the user-setting change entirely?

comment:26 in reply to:  25 ; Changed 14 months ago by gk

Replying to pospeselr:

So is the consensus here that we should just #ifdef out the offending code and instead return empty-string for the requested data, and undo the user-setting change entirely?

I think so, yes, given the power of XPCOM-based extensions which are the main targets of this patch.

comment:27 in reply to:  26 Changed 14 months ago by pospeselr

Is there a firefox #define for the major version already in use to place these changes behind?

Replying to gk:

Replying to pospeselr:

So is the consensus here that we should just #ifdef out the offending code and instead return empty-string for the requested data, and undo the user-setting change entirely?

I think so, yes, given the power of XPCOM-based extensions which are the main targets of this patch.

Changed 14 months ago by pospeselr

Attachment: 13398-pospeselr4.patch added

Changed 14 months ago by pospeselr

Attachment: 13398-pospeselr5.patch added

comment:28 Changed 14 months ago by pospeselr

Ok, latest patch (13398-pospeselr5.patch) just rips out the offending code and returns empty string for all requested properties, assuming ESR59 is not defined. Verified working on linux and windows, building on macOS. Version 5 is the same as version 4, except it doesn't 'fix' trailing whitespace (new settings config, who dis?).

I couldn't find any existing firefox version defines, but if any exist I'm happy to use them.

comment:29 Changed 14 months ago by pospeselr

Status: needs_revisionneeds_review

comment:30 Changed 14 months ago by pospeselr

Keywords: TorBrowserTeam201709R added; TorBrowserTeam201708 removed

comment:31 Changed 14 months ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201709R removed
Status: needs_reviewneeds_revision

There is no such define available. I guess you could check for the existence of TOR_BROWSER_VERSION to make sure the replacement code only kicks in in the Tor Browser case. However, I think we could just make sure an empty string is returned unconditionally right now and ask Mozilla what changes they would like to see to get the patch upstreamed.

Changed 14 months ago by pospeselr

Attachment: 13398-pospeselr6.patch added

#fdef ESR59 -> #ifndef TOR_BROWSER_VERSION

comment:32 Changed 14 months ago by pospeselr

Keywords: TorBrowserTeam201709R added; TorBrowserTeam201709 removed
Status: needs_revisionneeds_review

comment:33 Changed 14 months ago by cypherpunks

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201709R removed
Status: needs_reviewneeds_revision
return *XXX ? NS_OK : NS_ERROR_FAILURE;

for "" ? o_0
#if TOR_BROWSER_VERSION -> #ifndef TOR_BROWSER_VERSION

comment:34 Changed 14 months ago by pospeselr

I'm not sure I'm following. In each instance XXX is type char** or wchar**.

Sure if we fail to allocate the 1 (or 2) bytes needed for the empty string's null terminator we probably have bigger problems and the error handling isn't *technically* needed since we're bound to crash soon anyway, but seems sloppy to omit it.

Fixing the text replacement fail.

Changed 14 months ago by pospeselr

Attachment: 13398-pospeselr7.patch added

fixed #if -> #ifndef issue in nsUserInfoMac.mm

comment:35 Changed 14 months ago by pospeselr

Keywords: TorBrowserTeam201709R added; TorBrowserTeam201709 removed
Status: needs_revisionneeds_review

Changed 14 months ago by pospeselr

Properly formatted patch, no code changes

comment:36 Changed 14 months ago by gk

Keywords: TorBrowserTeam201710R added; TorBrowserTeam201709R removed

Moving reviews to October.

comment:37 Changed 14 months ago by gk

Looks good to me.

comment:38 Changed 14 months ago by arthuredelstein

The patch looks good to me as well.

comment:39 Changed 14 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks. Applied to tor-browser-52.4.0esr-7.5-1 as commit e3b99866dbdc29a2949d0a53806b99a94f34cc73.

Note: See TracTickets for help on using tickets.