Windows: Add explicit manifest to Windows build

[rodwiddowson]

Over Here Daniel noted (channelling Stefan Kanthak):

For the Windows build, please incorporate the following changes:

1. download https://curl.haxx.se/favicon.ico and save it as "curl.ico" in the top level directory

2. add the file "lib/libcurl.manifest" to disable DLL hijacking per external manifest, and more

---
+++ curl-patch/lib/libcurl.manifest
@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<assembly manifestVersion="1.0" xmlns="urn:schemas-microsoft-com:asm.v1">
+    <assemblyIdentity language="en-US" name="cURL" processorArchitecture="*" type="win32" version="7.58.0.0"/>
                                                                                                     ~~~~~~~~
+    <description>libcurl Shared Library</description>
+    <file name="advapi32.dll" loadFrom="%SystemRoot%\system32\advapi32.dll" />
+    <file name="crypt32.dll" loadFrom="%SystemRoot%\system32\crypt32.dll" />
+    <file name="kernel32.dll" loadFrom="%SystemRoot%\system32\kernel32.dll" />
+    <file name="wldap32.dll" loadFrom="%SystemRoot%\system32\wldap32.dll" />
+    <file name="ws2_32.dll" loadFrom="%SystemRoot%\system32\ws2_32.dll" />
+</assembly>

NOTE: the underlined version string MUST reflect cURL's current version!

3. apply the following changes to file "lib\libcurl.rc" to include icon and manifest

--- curl-7.58.0/lib/libcurl.rc    2017-12-03 00:33
+++ curl-patch/lib/libcurl.rc
@@ -22,4 +22,9 @@
+#include <winuser.h>
  #include <winver.h>
  #include "tool_version.h"

-LANGUAGE  0x09,0x01
+LANGUAGE 0, 0
+
+1 ICON "../curl.ico"
+
+CREATEPROCESS_MANIFEST_RESOURCE_ID RT_MANIFEST "../lib/libcurl.manifest"

NOTE: ALL resources used with cURL are language-independent!

4. add the file "src/curl.manifest" to disable DLL hijacking per external manifest, and more

---
+++ curl-patch/src/curl.manifest
@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<assembly manifestVersion="1.0" xmlns="urn:schemas-microsoft-com:asm.v1">
+    <assemblyIdentity language="en-US" name="cURL" processorArchitecture="*" type="win32" version="7.58.0.0"/>
                                                                                                     ~~~~~~~~
+    <description>The cURL executable</description>
+    <file name="advapi32.dll" loadFrom="%SystemRoot%\system32\advapi32.dll" />
+    <file name="crypt32.dll" loadFrom="%SystemRoot%\system32\crypt32.dll" />
+    <file name="kernel32.dll" loadFrom="%SystemRoot%\system32\kernel32.dll" />
+    <file name="wldap32.dll" loadFrom="%SystemRoot%\system32\wldap32.dll" />
+    <file name="ws2_32.dll" loadFrom="%SystemRoot%\system32\ws2_32.dll" />
+</assembly>

NOTE: the underlined version string MUST reflect cURL's current version!

5. apply the following changes to file "src\curl.rc" to include icon and manifest

--- curl-7.58.0/src/curl.rc    2017-12-03 00:33
+++ curl-patch/src/curl.rc
@@ -22,4 +22,9 @@
+#include <winuser.h>
  #include <winver.h>
  #include "tool_version.h"

-LANGUAGE  0x09,0x01
+LANGUAGE 0, 0
+
+1 ICON "../curl.ico"
+
+CREATEPROCESS_MANIFEST_RESOURCE_ID RT_MANIFEST "../src/curl.manifest"

---

I (Rod) will add that making strings match versions is the way to madness and the only sensible way to proceed is to exbed this into a single config file and pass the value in to the build "Somehow".

[gvanem]

"win32" version="7.58.0.0"/> I (Rod) will add that making strings match versions is the way to madness and the only sensible way to proceed is to exbed this into a single config file and pass the value in to the build "Somehow".

