Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: disable BIP70 support by default #15584

Merged
merged 2 commits into from Sep 13, 2019

Conversation

@fanquake
Copy link
Member

commented Mar 12, 2019

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:

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

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

Options used to compile and link:
  with wallet   = yes
  with gui / qt = yes
    with bip70  = yes

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

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

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

checking whether to build test_bitcoin-qt... yes
checking 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
@fanquake fanquake added this to the 0.19.0 milestone Mar 12, 2019
@fanquake fanquake force-pushed the fanquake:disable-bip70-by-default branch from 713fe4a to 1a8624a Mar 12, 2019
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

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?
@Sjors

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

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.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

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.

@jameshilliard

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2019

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.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

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

@jameshilliard

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

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.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Hate to say it, but we should probably do a release with something like #15064 before disabling BIP70 by default... :/

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

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

Conflicts

No conflicts as of last run.

@MarcoFalke MarcoFalke modified the milestones: 0.19.0, 0.20.0 Jun 26, 2019
@fanquake fanquake force-pushed the fanquake:disable-bip70-by-default branch from 1a8624a to c53da7c Aug 6, 2019
@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

ACK for 0.19.0

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Concept ACK

@achow101

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

ACK c53da7c

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Gitian builds for commit 00dad5e (master):

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

@luke-jr

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

That's incorrect. There is BIP70-specific protobuf-encoded metadata written to wallets for each BIP70 transaction made.

@fanquake fanquake added this to Blockers in High-priority for review Sep 11, 2019
fanquake added a commit that referenced this pull request Sep 11, 2019
1619684 Added libbitcoin_qt and bitcoin-qt to the msbuild configuration. (Aaron Clauson)

