Bug 216781 - net/qt5-network: fix build with libressl-devel
Summary: net/qt5-network: fix build with libressl-devel
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: kde
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-04 14:34 UTC by Piotr Kubaj
Modified: 2017-02-23 00:34 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (kde)


Attachments
patch (2.12 KB, patch)
2017-02-04 14:34 UTC, Piotr Kubaj
no flags Details | Diff
fix qt5-network with libressl (9.22 KB, patch)
2017-02-18 16:25 UTC, Matthew Rezny
no flags Details | Diff
fix qt4-network SSL patch (4.98 KB, patch)
2017-02-18 16:39 UTC, Matthew Rezny
no flags Details | Diff
fix qt5-network with libressl (9.14 KB, patch)
2017-02-19 08:14 UTC, Matthew Rezny
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Piotr Kubaj freebsd_committer 2017-02-04 14:34:08 UTC
Created attachment 179597 [details]
patch

LibreSSL 2.5.1 now defines SSL_CTRL_SET_CURVES, but it doesn't actually mean nothing anything.

Building qt5-network fails with:
(00:01:51) --- .obj/qsslcontext_openssl.o ---
(00:01:51) ssl/qsslcontext_openssl.cpp:349:33: error: use of undeclared identifier 'doesnt_exist'
(00:01:51)                                 SSL_CTRL_SET_CURVES,
(00:01:51)                                 ^
(00:01:51) /usr/local/include/openssl/ssl.h:1189:29: note: expanded from macro 'SSL_CTRL_SET_CURVES'
(00:01:51) #define SSL_CTRL_SET_CURVES doesnt_exist
(00:01:51)                             ^

The attached patch fixes this.
Comment 1 Matthew Rezny freebsd_committer 2017-02-18 16:25:00 UTC
Created attachment 180112 [details]
fix qt5-network with libressl

The quick change to make it compile is not the ideal change. Better would be to re-implement the curve control code in a more portable way. The issue is not the ability to set curves but the method for doing so; the SSL_CTRL_SET_CURVES "convenience" macro only exists in OpenSSL. The BoringSSL porting guide says use of that macro should be replaced with the function SSL_CTX_set1_curves, which is what the macro would have called anyway. That should be fully portable as that function exists in OpenSSL, and LibreSSL has a macro of the same name to call their function SSL_CTX_set1_groups which serves the same purpose. Unfortunately, Qt complicates matters by loading the libraries and resolving symbols itself, so that macro doesn't help us for LibreSSL and we still need a separate code path, but at least we can use one path for both OpenSSL and BoringSSL so we don't need three cases.

While implementing a proper solution to this issue, I was forced to dig into the dynamic loading of the SSL libraries, something I suspected was not working properly. What I found is that we have a patch meant to use the full path to find the correct OpenSSL libs first without searching many paths in Linux's preferred order. However, there is a mistake in that patch so it tried to open /usr/local/libssl.so and /usr/local/libcrypto.so instead of /usr/local/lib/libssl.so and /usr/local/lib/libcrypto.so. Of course those opens fail, so it would then proceed to go searching directories, and since it looks in /usr/lib before /usr/local/lib it picked up base OpenSSL and thus couldn't resolve numerous symbols. Even if the path in that patch were correct, the patch would still be incorrect, because LOCALBASE is not always /usr/local, and the correct SSL libs are not necessarily from ports (one could build against base OpenSSL). This has been corrected by using OPENSSLLIB which is the full path to the libs for the version of Open/LibreSSL we are compiling against, be it from ports or base.

I have been using this patch for a couple weeks with LibreSSL 2.5.1 now and the improvement is remarkable; almost all the SSL validation errors I'd been seeing have disappeared as have the "couldn't resolve symbol" messages that had been littering my logs.
Comment 2 Matthew Rezny freebsd_committer 2017-02-18 16:39:05 UTC
Created attachment 180114 [details]
fix qt4-network SSL patch

