Opened 6 years ago

Last modified 20 months ago

#7956 assigned defect

Tor uses Roaming (remote) %APPDATA% instead of %LOCALAPPDATA%

Reported by: cypherpunks Owned by: tbb-team
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: tor-client, win32, needs-design, AppData, Roaming, Local, Windows, tbb-disk-leak
Cc: blibbet@…, erinn, mttp Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

[First tor-bug post, hope I got this right...]

The Windows tor.exe -- all current versions, standalone expert bundle and TBB/etc bundles -- has outdated Windows Explorer COM interface code to get the user's %APPDATA% location. This location can be local or roaming (remote).

Remote data gives many more opportunities for adversaries to mess with Tor Windows users, especially when they are using a domained enterprise.

The solution is below. Until this is fixed, Windows Tor users should never use Tor on a Windows network.

The Windows version of Tor calls the Windows Explorer 'shell' special folder COM APIs, to obtain the location for %APPDATA%. App data can be local or roaming.

On my box, %USERPROFILE%\AppData\* has 3 directories where apps install files, Local, LocalLow, and Roaming, where about 55% of the apps use Local, 5% uses LocalLow, and 40% use Remote.

Tor uses Roaming ("%USERPROFILE%\AppData\Roaming\tor).

Roaming means that it gets remoted to other systems, which is probably a bad thing for Tor user data. The point of Roaming profiles is to let user data migrate to multiple boxes, and the remoting of that data happens with SMBs. So, Remote user data might be sniffable over the network, hopefully this is encrypted.

Depending on how enterprise is configured, Remote user data also
probably means replication/backups of that data in the domain
controller's Active Directory box. So the user's Tor data is available over multiple SMB hosts, making things easier for attackers.

This bug was probably added to Tor in 0.2.1.11-alpha - 2009-01-20:


  • Add a new --enable-local-appdata configuration switch to change

the default location of the datadir on win32 from APPDATA to
LOCAL_APPDATA. In the future, we should migrate to LOCAL_APPDATA
entirely. Patch from coderman.


Tor explicitly checks the location. in or/config.c's get_windows_conf_root(), in it's use of the Windows Explorer COM APIs SHGetSpecialFolderLocation() and SHGetPathFromIDList(), using this definition of the APP_DATA flag:


/* Find X:\documents and settings\username\application data\ .

  • We would use SHGetSpecialFolder path, but that wasn't added
  • until IE4.

*/
#ifdef ENABLE_LOCAL_APPDATA
#define APPDATA_PATH CSIDL_LOCAL_APPDATA
#else
#define APPDATA_PATH CSIDL_APPDATA
#endif


First, this comment is wrong on many levels. It begins with "X:" (instead of "C:") which would likely be remote, not local. It talks about IE4 which shipped aeons ago, maybe time to revisit this hesitation. And you you *do* use this COM API, anyway.

I am pretty sure that it is the above definitions that're causing you to install the Roaming data location on my Win7 box.

CSIDL_LOCAL_APPDATA == FOLDERID_LocalAppData

CSIDL_APPDATA == FOLDERID_RoamingAppData

I am not great at Autotools, but I don't believe anyone is setting
ENABLE_LOCAL_APPDATA, so the directive might be 0. It is defined/used in config.ac and orconfig.h.in.


/* Defined if we default to host local appdata paths on Windows */
#undef ENABLE_LOCAL_APPDATA


I don't understand why this Windows-centric feature needs to be abstracted with GNU/autotools in the first place. Can Erin's Mac OSX-based, MinGW cross-compiled Autotools determine that I'm running WinXP or Win7, etc? This abstraction, using legacy APIs, is probably why this bug has been there since 2009.

Tor already abstracts this Windows-centric code into it's own function, it would be clearer if you removed all the autotools absraction to the Windows Special Folder COM API that Tor uses, IMO.

Also, this function doesn't check this return code, and assumes to be true:

strlcpy(p,get_windows_conf_root(),MAX_PATH);

Also, in this Windows-centric error codepath, there is no warning to the
user, AFAICT:

if (!SUCCEEDED(result)) {

return NULL;

}

Also, elsewhere in get_windows_conf_root(), there are 2 comments, with potentially-open questions. Here are updated comments stated more clearly. Replace:

/* Convert the path from an "ID List" (whatever that is!) to a path. */

with:

/* Windows Explorer, aka 'Windows 'shell', uses IDLists to access various things, including special folders. We need to convert this COM-based explorer-centric array list to a Windows path. */

And replace:

/* Now we need to free the memory that the path-idl was stored in.

  • In typical Windows fashion, we can't just call 'free()' on it. */

with:

/* Windows explorer special folders APIs are COM APIs, not Win32 or C runtime library calls. To use special folders, we have to use the COM memory mgmt APIs, to allocate, and now release that memory. Read the SDK ShObj.h or MSDN for more info. */

Note also that you are using legacy, deprecated version of the Special folders API, there have been revisions, apparently during Vista and Win7. The newer API has more features than the one you're using, including the local/roaming granularity. I think f you explicitly stick to local data, I think you can stick with legacy APIs.

You can't just test this with one version of Windows, you'll need to test with each version you are supporting.

More info:
http://msdn.microsoft.com/en-us/library/windows/desktop/bb762181%28v=vs.85%29.aspx
http://msdn.microsoft.com/en-us/library/windows/desktop/dd378457%28v=vs.85%29.aspx
http://msdn.microsoft.com/en-us/library/windows/desktop/bb762494%28v=vs.85%29.aspx
http://technet.microsoft.com/en-us/library/cc766489.aspx
http://msdn.microsoft.com/en-us/library/windows/desktop/bb776779%28v=vs.85%29.aspx
http://www.blogtechnika.com/what-is-application-data-folder-in-windows-7/
http://superuser.com/questions/150012/what-is-the-difference-between-local-and-roaming-folders
http://superuser.com/questions/21458/why-are-there-directories-called-local-locallow-and-roaming-under-users-user
http://social.technet.microsoft.com/Forums/en/w7itprogeneral/thread/b4fd7ac2-8ad9-4a06-8c28-3f7993aa0272
http://blogs.technet.com/b/fdcc/archive/2009/07/08/fdcc-vista-application-development-requirements.aspx
http://social.msdn.microsoft.com/Forums/en/windowsgeneraldevelopmentissues/thread/8fa38285-e4f5-43f9-ab0b-8fe184d476f9
http://technet.microsoft.com/en-us/library/cc778976.aspx

Thanks,
Lee

Child Tickets

Change History (28)

comment:1 Changed 6 years ago by nickm

Keywords: tor-client win32 023-backport added
Milestone: Tor: 0.2.4.x-final
Priority: normalmajor

Hi, Lee! Is this something you could come up with a patch for (e.g., using "git diff" or "diff -u")? It seems you've got a pretty good handle on the issue here.

Would changing --enable-local-appdata to the default be an adequate fix for the big issue here?

comment:2 in reply to:  description Changed 6 years ago by nickm

Cc: erinn added

Longer reply:

Replying to cypherpunks:

[First tor-bug post, hope I got this right...]

So far so good! I'd recommend making an account if you're planning to stick around -- there are a lot of people who use the cypherpunks account, and it can get a little confusing when everybody's talking to each other with the same names.

[...]

This bug was probably added to Tor in 0.2.1.11-alpha - 2009-01-20:

Actually, that's not the case. That commit was 87124f54d0bb84 (svn revision r18122), which added LOCAL_APPDATA as an option. Previously, it was APPDATA unconditionally.

Have you tried picking up git at all? Once you know how, it's easy to use git to find out which commit actually introduced a change.

[...]

First, this comment is wrong on many levels. It begins with "X:" (instead of "C:") which would likely be remote, not local. It talks about IE4 which shipped aeons ago, maybe time to revisit this hesitation. And you you *do* use this COM API, anyway.

Yeah; we used to care about some really ancient versions of Windows when we wrote the code. The "X" there was probably meant as "whatever drive it happens to be on".

I am pretty sure that it is the above definitions that're causing you to install the Roaming data location on my Win7 box.

CSIDL_LOCAL_APPDATA == FOLDERID_LocalAppData

CSIDL_APPDATA == FOLDERID_RoamingAppData

I am not great at Autotools, but I don't believe anyone is setting
ENABLE_LOCAL_APPDATA, so the directive might be 0. It is defined/used in config.ac and orconfig.h.in.

It'll get set if --enable-local-appdata is passed to autoconf.


/* Defined if we default to host local appdata paths on Windows */
#undef ENABLE_LOCAL_APPDATA


I don't understand why this Windows-centric feature needs to be abstracted with GNU/autotools in the first place. Can Erin's Mac OSX-based, MinGW cross-compiled Autotools determine that I'm running WinXP or Win7, etc? This abstraction, using legacy APIs, is probably why this bug has been there since 2009.

It's not autotools that causing the issue; it's the lack of a migration plan when we added the fix from making it "optional, off by default" to "on by default."

Tor already abstracts this Windows-centric code into it's own function, it would be clearer if you removed all the autotools absraction to the Windows Special Folder COM API that Tor uses, IMO.

Also, this function doesn't check this return code, and assumes to be true:

strlcpy(p,get_windows_conf_root(),MAX_PATH);

okay, better fix that one when we change this.

Also, in this Windows-centric error codepath, there is no warning to the
user, AFAICT:

if (!SUCCEEDED(result)) {

return NULL;

}

Agreed; there should be one.

Also, elsewhere in get_windows_conf_root(), there are 2 comments, with potentially-open questions. Here are updated comments stated more clearly. Replace:

/* Convert the path from an "ID List" (whatever that is!) to a path. */

with:

/* Windows Explorer, aka 'Windows 'shell', uses IDLists to access various things, including special folders. We need to convert this COM-based explorer-centric array list to a Windows path. */

And replace:

/* Now we need to free the memory that the path-idl was stored in.

  • In typical Windows fashion, we can't just call 'free()' on it. */

with:

/* Windows explorer special folders APIs are COM APIs, not Win32 or C runtime library calls. To use special folders, we have to use the COM memory mgmt APIs, to allocate, and now release that memory. Read the SDK ShObj.h or MSDN for more info. */

Those seem sensible; getting the changes in patch format would be great.

Note also that you are using legacy, deprecated version of the Special folders API, there have been revisions, apparently during Vista and Win7. The newer API has more features than the one you're using, including the local/roaming granularity. I think f you explicitly stick to local data, I think you can stick with legacy APIs.

I think we will want to; we're still supporting XP at least.

You can't just test this with one version of Windows, you'll need to test with each version you are supporting.

agreed

comment:3 Changed 6 years ago by nickm

Hm. We need to think about migration on this one too. If anybody's got their configuration, data directory, or geoip stored roamingly, do we really want to ignore their old settings? Maybe so.

Also, I am pretty sure that TBB is *supposed* to override everything in tor.exe that would look at the default values that would actually look at %APPDATA% -- so that mitigates the issue some -- but it would still probably be a good idea to switch.

comment:4 Changed 6 years ago by cypherpunks

(not OP)

Maybe a three step approach?

1) If specified on command line, then use as told
2) If not, check if in Roaming, if so, then create necessary folders in Local and warn the user that they should manually move the files themselves and delete the folder in Roaming
3) If not in Roaming, then check Local, use or create files there

