openssl: fix BoringSSL symbol conflicts with LDAP and Schannel #9110

vszakats · 2022-07-06T00:18:49Z

Same issue as here [1], but this time when building curl with BoringSSL
for Windows with LDAP(S) or Schannel support enabled.

Apply the same fix [2] for these source files as well.

This can also be fixed by moving #include "urldata.h" before
including winldap.h and schnlsp.h respectively. This seems like
a cleaner fix, though I'm not sure why it works and if it has any
downside. (see below)

[1] #5669
[2] fbe07c6

vszakats · 2022-07-06T00:21:25Z

~~Alternative fix:~~ [UPDATE: This might break OpenSSL builds. See below.]

diff --git a/lib/ldap.c b/lib/ldap.c
index 51a32dc96..13b684104 100644
--- a/lib/ldap.c
+++ b/lib/ldap.c
@@ -37,6 +37,8 @@
  * OpenLDAP library versions, USE_OPENLDAP shall not be defined.
  */
 
+#include "urldata.h"
+
 #ifdef USE_WIN32_LDAP           /* Use Windows LDAP implementation. */
 # include <winldap.h>
 # ifndef LDAP_VENDOR_NAME
@@ -56,7 +58,6 @@
 # endif /* HAVE_LDAP_SSL && HAVE_LDAP_SSL_H */
 #endif
 
-#include "urldata.h"
 #include <curl/curl.h>
 #include "sendf.h"
 #include "escape.h"
diff --git a/lib/vtls/schannel.h b/lib/vtls/schannel.h
index 0b4c4d934..45ecb029b 100644
--- a/lib/vtls/schannel.h
+++ b/lib/vtls/schannel.h
@@ -28,12 +28,12 @@
 
 #ifdef USE_SCHANNEL
 
+#include "urldata.h"
+
 #include <schnlsp.h>
 #include <schannel.h>
 #include "curl_sspi.h"
 
-#include "urldata.h"
-
 /* <wincrypt.h> has been included via the above <schnlsp.h>.
  * Or in case of ldap.c, it was included via <winldap.h>.
  * And since <wincrypt.h> has this:

Same issue as here [1], but this time when building curl with BoringSSL for Windows with LDAP(S) or Schannel support enabled. Apply the same fix [2] for these source files as well. This can also be fixed by moving `#include "urldata.h"` _before_ including `winldap.h` and `schnlsp.h` respectively. This seems like a cleaner fix, though I'm not sure why it works and if it has any downside. [1] curl#5669 [2] curl@fbe07c6

jay · 2022-07-06T08:20:20Z

Interesting. Could boringssl have fixed it to undefine the symbols? For all versions we have the way that works, I would just copy that, note it must come after curl_setup.h which always comes first.

curl/lib/vtls/openssl.c

Lines 36 to 46 in 45ac4d0

    
           /* Wincrypt must be included before anything that could include OpenSSL. */ 
        
           #if defined(USE_WIN32_CRYPTO) 
        
           #include <wincrypt.h> 
        
           /* Undefine wincrypt conflicting symbols for BoringSSL. */ 
        
           #undef X509_NAME 
        
           #undef X509_EXTENSIONS 
        
           #undef PKCS7_ISSUER_AND_SERIAL 
        
           #undef PKCS7_SIGNER_INFO 
        
           #undef OCSP_REQUEST 
        
           #undef OCSP_RESPONSE 
        
           #endif

vszakats · 2022-07-06T09:06:15Z

Probably could be fixed in BoringSSL, but I see little chance getting it fixed there. Likely Google don't mix it with wincrypt.h in their own source code to justify a workaround. ~~A local note about include order should be fine I think.~~

Its downside is that this isn't supposed to be used as a dependency, has no versioning or formal releases. TLS-SRP also isn't supported, but its advantages outweigh it. A smaller downside is that it uses pthread even on Windows, which has an issue with static linking, possibly in combination with UCRT builds with the cross-toolchain I tested it with. With this fixed, it's the most promising alternative TLS backend. Binary size is also 2MB lower for both curl EXE and DLL. curl needs two patches, one merged [1], one fixing new iterations of a known issue [2] pending. [1] curl/curl#9109 [2] curl/curl#9110 curl would probably use a third patch from a CMake expert: ngtcp2 detection is broken, so we need to use an local workaround. curl autotools doesn't support BoringSSL because I've found no way to tell libtool to link pthread DLL instead of the static version.

vszakats · 2022-07-07T10:48:25Z

Found this in openssl.c:

/* Wincrypt must be included before anything that could include OpenSSL. */

This was a similar bug where apparently the exact reverse action (i.e. moving urldata.h after wincrypt.h) solved the problem for OpenSSL. Needing a subsequent patch (which I copied for this PR) to refix it for BoringSSL. So, after all, it looks like the urldata.h moving-around fix is no good and we should go with the current form of this PR.

jay · 2022-07-08T03:46:33Z

I would explicitly include wincrypt, followed by the undefines, as early as possible so that something that includes something that includes BoringSSL can't come before it.

vszakats · 2022-07-08T10:07:30Z

@jay: Do you have an alternative patch suggestion?

jay · 2022-07-09T18:50:12Z

I mean this, it's not really an alternative I just added the include

diff --git a/lib/ldap.c b/lib/ldap.c
index 51a32dc..292684b 100644
--- a/lib/ldap.c
+++ b/lib/ldap.c
@@ -26,6 +26,18 @@
 
 #if !defined(CURL_DISABLE_LDAP) && !defined(USE_OPENLDAP)
 