Since #include "../include/curl/curlver.h" is already included into libcurl.rc, it's simple to add a MANIFEST_VERSION_STR before "libcurl.manifest" gets included by rc. E.g.:

--- a/libcurl.rc 2017-01-19 19:43:19
+++ b/libcurl.rc 2018-02-22 16:15:53
@@ -22,7 +22,18 @@
 #include <winver.h>
 #include "../include/curl/curlver.h"

-LANGUAGE  0x09,0x01
+LANGUAGE 0, 0
+
+APPICON ICON "../curl.ico"
+
+#define _STR2(x) #x
+#define _STR(x)  _STR2(x)
+
+#define MANIFEST_VERSION_STR _STR(LIBCURL_VERSION_MAJOR) "." \
+                             _STR(LIBCURL_VERSION_MINOR) "." \
+                             _STR(LIBCURL_VERSION_PATCH) ".0"
+
+CREATEPROCESS_MANIFEST_RESOURCE_ID RT_MANIFEST "../lib/libcurl.manifest"

 #define RC_VERSION  LIBCURL_VERSION_MAJOR, LIBCURL_VERSION_MINOR, LIBCURL_VERSION_PATCH, 0

But I think manifests are not a good idea. And what is <winuser.h> for?

[monnerat]

download https://curl.haxx.se/favicon.ico and save it as "curl.ico" in the top level directory

add the file "lib/libcurl.manifest" to disable DLL hijacking per external manifest, and more

add the file "src/curl.manifest" to disable DLL hijacking per external manifest, and more

These are architecture-specific files: would'nt it be better (if possible) to store them in the corresponding architecture subdirectory ?

[jay]

I don't think we should make any guarantee against DLL hijacking (planting) attacks. In this advisory I wrote

"Also note it is may still be possible for planting attacks to be done against load-time DLLs used by libcurl and the curl tool. This is because Windows loads those DLLs and their dependencies without specifying a path. There is nothing we can do to fix this, it is endemic in the design of Windows. We advise you to guard write permissions on your application directory."

Stefan disagrees and offers the solution that is being discussed in this thread, specifically loadFrom to specify the path. The problem is to prevent against DLL hijacking we'd have to know not just all the DLLs curl and libcurl are using but all the DLLs their dependencies are using as well. Therefore I think it's reasonable that we can't prevent DLL hijacking at load time unless we know every single thing that's loaded, and who's to say that some third party dependency won't change its dependencies in the future, also having the same problem.

Also wasn't there some issue using a manifest or am I not remembering that right @vszakats?

edit: Also loadFrom has some quirks https://stackoverflow.com/a/2137463

[MarcelRaad]

I agree with @jay . It's the application's responsibility to guard against such things, not libcurl's. And such a list of DLLs can get pretty long in some configurations.

What we could do, though, is to call Windows API functions like SetSearchPathMode, SetDllDirectory, and SetDefaultDllDirectories at startup in the curl tool.

[vszakats]

I believe libcurl/curl should make sure to use best practices when dynamically loading a .dll, but this already seems to be the case in Curl_load_library() in lib/system_win32.c. As for altering OS behaviour when implicitly loading .dlls, it seems to be out of scope for a generic library.

As for the manifest issue, I had a patch proposal to resolve an issue caused by Windows versioning "peculiarities". This was done via a manifest embedded in curl.rc (as opposed to a separate .mft/.manifest file.) This helps including a manifest in an unobtrusive way while also allowing to use .h constants in it. It works with popular C compilers (MinGW, MSVC), though may break with some non-mainstream/niche ones (e.g. OpenWatcom — fixed since, or Borland C) due to bugs.

If the goal is to have the libcurl/curl version included in the linked manifest, the above method can be useful.

[rodwiddowson]

For avoidance of doubt: I have no position on this. I mostly hang out in kernel mode where thing are different. My role in this case was purely to capture it into github (and engender this conversation)

I'll happily close this case (or better still let @jay do it) if the community agree./

[jay]

I am partially for this for version information and the icon, and I think we should drop the other things.

