net: Drop support of the insecure miniUPnPc versions #15993

hebasto · 2019-05-09T16:49:18Z

Minimum supported miniUPnPc API version is set to 10:

Refs:

The hardcoded "Bitcoin" replaced with PACKAGE_NAME:
Also style-only commit applied.

Pardon: could not reopen my previous PR #15966.

DrahtBot · 2019-05-09T17:52:09Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

laanwj · 2019-05-09T19:35:33Z

Concept ACK, we definitely don't want to support miniUPnPc versions that are insecure

.travis.yml

maflcko · 2019-05-10T16:58:27Z

src/net.cpp

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

I agree we should ideally drop support for all vulnerable versions.

hebasto · 2019-05-11T01:05:46Z

@MarcoFalke
Thank you for your review. Your comments have been addressed.
PR title and description are updated.

practicalswift · 2019-05-12T19:44:11Z

Concept ACK

We should now allow dependencies with known flaws.

maflcko · 2019-05-13T16:36:50Z

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

Show signature and timestamp

Signature:

Timestamp of file with hash e55601d854a120567f2935cc7fb439ec8290e1fb985aa3a871fd0ce1c62777ba -

practicalswift · 2019-05-13T16:50:26Z

utACK fe94f276faa6f5905132d9062469432347bce747

gmaxwell · 2019-05-14T22:32:28Z

If this requires >= 16 it eliminates miniupnp versions that debian is still using (but with fixes), no?

laanwj · 2019-05-16T14:21:27Z

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

gmaxwell · 2019-05-17T01:00:08Z

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.

hebasto · 2019-05-17T18:25:42Z

This PR is becoming a bit controversial:

#15993 (comment), #15993 (comment) by @MarcoFalke:

Why 10?
Everything below (and including 14) is broken: #6789
MINIUPNPC_API_VERSION >= 16 should 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?

maflcko · 2019-05-17T18:42:18Z

Yeah, isn't this superseded by Changes to support NAT-PMP #15717

laanwj · 2019-05-20T15:42:20Z

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.

hebasto · 2019-05-21T05:12:46Z

@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.

@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.

laanwj · 2019-05-21T09:44:26Z

LGTM now, utACK 12a0de5931fcbbad388e56447e297e38ff54bfcf

hebasto · 2019-05-21T16:16:14Z

Release notes have been added.

maflcko · 2019-05-21T17:21:46Z

doc/release-notes-15993.md

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:

CVE-2017-8798

CVE-2017-1000494

On Debian:

in jessy miniupnpc package is still vulnerable for CVE-2017-8798 and CVE-2017-1000494

in stretch miniupnpc package is still vulnerable for CVE-2017-1000494

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.)

hebasto · 2019-06-06T20:43:50Z

@luke-jr @laanwj

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.)

Are you going to do that here @hebasto ?

Done.

luke-jr · 2019-06-07T03:09:15Z

configure.ac

I don't recommend referring to GitHub issues. I think we're supposed to have a self-contained [set of] git repo(s)?

luke-jr · 2019-06-07T03:11:19Z

configure.ac

--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).

doc/release-notes-15993.md

The minimum supported miniUPnPc API version is set to 10.

hebasto · 2019-06-07T07:55:46Z

@luke-jr
Thank you for your review. All your comments have been addressed.

luke-jr

utACK, with a few nits

luke-jr · 2019-06-07T20:33:04Z

configure.ac

Some API v10 is also vulnerable, right? I think we just want to say "unsupported" here.

luke-jr · 2019-06-07T20:34:42Z

src/net.cpp

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

luke-jr · 2019-06-07T20:36:57Z

src/net.cpp

I agree we should ideally drop support for all vulnerable versions.

laanwj · 2019-06-13T11:14:31Z

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.

hebasto · 2019-06-13T20:55:39Z

"vulnerable" replaced with "unsupported" as @luke-jr suggested.

ryanofsky

utACK 59cb722. 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)

59cb722 Update configure to reject unsafe miniUPnPc API ver (Hennadii Stepanov) ab21905 doc: Add release notes for 15993 (Hennadii Stepanov) 02709e9 Align formatting with clang-format (Hennadii Stepanov) 91a1b85 Use PACKAGE_NAME in UPnP description (Hennadii Stepanov) 9f76e45 Drop support of insecure miniUPnPc versions (Hennadii Stepanov) Pull request description: 1. Minimum supported miniUPnPc API version is set to 10: - https://packages.ubuntu.com/xenial/libminiupnpc-dev - https://packages.debian.org/jessie/libminiupnpc-dev Refs: - #6583 - #6789 - #10414 2. The hardcoded "Bitcoin" replaced with `PACKAGE_NAME`: ![Screenshot from 2019-05-06 23-10-29](https://user-images.githubusercontent.com/32963518/57253178-afc60780-7056-11e9-83c9-e85670c58c1e.png) 3. Also style-only commit applied. Pardon: could not reopen my previous PR #15966. ACKs for top commit: ryanofsky: utACK 59cb722. 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) Tree-SHA512: 42ed11bc2fb2ec83d5dd58e2383da5444a24fd572707f6cf10b622cb8943e28adfcca4750d06801024c4472625b5ea9279516fbd9d2ccebc9bbaafe1d148e80d