The other option for 2) is to just do a hard cut-over and have some warnings to the user that it's happening. The issue with this is that it doesn't give the user an opportunity to continue using the old files. Or Tor could copy everything itself and delete the folder in Roaming, which may not be a terrible idea given the situation, either.

comment:5 Changed 6 years ago by nickm

Keywords: 024-backport added; 023-backport removed
Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:6 Changed 6 years ago by nickm

This is important, but doing a proper migration is going to have stability issues; it needs to get tried in a very alpha alpha first.

comment:7 Changed 6 years ago by erinn

I looked into this for a while today and the problem, I think, is that it would be easy to fix for everything except WinXP. Some notes I took:

  1. We could switch $APPDATA to $LOCALAPPDATA in tor-mingw.nsi.in

By default this will install stuff into the right Local directory. The problem is that $LOCALAPPDATA is only supported on Win2k and up. As far as I understood things, this is true not only for NSIS but in general on WinXP -- there is %LOCALAPPDATA% but only for some post-XP Windows releases. (As an aside, in XP %APPDATA% is C:\Documents and Settings\{user}\Application Data).

  1. Use --enable-local-appdata by default. But I think this has the issues above? So we need to find a way to hack around the WinXP issue in both the installer and Tor itself, is that right?

UPDATE: I was completely confused about the order of Windows releases so I think actually both options above should be fine. I will create a test bundle with both changes and see if they work.

