[2.2.{1,2}] Windows LoadLibrary DLL hijacking vulnerability (CVE-2017-11742)

[vszakats]

Starting with 2.2.1, libexpat added a LoadLibrary() call to load the ADVAPI32.DLL Windows system DLL to improve random numbers. This call however is prone to a known DLL hijacking vulnerability, with no (trivial) way to opt-out from this by apps making use of libexpat. The attack works by building a tailor-made ADVAPI32.DLL that exports the function required and called by libexpat, and copying that DLL to the directory of the user application or to the current directory.

My (already proposed in #62) patch is this:

--- expat.orig/xmlparse.c	2017-07-12 21:55:49.000000000 +0000
+++ expat/xmlparse.c	2017-07-14 19:46:29.000000000 +0000
@@ -777,6 +779,10 @@
 
 typedef BOOLEAN (APIENTRY *RTLGENRANDOM_FUNC)(PVOID, ULONG);
 
+#ifndef LOAD_LIBRARY_SEARCH_SYSTEM32
+# define LOAD_LIBRARY_SEARCH_SYSTEM32  0x00000800
+#endif
+
 /* Obtain entropy on Windows XP / Windows Server 2003 and later.
  * Hint on RtlGenRandom and the following article from libsodioum.
  *
@@ -786,7 +792,7 @@
 static int
 writeRandomBytes_RtlGenRandom(void * target, size_t count) {
   int success = 0;  /* full count bytes written? */
-  const HMODULE advapi32 = LoadLibrary(TEXT("ADVAPI32.DLL"));
+  const HMODULE advapi32 = LoadLibraryEx(TEXT("ADVAPI32.DLL"), NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
 
   if (advapi32) {
     const RTLGENRANDOM_FUNC RtlGenRandom

It will resolve the problem for Windows Vista and newer versions, thus covering all officially supported versions of Windows. For older versions the generally recommended method is to detect SYSTEM32 directory and prepend that to the loaded DLL name.

[hartwork]

Hi Viktor,

quoting MSDN on flag LOAD_LIBRARY_SEARCH_SYSTEM32 to LoadLibraryEx:

Windows Server 2003 and Windows XP: This value is not supported.

I am unsure if we can drop Support for Windows XP from Expat, so passing LOAD_LIBRARY_SEARCH_SYSTEM32 unconditionally would exclude Windows XP.

I only now start to wonder if it would be an option to call LoadLibraryEx twice:

• first with dwFlags=LOAD_LIBRARY_SEARCH_SYSTEM32, then (on failure)
• once more with dwFlags=0 (which I understand to use (insecure) default behavior).

On Windows XP the first call would fail but the second would succeed. On later systems, the first would succeed right away. That would add security for post-XP users and keep XP supported. Am I missing something?

[vszakats]

Passing LOAD_LIBRARY_SEARCH_SYSTEM32 unconditionally on Windows XP is a no-op. It's safe to pass. By "Not supported", the MSDN article means that it won't trigger the described functionality, but the call will succeed anyway, with the default search order, as if the flag wasn't passed at all.

IOW this patch doesn't affect Windows XP in any way: It won't fix the vulnerability, but let it continue to work just as it was working before the patch.

[hartwork]

Is that behaviour documented somewhere?

[vszakats]

I don't have a link for that, but it would be expected in order to retain backwards compatibility. To avoid assumptions, it'd be best to test this on a real Windows XP system (I don't have access to one though.)

BTW, to make it more confusing, the flag is only supported on and above Windows 8. For Vista/7/etc, it requires patch KB2533623 to be installed. (To me this suggests it's even more likely that the flag is just ignored if passed and the patch isn't there.)

To detect availability of this functionality (f.e. to fall-back to an alternate fix), this logic may be used:

static int _has_search_system32()
{
   if (_ISWIN8())
      return 1;
   else
   {
      HMODULE hKernel32 = GetModuleHandle(TEXT("kernel32.dll"));

      if (hKernel32)
         return _GETPROCADDRESS(hKernel32, "AddDllDirectory") != NULL;  /* Detect KB2533623 */
   }

   return 0;
}

(source: https://github.com/vszakats/harbour-core/blob/master/contrib/hbwin/wapi_misc.c, own work, thus not a proof.)

[hartwork]

For Windows, I depend on pull requests. I have added the help wanted label now.

[kwaclaw]

IMO, Expat should only do a reasonable minimum in these cases, following the principle of spearation of concerns. In controlled environments one should probably use AppLocker: see https://blogs.technet.microsoft.com/srd/2014/05/13/load-library-safely/. One can also check the signature - it seems ADVAPI32.DLL has one, at least on newer Windows versions. I would not go out of our way to support XP, it is unsecure as it is already. Karl

…

On Fri, Jul 14, 2017 at 5:16 PM, Sebastian Pipping ***@***.*** > wrote: For Windows, I rely on pull request. I have added the help wanted label now. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#82 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAoAqoPg7jCcNYMT0dU5KLHc78MOiD8Lks5sN9qtgaJpZM4OYr4O> .

[vszakats]

I also feel that a patch that solves this for recent Windows versions is much better than nothing. Waiting for someone interested enough to provide a full-blown solution that offers fix for XP and below as well, leaves the vulnerability open on the majority of machines in the meantime, which is not ideal.

[hartwork]

Wine 2.0 does ignore LOAD_LIBRARY_SEARCH_SYSTEM32 and succeeds, reporting

fixme:module:load_library unsupported flag(s) used (flags: 0x00000800)

but before we know 110% that LOAD_LIBRARY_SEARCH_SYSTEM32 is just ignored in "actual Windows", let's play it safe. Please review/test #83. We can still squash it to a single call later but I consider that an optimization.

[hartwork]

Fixed by 99fb4b5 on master, closing.

[vszakats]

I'd argue it's not fixed for Windows XP and unpatched Vista/7 plus the unusual fall-back logic to the standard search-path opened potentially new options for an attacker. It's the worse of both worlds :(

[vszakats]

FWIW, here's the patch to fix the exact same issue (CVE-2016-4802) in libcurl from a year ago:
https://curl.haxx.se/CVE-2016-4802.patch

It resolves the problem for all Windows versions and would only add the flag if the required Windows patch is installed, without the unusual fall-back logic that's currently committed to libexpat. It's very similar to the source code I posted earlier, but directly from a well-known, battle-tested project.

[hartwork]

I'd argue it's not fixed for Windows XP and unpatched Vista/7

If it's not, then LOAD_LIBRARY_SEARCH_SYSTEM32 has no chance of helping these users in a one-shot version, either. A single LoadLibraryEx(TEXT("ADVAPI32.DLL"), NULL, LOAD_LIBRARY_SEARCH_SYSTEM32) cannot support more platforms than the two step one we have now.

plus the unusual fall-back logic to the standard search-path opened potentially new options for an attacker.

For new vectors from the retry code, the scenario would be that an attacker can make the first call to LoadLibraryEx fail (from outside the process) and then have the second call to LoadLibraryEx succeed with his own file put in place. I'm not arguing against race conditions, but I doubt that from outside the process you can make the first one fail for advapi32.dll (since it's not an arbitrary file name but a core Windows library).

It's the worse of both worlds :(

It's better than what we had in the code, before.

FWIW, here's the patch to fix the exact same issue (CVE-2016-4802) in libcurl a year ago:
https://curl.haxx.se/CVE-2016-4802.patch

Curl_load_library looks pretty good, their license works for us as well.

[hartwork]

I believe we should request a CVE for this issue using the form on https://cveform.mitre.org/ .

@vszakats could you review my suggestion on form input below before I submit? If you'd rather submit yourself or already have, please let me know, so we can save save duplicate work.

Select a request type
    Request a CVE ID

Number of vulnerabilities reported or IDs requested (1-10)
    1

Vulnerability type
    Other or unknown

Other vulnerability type
    DLL hijacking

Vendor of the product(s)
    Expat

Affected product(s)/code base
    Expat 2.2.1
    Expat 2.2.2

Has vendor confirmed or acknowledged the vulnerability?
    Yes

Attack type
    Other

Other attack type
    User assisted arbitrary code execution

Impact
    Code Execution

Attack vector(s)
    Parsing XML file from current working directory with malicious advapi32.dll next to it,
    e.g. after extracting an archive containing multiple files.

Suggested description of the vulnerability for use in the CVE
    Expat 2.2.{1,2} LoadLibrary DLL hijacking vulnerability on Windows

Discoverer(s)/Credits
    Reported by Viktor Szakats
    Fix based on code by Steve Holme

Reference(s)
    https://github.com/libexpat/libexpat/issues/82

[vszakats]

@hartwork Thank you submitting this, it looks good to me.

[hartwork]

Thanks for the review. Request submitted now.

[hartwork]

MITRE assigned CVE-2017-11742 to this vulnerability now.