…ions 59cb722 Update configure to reject unsafe miniUPnPc API ver (Hennadii Stepanov) ab21905 doc: Add release notes for 15993 (Hennadii Stepanov) 02709e9 Align formatting with clang-format (Hennadii Stepanov) 91a1b85 Use PACKAGE_NAME in UPnP description (Hennadii Stepanov) 9f76e45 Drop support of insecure miniUPnPc versions (Hennadii Stepanov) Pull request description: 1. Minimum supported miniUPnPc API version is set to 10: - https://packages.ubuntu.com/xenial/libminiupnpc-dev - https://packages.debian.org/jessie/libminiupnpc-dev Refs: - bitcoin#6583 - bitcoin#6789 - bitcoin#10414 2. The hardcoded "Bitcoin" replaced with `PACKAGE_NAME`: ![Screenshot from 2019-05-06 23-10-29](https://user-images.githubusercontent.com/32963518/57253178-afc60780-7056-11e9-83c9-e85670c58c1e.png) 3. Also style-only commit applied. Pardon: could not reopen my previous PR bitcoin#15966. ACKs for top commit: ryanofsky: utACK 59cb722. 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) Tree-SHA512: 42ed11bc2fb2ec83d5dd58e2383da5444a24fd572707f6cf10b622cb8943e28adfcca4750d06801024c4472625b5ea9279516fbd9d2ccebc9bbaafe1d148e80d