One other note:

SHGetSpecialFolderLocation function is deprecated and SHGetFolderLocation should
be used instead.

http://msdn.microsoft.com/en-us/library/windows/desktop/bb762203%28v=vs.85%29.aspx

Last edited 5 years ago by erinn (previous) (diff)

comment:8 in reply to:  description Changed 5 years ago by GITNE

SHGetFolderPath() is the way to go as long as Tor wants to support Windows 2000 and later. Don't get yourself confused by this function being marked as deprecated. Before Windows Vista this was the way to get any special profile folders.

SHGetFolderPathW(
	0,
	CSIDL_LOCAL_APPDATA | CSIDL_FLAG_CREATE,
	NULL,
	SHGFP_TYPE_CURRENT,
	ptrPathWchar_t
);

And by the way, this has nothing to do with COM. CSIDL stands for "constant special item ID list".

Oh, and I strongly encourage you to use the Unicode version of it SHGetFolderPathW() because paths returned by this function may contain characters that are outside of the user's current ANSI codepage.

Last edited 5 years ago by GITNE (previous) (diff)

comment:9 Changed 5 years ago by nickm

I'm in favor of this change, but we need to figure out the migration issues. If people have a good installation in APPDATA, and none in LOCAL_APPDATA what should we do? Should we move things over, ignore the old one, copy only configuration information, give a warning, or something else?

