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

fix(electrum): bdk_electrum exploit fixes #1675

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented Nov 6, 2024

Fixes #1698, #1697.

Description

The aim of this PR is to fix a few exploits that were found within bdk_electrum. Most notably:

  • Transaction Merkle proof verification does not check the Merkle proof for the coinbase. This enables an attack by which an attacker can fake a deeply-confirmed payment to a BDK wallet. (https://delvingbitcoin.org/t/great-consensus-cleanup-revival/710#merkle-tree-attacks-using-64-bytes-transactions-8)
  • An Electrum server can trigger a panic in populate_with_txids by returning a transaction with no output.
  • fetch_prev_txout should not try to query the prevouts of coinbase transactions. This will query a transaction to the Electrum server which will return an error and will make the overall sync fail.

Notes to the reviewers

  • This is draft PR that is dependent on the following rust-electrum-client PR: feat: add id_from_pos support rust-electrum-client#155
  • The validate_merkle test commit needs significant review. My initial thoughts were to put MockElectrumClient into bdk_testenv, but because bdk_testenv's electrum-client is re-exported from electrsd, a separate dependency would have to be added for just electrum-client as such that support for txid_with_pos will be available when PR#155 is merged. Or perhaps there is a better way to specifically test validate_merkle_for_anchor that does not involve creating a MockElectrumClient.

Changelog notice

TODO

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@LagginTimes LagginTimes marked this pull request as draft November 6, 2024 17:40
@LagginTimes LagginTimes changed the title fix(electrum): bdk_electrum exploit fixes draft fix(electrum): bdk_electrum exploit fixes Nov 6, 2024
@LagginTimes LagginTimes force-pushed the electrum_exploit_fixes branch 3 times, most recently from 4c62df8 to d7eac3e Compare November 13, 2024 11:39
…sactions

\`fetch_prev_txout\` should not try to query the prevouts of coinbase
transactions, as it may result in the Electrum server returning an error
which would cause the overall `sync` to fail.
This fix validates the coinbase merkle path when checking the merkle
proof of a transaction we are inserting within the same block. This check
mitigates a known brute force exploit that could insert an invalid
transaction.

f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

bdk_electrum: don't query prevouts of coinbase tx
1 participant