After fixing up the situation in Qt5, I went back to re-check the situation in qt4-network because I had my doubts about the correctness of the patches there.

The situation with dynamic loading was ok in that the path was correct to find SSL libs from ports, but as explained that is not totally correct so I made the same change to use OPENSSLLIB for the path.

The previous change to remove SSLv3 support was improved so it depends on the availablility, allowing one to build a version with SSLv3 support if desired.

More importantly, I undid the redefinition of SecurePtotocols and default. That change was forcing the default case and the SecureProtocols case to be TLSv1.0 only. To allow those to support TLSv1+ they must call the SSLv23 methods, not the TLSv1 methods. OpenSSL API is strange; TLSv1 methods are TLSv1.0 only, full TLSv1.0+ is available through SSLv23 methods. I adjusted the masking for SecureProtocols to disable SSLv3 as well as SSLv2 so that SecureProtocols means TLSv1+ instead of TLSv1.0 (prior patch) or SSLv3+ (original). Default remains SSLv3+ for compatibility reasons, but of course that depends on the SSL library implementing SSLv3. Given that none of base OpenSSL, port OpenSSL, and libSSL make SSLv3 available by default, there is no risk in inadvertent use of SSLv3; the user would have to build OpenSSL from ports with SSLv3 explicitly enabled, in which case SSLv3 should work for them.

Also, convert USE_OPENSSL to USES=ssl while here.
Comment 3 Matthew Rezny freebsd_committer 2017-02-19 08:14:39 UTC
Created attachment 180133 [details]
fix qt5-network with libressl

Update the Qt5 patch since 5.7.1 just landed in ports.
Comment 4 commit-hook freebsd_committer 2017-02-22 19:30:11 UTC
A commit references this bug:

Author: rezny
Date: Wed Feb 22 19:29:57 UTC 2017
New revision: 434633
URL: https://svnweb.freebsd.org/changeset/ports/434633

Log:
  Fix build with libressl-devel by implementing portable curve control.
  Correct the path used for loading SSL libraries at runtime.

  PR:		216781
  Approved by:	swills (mentor)
  Differential Revision:	https://reviews.freebsd.org/D9726

Changes:
  head/net/qt5-network/Makefile
  head/net/qt5-network/files/patch-src_network_ssl_qsslcontext__openssl.cpp
  head/net/qt5-network/files/patch-src_network_ssl_qsslsocket__openssl__symbols.cpp
  head/net/qt5-network/files/patch-src_network_ssl_qsslsocket__openssl__symbols__p.h
Comment 5 commit-hook freebsd_committer 2017-02-22 19:39:21 UTC
A commit references this bug:

Author: rezny
Date: Wed Feb 22 19:38:32 UTC 2017
New revision: 434634
URL: https://svnweb.freebsd.org/changeset/ports/434634

Log:
  Adjust SSL patches to match behavior of Qt5 in regards to SSL/TLS versions.

  PR:		216781
  Approved by:	swills (mentor)
  Differential Revision:	https://reviews.freebsd.org/D9727

Changes:
  head/net/qt4-network/files/patch-src_network_ssl_qsslsocket__openssl.cpp
  head/net/qt4-network/files/patch-src_network_ssl_qsslsocket__openssl__symbols.cpp
Comment 6 commit-hook freebsd_committer 2017-02-22 19:51:32 UTC
A commit references this bug:

Author: rezny
Date: Wed Feb 22 19:51:08 UTC 2017
New revision: 434635
URL: https://svnweb.freebsd.org/changeset/ports/434635

Log:
  Replace USE_OPENSSL with USES=ssl and bump portrevision for SSL changes.
  This should have been part of the prior commit, oops.

  PR:		216781
  Approved by:	swills (mentor)
  Differential Revision:	https://reviews.freebsd.org/D9727

Changes:
  head/net/qt4-network/Makefile