Refs:
The hardcoded "Bitcoin" replaced with PACKAGE_NAME:
Also style-only commit applied.
Pardon: could not reopen my previous PR #15966.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
Concept ACK, we definitely don't want to support miniUPnPc versions that are insecure
138 | @@ -139,7 +139,8 @@ jobs: 139 | env: >- 140 | HOST=x86_64-unknown-linux-gnu 141 | DOCKER_NAME_TAG=ubuntu:14.04 142 | - PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools libicu-dev libpng-dev libssl-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-test-dev libboost-thread-dev libdb5.1++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev" 143 | + PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools libicu-dev libpng-dev libssl-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-test-dev libboost-thread-dev libdb5.1++-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev" 144 | + DEP_OPTS="NO_UPNP=1"
Depends is off, so is this needed?
@MarcoFalke
Fixed. Using --with-miniupnpc=no to make things explicit.
35 | @@ -36,6 +36,7 @@ 36 | #include <miniupnpc/miniwget.h> 37 | #include <miniupnpc/upnpcommands.h> 38 | #include <miniupnpc/upnperrors.h> 39 | +static_assert(MINIUPNPC_API_VERSION >= 10, "miniUPnPc API version >= 10 assumed");
Why 10?
Everything below (and including 14) is broken: #6789
Why 10?
This is API version for xenial and jessy libminiupnpc-dev.
Everything below (and including 14) is broken: #6789
You are right. This PR is a kind of trade-off. Otherwise, a user should be forced to build libminiupnpc-dev by himself.
Anyway, I'll appreciate any smart solution for this security issue.
I don't think we should support building against known broken versions. If someone really needs to build against such an old version, they could update the code themselves (remove the assert) or build a newer version of miniupnpc (which conveniently is shipped in our ./depends)
I cannot find a suitable assertion to let only build with CVE-2017-8798 fix commit (see: #10414):
MINIUPNPC_API_VERSION >= 17 is too narrow (see: miniupnpc-2.1 changelog)MINIUPNPC_API_VERSION >= 16 is too wide: versions before miniupnpc-2.0.20170509 are included
MINIUPNPC_API_VERSION >= 16 should be fine. The code might be removed soon anyway
There should be a code comment explaining this version requirement so we can remember the reasoning behind it in the future. You could copy and paste the text from the release-notes here.
Done.
I agree we should ideally drop support for all vulnerable versions.
@MarcoFalke Thank you for your review. Your comments have been addressed. PR title and description are updated.
Concept ACK
We should now allow dependencies with known flaws.
utACK fe94f276faa6f5905132d9062469432347bce747
Only checked that this is removing code and adding the static assert Didn't compile to see if the PACKAGE_NAME makes it through
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
utACK fe94f276faa6f5905132d9062469432347bce747
Only checked that this is removing code and adding the static assert
Didn't compile to see if the PACKAGE_NAME makes it through
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg4oQv/e3VkOJrARmameKhDCpGtwE/QvZrIGYlSFEqOm0LNHLgMfLg+Siy3hCy4
Vbw/RX/scVghRtDk/oFMQvQaNHFTYe+6cWL95yBT9LvIrfhga/NKxk2ydEXO1BT2
7VgmK7XMMY16BbMiYUk6XrgDe9xepXP/PZsg0qquhnwA0bANOjwROPVi6bEfHgkF
kTPM+c0VEHXp9iPdQ8/aIQfMjktQGT1p3Yy+Q93oQzh/TPukBgVCJVLYssVzUCQL
q5aqCYX+L0dazMKk+CV4564vNOQ5wt2fna3TLogp/VYYdS3C3TVuj+AuZYmTiXgS
wd9KOLEyIPK4Yx+Iv3JnBU1jNs0i420iV5IO2EBIMLbFv7Uy2fp9BAubLoZRUeTX
FK9k4kbkEVdgCw0MGRkQulQTG746tZCnCgGKtaZLrj+tNR9BTTEH/x1qMmOZs060
CH9X5sJP4fE5ZYXCgd3DHzK3KvvFqdxlVbdbFUWyW1vki/Kgr90AWOijH3moSUMc
PgWMPjzU
=dAQ4
-----END PGP SIGNATURE-----
Timestamp of file with hash e55601d854a120567f2935cc7fb439ec8290e1fb985aa3a871fd0ce1c62777ba -
</details>
utACK fe94f276faa6f5905132d9062469432347bce747
If this requires >= 16 it eliminates miniupnp versions that debian is still using (but with fixes), no?
If this requires >= 16 it eliminates miniupnp versions that debian is still using (but with fixes), no?
yes
stretch (stable) (net): UPnP IGD client lightweight library client
1.9.20140610-4: amd64 arm64 armel armhf i386 mips mips64el mipsel ppc64el s390x
which defines:
./miniupnpc.h:#define MINIUPNPC_API_VERSION 10
This should be changed to 10 then, the (bulk of the?) API changes are compatible with 10-- and there are patched systems that return 10. I don't think we have any strong reason to gratuitously break compatibility beyond that.
This PR is becoming a bit controversial:
#15993 (review), #15993 (review) by @MarcoFalke:
Why 10? Everything below (and including 14) is broken: #6789
MINIUPNPC_API_VERSION >= 16should be fine. The code might be removed soon anyway
#15993 (comment) by @gmaxwell:
This should be changed to 10 then, the (bulk of the?) API changes are compatible with 10-- and there are patched systems that return 10. I don't think we have any strong reason to gratuitously break compatibility beyond that.
Can we reach the consensus? Or should this PR be closed for now?
Yeah, isn't this superseded by Changes to support NAT-PMP #15717
Can we reach the consensus? Or should this PR be closed for now?
Just change it to 10 and it's OK imo. Dropping support for other versions can be done later but only when it's gone from debian stable.
This should be changed to 10 then, the (bulk of the?) API changes are compatible with 10-- and there are patched systems that return 10. I don't think we have any strong reason to gratuitously break compatibility beyond that. @laanwj Just change it to 10 and it's OK imo. Dropping support for other versions can be done later but only when it's gone from debian stable. @gmaxwell, @laanwj Thank you for your reviews. Minimum API reverted to 10. The PR description has been reverted to the initial state as well.
LGTM now, utACK 12a0de5931fcbbad388e56447e297e38ff54bfcf
Release notes have been added.
0 | @@ -0,0 +1,3 @@ 1 | +Build system changes 2 | +-------------------- 3 | +The minimum supported miniUPnPc API version is set to 10. This keeps compatibility with Ubuntu 16.04 and Debian 8 `libminiupnpc-dev` packages. Nevertheless, these packages are still vulnerable to [CVE-2017-8798](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-8798). It is fixed since `miniupnpc-2.0.20170509` used in depends.
Aren't they patched? Otherwise we would set the API version higher, no?
Aren't they patched?
The libminiupnpc10 package has been patched on Ubuntu only:
On Debian:
miniupnpc package is still vulnerable for CVE-2017-8798 and CVE-2017-1000494miniupnpc package is still vulnerable for CVE-2017-1000494
Concept ACK, but this needs to update configure to reject incompatible versions. (Ideally rejecting vulnerable versions even if they are API-compatible with the code.)
Release notes have been updated; see: #15993 (review).
Looking good to me.
Removed 'needs release note' label as it includes a release note.
utACK ef43e373e3abdd02e997c66dde63cb54d45fb31f
re: #15993#pullrequestreview-240311400 from luke-jr
Concept ACK, but this needs to update configure to reject incompatible versions. (Ideally rejecting vulnerable versions even if they are API-compatible with the code.)
It seems like the PR already does reject old versions with the static_assert. I think it would probably be an improvement to add an autoconf check similar to the zmq check:
This way, if an user has an incompatible pnp library installed, by default the build system would just build without pnp support instead of failing to compile. But I think this would just be a minor usability improvement to the build system and wouldn't need to block the PR.
Builds should never fail after configure succeeds. That indicates a bug in our dependency checks. Especially when the feature can be turned off, I don't think fixing this is merely a "minor usability improvement".
927 | @@ -928,6 +928,25 @@ if test x$use_upnp != xno; then 928 | [AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS=-lminiupnpc], [have_miniupnpc=no])], 929 | [have_miniupnpc=no] 930 | ) 931 | +dnl The minimum supported miniUPnPc API version is set to 10. This keeps compatibility 932 | +dnl with Ubuntu 16.04 LTS and Debian 8 libminiupnpc-dev packages. See details in #15993.
I don't recommend referring to GitHub issues. I think we're supposed to have a self-contained [set of] git repo(s)?
941 | + # error miniUPnPc API version is too old 942 | + #endif 943 | + ]])],[ 944 | + AC_MSG_RESULT(yes) 945 | + ],[ 946 | + AC_MSG_ERROR([miniUPnPc API versions < 10 are vulnerable. Use --without-miniupnpc.])
--with-miniupnpc=auto (ie, the default) won't work with this check.
Just set have_miniupnpc=no here instead (and change this to AC_MSG_WARNING).
0 | @@ -0,0 +1,3 @@ 1 | +Build system changes 2 | +-------------------- 3 | +The minimum supported miniUPnPc API version is set to 10. This keeps compatibility with Ubuntu 16.04 LTS and Debian 8 `libminiupnpc-dev` packages. Please note, on Debian this package is still vulnerable to [CVE-2017-8798](https://security-tracker.debian.org/tracker/CVE-2017-8798) (in jessy only) and [CVE-2017-1000494](https://security-tracker.debian.org/tracker/CVE-2017-1000494) (both in jessy and in stretch).
jessie*
The minimum supported miniUPnPc API version is set to 10.
942 | + #endif 943 | + ]])],[ 944 | + AC_MSG_RESULT(yes) 945 | + ],[ 946 | + AC_MSG_RESULT(no) 947 | + AC_MSG_WARN([miniUPnPc API version < 10 is vulnerable, disabling UPnP support.])
Some API v10 is also vulnerable, right? I think we just want to say "unsupported" here.
Done.
1405 | @@ -1403,16 +1406,10 @@ static void ThreadMapPort() 1406 | struct UPNPDev * devlist = nullptr; 1407 | char lanaddr[64]; 1408 | 1409 | -#ifndef UPNPDISCOVER_SUCCESS 1410 | - /* miniupnpc 1.5 */ 1411 | - devlist = upnpDiscover(2000, multicastif, minissdpdpath, 0); 1412 | -#elif MINIUPNPC_API_VERSION < 14 1413 | - /* miniupnpc 1.6 */
Why remove the version comments?
They are irrelevant now. See: miniupnpc/apiversions.txt
I don't see how they are irrelevant...
Prior to version 1.7, there was no MINIUPNPC API_VERSION macro. Therefore, different ways were used to distinguish between different versions. Comments were needed to make those "hacks" obvious.
Nowadays, the difference in the upnpDiscover() function signature is clear stated in different API versions.
Particularly,
API version 14
miniupnpc.h
add ttl argument to upnpDiscover() upnpDiscoverAll() upnpDiscoverDevice()
...
updated macro :
#define MINIUPNPC_API_VERSION 14
utACK, with a few nits
I think this is dragging on way too long, and maybe getting out of hand, getting round after round of nits for essentially removing a few #ifdefed lines that were probably never getting compiled in the first place.
Please, let's try to move forward and merge this to have this off our radar.
Also fixes behavior when libminiupnpc is not installed.
utACK 59cb722fd050393a69f1e0df97d857c893d19d80. Changes since last review: adding a new commit which updates configure script to fall back to disabling upnp if version is too old, adding a requested comment explaining static_assert condition, and fixing a spelling (jessy/jessie)