build: disable BIP70 support by default #15584

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:disable-bip70-by-default changing 10 files +24 −12
  1. fanquake commented at 3:01 am on March 12, 2019: member

    Disable BIP70 support in the GUI by default for 0.19.0 (for eventual removal in 0.20.0?).

    Users who want to compile with BIP70 support enabled can pass --enable-bip70 to ./configure.

    I’ve inverted the current --disable-bip70 test to instead pass --enable-bip70.

    Tested configurations on macOS (protobuf installed with brew). Protobuf available and ./configure:

    0Options used to compile and link:
1  with wallet   = yes
2  with gui / qt = yes
3    with bip70  = no

    Protobuf available and ./configure --enable-bip70:

    0Options used to compile and link:
1  with wallet   = yes
2  with gui / qt = yes
3    with bip70  = yes

    Protobuf not available (i.e brew unlink protobuf) and ./configure:

    0Options used to compile and link:
1  with wallet   = yes
2  with gui / qt = yes
3    with bip70  = no

    Protobuf not available and ./configure --enable-bip70:

    0checking whether to build test_bitcoin-qt... yes
1checking whether to build BIP70 support... configure: error: protobuf missing

    TODO:

    • Remove protobuf from other Travis builds
    • Documentation updates (mention that protobuf is now optional)?
    • Could split release notes into GUI and build
  2. fanquake added the label Build system on Mar 12, 2019
  3. fanquake added the label Needs gitian build on Mar 12, 2019
  4. fanquake added this to the milestone 0.19.0 on Mar 12, 2019
  5. fanquake force-pushed on Mar 12, 2019
  6. DrahtBot removed the label Needs gitian build on Mar 13, 2019
  7. jonasschnelli commented at 6:19 am on March 15, 2019: contributor

    Concept ACK

    • IMO travis (not all jobs though) should build with BIP70 as long as the feature is available.
    • Could we remove protobuf from the depends-build in the same pull?
  8. Sjors commented at 8:27 am on March 15, 2019: member
    So is this removing BIP70 from the release as well? I’m reluctant to turn it off for developers by default while still shipping it. At minimum Travis should continue to check it in that case. But even then, GUI test coverage is too low to rely on that.
  9. jonasschnelli commented at 9:24 am on March 15, 2019: contributor

    So is this removing BIP70 from the release as well?

    Not for the release but for the gitian built binaries. We could re-add the BIP70 support to the gitian build. But as this is, it will disable BIP70 in gitian.

  10. jameshilliard commented at 4:30 pm on March 23, 2019: contributor

    I would recommend holding off on disabling support for BIP70 for release binaries until all major merchant processors have stopped requiring it. Dropping BIP70 support before the merchant payment processors stop requiring it would incentivize users to switch to less secure wallets that have BIP70 implementations with known unpatched vulnerabilities.

    I would instead recommend making the deprecation warning more aggressive, maybe add to the deprecation warning that it’s recommended users contact/inform the merchant that they are using an deprecated and insecure protocol.

    Unfortunately the largest merchant payment processor that requires BIP70 is still publishing misinformation in regards to the security of BIP70(such as disproven claims that BIP70 improves protection against MITM attacks) so it may be necessary in the deprecation warning to indicate that merchants should not be trusted to provide accurate information about BIP70 and that users should not follow any merchant recommendations to switch wallets.

  11. luke-jr commented at 2:10 am on March 24, 2019: member
    @jameshilliard Do any merchants require BIP70 at this point? (A particular payment processor claims it does, but AFAIK their implementation is broken and already doesn’t work with Core.)
  12. jameshilliard commented at 2:33 am on March 24, 2019: contributor

    Do any merchants require BIP70 at this point? (A particular payment processor claims it does, but AFAIK their implementation is broken and already doesn’t work with Core.)

    Well Core can make payments to that particular processor, the incompatible/non-standard part of their implementation introduces an unfixed security vulnerability but it doesn’t seem to affect the ability to send payments(due to the incompatible/non-standard part being after the payment is broadcast by Core to the network).

    My worry is that prematurely removing BIP70 in Core would result in users switching to vulnerable wallets when they see the error message indicating they can’t make a payment.

    Having a warning message encourage users to complain to the merchant about using a deprecated/insecure protocol on the other hand would help increase their support costs for processing BIP70 transactions which is the main reason they refuse to fix their security vulnerability or support BIP21. The idea is essentially to have the warning make their support costs for BIP21 lower than their BIP70 support costs by increasing merchant/customer complaints and support costs for BIP70.

  13. luke-jr commented at 8:28 pm on April 17, 2019: member
    Hate to say it, but we should probably do a release with something like #15064 before disabling BIP70 by default… :/
  14. DrahtBot commented at 7:39 pm on June 18, 2019: member

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

    Conflicts

    No conflicts as of last run.

  15. fanquake added the label Needs Conceptual Review on Jun 25, 2019
  16. MarcoFalke removed this from the milestone 0.19.0 on Jun 26, 2019
  17. MarcoFalke added this to the milestone 0.20.0 on Jun 26, 2019
  18. MarcoFalke removed this from the milestone 0.20.0 on Jun 26, 2019
  19. fanquake force-pushed on Aug 6, 2019
  20. laanwj commented at 2:29 pm on August 12, 2019: member
    ACK for 0.19.0
  21. TheBlueMatt commented at 2:39 pm on August 12, 2019: member
    Concept ACK
  22. achow101 commented at 4:09 pm on August 12, 2019: member

    ACK c53da7c5f166a95f3ae37f817f8cd7b94812077a

    Tested that a build with the default configure does not download BIP 70 requests.

  23. MarcoFalke deleted a comment on Aug 12, 2019
  24. MarcoFalke removed the label Needs Conceptual Review on Aug 12, 2019
  25. MarcoFalke added the label Needs gitian build on Aug 12, 2019
  26. MarcoFalke added the label Needs Conceptual Review on Aug 12, 2019
  27. fanquake added this to the milestone 0.19.0 on Aug 12, 2019
  28. fanquake removed the label Needs Conceptual Review on Aug 12, 2019
  29. DrahtBot commented at 10:29 am on August 13, 2019: member

    Gitian builds for commit 00dad5e0e1ddd6047bd4ff0a3032aa8950817b5f (master):

    Gitian builds for commit 991b570e11fc3a18bb65267de19628723b248ba5 (master and this pull):

  30. DrahtBot removed the label Needs gitian build on Aug 13, 2019
  31. dongcarl commented at 8:14 pm on August 15, 2019: member
    We probably should also remove native_protobuf and protobuf as default depends packages, with a flag to enable them explicitly.
  32. fanquake commented at 8:04 am on August 16, 2019: member
    @dongcarl sure. I can also add/update documentation about Protobuf being optional.
  33. DrahtBot added the label Needs rebase on Aug 19, 2019
  34. DrahtBot commented at 1:56 pm on August 19, 2019: member
  35. fanquake force-pushed on Aug 20, 2019
  36. fanquake removed the label Needs rebase on Aug 20, 2019
  37. luke-jr commented at 8:39 pm on September 10, 2019: member

    @achow101 Since you have the sole ack here: did you consider the effect of essentially losing wallet metadata?

    Maybe BIP70 support should be split between network/new transactions, and parsing/old transactions, and the latter retained in some form longer until we can migrate the metadata…

  38. achow101 commented at 9:20 pm on September 10, 2019: member

    did you consider the effect of essentially losing wallet metadata?

    What metadata? AFAIK no BIP70 specific things are written to wallet files.

  39. luke-jr commented at 1:32 am on September 11, 2019: member
    That’s incorrect. There is BIP70-specific protobuf-encoded metadata written to wallets for each BIP70 transaction made.
  40. fanquake added this to the "Blockers" column in a project
  41. fanquake referenced this in commit 2324aa1dc4 on Sep 11, 2019
  42. in configure.ac:226 in a3fb456534 outdated 
    223@@ -224,9 +224,9 @@ AC_ARG_ENABLE([zmq],
224   [use_zmq=yes])
225 AC_ARG_ENABLE([bip70],
226   [AS_HELP_STRING([--disable-bip70],
    ryanofsky commented at 8:04 pm on September 11, 2019:
    Should probably change the help string to –enable-bip70 so it is more comprehensible.
  43. ryanofsky approved
  44. ryanofsky commented at 8:06 pm on September 11, 2019: member

    I’m confused by these comments:

    So is this removing BIP70 from the release as well?

    Not for the release but for the gitian built binaries.

    What’s the difference between the “release” and the gitian built binaries? The gitian binaries along with the source tarball (where BIP70 will be disabled by default) are the release right?

    But utACK a3fb4565348f14f19749aba9b74f4e5918aae4c3 assuming this does turn off bip70 by default in all release artifacts.

  45. sidhujag referenced this in commit 5185fb3d53 on Sep 12, 2019
  46. laanwj commented at 10:09 am on September 12, 2019: member

    ACK a3fb4565348f14f19749aba9b74f4e5918aae4c3 (agree on ryanofsky’s comment though) I would really like to merge this for 0.19, with or without #16852.

    ACK e09913f1c47e693b0c6fafef55b9ca78e5f3abc0

  47. build: disable BIP70 support by default 376f4929f8
  48. doc: specify protobuf as optional in build docs e09913f1c4
  49. fanquake force-pushed on Sep 12, 2019
  50. fanquake commented at 10:31 am on September 12, 2019: member

    Should probably change the help string to –enable-bip70 so it is more comprehensible.

    Done & rebased. Also added a commit to specify Protobuf as optional in the build docs.

  51. jameshilliard commented at 11:13 am on September 12, 2019: contributor
    I think we need something like #16858 before merging this so that users don’t switch to BIP70 compatible wallets with insecure BIP70 implementations.
  52. laanwj commented at 11:16 am on September 12, 2019: member
    @jameshilliard I agree that would be nice to have, but I don’t personally think that’s enough reason to make this miss the 0.19 deadline.
  53. jameshilliard commented at 11:19 am on September 12, 2019: contributor
    @laanwj Should we be able to get something like #16858 in by the deadline as well since it should be a trivial change?
  54. elichai commented at 8:13 pm on September 12, 2019: contributor

    ACK e09913f1c47e693b0c6fafef55b9ca78e5f3abc0 Read the autotools changes. awesome that this removes the protobuf requirement.

    One thing i’m not sure I get is the change to the 00_setup_env_i686.sh test, from not using bip70 to using it.

  55. MarcoFalke commented at 2:55 am on September 13, 2019: member

    awesome that this removes the protobuf requirement.

    protobuf is still required when BIP70 is compiled and was never required when BIP70 is not compiled.

    One thing i’m not sure I get is the change to the 00_setup_env_i686.sh test, from not using bip70 to using it.

    While we disable it by default, the option still exists and at least one ci config should build it

  56. practicalswift commented at 12:07 pm on September 13, 2019: contributor

    ACK e09913f1c47e693b0c6fafef55b9ca78e5f3abc0 – diff looks correct

    Smaller attack surface is better!

  57. laanwj referenced this in commit 102998ea03 on Sep 13, 2019
  58. laanwj merged this on Sep 13, 2019
  59. laanwj closed this on Sep 13, 2019
  60. fanquake removed this from the "Blockers" column in a project
  61. fanquake referenced this in commit 5cb34e6c47 on Sep 14, 2019
  62. sidhujag referenced this in commit 92e2aed54b on Sep 15, 2019
  63. jonasschnelli referenced this in commit a40ccbb195 on Sep 15, 2019
  64. laanwj referenced this in commit 19f301def7 on Sep 16, 2019
  65. sidhujag referenced this in commit b2edf4eee8 on Sep 16, 2019
  66. sidhujag referenced this in commit 45053ec42a on Sep 16, 2019
  67. shesek commented at 10:06 pm on September 24, 2019: contributor

    @gmaxwell wrote a thoughtful comment on reddit explaining some of the motives behind this decision and the issues surrounding BIP 70. I thought it would be useful to copy this here for future reference:

    BIP70 has resulted in remotely exploitable vulnerabilities that would allow an attacker to steal the wallet’s private keys.

    BIP70 has had multiple serious privacy vulnerabilities, and the design of BIP70 where the client makes a connection to a requester specified host is generally prone to additional privacy vulnerabilities.

    BIP70 requires access to a relatively complete x509 implementation as well as a TLS implementation, tens to hundreds of thousands of lines of security critical code that get exposed to the network.

    BIP70 support has been blocking the removal of openssl as a dependency of the Bitcoin software. OpenSSL has a half dozen times or so been the origin of needing to perform short notice emergency upgrades of the software. Emergency upgrades deprive users of their own free choice of software and are an attack vector for introducing vulnerabilities. Additional in some cases OpenSSL’s updates have come in the form of an opaque monolithic path that was hundreds of thousands of lines long, making actual review of their changes effectively impossible.

    Bitpay’s original incompatible modified version of BIP70, which was never used in Bitcoin Core but would have been needed for bitpay compatibility, introduced a serious vulnerability which which would allow bitpay (or someone who compromised their systems) to steal users coins or also just accidentally and almost invisibly take them via an easy mistake.

    BIP70’s implementation in bitcoin is extensively tied into the GUI and so uniform functionality cannot be provided for the RPC or CLI without essentially rewriting it, an effort that would not be justified given that the support is, as far as anyone can tell, largely unused. The lack of an automatable interface to it also makes it very difficult to subject to automated testing. The GUI tie-in also makes it difficult to separate BIP70 into another process in order to reduce the risk from vulnerabilities in the bip70 code. BIP70 also creates a dependency on protobuf, another piece of large network exposed code that could result in vulnerabilities or incompatibilities.

    So, in summary: BIP70 in Core is largely unused, the implementation in Core has never been compatible with the incompatible version used by the only well known user (a company started using it after a decision had been made to remove it… which delayed the removal to see what would happen). BIP70’s design is not particularly fit-for-purpose, it doesn’t accomplish what people wanted it to do (thus the incompatible proprietary modifications and the lack of adoption), and the implementation has been a repeated source of privacy and security vulnerabilities. Attempts to improve BIP70 have been the source of security vulnerabilities. BIP70’s implementation also requires pulling in an enormous amount of third party cryptographic code, which again, have frequently been the source of security vulnerabilities and emergency updates for the software.

    And sure, lots of people justifiably dislike Bitpay– but the removal of BIP70 started before bitpay was even using it and has essentially nothing to do with them beyond the fact that their usage, almost alone, of it isn’t perceived to be enough justification to take the ongoing cost and risk of continuing to support it.

    Of course, if you want to continue to personally use and support it– you’re free to do so. But it isn’t reasonable to demand that other parties do so at their own cost, particularly without a clear justification as to how it benefits their users.

  68. fanquake deleted the branch on Oct 17, 2019
  69. Munkybooty referenced this in commit a617230df4 on Dec 7, 2021
  70. Munkybooty referenced this in commit 70e8603397 on Dec 15, 2021
  71. Munkybooty referenced this in commit 1d183776cb on Dec 15, 2021
  72. DrahtBot locked this on Dec 16, 2021


fanquake jonasschnelli Sjors jameshilliard luke-jr DrahtBot laanwj TheBlueMatt achow101 dongcarl ryanofsky elichai MarcoFalke practicalswift shesek

Labels
Build system

Milestone
0.19.0

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: 2024-11-17 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me