Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

Fix K generator in ECDSA #51

Merged
merged 1 commit into from
Mar 24, 2016
Merged

Fix K generator in ECDSA #51

merged 1 commit into from
Mar 24, 2016

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Mar 22, 2016

This bug gives different signatures for same transactions. This means that you got different txIds for same data.

/cc @killerstorm

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0004%) to 97.902% when pulling 0249e8d on fanatid:fix/ecdsa-k into 4430479 on bitpay:master.

@matiu
Copy link
Contributor

matiu commented Mar 22, 2016

Related + some Test cases: bitpay/bitcore#1269

@braydonf
Copy link
Contributor

I see, so the issue is that the sighash is reversed when it's signed but not when generating k, and leading to discrepancies between libraries in the situations that "little" endian is used.

Also verified against libsecp256k1 and with this PR it will pass without errors:

var assert = require('assert');
var bitcore = require('bitcore-lib');

var message = new Buffer('52204d20fd0131ae1afd173fd80a3a746d2dcc0cddced8c9dc3d61cc7ab6e966', 'hex');
var reversed = new Buffer('66e9b67acc613ddcc9d8cedd0ccc2d6d743a0ad83f17fd1aae3101fd204d2052', 'hex');                                                                                                                                           

var keyBuffer = new Buffer('16f243e962c59e71e54189e67e66cf2440a1334514c09c00ddcc21632bac9808', 'hex');
var key = bitcore.PrivateKey.fromBuffer(keyBuffer);

var signature1 = bitcore.crypto.ECDSA.sign(message, key).toBuffer().toString('hex');
var signature2 = bitcore.crypto.ECDSA.sign(message, key, 'little').toBuffer().toString('hex');

var secp256k1 = require('secp256k1');

var signature3 = secp256k1.sign(message, keyBuffer);
var signature4 = secp256k1.sign(reversed, keyBuffer);

assert.equal(secp256k1.signatureExport(signature3.signature).toString('hex'), signature1);
assert.equal(secp256k1.signatureExport(signature4.signature).toString('hex'), signature2);

@@ -88,9 +88,10 @@ ECDSA.prototype.deterministicK = function(badrs) {
var x = this.privkey.bn.toBuffer({
size: 32
});
k = Hash.sha256hmac(Buffer.concat([v, new Buffer([0x00]), x, this.hashbuf]), k);
var hashbuf = this.endian === 'little' ? [].reverse.call(new Buffer(this.hashbuf)) : this.hashbuf
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to use BufferUtil.reverse() here, since we have benchmarks around it, and use it elsewhere.

@braydonf
Copy link
Contributor

LGTM, except nit from above. I think this is also backwards compatible and should be able to include in 0.13.14.

@matiu
Copy link
Contributor

matiu commented Mar 23, 2016

LGTM. Agree with @braydonf 's suggestion.

Great Job @fanatid and @braydonf checking it.

@crwatkins this will make Copay's signatures to match other libraries (as mentioned in bitcoin-dot-org/Bitcoin.org#888).

@fanatid
Copy link
Contributor Author

fanatid commented Mar 24, 2016

Updated.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0004%) to 97.902% when pulling 3579305 on fanatid:fix/ecdsa-k into 4430479 on bitpay:master.

@braydonf braydonf merged commit 8c948ef into bitpay:master Mar 24, 2016
@braydonf
Copy link
Contributor

Thanks! Landed in 0.13.14

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants