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

Curl static compilation problem (X509_NAME) on Windows with BoringSSL 1.1.0 #5669

Closed
samuel-tranchet opened this issue Jul 9, 2020 · 10 comments
Closed
Labels

Comments

@samuel-tranchet
Copy link

@samuel-tranchet samuel-tranchet commented Jul 9, 2020

I have a recent problem when I try to compile Curl static lib on windows with BoringSSL. The above commands where working fine 6 months ago, but with latest versions of Curl and BoringSSL I came into an issue in the nmake command.

I did this

call buildconf.bat

cd "winbuild"

set RTLIBCFG=static
set _CL_=-MTd -Ob0 -Od -Zi -FS

call nmake /NOLOGO /X - /F MakeFile.vc VC=16 mode=static MACHINE=x64 DEBUG=yes WITH_PREFIX="C:/cpplibs/Curl" WITH_SSL=static SSL_PATH="C:/cpplibs/BoringSSL" ENABLE_SSPI=yes ENABLE_IPV6=yes ENABLE_IDN=yes ENABLE_WINSSL=no

I expected the following

I expected a successful build but I get the following in the console :

Generating prerequisite files
* C:\cpplibs\Sources\Curl\latest\Makefile
* C:\cpplibs\Sources\Curl\latest\src\tool_hugehelp.c
configuration name: libcurl-vc16-x64-release-static-ssl-static-ipv6-sspi

cl /O2 /DNDEBUG /MT /DCURL_STATICLIB /I. /I ../lib /I../include /nologo /W4 /EHsc /DWIN32 /FD /c /DBUILDING_LIBCURL /DUSE_OPENSSL /I"C:/cpplibs/BoringSSL"  /DUSE_WIN32_IDN /DWANT_IDN_PROTOTYPES  /DUSE_IPV6  /DUSE_WINDOWS_SSPI /Fo"..\builds\libcurl-vc16-x64-release-static-ssl-static-ipv6-sspi-obj-lib/altsvc.obj"  ..\lib\altsvc.c
altsvc.c

... 100+ successfull calls

cl /O2 /DNDEBUG /MT /DCURL_STATICLIB /I. /I ../lib /I../include /nologo /W4 /EHsc /DWIN32 /FD /c /DBUILDING_LIBCURL /DUSE_OPENSSL /I"C:/cpplibs/BoringSSL"  /DUSE_WIN32_IDN /DWANT_IDN_PROTOTYPES  /DUSE_IPV6  /DUSE_WINDOWS_SSPI /Fo"..\builds\libcurl-vc16-x64-release-static-ssl-static-ipv6-sspi-obj-lib/vtls/openssl.obj"  ..\lib\vtls\openssl.c
openssl.c

C:/cpplibs/BoringSSL/openssl/base.h(362): error C2059: syntax error: '<parameter-list>'
C:/cpplibs/BoringSSL/openssl/x509.h(154): error C2059: syntax error: '<parameter-list>'
C:/cpplibs/BoringSSL/openssl/x509.h(154): error C2143: syntax error: missing ')' before '('
C:/cpplibs/BoringSSL/openssl/x509.h(154): error C2059: syntax error: ')'
C:/cpplibs/BoringSSL/openssl/x509.h(154): error C2143: syntax error: missing ')' before 'constant'
C:/cpplibs/BoringSSL/openssl/x509.h(154): error C2091: function returns function
...
C:/cpplibs/BoringSSL/openssl/x509_vfy.h(508): error C2061: syntax error: identifier 'X509_STORE_get_lookup_crls'
C:/cpplibs/BoringSSL/openssl/x509_vfy.h(508): error C2059: syntax error: ';'
C:/cpplibs/BoringSSL/openssl/x509_vfy.h(508): error C2059: syntax error: '<parameter-list>'

C:/cpplibs/BoringSSL/openssl/x509_vfy.h(508): fatal error C1003: error count exceeds 100; stopping compilation

NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.26.28801\bin\HostX64\x64\cl.EXE"' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.26.28801\bin\HostX64\x64\nmake.exe"' : return code '0x2'
Stop.

So I've investigated, and all the errors at the indicated lines seems to have in common the X509_NAME typedef usage, but the definition is correct in openssl/base.h:

typedef struct X509_name_st X509_NAME;

And in openssl/x509.h:

struct X509_name_st {
    STACK_OF(X509_NAME_ENTRY) * entries;
    int modified;  // true if 'bytes' needs to be built
    BUF_MEM *bytes;
    //  unsigned long hash; Keep the hash around for lookups
    unsigned char *canon_enc;
    int canon_enclen;
} /* X509_NAME */;

Furthermore the file ..\lib\vtls\openssl.c in Curl lib includes <openssl/x509v3.h> that includes <openssl/x509.h> that includes <openssl/base.h> ... So everything should be OK, but it's not.

The issue is also reported here.

curl/libcurl version

Latest commit on master branch

operating system

Windows 10 - Visual Studio 2019 16.6.3

@bagder
Copy link
Member

@bagder bagder commented Jul 9, 2020

We build curl with current BoringSSL in the CI twice in every commit and PR, for Linux. Successfully...

@samuel-tranchet
Copy link
Author

@samuel-tranchet samuel-tranchet commented Jul 10, 2020

Yes, I think it's really a Windows specific issue

@samuel-tranchet
Copy link
Author

@samuel-tranchet samuel-tranchet commented Jul 10, 2020

I got something, but the way I solved it isn't very clean IMO :

I had to undef X509_NAME and X509_EXTENSIONS after including wincrypt.h as USE_WIN32_CRYPTO seems to always be defined when compiling for windows

In file ..\lib\vtls\openssl.c line 34:

/* Wincrypt must be included before anything that could include OpenSSL. */
#if defined(USE_WIN32_CRYPTO)
#include <wincrypt.h>
#undef X509_NAME
#undef X509_EXTENSIONS
#endif

Is there a better way to fix the issue ?

@bagder
Copy link
Member

@bagder bagder commented Jul 10, 2020

And this issue isn't what #5606 already fixed then I presume?

@jay jay added the duplicate label Jul 15, 2020
@jay
Copy link
Member

@jay jay commented Jul 15, 2020

No response. This looks to be a duplicate of #5606.

@jay jay closed this Jul 15, 2020
@samuel-tranchet
Copy link
Author

@samuel-tranchet samuel-tranchet commented Jul 30, 2020

Sorry for late answer,

It is not a duplicate of #5606 because it's not a problem of include order (this other problem is fixed as you said, and the latest commit I use include this fix in openssl.c)

When compiling the openssl.c file, the wincrypt.h is included before openssl/base.h as it should so this is really not the issue :

Note: including file: C:\Program Files (x86)\Windows Kits\10\include\10.0.19041.0\um\wincrypt.h
Note: including file: C:\Program Files (x86)\Windows Kits\10\include\10.0.19041.0\shared\bcrypt.h
Note: including file: C:\Program Files (x86)\Windows Kits\10\include\10.0.19041.0\um\ncrypt.h
Note: including file: C:\Program Files (x86)\Windows Kits\10\include\10.0.19041.0\um\dpapi.h
...
Note: including file: C:\cpplibs\Build\BoringSSL\latest\Windows\x64\Debug\include\openssl/base.h
Note: including file: C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.26.28801\include\stdint.h
Note: including file: C:\cpplibs\Build\BoringSSL\latest\Windows\x64\Debug\include\openssl/is_boringssl.h
Note: including file: C:\cpplibs\Build\BoringSSL\latest\Windows\x64\Debug\include\openssl/opensslconf.h

The problem don't seems to be linked directly with Curl but . In fact it seems that the latest version of BoringSSL, and perhaps also OpenSSL, is completly incompatible with wincrypt.h because of the way X509_NAME and several things are declared or defined in both libraries .

in wincrypt.h :

#define X509_NAME                           ((LPCSTR) 7)