Pull request description:

  This PR has ~~90%~~ all of the work done to allow the bitcoin Qt programs to be built with msvc and the appveyor script.

  Outstanding issues:

  - ~~There are ~~3~~ ~~6~~ 5 code tweaks required for the bitcoin Qt components to be built without warnings with msvc. They seem minor~~,
  - Building Qt as a static library for Windows is painful and time consuming. I doubt it will ever be possible to build Qt from source as part of an appveyor job (and it would probably take over an hour even if it was). My tentative solution is to build locally and upload the binaries as a [github release](https://github.com/sipsorcery/qt_win_binary/releases). The msvc build is only for testing and tinkering but even so this doesn't feel like the ideal solution. Open to suggestions?
  - ~~There is still an issue to sort out with the payment request URL handling. Building Qt with openssl is an extra headache. I will continue to work on getting this working.~~

  The big benefit of this PR is the ability to run bitcoin-qt within a Visual Studio debugging session which could expedite tracking down issues on Windows.

  On a side note the test-bitcoin-qt tests fail very early, probably due to *nix specific tests. I haven't dug into them at this point.

  **Update 28 Jun 2019**: The ENABLE_BIP70 option is now off (it's flagged for removal as per #15584). With it disabled msbuild does not require any code changes to build the Bitcoin Core Qt applications.

ACKs for top commit:
  fanquake:
    re-ACK 1619684 - AppVeyor looks ok now.

Tree-SHA512: c0d3fd53b3ff99096b2505d519ed5ca6791bc4bce77addf9c520dc042eec5980a51a1fb9f0aa72e9cc53773085c43218793ca7a915a47806a3a1ffb84d9409f9
Copy link
Contributor

left a comment

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 a3fb456 assuming this does turn off bip70 by default in all release artifacts.

configure.ac Outdated Show resolved Hide resolved
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 12, 2019
…changes)

1619684 Added libbitcoin_qt and bitcoin-qt to the msbuild configuration. (Aaron Clauson)

Pull request description:

  This PR has ~~90%~~ all of the work done to allow the bitcoin Qt programs to be built with msvc and the appveyor script.

  Outstanding issues:

  - ~~There are ~~3~~ ~~6~~ 5 code tweaks required for the bitcoin Qt components to be built without warnings with msvc. They seem minor~~,
  - Building Qt as a static library for Windows is painful and time consuming. I doubt it will ever be possible to build Qt from source as part of an appveyor job (and it would probably take over an hour even if it was). My tentative solution is to build locally and upload the binaries as a [github release](https://github.com/sipsorcery/qt_win_binary/releases). The msvc build is only for testing and tinkering but even so this doesn't feel like the ideal solution. Open to suggestions?
  - ~~There is still an issue to sort out with the payment request URL handling. Building Qt with openssl is an extra headache. I will continue to work on getting this working.~~

  The big benefit of this PR is the ability to run bitcoin-qt within a Visual Studio debugging session which could expedite tracking down issues on Windows.

  On a side note the test-bitcoin-qt tests fail very early, probably due to *nix specific tests. I haven't dug into them at this point.

  **Update 28 Jun 2019**: The ENABLE_BIP70 option is now off (it's flagged for removal as per bitcoin#15584). With it disabled msbuild does not require any code changes to build the Bitcoin Core Qt applications.

ACKs for top commit:
  fanquake:
    re-ACK 1619684 - AppVeyor looks ok now.

Tree-SHA512: c0d3fd53b3ff99096b2505d519ed5ca6791bc4bce77addf9c520dc042eec5980a51a1fb9f0aa72e9cc53773085c43218793ca7a915a47806a3a1ffb84d9409f9
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

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

ACK e09913f

fanquake added 2 commits Mar 12, 2019
@fanquake fanquake force-pushed the fanquake:disable-bip70-by-default branch from a3fb456 to e09913f Sep 12, 2019
@fanquake

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

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.

@jameshilliard

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

I think we need something like #16858 before merging this so that users don't switch to BIP70 compatible wallets with insecure BIP70 implementations.

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

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

@jameshilliard

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@laanwj Should we be able to get something like #16858 in by the deadline as well since it should be a trivial change?

@elichai

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

ACK e09913f 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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

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

@practicalswift

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

ACK e09913f -- diff looks correct

Smaller attack surface is better!

laanwj added a commit that referenced this pull request Sep 13, 2019
e09913f doc: specify protobuf as optional in build docs (fanquake)
376f492 build: disable BIP70 support by default (fanquake)

Pull request description:

  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`:
  ```
  Options used to compile and link:
    with wallet   = yes
    with gui / qt = yes
      with bip70  = no
  ```

  Protobuf available and `./configure --enable-bip70`:
  ```
  Options used to compile and link:
    with wallet   = yes
    with gui / qt = yes
      with bip70  = yes
  ```

  Protobuf not available (i.e `brew unlink protobuf`) and `./configure`:
  ```
  Options used to compile and link:
    with wallet   = yes
    with gui / qt = yes
      with bip70  = no
  ```

  Protobuf not available and `./configure --enable-bip70`:
  ```
  checking whether to build test_bitcoin-qt... yes
  checking whether to build BIP70 support... configure: error: protobuf missing
  ```

  TODO:
  - [x] Remove `protobuf` from other Travis builds
  - [ ] Documentation updates (mention that `protobuf` is now optional)?
  - [ ] Could split release notes into GUI and build

ACKs for top commit:
  laanwj:
    ACK e09913f
  elichai:
    ACK e09913f Read the autotools changes. awesome that this removes the protobuf requirement.
  practicalswift:
    ACK e09913f -- diff looks correct

Tree-SHA512: 7bf87ae8555e24db2da2e89cc4d4e90d09be27499ad386ad65879d05df8f96d9a1384379891ac8963d17728c90e55961560813df97e849e631e2de8c08e210c8
@laanwj laanwj merged commit e09913f into bitcoin:master Sep 13, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fanquake fanquake removed this from Blockers in High-priority for review Sep 13, 2019
fanquake added a commit to fanquake/bitcoin that referenced this pull request Sep 14, 2019
As mentioned by dongcarl in [bitcoin#15584](bitcoin#15584 (comment))
make building protobuf optional in depends.

Those that want to build it can now pass PROTOBUF=1.
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 15, 2019
e09913f doc: specify protobuf as optional in build docs (fanquake)
376f492 build: disable BIP70 support by default (fanquake)

Pull request description:

  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`:
  ```
  Options used to compile and link:
    with wallet   = yes
    with gui / qt = yes
      with bip70  = no
  ```

  Protobuf available and `./configure --enable-bip70`:
  ```
  Options used to compile and link:
    with wallet   = yes
    with gui / qt = yes
      with bip70  = yes
  ```

  Protobuf not available (i.e `brew unlink protobuf`) and `./configure`:
  ```
  Options used to compile and link:
    with wallet   = yes
    with gui / qt = yes
      with bip70  = no
  ```

  Protobuf not available and `./configure --enable-bip70`:
  ```
  checking whether to build test_bitcoin-qt... yes
  checking whether to build BIP70 support... configure: error: protobuf missing
  ```

  TODO:
  - [x] Remove `protobuf` from other Travis builds
  - [ ] Documentation updates (mention that `protobuf` is now optional)?
  - [ ] Could split release notes into GUI and build

ACKs for top commit:
  laanwj:
    ACK e09913f
  elichai:
    ACK e09913f Read the autotools changes. awesome that this removes the protobuf requirement.
  practicalswift:
    ACK e09913f -- diff looks correct

Tree-SHA512: 7bf87ae8555e24db2da2e89cc4d4e90d09be27499ad386ad65879d05df8f96d9a1384379891ac8963d17728c90e55961560813df97e849e631e2de8c08e210c8
jonasschnelli added a commit that referenced this pull request Sep 15, 2019
…IP70 URI.

1153caf Qt: advise users not to switch wallets when opening a BIP70 URI. (James Hilliard)

Pull request description:

  It would probably be a good idea to have something like this before #15584 is merged.

ACKs for top commit:
  jonasschnelli:
    utACK 1153caf
  fanquake:
    ACK 1153caf

Tree-SHA512: 6e682dd280c44eaafb1206c32439df42a20173c33297bf93dd607f0a7a2faec8e2d17fff83c85027083ebd11a71795b443e707992251574370dd1d46b7bff060
laanwj added a commit that referenced this pull request Sep 16, 2019
107e030 build: make protobuf optional in depends (fanquake)
ff6122f doc: clarify protobuf build requirements (fanquake)

Pull request description:

  As mentioned by dongcarl in #15584 (comment), make building `protobuf` optional in depends. With this change it will only be built if you pass `PROTOBUF=1`.

ACKs for top commit:
  laanwj:
    code review ACK 107e030
  Sjors:
    tACK 107e030 on macOS 10.14. When I build depends with `PROTOBUF=1` then `./configure` has `bip70` enabled.

Tree-SHA512: 49bc247a6879aaf55b943a3d0b930544ddef1e69a481955a8bebe0b02c9ad0fe168b93025f34168334cef34bb567478eb98eacab62ba909f2f64fb21119c71b8
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 16, 2019
…ing a BIP70 URI.

1153caf Qt: advise users not to switch wallets when opening a BIP70 URI. (James Hilliard)

Pull request description:

  It would probably be a good idea to have something like this before bitcoin#15584 is merged.

ACKs for top commit:
  jonasschnelli:
    utACK 1153caf
  fanquake:
    ACK 1153caf

Tree-SHA512: 6e682dd280c44eaafb1206c32439df42a20173c33297bf93dd607f0a7a2faec8e2d17fff83c85027083ebd11a71795b443e707992251574370dd1d46b7bff060
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 16, 2019
107e030 build: make protobuf optional in depends (fanquake)
ff6122f doc: clarify protobuf build requirements (fanquake)

Pull request description:

  As mentioned by dongcarl in bitcoin#15584 (comment), make building `protobuf` optional in depends. With this change it will only be built if you pass `PROTOBUF=1`.

ACKs for top commit:
  laanwj:
    code review ACK 107e030
  Sjors:
    tACK 107e030 on macOS 10.14. When I build depends with `PROTOBUF=1` then `./configure` has `bip70` enabled.

Tree-SHA512: 49bc247a6879aaf55b943a3d0b930544ddef1e69a481955a8bebe0b02c9ad0fe168b93025f34168334cef34bb567478eb98eacab62ba909f2f64fb21119c71b8
@shesek

This comment has been minimized.

Copy link

commented Sep 24, 2019

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

@fanquake fanquake deleted the fanquake:disable-bip70-by-default branch Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.