Summary: The minimum supported miniUPnPc API version is set to 10 (debian 8 and ubuntu 16.04) This is a backport of Core [[bitcoin/bitcoin#15993 | PR15993]] Included commits: - [[bitcoin/bitcoin@9f76e45 | 9f76e45 Drop support of insecure miniUPnPc versions]] - [[bitcoin/bitcoin@91a1b85 | 91a1b85 Use PACKAGE_NAME in UPnP description]] - [[bitcoin/bitcoin@ab21905 | ab21905 Add release notes]] Test Plan: `ninja all check-all` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8211

60cd984 doc: Add release notes about minimum required miniupnpc API version (Fuzzbawls) d9a2024 CMake: Require miniupnpc API version 10 or greater (Fuzzbawls) d0ed7d2 Update configure to reject unsafe miniUPnPc API ver (Hennadii Stepanov) bd0ed0c Align formatting with clang-format (Fuzzbawls) f43781f Use PACKAGE_NAME in UPnP description (Fuzzbawls) f2e1e09 Drop support of insecure miniUPnPc versions (Fuzzbawls) Pull request description: Backport of bitcoin#15993 This results in a rejection of older versions of miniupnpc for both autotools and CMake build systems. API Version 10 was used to maintain compatibility with Ubuntu Xenial (which uses a patched version of miniupnpc but still reports API version 10) and Debian 8 (which is not fully patched). Miniupnp support as a whole remains disabled by default ACKs for top commit: furszy: utACK 60cd984 random-zebra: utACK 60cd984 random-zebra: utACK 60cd984 Tree-SHA512: b3294c3474797226b5d154cc961efc1209fde58c6af70a1a292fcd3bc18de712ab3bcc6e1de6f6ba67ccf5ca9c8db90a116a4c43fbfd12b42d622b17523585df

Minimum supported miniUPnPc API version is set to 10. This prevents numerous security issues by enforcing a 2015 UPnP version (keeps compatibility with Ubuntu 16.04 LTS and Debian 8) Ref: bitcoin/bitcoin#15993

…ions 59cb722 Update configure to reject unsafe miniUPnPc API ver (Hennadii Stepanov) ab21905 doc: Add release notes for 15993 (Hennadii Stepanov) 02709e9 Align formatting with clang-format (Hennadii Stepanov) 91a1b85 Use PACKAGE_NAME in UPnP description (Hennadii Stepanov) 9f76e45 Drop support of insecure miniUPnPc versions (Hennadii Stepanov) Pull request description: 1. Minimum supported miniUPnPc API version is set to 10: - https://packages.ubuntu.com/xenial/libminiupnpc-dev - https://packages.debian.org/jessie/libminiupnpc-dev Refs: - dashpay#6583 - dashpay#6789 - bitcoin#10414 2. The hardcoded "Bitcoin" replaced with `PACKAGE_NAME`: ![Screenshot from 2019-05-06 23-10-29](https://user-images.githubusercontent.com/32963518/57253178-afc60780-7056-11e9-83c9-e85670c58c1e.png) 3. Also style-only commit applied. Pardon: could not reopen my previous PR bitcoin#15966. ACKs for top commit: ryanofsky: utACK 59cb722. 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) Tree-SHA512: 42ed11bc2fb2ec83d5dd58e2383da5444a24fd572707f6cf10b622cb8943e28adfcca4750d06801024c4472625b5ea9279516fbd9d2ccebc9bbaafe1d148e80d

DrahtBot added P2P Tests labels May 9, 2019

maflcko reviewed May 9, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

hebasto force-pushed the 20190506-drop-ancient-miniupnpc-api branch from 67b1263 to f8a381e Compare May 10, 2019 14:36

maflcko removed the Tests label May 10, 2019

maflcko reviewed May 10, 2019

View reviewed changes

hebasto force-pushed the 20190506-drop-ancient-miniupnpc-api branch from f8a381e to fe94f27 Compare May 11, 2019 00:34

hebasto changed the title ~~net: Drop the ancient miniUPnPc API version support~~ net: Drop support of the insecure miniUPnPc versions May 11, 2019

fanquake added the Needs release note label May 17, 2019

hebasto force-pushed the 20190506-drop-ancient-miniupnpc-api branch from fe94f27 to 12a0de5 Compare May 21, 2019 05:01

maflcko reviewed May 21, 2019

View reviewed changes

luke-jr suggested changes May 21, 2019

View reviewed changes

hebasto force-pushed the 20190506-drop-ancient-miniupnpc-api branch 3 times, most recently from 71919be to ef43e37 Compare May 22, 2019 16:27

hebasto force-pushed the 20190506-drop-ancient-miniupnpc-api branch from ef43e37 to 3942fca Compare June 6, 2019 20:40

hebasto force-pushed the 20190506-drop-ancient-miniupnpc-api branch from 3942fca to 5993b87 Compare June 6, 2019 20:58

luke-jr reviewed Jun 7, 2019

View reviewed changes

luke-jr suggested changes Jun 7, 2019

View reviewed changes

hebasto added 4 commits June 7, 2019 09:37

Drop support of insecure miniUPnPc versions

9f76e45

The minimum supported miniUPnPc API version is set to 10.

Use PACKAGE_NAME in UPnP description

91a1b85

Align formatting with clang-format

02709e9

doc: Add release notes for 15993

ab21905

hebasto force-pushed the 20190506-drop-ancient-miniupnpc-api branch from 5993b87 to ded2de5 Compare June 7, 2019 07:51

luke-jr approved these changes Jun 7, 2019

View reviewed changes

Update configure to reject unsafe miniUPnPc API ver

59cb722

Also fixes behavior when libminiupnpc is not installed.

hebasto force-pushed the 20190506-drop-ancient-miniupnpc-api branch from ded2de5 to 59cb722 Compare June 13, 2019 20:53

ryanofsky approved these changes Jun 27, 2019

View reviewed changes

laanwj merged commit 59cb722 into bitcoin:master Jul 29, 2019

hebasto deleted the 20190506-drop-ancient-miniupnpc-api branch July 31, 2019 14:41

Fuzzbawls mentioned this pull request Apr 22, 2021

[Net] Drop support of the insecure miniUPnPc versions PIVX-Project/PIVX#2330

Merged

barton2526 mentioned this pull request Jun 12, 2021

net: Drop support of the insecure miniUPnPc versions gridcoin-community/Gridcoin-Research#2178

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

net: Drop support of the insecure miniUPnPc versions #15993

net: Drop support of the insecure miniUPnPc versions #15993

Uh oh!

Conversation

hebasto commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented May 9, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented May 11, 2019

Uh oh!

practicalswift commented May 12, 2019

Uh oh!

maflcko commented May 13, 2019

Uh oh!

practicalswift commented May 13, 2019

Uh oh!

gmaxwell commented May 14, 2019

Uh oh!

laanwj commented May 16, 2019

Uh oh!

gmaxwell commented May 17, 2019

Uh oh!

hebasto commented May 17, 2019

Uh oh!

maflcko commented May 17, 2019

Uh oh!

laanwj commented May 20, 2019

Uh oh!

hebasto commented May 21, 2019

Uh oh!

laanwj commented May 21, 2019

Uh oh!

hebasto commented May 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Jun 7, 2019

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hebasto commented May 9, 2019 •

edited

Loading

DrahtBot commented May 9, 2019 •

edited

Loading

hebasto Jun 14, 2019 •

edited

Loading