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

A fix for transaction malleability #1144

Merged
merged 16 commits into from Aug 5, 2016

Conversation

Projects
None yet
8 participants
@bitcartel
Copy link
Contributor

bitcartel commented Jul 27, 2016

This PR fixes transaction malleability by not including the sigscript of transaction inputs and joinsplit sigs when hashing the txid.

This PR supercedes PR #1101 which was a minimal solution based on a new serialization flag.

This PR introduces GetTxid() to distinguish between getting a transaction id and the double sha256 hash.

The key changes are:

  • Adding GetTxid() method to CTransaction which makes a copy of the transaction, clearing out the sigscript and joinsplitsig fields, before hashing.
  • Verifying that every call to GetHash() actually wants a txid, and replacing with GetTxid().
  • Renaming GetHash() to GetSerializeHash()
    • Rationale: In future, upstream code we want to merge will use GetHash() but we don't know the intent. We should check to see if the intent is to receive a txid (most likely) in which case we replace with GetTxid(), or if upstream actually wants a double hash of the transaction we can use GetSerializeHash().
  • Updated genesis data in chainparams.cpp

Note that coinbase transactions are excluded as they need the sigscript hashed to help avoid duplicate txids per BIP34:

  • This modification is related to a question from @ebfull on PR #1101 - "Can we think of a way this change allows us to construct two transactions with the same txid which can simultaneously appear in the blockchain? My guess is it would be possible to construct a coinbase transaction of such a form... this surely breaks invariants."

This PR Passes all tests in test_bitcoin (test data was updated in bloom_tests, miner_tests and script_tests).

bitcartel added some commits Jul 22, 2016

Rename GetHash() method to GetSerializeHash().
When pulling from upstream we are now forced to examine GetHash() usage
and replace with GetSerializeHash() if the caller wants a double SHA256
hash, or with GetTxid() if the caller wants a transaction id.
Replace calls to GetHash() with GetTxid() for transaction objects.
Where the caller intends to receive a transaction id and not a double
SHA256 hash.
Refactor GetTxid() into UpdateTxid() to match coding style of hash me…
…mber variable.

UpdateTxid() is called alongside UpdateHash() when a CTransaction is
deserialized or constructed.  GetTxid() now returns a const reference.

@bitcartel bitcartel self-assigned this Jul 27, 2016

@bitcartel bitcartel changed the title Fix transaction malleability Fix transaction malleability with GetTxid() Jul 27, 2016

@bitcartel bitcartel changed the title Fix transaction malleability with GetTxid() A fix for transaction malleability Jul 27, 2016

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Jul 27, 2016

Are we preventing coinbases from appearing with the same txid as a consensus rule? The changes in this PR sound like they only prevent it locally, but a miner might be able to abuse it to cause double-spends or something.

@ebfull

This comment has been minimized.

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Jul 27, 2016

Only prevents it locally. Not consensus.

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Jul 27, 2016

Thx. Based on BIP34 I think we just need to have the txid of a coinbase tx be computed the old way, to include the sigscript. So I can then remove the change in CreateNewBlock. Will keep you posted.

Update: Changes complete and test data updated. Hat tip @ebfull.

@defuse

This comment has been minimized.

Copy link
Contributor

defuse commented Jul 27, 2016

