net: Drop support of the insecure miniUPnPc versions #15993

pull hebasto wants to merge 5 commits into bitcoin:master from hebasto:20190506-drop-ancient-miniupnpc-api changing 4 files +43 −33
  1. hebasto commented at 4:49 PM on May 9, 2019: member
    1. Minimum supported miniUPnPc API version is set to 10:

    Refs:

    1. The hardcoded "Bitcoin" replaced with PACKAGE_NAME: Screenshot from 2019-05-06 23-10-29

    2. Also style-only commit applied.

    Pardon: could not reopen my previous PR #15966.

  2. DrahtBot added the label P2P on May 9, 2019
  3. DrahtBot added the label Tests on May 9, 2019
  4. DrahtBot commented at 5:52 PM on May 9, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  5. laanwj commented at 7:35 PM on May 9, 2019: member

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

  6. in .travis.yml:143 in 67b12635f1 outdated
     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"
    


    MarcoFalke commented at 7:42 PM on May 9, 2019:

    Depends is off, so is this needed?


    hebasto commented at 2:37 PM on May 10, 2019:

    @MarcoFalke Fixed. Using --with-miniupnpc=no to make things explicit.

  7. hebasto force-pushed on May 10, 2019
  8. MarcoFalke removed the label Tests on May 10, 2019
  9. in src/net.cpp:41 in f8a381e40b outdated
      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");
    


    MarcoFalke commented at 4:58 PM on May 10, 2019:

    Why 10?

    Everything below (and including 14) is broken: #6789


    hebasto commented at 5:26 PM on May 10, 2019:

    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.


    MarcoFalke commented at 5:52 PM on May 10, 2019:

    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)


    hebasto commented at 6:57 PM on May 10, 2019:

    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

    MarcoFalke commented at 7:55 PM on May 10, 2019:

    MINIUPNPC_API_VERSION >= 16 should be fine. The code might be removed soon anyway


    ryanofsky commented at 12:16 PM on June 6, 2019:

    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.


    hebasto commented at 8:42 PM on June 6, 2019:

    Done.


    luke-jr commented at 8:36 PM on June 7, 2019:

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

  10. hebasto force-pushed on May 11, 2019
  11. hebasto renamed this:
    net: Drop the ancient miniUPnPc API version support
    net: Drop support of the insecure miniUPnPc versions
    on May 11, 2019
  12. hebasto commented at 1:05 AM on May 11, 2019: member

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

  13. practicalswift commented at 7:44 PM on May 12, 2019: contributor

    Concept ACK

    We should now allow dependencies with known flaws.

  14. MarcoFalke commented at 4:36 PM on May 13, 2019: member

    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>

  15. practicalswift commented at 4:50 PM on May 13, 2019: contributor

    utACK fe94f276faa6f5905132d9062469432347bce747

  16. gmaxwell commented at 10:32 PM on May 14, 2019: contributor

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

  17. laanwj commented at 2:21 PM on May 16, 2019: member

    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
    
  18. gmaxwell commented at 1:00 AM on May 17, 2019: contributor

    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.

  19. fanquake added the label Needs release note on May 17, 2019
  20. hebasto commented at 6:25 PM on May 17, 2019: member

    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 >= 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?

  21. MarcoFalke commented at 6:42 PM on May 17, 2019: member

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

  22. laanwj commented at 3:42 PM on May 20, 2019: member

    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.

  23. hebasto force-pushed on May 21, 2019
  24. hebasto commented at 5:12 AM on May 21, 2019: member

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

  25. laanwj commented at 9:44 AM on May 21, 2019: member

    LGTM now, utACK 12a0de5931fcbbad388e56447e297e38ff54bfcf

  26. hebasto commented at 4:16 PM on May 21, 2019: member

    Release notes have been added.

  27. in doc/release-notes-15993.md:3 in 060acc2357 outdated
       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.
    


    MarcoFalke commented at 5:21 PM on May 21, 2019:

    Aren't they patched? Otherwise we would set the API version higher, no?


    hebasto commented at 4:29 PM on May 22, 2019:

    Aren't they patched?

    The libminiupnpc10 package has been patched on Ubuntu only:

    On Debian:

  28. luke-jr changes_requested
  29. luke-jr commented at 9:32 PM on May 21, 2019: member

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

  30. hebasto force-pushed on May 22, 2019
  31. hebasto force-pushed on May 22, 2019
  32. hebasto force-pushed on May 22, 2019
  33. hebasto commented at 4:31 PM on May 22, 2019: member

    Release notes have been updated; see: #15993 (review).

  34. gmaxwell commented at 1:59 AM on May 24, 2019: contributor

    Looking good to me.

  35. laanwj removed the label Needs release note on Jun 5, 2019
  36. laanwj commented at 2:23 PM on June 5, 2019: member

    Removed 'needs release note' label as it includes a release note.

  37. laanwj commented at 10:40 AM on June 6, 2019: member

    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 ?

  38. ryanofsky approved
  39. ryanofsky commented at 12:25 PM on June 6, 2019: member

    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:

    https://github.com/bitcoin/bitcoin/blob/36fb968825a19aa1b01661228ba53209ec7a033e/configure.ac#L1153-L1157

    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.

  40. luke-jr commented at 6:33 PM on June 6, 2019: member

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

  41. hebasto force-pushed on Jun 6, 2019
  42. hebasto commented at 8:43 PM on June 6, 2019: member

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

  43. hebasto force-pushed on Jun 6, 2019
  44. in configure.ac:932 in 5993b87602 outdated
     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.
    


    luke-jr commented at 3:09 AM on June 7, 2019:

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

  45. in configure.ac:946 in 5993b87602 outdated
     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.])
    


    luke-jr commented at 3:11 AM on June 7, 2019:

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

  46. in doc/release-notes-15993.md:3 in 5993b87602 outdated
       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).
    


    luke-jr commented at 3:12 AM on June 7, 2019:

    jessie*

  47. luke-jr changes_requested
  48. Drop support of insecure miniUPnPc versions
    The minimum supported miniUPnPc API version is set to 10.
    9f76e45b9d
  49. Use PACKAGE_NAME in UPnP description 91a1b85083
  50. Align formatting with clang-format 02709e9560
  51. doc: Add release notes for 15993 ab2190557e
  52. hebasto force-pushed on Jun 7, 2019
  53. hebasto commented at 7:55 AM on June 7, 2019: member

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

  54. in configure.ac:947 in ded2de5d6e outdated
     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.])
    


    luke-jr commented at 8:33 PM on June 7, 2019:

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


    hebasto commented at 8:55 PM on June 13, 2019:

    Done.

  55. in src/net.cpp:1410 in ded2de5d6e outdated
    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 */
    


    luke-jr commented at 8:34 PM on June 7, 2019:

    Why remove the version comments?


    hebasto commented at 8:59 PM on June 13, 2019:

    They are irrelevant now. See: miniupnpc/apiversions.txt


    luke-jr commented at 2:52 AM on June 14, 2019:

    I don't see how they are irrelevant...


    hebasto commented at 5:49 AM on June 14, 2019:

    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
    
  56. luke-jr approved
  57. luke-jr commented at 8:37 PM on June 7, 2019: member

    utACK, with a few nits

  58. laanwj commented at 11:14 AM on June 13, 2019: member

    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.

  59. Update configure to reject unsafe miniUPnPc API ver
    Also fixes behavior when libminiupnpc is not installed.
    59cb722fd0
  60. hebasto force-pushed on Jun 13, 2019
  61. hebasto commented at 8:55 PM on June 13, 2019: member

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

  62. ryanofsky approved
  63. ryanofsky commented at 5:58 PM on June 27, 2019: member

    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)

  64. laanwj merged this on Jul 29, 2019
  65. laanwj closed this on Jul 29, 2019

  66. laanwj referenced this in commit b21acab82f on Jul 29, 2019
  67. sidhujag referenced this in commit 1f6ab18969 on Jul 30, 2019
  68. hebasto deleted the branch on Jul 31, 2019
  69. jasonbcox referenced this in commit 5d0575f2a4 on Nov 3, 2020
  70. furszy referenced this in commit 3caa69e73c on May 8, 2021
  71. barton2526 referenced this in commit 3dec942c6c on Jun 12, 2021
  72. PastaPastaPasta referenced this in commit d8925fe0f6 on Jun 26, 2021
  73. PastaPastaPasta referenced this in commit 30b1e1da4f on Jun 26, 2021
  74. PastaPastaPasta referenced this in commit 6e054f897b on Jun 26, 2021
  75. DrahtBot locked this on Dec 16, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 18:14 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me