+/* Wincrypt must be included before anything that could include OpenSSL. */
+#if defined(USE_WIN32_CRYPTO)
+#include <wincrypt.h>
+/* Undefine wincrypt conflicting symbols for BoringSSL. */
+#undef X509_NAME
+#undef X509_EXTENSIONS
+#undef PKCS7_ISSUER_AND_SERIAL
+#undef PKCS7_SIGNER_INFO
+#undef OCSP_REQUEST
+#undef OCSP_RESPONSE
+#endif
+
 /*
  * Notice that USE_OPENLDAP is only a source code selection switch. When
  * libcurl is built with USE_OPENLDAP defined the libcurl source code that
diff --git a/lib/vtls/schannel.h b/lib/vtls/schannel.h
index 0b4c4d9..05323c7 100644
--- a/lib/vtls/schannel.h
+++ b/lib/vtls/schannel.h
@@ -28,6 +28,18 @@
 
 #ifdef USE_SCHANNEL
 
+/* Wincrypt must be included before anything that could include OpenSSL. */
+#if defined(USE_WIN32_CRYPTO)
+#include <wincrypt.h>
+/* Undefine wincrypt conflicting symbols for BoringSSL. */
+#undef X509_NAME
+#undef X509_EXTENSIONS
+#undef PKCS7_ISSUER_AND_SERIAL
+#undef PKCS7_SIGNER_INFO
+#undef OCSP_REQUEST
+#undef OCSP_RESPONSE
+#endif
+
 #include <schnlsp.h>
 #include <schannel.h>
 #include "curl_sspi.h"

either that or maybe it could just be included once in whatever curl header is actually causing this problem by including boringssl, rather than putting it in each unit

vszakats · 2022-07-10T10:51:06Z

@jay: Thanks, got it. But, speaking of MinGW, both schannel.h and winldap.h include wincrypt.h unconditionally anyway, so this reduces the fix to cases when USE_WIN32_CRYPTO is defined in the build. Do we want that?

jay · 2022-07-11T18:59:55Z

this reduces the fix to cases when USE_WIN32_CRYPTO is defined in the build. Do we want that?

That works in openssl.c but maybe WIN32 is the right check in ldap.

vszakats · 2022-07-11T19:52:13Z

Tried with WIN32. ldap.c look fine, but schannel.h ends up a little bit confusing. WIN32 looks redundant inside USE_SCHANNEL. USE_WIN32_CRYPTO is difficult trace back to its origin and most likely always defined there anyway (or maybe not in UWP apps?). A few lines below, there is already a wincypt.h include, which looks odd after this patch.

But, it resolves the problem, so for me this version is also fine.

vszakats · 2022-07-12T04:52:20Z

I re-set it to use USE_WIN32_CRYPTO. This macro should always be set for MinGW (and in all "classic" Win32 builds). I assume on systems where it isn't set, schannel.h and winldap.h would not attempt to pull in wincrypt.h. In UWP apps maybe.

Ref: curl/curl@4c46c82 Ref: curl/curl#9110 Follow-up of: 8a3a331

vszakats added build TLS LDAP Windows Windows-specific labels Jul 6, 2022

vszakats force-pushed the win-boringssl-clash-more branch from f7c6a4b to 6d5f552 Compare July 6, 2022 00:19

vszakats force-pushed the win-boringssl-clash-more branch from 6d5f552 to f51fe7b Compare July 6, 2022 00:29

vszakats changed the title ~~openssl: fix boringssl symbol conflicts with ldap and schannel~~ openssl: fix BoringSSL symbol conflicts with LDAP and Schannel Jul 6, 2022

vszakats force-pushed the win-boringssl-clash-more branch from f51fe7b to 6eea6a7 Compare July 6, 2022 00:30

tweak

22b54b4

tweak 2: use USE_WIN32_CRYPTO

403c9b2

vszakats closed this in 4c46c82 Jul 14, 2022

vszakats deleted the win-boringssl-clash-more branch July 14, 2022 07:15

vszakats added a commit to curl/curl-for-win that referenced this pull request Jul 14, 2022

curl: update patch with upstream final version [ci skip]

8625ab0

Ref: curl/curl@4c46c82 Ref: curl/curl#9110 Follow-up of: 8a3a331

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openssl: fix BoringSSL symbol conflicts with LDAP and Schannel #9110

openssl: fix BoringSSL symbol conflicts with LDAP and Schannel #9110

vszakats commented Jul 6, 2022 •

edited

vszakats commented Jul 6, 2022 •

edited

jay commented Jul 6, 2022

vszakats commented Jul 6, 2022 •

edited

vszakats commented Jul 7, 2022 •

edited

jay commented Jul 8, 2022

vszakats commented Jul 8, 2022

jay commented Jul 9, 2022

vszakats commented Jul 10, 2022

jay commented Jul 11, 2022

vszakats commented Jul 11, 2022

vszakats commented Jul 12, 2022 •

edited

openssl: fix BoringSSL symbol conflicts with LDAP and Schannel #9110

openssl: fix BoringSSL symbol conflicts with LDAP and Schannel #9110

Conversation

vszakats commented Jul 6, 2022 • edited

vszakats commented Jul 6, 2022 • edited

jay commented Jul 6, 2022

vszakats commented Jul 6, 2022 • edited

vszakats commented Jul 7, 2022 • edited

jay commented Jul 8, 2022

vszakats commented Jul 8, 2022

jay commented Jul 9, 2022

vszakats commented Jul 10, 2022

jay commented Jul 11, 2022

vszakats commented Jul 11, 2022

vszakats commented Jul 12, 2022 • edited

vszakats commented Jul 6, 2022 •

edited

vszakats commented Jul 6, 2022 •

edited

vszakats commented Jul 6, 2022 •

edited

vszakats commented Jul 7, 2022 •

edited

vszakats commented Jul 12, 2022 •

edited