iterate array correctly so that random number is actually used in signing

[ryanxcharles]

The recently refactored signSync function was not working correctly, making signatures insecure. If you signed two different transactions with the same private key, it was possible to rederive your private key. The problem was the "array" being passed to nextBytes had a length set to some value, but all of the values were undefined, so it couldn't be iterated. This meant that after we generated the random bytes, the random bytes were not being copied to the array. The solution is to iterate over a instead, which has all the values defined and thus can be iterated.

Here's the faulty code:

  rng.nextBytes = function(array) {
    var buf = SecureRandom.getRandomBuffer(array.length);
    var a = bufferToArray(SecureRandom.getRandomBuffer(array.length));
    for (var i in array) {
      array[i] = a[i];
    }
  };

Here's the corrected code:

  rng.nextBytes = function(array) {
    var buf = SecureRandom.getRandomBuffer(array.length);
    var a = bufferToArray(SecureRandom.getRandomBuffer(array.length));
    for (var i in a) {
      array[i] = a[i];
    }
  };

I investigated the BigInteger code and found this:

    var x = new Array(), t = a&7;
    x.length = (a>>3)+1;
    b.nextBytes(x);
    if(t > 0) x[0] &= ((1<<t)-1); else x[0] = 0;
    this.fromString(x,256);

BigInteger was generating a blank array, and then setting the length to (a>>3)+1, which is why all the values were undefined when it called nextBytes.

In order to make sure this particular issue doesn't occur again, I've added two new tests: One to make sure that several signatures generated in a row are all unique, and another to make sure that many random byte arrays generated in a row are all unique.

The error was introduced in this PR: #289 Ironically, that PR was trying to increase the security of signatures in the browser by using the SecureRandom class, but actually decreased security instead by not actually using the secure random numbers.

[matiu]

ACK

[jprichardson]

You may want to consider adding RFC 6979 in the near future. It's a secure deterministic way to generate the k value without using random numbers.

See for more info:

• http://tools.ietf.org/html/rfc6979
• https://bitcointalk.org/index.php?topic=285142.40

[ryanxcharles]

Thanks @jprichardson, we'll take a look at that.