comment:10 Changed 5 years ago by erinn

My current plan is to first create a Windows package with 0.2.5.x that has these changes without a formal migration plan. Nick had an idea for a migration plan that can follow which I'm posting here so it doesn't get lost:

16:36 <nickm Basically, check whether the stuff is in the new location and in the old location, and check timestamps at both locations.
16:36 <nickm> If the new location has newer state file, just use that.
16:37 <nickm> If the old location doesn't exist, use the new one.
16:37 <nickm> If the old location exists and the new one doesn't, warn the user that they should move their files over, or eventually Tor will do it for them.
16:38 <nickm> if the old location has a newer state file, that means that they ran an older Tor that didn't know about the new location after running a newer Tor. So warn, and use the new location.

comment:11 Changed 5 years ago by nickm

(to be clear: that migration plan is a draft and it is probably improvable)

comment:12 in reply to:  11 Changed 5 years ago by GITNE

Replying to nickm and nickm:

(to be clear: that migration plan is a draft and it is probably improvable)

This plan makes sense but only theoretically because it won't work in practice.


A changeset implementing this ticket should also implement #10027 (and vice versa) because both tickets have equal side effects. They cause Tor to regenerate its private key (identity) and caches. Hence, users should be spared the double hassle.

Adding code to Tor migrating its configuration and cache folder from %APPDATA% to %LOCALAPPDATA% is rather trivial but migrating it from one account to another is not. The problem is that moving a folder from one account profile to another requires running under the "NT AUTHORITY\SYSTEM" (LocalSystem) pseudo account or an account in the Administrators group. Windows Vista and later offer UAC (equivalent to sudo) to easily aquire an administrator's account for such an — one time — operation so this seems like a viable solution to this migration problem. But, it's not. Windows versions prior to Windows Vista don't support UAC. Therefor this approach is limited.

Because by convention Windows applications do not do setup or installation tasks they are left to or taken care of by the installer. So the preferred way would be to put the migration code into the installer. Installers are always executed under an administrator's or the LocalSystem account (or with the "NT SERVICE\TrustedInstaller" pseudo account since Windows Vista, but this is of no concern here). Hence an installer can almost literally do anything to a machine while running; like moving folders between profiles and modifying their ownerships and DACLs. This way, there is no need to add and maintain migration code in the Tor executable. And, Tor is free of the hassle to aquire or impersonate an administrator's account.

The last time I have checked the downloads page of the Tor project, I have seen that the Tor standalone bundle is wrapped into an installer too. So the above described migration path via the installer should also be applicable to the Tor standalone bundle. Only users who compile Tor or extract Tor from the install bundles and install it manually would need to be aware of this change and handle the migration manually. But, since they are installing Tor manually anyway they can be expected and should be able to do it on their own.

comment:13 in reply to:  description Changed 5 years ago by GITNE

Summary: Tor uses Roaming (remote) AppData, not LocalTor uses Roaming (remote) %APPDATA% instead of %LOCALAPPDATA%

I have been thinking about the migration problem today a little bit more and it occured to me that putting migration code into the installer only will not solve the problem for users who run Tor under an arbitrary account in the Users security group. So, those users would be screwed or would have to move thier %APPDATA%\tor folder to %LOCALAPPDATA% manually. Although nobody should do this because it is against best practice, some users may still have done so.

The Windows TBB may be affected by it too if it does not install Tor as a service (I have not used nor installed it so I have no idea. Hence I am just hypothesizing here).