@gvanem wrote":

But I think manifests are not a good idea.

why?

[bagder]

I'm sensing that the team is leaning towards a "don't merge" on this suggestion.

[rb07]

The library doesn't need a manifest, the curl executable does.

If the executable doesn't have a manifest it runs in a restricted environment (an emulation, which you can see in task manager), similar to what Windows does to 32-bit binaries running in the 64-bit OS.

I (as a user) would prefer the executable with a manifest. Just as anybody would prefer an installer, and executable signed with a developer key to make sure they are original releases, not 3rd party, maybe hacked versions; of course the manifest doesn't do anything about this.

[vszakats]

Lack of a manifest will alter behaviour of specific Win32 API calls. One of these is the infamous case of version detection functions returning phoney values. In context of curl, this will cause at least one feature to stay disabled even if the host OS version does support it. Specifically this condition will always return FALSE without a manifest. So, a manifest would be required to make this specific case work as expected.

As for a digital signature, it'd be real nice to be able to create them for free/open-source software built for Windows, but the reality is that it costs money and it's likely to cause a recurring (yearly?) administration burden (and certain privacy concerns) on the top of it. For projects/efforts without income, there isn't a reasonable solution for this ATM. (The infrastructure/tooling support is solvable/solved though, even with open tools. One slight downside is that a signature plays badly with reproducibility due to the embedded timestamp.)

UPDATE (2018-05-31): Real-life experience with code signing in an open-source project: https://github.com/ImageOptim/gifski/releases/tag/0.8.3 and related HN thread.

The curl project itself doesn't release binaries on its own, so it's always up to 3rd parties to create them. The next best (or maybe the best?) method for verifiability is reproducibility via deterministic builds.

[bagder]

It seems #2591 happens because of the lack of a manifest!?

[vszakats]

@bagder Yes, it seems so.

[bagder]

Anyone up for writing a PR (or more) to make sure curl builds with a manifest? It seems like an easy way to fix #2591 and perhaps more.

[vszakats]

This earlier PR (of mine) offered a viable (or at least potential) solution for this: https://github.com/curl/curl/pull/1221

[vszakats]

Manifest has been added to curl.rc in https://github.com/curl/curl/commit/ebd213270a017a6830928ee2e1f4a9cabc799898. Some build tools are already opted-in, remaining ones can opt-in by adding a resource compiler flag. The manifest automatically pulls curl version from existing curl headers. Same principal can be used with libcurl.rc, should a manifest ever be needed there.

I've also made a brief test with the undocumented loadFrom manifest items, but the resulting curl.exe failed to run. When reading up about the topic, I learned that these Windows system .dlls (aka KnownDLLs) are always loaded from their fixed location, but it's even more likely they are already loaded into memory, in which case the existing copy will be reused. I don't see why these items would improve security for curl.exe and considering their undocumented nature and the problems experienced, I'd side with not adding them. [Anyhow, in case any manifest item turns out to be beneficial, it's now a no-brainer to add it to curl.exe. ]

[vszakats]

Updated https://github.com/curl/curl/issues/2326#issuecomment-390327512 with links to a real-life recount of firing up a code signing certificate for an open-source project by its brave author.

[bagder]

So based on comment, can we say that this issue is now fixed? @vszakats ?

[vszakats]

Yes, the manifest issue is fixed.

I had omitted the loadFrom trick from above, because it's an undocumented keyword and I wasn't convinced it's necessary, though technically any further manifest elements can be added in the future as needed. Focus was on mainline C toolchains, i.e. Visual C and MinGW. For the rest (e.g. Open Watcom), some caveats apply, as detailed in the commit message.

The new manifest in turn fixes Windows version detection, which fixes ALPN use with SCHANNEL, for Visual C builds. MinGW builds have an unrelated, upstream issue detailed here. When upstream is fixed, libcurl code will need a tweak to detect ALPN support in Windows SDK (= Win32 headers) in a C compiler-agnostic way.

[bagder]

Thanks a lot, then I consider this particular issue closed!