in openssl/base.h :

typedef struct X509_name_st X509_NAME;

In both include order this will generate a conflict. The solution is to avoid using Win32 Crypto when OpsenSSL is used. This can be easily done by adding a simple condition where USE_WIN32_CRYPTO is defined when running CMake.

In CMakeList.txt line 498

Change this

if(WIN32)
  set(USE_WIN32_CRYPTO ON)
endif()

By this

if(WIN32 AND NOT USE_OPENSSL)
  set(USE_WIN32_CRYPTO ON)
endif()

I don't think this is a problem because I see no reason to use Win32 Crypto when OpenSSL/BoringSSL is available

@jay
Copy link
Member

@jay jay commented Aug 25, 2020

#5606 says OpenSSL undefines it, maybe BoringSSL doesn't and should?

I don't think this is a problem because I see no reason to use Win32 Crypto when OpenSSL/BoringSSL is available

I'm sure there is a reason for this but I don't recall offhand.

/cc @jblazquez

@jay jay reopened this Aug 25, 2020
@jay jay removed the duplicate label Aug 25, 2020
@samuel-tranchet
Copy link
Author

@samuel-tranchet samuel-tranchet commented Aug 25, 2020

Yes I can confirm that it works "out of box" with OpenSSL.

Another way to fix it at Curl level is to explicitly add undef statements in lib/vtls/openssl.c :

#if defined(USE_WIN32_CRYPTO)
#include <wincrypt.h>
#undef X509_NAME
#undef X509_EXTENSION
#endif

I think these two are enough

@jblazquez
Copy link
Contributor

@jblazquez jblazquez commented Aug 25, 2020

I don't think this is a problem because I see no reason to use Win32 Crypto when OpenSSL/BoringSSL is available

As far as I can tell, the use of wincrypt.h was added on this commit, which reads root CA certificates from the trusted system store:

148534d#diff-7dafef8d70eee3940c9184e4e4e93448

The code is only compiled when USE_WIN32_CRYPTO is defined, but according to #5669 (comment) that macro is always defined on Windows. In any case, I think that using the Win32 Crypto API is needed to read the trusted root store, even if the OpenSSL backend is being used for TLS, so I think this conflict between Win32 Crypto and OpenSSL should be resolved.

jay added a commit to jay/curl that referenced this issue Aug 26, 2020
OpenSSL undefines the conflicting symbols but BoringSSL does not so we
must do it ourselves.

Reported-by: Samuel Tranchet
Assisted-by: Javier Blazquez

Fixes curl#5669
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Aug 26, 2020
OpenSSL undefines the conflicting symbols but BoringSSL does not so we
must do it ourselves.

Reported-by: Samuel Tranchet
Assisted-by: Javier Blazquez

Ref: https://github.com/openssl/openssl/blob/OpenSSL_1_1_1g/include/openssl/ossl_typ.h#L66-L73

Fixes curl#5669
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Aug 26, 2020
OpenSSL undefines the conflicting symbols but BoringSSL does not so we
must do it ourselves.

Reported-by: Samuel Tranchet
Assisted-by: Javier Blazquez

Ref: https://bugs.chromium.org/p/boringssl/issues/detail?id=371
Ref: https://github.com/openssl/openssl/blob/OpenSSL_1_1_1g/include/openssl/ossl_typ.h#L66-L73

Fixes curl#5669
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Aug 26, 2020
OpenSSL undefines the conflicting symbols but BoringSSL does not so we
must do it ourselves.

Reported-by: Samuel Tranchet
Assisted-by: Javier Blazquez

Ref: https://bugs.chromium.org/p/boringssl/issues/detail?id=371
Ref: https://github.com/openssl/openssl/blob/OpenSSL_1_1_1g/include/openssl/ossl_typ.h#L66-L73

Fixes curl#5669
Closes #xxxx
@jay jay closed this in fbe07c6 Aug 27, 2020
@jay
Copy link
Member

@jay jay commented Aug 27, 2020

Thanks guys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.