Btw, this problem would not have existed if Tor had been adapted to Windows correctly in the first place... :(

comment:14 Changed 5 years ago by erinn

I am still playing around with this, but I wanted to quickly note that I have made a demo package that at least uses the correct AppData/Local. Currently it doesn't want to install to Program Files (x86), but I'm looking into that more. In any case, it's pretty close!

(And Nick, it is worth noting that within this potentially correctly-working package there is a 64-bit, hardened, deterministically built 0.2.5.1-alpha tor with those OpenSSL NIST curves enabled.)

comment:15 Changed 5 years ago by erinn

And here's a test package: https://people.torproject.org/~erinn/tor-0.2.5.1-alpha-dev-win32.exe
197d8420e68c37f0a48b183ee6a8a29fda51596b5606d32c99976300a5ad8cc0 tor-0.2.5.1-alpha-dev-win32.exe

It'll install to e.g. the user's directories but it fails to install in Program Files and also doesn't give any kind of reason for failure, nor tell you it failed. I assume it's some kind of NSIS issue -- maybe I need to elevate installation privileges (currently we have RequestExecutionLevel user)? -- but we never did that in the past. No idea what is happening right now.

comment:16 Changed 5 years ago by erinn

It just occurred to me that this is also crosscompiled with possibly a different version of NSIS than I have on the Windows build machine. Looking into that as a potential cause as well.

comment:17 Changed 5 years ago by erinn

Okay, I have another update. The problem was the RequestExecutionlevel user, but also the fact that the default $INSTDIR was $PROGRAMFILES. I have updated the bundle to use $LOCALAPPDATA/Tor for $INSTDIR so that it will install everything to $LOCALAPPDATA and run properly. This will not work for multi-user accounts, but I think that is at this point such a small userbase that for a preliminary alpha release that it is fine and we can revisit if anyone complains. These won't work on non-64-bit systems so we need to make sure that is mentioned during release. :)

So to recap, here is what's included in this test bundle:

  • Tor 0.2.5.1-alpha
  • 64-bit deterministically built tor.exe
  • hardening (DEP/ASLR)
  • OpenSSL with NIST curves
  • everything installed to and writing to $LOCALAPPDATA
  • --enable-local-appdata flag for Tor

I'd appreciate it if anyone can look it over. Regardless of your inability to run the .exe it should install properly on everything from win98 - current Windows. (Shout out to bobnomnom for testing on Win98 and WinXP! Plus all his help with debugging.) Once this is confirmed working for everyone, we can begin to investigate fixing the win32 bundles to do the same thing, and a migration path. Here's the test bundle:

https://people.torproject.org/~erinn/qa/tor-0.2.5.1-alpha-dev-win32-localappdata.exe
2b496ac784582359b7e6d5cfe992e2779accba1b27f8a2bf4b40d15a02cea7eb

BTW: the tor.exe in here is statically built because the installation scripts expect that. I can change it (longer-term that is better so the tor.exe in these bundles also matches the ones in TBB et al), but that takes more installation script fixing and gitian interaction. I'll also make sure to rename the file to use win64 next time. :) And will put everything in git soon.

comment:18 Changed 5 years ago by erinn

Owner: set to erinn
Parent ID: #10205
Status: newassigned

comment:19 Changed 5 years ago by erinn

Just wanted to give another update. Things are still moving. When bobnomnom and I were investigating this more we discovered that because of the way the current bundle requests permissions, it was failing to install to Program Files (x86 or other) but silently failing because it didn't have write access. The fix we decided on is that it will alert the user to the failure and return them to the path selection screen, although they will get $LOCALAPPDATA by default. The error message could probably use some improvement, but the current version of what we're testing is here:

https://gitweb.torproject.org/erinn/tor.git/shortlog/refs/heads/bug7956-2

Longer term, I think we will want to remove basically everything in contrib/ that is related to packaging (and re-examine the changes suggested in #8966) and migrate the relevant parts to Gitian. Currently I am migrating the package_nsis-mingw.sh script into the Tor expert bundle descriptor.

comment:20 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

tentatively moving these out of 0.2.5.

comment:21 Changed 5 years ago by mttp

Cc: mttp added

comment:22 Changed 4 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???
Status: assignedneeds_revision

comment:23 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:24 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:25 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:26 Changed 2 years ago by nickm

Keywords: 024-backport removed

None of these is ripe for backport to 0.2.4 even if it does get fixed.

comment:27 Changed 23 months ago by nickm

Keywords: needs-design added
Severity: Normal

comment:28 Changed 20 months ago by cypherpunks

Keywords: tbb-disk-leak added
Owner: changed from erinn to tbb-team
Parent ID: #10205
Status: needs_revisionassigned

TBB shouldn't use anything in %USERPROFILE%.

Note: See TracTickets for help on using tickets.