Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jul 6, 2022

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 vszakats force-pushed the win-boringssl-clash-more branch from f7c6a4b to 6d5f552 Compare Jul 6, 2022
@vszakats
Copy link
Member Author

vszakats commented Jul 6, 2022

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:

@vszakats vszakats force-pushed the win-boringssl-clash-more branch from 6d5f552 to f51fe7b Compare Jul 6, 2022
@vszakats 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
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
@vszakats vszakats force-pushed the win-boringssl-clash-more branch from f51fe7b to 6eea6a7 Compare Jul 6, 2022
@jay
Copy link
Member

jay commented Jul 6, 2022

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
Copy link
Member Author

vszakats commented Jul 6, 2022

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.

vszakats added a commit to curl/curl-for-win that referenced this issue Jul 6, 2022
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
Copy link
Member Author

vszakats commented Jul 7, 2022

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
Copy link
Member

jay commented Jul 8, 2022

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
Copy link
Member Author

vszakats commented Jul 8, 2022

@jay: Do you have an alternative patch suggestion?

@jay
Copy link
Member

jay commented Jul 9, 2022

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
Copy link
Member Author

vszakats commented Jul 10, 2022

@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
Copy link
Member

jay commented Jul 11, 2022

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
Copy link
Member Author

vszakats commented Jul 11, 2022

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
Copy link
Member Author

vszakats commented Jul 12, 2022

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.

@vszakats vszakats closed this in 4c46c82 Jul 14, 2022
@vszakats vszakats deleted the win-boringssl-clash-more branch Jul 14, 2022
vszakats added a commit to curl/curl-for-win that referenced this issue Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants