Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#1954 closed defect (fixed)

LoadLibrary used without restrictions for search path

Reported by: Sebastian Owned by:
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (15)

comment:1 Changed 9 years ago by Sebastian

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

comment:2 Changed 9 years ago by arma

Milestone: Tor: 0.2.2.x-final
Priority: normalmajor

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

Nick?

comment:3 Changed 9 years ago by nickm

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?

comment:4 Changed 9 years ago by nickm

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?

comment:5 Changed 9 years ago by ioerror

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

comment:6 Changed 9 years ago by mikeperry

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.

comment:7 Changed 9 years ago by nickm

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'.")

comment:8 Changed 9 years ago by nickm

Status: newneeds_review

See bug1954 in my public.

comment:9 Changed 9 years ago by Sebastian

+  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.

comment:10 Changed 9 years ago by nickm

Branch updated. Thanks!

comment:11 Changed 9 years ago by Sebastian

hm. Do we want this on 0.2.1.x?

comment:12 Changed 9 years ago by nickm

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

comment:13 Changed 9 years ago by Sebastian

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... :(

comment:14 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Also tested by chrisd. Merged; closing.

comment:15 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.