Skip to content

build: disable BIP70 support by default #15584

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 2 commits into from
Sep 13, 2019
Merged

build: disable BIP70 support by default #15584

laanwj merged 2 commits into from
Sep 13, 2019

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Mar 12, 2019 •
edited
Loading

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 mentioned this pull request Mar 12, 2019
Merged
@fanquake fanquake force-pushed the disable-bip70-by-default branch from 713fe4a to 1a8624a Compare March 12, 2019 03:06
@jonasschnelli
Copy link
Contributor

jonasschnelli 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
Copy link
Member

Sjors 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
Copy link
Contributor

jonasschnelli 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
Copy link
Contributor

jameshilliard 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
Copy link
Member

luke-jr 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
Copy link
Contributor

jameshilliard 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
Copy link
Member

luke-jr 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
Copy link
Contributor

DrahtBot commented Jun 18, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@maflcko maflcko modified the milestones: 0.19.0, 0.20.0 Jun 26, 2019
@sipsorcery sipsorcery mentioned this pull request Jun 28, 2019
Merged
@fanquake fanquake force-pushed the disable-bip70-by-default branch from 1a8624a to c53da7c Compare August 6, 2019 01:12
@laanwj
Copy link
Member

laanwj commented Aug 12, 2019

ACK for 0.19.0

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Aug 12, 2019

Concept ACK

@achow101
Copy link
Member

achow101 commented Aug 12, 2019 •
edited
Loading

ACK c53da7c5f166a95f3ae37f817f8cd7b94812077a

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

@bitcoin bitcoin deleted a comment from DrahtBot Aug 12, 2019
@fanquake fanquake added this to the 0.19.0 milestone Aug 12, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 13, 2019

Gitian builds for commit 00dad5e (master):

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

@fanquake fanquake force-pushed the disable-bip70-by-default branch from a3fb456 to e09913f Compare September 12, 2019 10:30
@fanquake
Copy link
Member Author

fanquake 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 jameshilliard mentioned this pull request Sep 12, 2019
Merged
@jameshilliard
Copy link
Contributor

jameshilliard 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
Copy link
Member

laanwj 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
Copy link
Contributor

jameshilliard 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
Copy link
Contributor

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

@maflcko
Copy link
Member

maflcko commented Sep 13, 2019 •
edited
Loading

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
Copy link
Contributor

practicalswift 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
@laanwj
Merge #15584: build: disable BIP70 support by default
102998e 
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
@fanquake fanquake mentioned this pull request Sep 14, 2019
Merged
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 15, 2019
@laanwj @sidhujag
Merge bitcoin#15584: build: disable BIP70 support by default
92e2aed 
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
@jonasschnelli
Merge #16858: Qt: advise users not to switch wallets when opening a B…
a40ccbb 
…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
@laanwj
Merge #16871: build: make building protobuf optional in depends
19f301d 
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 pushed a commit to syscoin/syscoin that referenced this pull request Sep 16, 2019
@jonasschnelli @sidhujag
Merge bitcoin#16858: Qt: advise users not to switch wallets when open…
b2edf4e 
…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 pushed a commit to syscoin/syscoin that referenced this pull request Sep 16, 2019
@laanwj @sidhujag
Merge bitcoin#16871: build: make building protobuf optional in depends
45053ec 
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
Copy link
Contributor

shesek commented Sep 24, 2019 •
edited
Loading

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

@Sjors Sjors mentioned this pull request Sep 30, 2019
Closed
@fanquake fanquake deleted the disable-bip70-by-default branch October 17, 2019 15:14
@Naviabheeman Naviabheeman mentioned this pull request Feb 4, 2020
Closed
@kwvg kwvg mentioned this pull request Mar 2, 2021
Merged
@pinheadmz pinheadmz mentioned this pull request Sep 7, 2021
Open
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

Assignees

No one assigned

Labels

Build system

Projects

None yet

Milestone

0.19.0

Development

Successfully merging this pull request may close these issues.