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 coinbase merkle and populate_with_txids exploit fixes #1675

Open
wants to merge 4 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

  • The "validate coinbase 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 just for electrum-client 0.22 support. However, since we are doing in-line tests to access private functions, there is a strong argument for leaving the test structures where they are currently at. Or perhaps there is a better way to specifically test validate_merkle_for_anchor that does not involve creating a MockElectrumClient, similar to what @evanlinjin is doing here: https://github.com/evanlinjin/experiments/blob/main/electrum_c/src/state.rs.

Changelog notice

  • fetch_prev_txout no longer queries coinbase transactions.
  • Coinbase tx merkle path is also validated during tx merkle path verification.
  • populate_with_txids will no longer panic on transactions with no outputs.

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
@LagginTimes LagginTimes force-pushed the electrum_exploit_fixes branch from 9370150 to 4d0af70 Compare November 16, 2024 07:18
@LagginTimes LagginTimes force-pushed the electrum_exploit_fixes branch from 4d0af70 to 721bae4 Compare November 29, 2024 11:09
@LagginTimes LagginTimes self-assigned this Nov 29, 2024
@LagginTimes LagginTimes force-pushed the electrum_exploit_fixes branch 3 times, most recently from ba7e5cc to f4be0d0 Compare November 29, 2024 15:32
@LagginTimes LagginTimes marked this pull request as ready for review December 3, 2024 11:56
@LagginTimes LagginTimes force-pushed the electrum_exploit_fixes branch 4 times, most recently from 1ed60dd to 94296ca Compare December 4, 2024 15:33
@oleonardolima
Copy link
Contributor

Isn't this one part of 1.0 milestone? I guess it's missing it and not appearing on the project board.

@notmandatory notmandatory added new feature New feature or request module-blockchain audit Suggested as result of external code audit and removed new feature New feature or request audit Suggested as result of external code audit labels Dec 6, 2024
@notmandatory
Copy link
Member

notmandatory commented Dec 6, 2024

For the audit issue #1698 you fixed it with #1756. Can you update this to just be checking Merkle proofs? And if so then I'd say this is a new feature that can be put off to a post 1.0.0 release.

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

I'm planning giving this one a test throughout the weekend, and think more about the approach with MockElectrumClient.

For the audit issue #1698 you fixed it with #1756. Can you update this to just be checking Merkle proofs? And if so then I'd say this is a new feature that can be put off to a post 1.0.0 release.

Agree, the commit with the fix on coinbase prevout should be dropped. The remaining two fixes (I guess) can either be together in this PR or separate, it'd depend on the priority of each one on 1.0.0-beta milestone.

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
@LagginTimes LagginTimes force-pushed the electrum_exploit_fixes branch from 94296ca to 122b697 Compare December 8, 2024 08:55
@LagginTimes LagginTimes changed the title fix(electrum): bdk_electrum exploit fixes fix(electrum): bdk_electrum coinbase merkle and populate_with_txids exploit fixes Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module-blockchain new feature New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

bdk_electrum: don't query prevouts of coinbase tx
3 participants