LoadLibrary used without restrictions for search path

We use it to load advapi32.dll and iphlpapi.dll. These dlls would seem to be generally available on Windows so they should be loaded correctly, but it still seems like we should fix this. Originally RATS warned about this in 2007. We should analyze how dangerous our current behaviour is/if it is dangerous at all; and then either fix it or add a comment why we're safe.

changed milestone to %Tor: 0.2.2.x-final

added component::core tor/tor milestone::Tor: 0.2.2.x-final priority::high resolution::fixed status::closed type::defect labels

libevent shows the same behaviour for iphlpapi.dll, by the way.

Sebastian thinks this may be serious; so setting it to an 0.2.2.x milestone until we learn more.

Nick?

Trac:
Milestone: N/A to Tor: 0.2.2.x-final
Priority: normal to major

If there is a security issue here -- or even if it would just make our code less scary -- we should fix and backport.

But what's the fix? Looking at codesearch.google.com, it seems that everybody else is doing this the same as we are. What do you maintain is the right way?

I suppose we could use GetSystemDirectory or SHGetFolderPath to find the system directory, and build an absolute path based on that, and hope for the best. The system directory is where these dlls are supposed to live, right?

Yes, we should absolutely fix this if it's an issue.

Yes, this is bad, but the reality is this is Windows. There are tons of ways an attacker can inject code into processes easily, especially if they have write access to either the CWD or the directory of the exe. The windows exe loader is actually specifically written to make this easy. It automatically loads any DLLs in the CWD and/or the exe's dir that match the imports list of that exe. It also loads any DLLs listed in the AppInitDlls registry key. Any user with the DEBUG privilege can also inject DLLs into any other processes running as that user (I believe this is most/all users). Any app with write privs to the exe's directory can also edit its import table on disk to add new dlls.

Most of this was done to make binary compatibility easier. But it is also one of the things that makes windows a nightmare wrt spyware and malware.

Windows may have also recently created a way to build executables that want to disable some of these injection vectors, but I'm also not sure on that. And I bet some vectors (such as the DEBUG one) will still remain open.

Both DLLs are present in Windows 98 and later, so far as I can find out online. They may also be in Windows 95, though it's kind of hard to tell.[*]

The behavior of LoadLibrary is explained here on MSDN: http://msdn.microsoft.com/en-us/library/ms684175(VS.85).aspx ; the search path is here: http://msdn.microsoft.com/en-us/library/ms682586(v=VS.85).aspx .

If I am reading that right (and somebody should re-read it!) there are circumstances where the cwd can get searched before the system directory. That's a problem if anybody is invoking Tor from an someplace where a potentially hostile party might have placed DLLs. Vidalia should prevent this from happening for most users. Still, let's be belt-and-suspenders about this and use explicit paths to handle this case, in case it matters.

[*] (Insert standard gripes about how microsoft has interpreted a very reasonable "Windows 98 is no longer supported" position to mean "All information on MSDN pertaining to windows 98 shall be thrown into the memory hole. All APIs introduced in windows 98 and earlier will be listed as 'Since Windows 2000'.")

See bug1954 in my public.

Trac:
Status: new to needs_review

+  TCHAR path[MAX_PATH];
+  unsigned n;
+  n = GetSystemDirectory(path, 1024);

that looks weird. MAX_PATH once, then 1024?

Also you'll want to remove these two XXX I think:

   /* XXXX Possibly, we should hardcode the location of this DLL. */

I'm not done reviewing but am running of for a bit.

Branch updated. Thanks!

hm. Do we want this on 0.2.1.x?

Probably, if it is tested and works. Have you had a chance to test it?

Yeah, it compiles with the patch in my bug1954 branch on win7. I haven't been able to run it successfully though due to openssl errors at startup; but those are likely to be the fault of my setup... :(

Also tested by chrisd. Merged; closing.

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Trac:
Component: Tor Client to Tor

closed

moved to tpo/core/tor#1954 (closed)