11 ms·

I took a quick look at your implementation of ECDSA and I think it has a bug at line 311 [1]. It looks like I could bypass the check if r or s is negative. One

by cryptbe 12y ago

I took a quick look at your implementation of ECDSA and I think it has a bug at line 311 [1]. It looks like I could bypass the check if r or s is negative. One thing that I don't understand is why big integer libraries developed exclusively for crypto need negative numbers. The library [2] that I contribute to doesn't need them, and it works just fine. Actually I could argue that having only non-negative numbers make it simpler and faster. [1] https://github.com/cryptocoinjs/ecdsa/blob/master/lib/ecdsa.js#L311 https://github.com/cryptocoinjs/ecdsa/blob/master/lib/ecdsa.... [2] https://code.google.com/p/end-to-end/source/browse/javascript/crypto/e2e/ecc/ https://code.google.com/p/end-to-end/source/browse/javascrip...

dcousens 12y ago
Its not really a bug, the operations after it would still be valid (it is almost immediately reduced to the field order), its just that those parameters would not be akin to the SEC paper specification. I agree that the honus isn't on the users to check that though, so I'm probably going to make a pull request to change this.[1] [1] https://github.com/bitcoinjs/bitcoinjs-lib/pull/250 https://github.com/bitcoinjs/bitcoinjs-lib/pull/250
- cryptbe 12y ago
  What might happen if r = s = -n? I think it's pure luck that this doesn't lead to a signature forgery.
  - dcousens 12y ago
    You're not wrong. Thanks for pointing this out, thankfully the implementation already failed on a negative s value, but you're correct in that it wasn't definitive. I also whole-heartedly agree with your comment about the unnecessary inclusion of a bignum that allows for negative values. The lack of typing in this (and other cases) has lead to several problematic scenarios for users to the point we have littered the code with assertions to enforce whatever we can.

11 ms·

I took a quick look at your implementation of ECDSA and I think it has a bug at line 311 [1]. It looks like I could bypass the check if r or s is negative. One

by cryptbe 12y ago

dcousens 12y ago
Its not really a bug, the operations after it would still be valid (it is almost immediately reduced to the field order), its just that those parameters would not be akin to the SEC paper specification. I agree that the honus isn't on the users to check that though, so I'm probably going to make a pull request to change this.[1] [1] https://github.com/bitcoinjs/bitcoinjs-lib/pull/250 https://github.com/bitcoinjs/bitcoinjs-lib/pull/250
- cryptbe 12y ago
  What might happen if r = s = -n? I think it's pure luck that this doesn't lead to a signature forgery.
  - dcousens 12y ago
    You're not wrong. Thanks for pointing this out, thankfully the implementation already failed on a negative s value, but you're correct in that it wasn't definitive. I also whole-heartedly agree with your comment about the unnecessary inclusion of a bignum that allows for negative values. The lack of typing in this (and other cases) has lead to several problematic scenarios for users to the point we have littered the code with assertions to enforce whatever we can.