It might be worthwhile to add domain separation between GetSerializeHash() and GetTxid(). For example, prefix the serialization with a string like "TXID" or something (just make sure it isn't a valid serialization). I can't think of a good reason this would be necessary, but here's a super contrived protocol that would require this domain separation:

There's a server that holds a secret random transaction. Everyone on the Internet is allowed to do three things: 1. Query for the secret transaction's GetSerializeHash(), 2. Tell the server to re-randomize the transaction, and 3. Submit a guess for the txid. If you guess the txid correctly, the server sends you 100 bitcoins. Without domain separation, this game can be won by re-randomizing and submitting the GetSerializeHash() as the txid, hoping to land on one of the transactions that have the same txid as their hash. With domain separation, it shouldn't be winnable.

We don't need domain separation if nowhere in the Zcash protocol are there interactions between the two like this, but I think it's easier to add domain separation than to check all the uses.

(Background on chosen protocol attacks: https://www.schneier.com/academic/archives/1997/01/protocol_interaction.html)

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Jul 27, 2016

BIP62 provides background on tx malleability: https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Jul 27, 2016

+1 for domain separation here. Note that we have a domain separation scheme for SHA-256 hashes already; 0xB1 is the next unused lead byte. (I don't know whether that forms an invalid first byte for a serialised transaction.)

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Aug 3, 2016

Self-ACK...

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 3, 2016

This PR is a bit scary for me. Not because it isn't done well, but because there are almost certainly side-effects we aren't considering. I can't think of any, and my previous nits have been fixed, so I am tempted to ACK this. I'll take a look at it again tonight when I get home.

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Aug 3, 2016

@ebfull Possible but unlikely scenario for transparent txs. On the last day of every month, Employer (address A) pays salary to Worker (address B) who transfers a fixed amount to Savings (address C). Someone could spot the pattern and craft a transaction, using the UTXO at B as an input, and an output to C. This might result in a tx with the same txid as the legitimate tx which the Worker has not yet created. Broadcasting this txid with fake sigs will result in the txid being rejected from entering the mempool and the txid getting placed in the recentRejects filter, so that eventually when the legitimate tx is broadcast it is rejected too. However, this scenario is unlikely since the change address should be different each time, no guarantee the wallet will use that UTXO rather than a bunch of others, output order could differ, nLockTime (timestamp) might be set, etc. making it hard to predict and craft a transaction with the same txid. Also since nodes won't relay a bad transaction, it will be very hard to propagate unless the attacker manually crawls the network and connects to peers in order to share the bad transaction.

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 4, 2016

Considering this kind of stuff five days before an audit is really suboptimal, but given my understanding of the PR and Bitcoin, ACK.

tx.joinSplitSig.assign(0);
}

*const_cast<uint256*>(&txid) = SerializeHash(tx);

This comment has been minimized.

@str4d

str4d Aug 4, 2016

Contributor

Does this mean that we aren't actually "skipping" the sigs as such, but instead are including them as zeroes? That needs to be clear for people to recreate the txids.

This comment has been minimized.

@bitcartel

bitcartel Aug 4, 2016

Author Contributor

Ok, will make a ticket about documenting this with examples.

filter.insert(ParseHex("044a656f065871a353f216ca26cef8dde2f03e8c16202d2e8ad769f02032cb86a5eb5e56842e92e19141d60a01928f8dd2c875a390f67c1f6c94cfc617c0ea45af"));
// Match an output from the second transaction (the pubkey for address mjuZa8Dy12HKyUNjc1whNTxRaGU4gGGLxY)
// It also matches the third transaction, which spends to the pubkey again
// This should match the last transaction because it spends the output matched

This comment has been minimized.

@str4d

str4d Aug 4, 2016

Contributor

Which code changes cause this semantic change in the test?

filter.insert(ParseHex("044a656f065871a353f216ca26cef8dde2f03e8c16202d2e8ad769f02032cb86a5eb5e56842e92e19141d60a01928f8dd2c875a390f67c1f6c94cfc617c0ea45af"));
// Match an output from the second transaction (the pubkey for address mjuZa8Dy12HKyUNjc1whNTxRaGU4gGGLxY)
// It will match the third transaction, which has another pay-to-pubkey output to the same address
// This should not match the last transaction though it spends the output matched

This comment has been minimized.

@str4d

str4d Aug 4, 2016

Contributor

Ditto.

This comment has been minimized.

@bitcartel

bitcartel Aug 4, 2016

Author Contributor

When deserializing the hex string into a block object, the txids are computed the new way so they don't match the txids used in the test.

This comment has been minimized.

@str4d

str4d Aug 4, 2016

Contributor

I understand that; I was asking why the semantic ordering in the comment changed. Are the transactions ordered by txid?

This comment has been minimized.

@bitcartel

bitcartel Aug 4, 2016

Author Contributor

I see what you mean. I generated the transactions by hand to try and match the original test data (transactions in a real block). The exact order of the transactions was based on the regtest miner.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 4, 2016

Impl ACK modulo comments. I am not enough of a Bitcoiner to understand all the possible side-effects of not including the signatures, but I'll assume that they are all less bad than the known issue of transaction malleability.

@bitcartel

This comment has been minimized.

Copy link
Contributor Author

bitcartel commented Aug 5, 2016

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 5, 2016

📌 Commit 74cd882 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 5, 2016

⌛️ Testing commit 74cd882 with merge 95277e0...

zkbot pushed a commit that referenced this pull request Aug 5, 2016

zkbot
Auto merge of #1144 - bitcartel:zc.v0.11.2.z7_tx_malleability_gettxid…
…, r=bitcartel

A fix for transaction malleability

This PR fixes transaction malleability by not including the sigscript of transaction inputs and joinsplit sigs when hashing the txid.

This PR supercedes PR #1101 which was a minimal solution based on a new serialization flag.

This PR introduces GetTxid() to distinguish between getting a transaction id and the double sha256 hash.

The key changes are:
- Adding GetTxid() method to CTransaction which makes a copy of the transaction, clearing out the sigscript and joinsplitsig fields, before hashing.
- Verifying that every call to GetHash() actually wants a txid, and replacing with GetTxid().
- Renaming GetHash() to GetSerializeHash()
  - Rationale: In future, upstream code we want to merge will use GetHash() but we don't know the intent.  We should check to see if the intent is to receive a txid (most likely) in which case we replace with GetTxid(), or if upstream actually wants a double hash of the transaction we can use GetSerializeHash().
- Updated genesis data in chainparams.cpp

Note that coinbase transactions are excluded as they need the sigscript hashed to help avoid duplicate txids per BIP34:
  - This modification is related to a question from @ebfull on PR #1101 - "Can we think of a way this change allows us to construct two transactions with the same txid which can simultaneously appear in the blockchain? My guess is it would be possible to construct a coinbase transaction of such a form... this surely breaks invariants."

This PR Passes all tests in test_bitcoin (test data was updated in bloom_tests, miner_tests and script_tests).
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 5, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit 74cd882 into zcash:zc.v0.11.2.latest Aug 5, 2016

1 check passed

homu Test successful
Details

bitcartel added a commit to bitcartel/zcash that referenced this pull request Aug 27, 2016

Remove zcash#1144 from transaction.h.
Reverts to 4bc00dc with commits f0dab51 (snark) and f5e5707 (joinsplit) retained.
GetTxid() is now an alias for GetHash().

bitcartel added a commit to bitcartel/zcash that referenced this pull request Aug 27, 2016

bitcartel added a commit to bitcartel/zcash that referenced this pull request Aug 27, 2016

bitcartel added a commit to bitcartel/zcash that referenced this pull request Aug 27, 2016

Remove zcash#1144 from input data of script_tests.
Revert script_invalid.json to commit df1609f.
Revert script_valid.json to commit 1c54757.

bitcartel added a commit to bitcartel/zcash that referenced this pull request Aug 27, 2016

zkbot pushed a commit that referenced this pull request Sep 7, 2016

zkbot
Auto merge of #1316 - bitcartel:zc.v0.11.2.z9_unwind_1144, r=ebfull
Unwind #1144 txid malleability

This PR unwinds #1144, retaining GetTxid() as an alias for GetHash() to avoid having to rename across many files and any code in progress.

The txid gtest has been updated to verify that GetTxid() returns the same result as GetHash().

Both zcash-gtest and test_bitcoin pass.

Reviewers: It's easiest to review the 5 commits in chronological order.  Each commit is quite small.  Test files that have been rolled back (bloom_tests, script_invalid, script_valid) appear to be a big change, so it's easier to diff against the commit mentioned in the log.

bitcartel added a commit to bitcartel/zcash that referenced this pull request Sep 7, 2016

Remove zcash#1144 from transaction.h.
Reverts to 4bc00dc with commits f0dab51 (snark) and f5e5707 (joinsplit) retained.
GetTxid() is now an alias for GetHash().

bitcartel added a commit to bitcartel/zcash that referenced this pull request Sep 7, 2016

bitcartel added a commit to bitcartel/zcash that referenced this pull request Sep 7, 2016

bitcartel added a commit to bitcartel/zcash that referenced this pull request Sep 7, 2016

Remove zcash#1144 from input data of script_tests.
Revert script_invalid.json to commit df1609f.
Revert script_valid.json to commit 1c54757.

bitcartel added a commit to bitcartel/zcash that referenced this pull request Sep 7, 2016

@solardiz solardiz referenced this pull request Nov 15, 2018

Merged

New txid calculation #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment