Only create signatures with even S, and verification mode to check. #2131
Conversation
| { | ||
| vchSig.clear(); | ||
| ECDSA_SIG *sig = ECDSA_do_sign((unsigned char*)&hash, sizeof(hash), pkey); | ||
| if (sig == NULL) |
There was a problem hiding this comment.
Subtle change in semantics here: if sig == NULL, the old code cleared vchSig.
For defensive programming, I'd suggest a vchSig.clear(); as the first statement in this routine.
|
@gavinandresen Updated. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3e4655404a8839a03c241fdcf67e063940eb462b for binaries and test log. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/e1771b85af3d16e12bab2243ea50adf2d52eaf3c for binaries and test log. |
|
I think this goes one step too far. Some almost-baked thoughts on the problem we're actually trying to solve (we'd like transactions ids to be immutable): I think this ties into a bunch of other almost-baked thoughts I've had surrounding bumping the transaction.version. I think it might make sense to introduce transaction.version=2 messages onto the network, with additional rules for relaying/DoS-banning. In particular, I'm imagining a signature from one of the keys used to sign the inputs (maybe.. . more thought needed on how the signing is tied to the transaction's creator) that covers the transaction id and transaction fee paid. That should solve the "relayer modifies the signature to change the txid". (the transaction fee paid would be to help out SPV clients, which is a separate feature) |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/e1771b85af3d16e12bab2243ea50adf2d52eaf3c for binaries and test log. |
|
DOS rules don't prevent a miner from modifying a transaction. We really do need to actually remove the malleability. ... though the actual enforcement would need to be on a version=2 transaction. |
|
@gavinandresen I really don't like the fact that this would mean rules at the transaction validation level would need knowledge about the precise inner script semantics. IMHO, we should just gradually introduce rules to remove malleabilities, and then perhaps use tx.nVersion==2 rule to enforce them in the block chain at some later point. |
|
@sipa Needs rebase (or close if it was merged in another form?). |
To fix a minor malleability found by Sergio Lerner (reported here: https://bitcointalk.org/index.php?topic=8392.msg1245898#msg1245898) The problem is that if (R,S) is a valid ECDSA signature for a given message and public key, (R,-S) is also valid. Modulo N (the order of the secp256k1 curve), this means that both (R,S) and (R,N-S) are valid. Given that N is odd, S and N-S have a different lowest bit. We solve the problem by forcing signatures to have an even S value, excluding one of the alternatives. This commit just changes the signing code to always produce even S values, and adds a verification mode to check it. This code is not enabled anywhere yet. Existing tests in key_tests.cpp verify that the produced signatures are still valid.
|
Rebased. @jgarzik @gmaxwell @gavinandresen @laanwj: opinions? |
|
Without this, or a substantially similar, change we cannot eliminate malleability attacks on unconfirmed transaction chains. Though I almost wish the evenness procedure had been specified in BIP32, as now we're going to see hardware wallet deployed that produce odd signatures. They can be fixed with an after the fact mutation, so perhaps thats not the end of the world. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a81cd96805ce6b65cca3a40ebbd3b2eb428abb7b for binaries and test log. |
|
ACK. |
Only create signatures with even S, and verification mode to check.
To fix a minor malleability found by Sergio Lerner (reported here: https://bitcointalk.org/index.php?topic=8392.msg1245898#msg1245898) Thanks to Sipa, bitcoin#2131
Instead just mark them as MASTERNODE_UPDATE_REQUIRED and proceed further.
Don't drop mnb-s for outdated MNs (bitcoin#2131)
Instead just mark them as MASTERNODE_UPDATE_REQUIRED and proceed further.
Instead just mark them as MASTERNODE_UPDATE_REQUIRED and proceed further.
Instead just mark them as MASTERNODE_UPDATE_REQUIRED and proceed further.
Instead just mark them as MASTERNODE_UPDATE_REQUIRED and proceed further.
To fix a minor malleability found by Sergio Lerner (reported here: https://bitcointalk.org/index.php?topic=8392.msg1245898#msg1245898)
The problem is that if (R,S) is a valid ECDSA signature for a given message and public key, (R,-S) is also valid. Modulo N (the order of the secp256k1 curve), this means that both (R,S) and (R,N-S) are valid. Given that N is odd, S and N-S have a different lowest bit. We solve the problem by forcing signatures to have an even S, excluding one of the alternatives.
This pull request just changes the signing code to always produce even S values, and adds a verification mode to check it. This code is not enabled anywhere yet. Existing tests in key_tests.cpp verify that the produced signatures are still valid.