Tests failing for Node 6

[dcousens]

Looks to be an issue with Buffers and utf8/binary again. Did something change in Node 6 that is going to make us incompatible?

@feross any ideas?

[feross]

Link to the failing tests?

[dcousens]

@feross https://travis-ci.org/crypto-browserify/pbkdf2/jobs/139344804

The bug presented is a sudden change in the handling of unicode causing a mismatch of the output result.

To be clear, this doesn't appear to be a bug in https://github.com/feross/buffer, but I had hoped you might know if anything official upstream (aka, the many API deprecations for Buffer in Node 6) was causing this.

[dcousens]

ping @indutny and @dominictarr

[indutny]

Random thought, but could it be related to https://github.com/nodejs/node/pull/7310 cc @mscdex

[feross]

@dcousens Sorry -- I have no idea what this could be.

[dominictarr]

in node@<6 Hash.update(string) defaulted to Hash.update(string, 'binary') depite the fact that everywhere else in node strings are interpreted as utf8 by default. in node@6 this was fixed, breaking everything.

[dominictarr]

it looks like this module is very explicit about encodings... but this module depends on create-hmac which is much more lax. https://github.com/crypto-browserify/createHmac/blob/master/browser.js#L42

my bet is that it's in create-hmac

[dcousens]

@dominictarr that makes sense, except that this is happening in Node 6, so it shouldn't even be using create-hmac/browser?

[dominictarr]

aha. okay so your test cases are assuming the implicit binary encoding.

var crypto = require('crypto')
//from pbkdf2/test/fixtures
var input =    {
      "key": "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about",
      "salt": "mnemonicメートルガバヴァぱばぐゞちぢ十人十色",
      "iterations": 2048,
      "dkLen": 64,
      "results": {
        "sha1": "d85d14adcb7bdb5d976160e504f520a98cf71aca4cd5fceadf37759743bd6e1d2ff78bdd4403552aef7658094384b341ede80fffd334182be076f9d988a0a40f",
        "sha256": "b86b5b900c29ed2724359afd793e10ffc1eb0e7d6f624fc9c85b8ac1785d9a2f0575af52a2338e611f2e6cffdee544adfff6f3d4f43be2ba0e2bd7e917b38a14",
        "sha512": "3a863fa00f2e97a83fa9b18805e0047a6282cbae0ff48438b33a14475771c52d05137daa12e364cb34d84547ac07568b801c5c7f8dd4baaeee18a67a5c6a3377",
        "sha224": "95727793842437774ad9ae27b8154a6f37f208b75a03d3a4d4a2443422bb6bc85efcfa92aa4376926ea89a8f5a63118eecdb58c8ca28ab31007da79437e0a1ef",
        "sha384": "1a7e02e8ba0e357269a55642024b85738b95238d6cdc49bc440204995aefeff499e22cba76d4c7e96b7d4a9596a70e744f53fa94f3547e7dc506fcaf16ceb4a2"
      }
    }

for (var alg in input.results) {
  console.log('EXPECTED', input.results[alg])
//  console.log( pbkdf2.pbkdf2Sync(input.key, input.salt, input.iterations, input.dkLen, alg).toString('hex') )
  console.log( crypto.pbkdf2Sync(new Buffer(input.key, 'binary'), new Buffer(input.salt, 'binary'), input.iterations, input.dkLen, alg).toString('hex') )
  console.log( crypto.pbkdf2Sync(input.key, input.salt, input.iterations, input.dkLen, alg).toString('hex') )
}

node's output fails the tests now, because it uses utf8 encoding, but the tests used binary.

[dominictarr]

output, first line after EXPECTED is with binary, row after that is with utf8, in node@6

EXPECTED d85d14adcb7bdb5d976160e504f520a98cf71aca4cd5fceadf37759743bd6e1d2ff78bdd4403552aef7658094384b341ede80fffd334182be076f9d988a0a40f
d85d14adcb7bdb5d976160e504f520a98cf71aca4cd5fceadf37759743bd6e1d2ff78bdd4403552aef7658094384b341ede80fffd334182be076f9d988a0a40f
7e042a2f41ba17e2235fbc794e22a150816b0f54a1dfe113919fccb7a056066a109385e538f183c92bad896ae8b7d8e0f4cd66df359c77c8c7785cd1001c9a2c
EXPECTED b86b5b900c29ed2724359afd793e10ffc1eb0e7d6f624fc9c85b8ac1785d9a2f0575af52a2338e611f2e6cffdee544adfff6f3d4f43be2ba0e2bd7e917b38a14
b86b5b900c29ed2724359afd793e10ffc1eb0e7d6f624fc9c85b8ac1785d9a2f0575af52a2338e611f2e6cffdee544adfff6f3d4f43be2ba0e2bd7e917b38a14
0b57118f2b6b079d9371c94da3a8315c3ada87a1e819b40c4c4e90b36ff2d3c8fd7555538b5119ac4d3da7844aa4259d92f9dd2188e78ac33c4b08d8e6b5964b
EXPECTED 3a863fa00f2e97a83fa9b18805e0047a6282cbae0ff48438b33a14475771c52d05137daa12e364cb34d84547ac07568b801c5c7f8dd4baaeee18a67a5c6a3377
3a863fa00f2e97a83fa9b18805e0047a6282cbae0ff48438b33a14475771c52d05137daa12e364cb34d84547ac07568b801c5c7f8dd4baaeee18a67a5c6a3377
ba553eedefe76e67e2602dc20184c564010859faada929a090dd2c57aacb204ceefd15404ab50ef3e8dbeae5195aeae64b0def4d2eead1cdc728a33ced520ffd
EXPECTED 95727793842437774ad9ae27b8154a6f37f208b75a03d3a4d4a2443422bb6bc85efcfa92aa4376926ea89a8f5a63118eecdb58c8ca28ab31007da79437e0a1ef
95727793842437774ad9ae27b8154a6f37f208b75a03d3a4d4a2443422bb6bc85efcfa92aa4376926ea89a8f5a63118eecdb58c8ca28ab31007da79437e0a1ef
d76474c525616ce2a527d23df8d6f6fcc4251cc3535cae4e955810a51ead1ec6acbe9c9619187ca5a3c4fd636de5b5fe58d031714731290bbc081dbf0fcb8fc1
EXPECTED 1a7e02e8ba0e357269a55642024b85738b95238d6cdc49bc440204995aefeff499e22cba76d4c7e96b7d4a9596a70e744f53fa94f3547e7dc506fcaf16ceb4a2
1a7e02e8ba0e357269a55642024b85738b95238d6cdc49bc440204995aefeff499e22cba76d4c7e96b7d4a9596a70e744f53fa94f3547e7dc506fcaf16ceb4a2
15010450f456769467e834db7fa93dd9d353e8bb733b63b0621090f96599ac3316908eb64ac9366094f0787cd4bfb2fea25be41dc271a19309710db6144f9b34

[dominictarr]

but with node@5

EXPECTED d85d14adcb7bdb5d976160e504f520a98cf71aca4cd5fceadf37759743bd6e1d2ff78bdd4403552aef7658094384b341ede80fffd334182be076f9d988a0a40f
d85d14adcb7bdb5d976160e504f520a98cf71aca4cd5fceadf37759743bd6e1d2ff78bdd4403552aef7658094384b341ede80fffd334182be076f9d988a0a40f
d85d14adcb7bdb5d976160e504f520a98cf71aca4cd5fceadf37759743bd6e1d2ff78bdd4403552aef7658094384b341ede80fffd334182be076f9d988a0a40f
EXPECTED b86b5b900c29ed2724359afd793e10ffc1eb0e7d6f624fc9c85b8ac1785d9a2f0575af52a2338e611f2e6cffdee544adfff6f3d4f43be2ba0e2bd7e917b38a14
b86b5b900c29ed2724359afd793e10ffc1eb0e7d6f624fc9c85b8ac1785d9a2f0575af52a2338e611f2e6cffdee544adfff6f3d4f43be2ba0e2bd7e917b38a14
b86b5b900c29ed2724359afd793e10ffc1eb0e7d6f624fc9c85b8ac1785d9a2f0575af52a2338e611f2e6cffdee544adfff6f3d4f43be2ba0e2bd7e917b38a14
EXPECTED 3a863fa00f2e97a83fa9b18805e0047a6282cbae0ff48438b33a14475771c52d05137daa12e364cb34d84547ac07568b801c5c7f8dd4baaeee18a67a5c6a3377
3a863fa00f2e97a83fa9b18805e0047a6282cbae0ff48438b33a14475771c52d05137daa12e364cb34d84547ac07568b801c5c7f8dd4baaeee18a67a5c6a3377
3a863fa00f2e97a83fa9b18805e0047a6282cbae0ff48438b33a14475771c52d05137daa12e364cb34d84547ac07568b801c5c7f8dd4baaeee18a67a5c6a3377
EXPECTED 95727793842437774ad9ae27b8154a6f37f208b75a03d3a4d4a2443422bb6bc85efcfa92aa4376926ea89a8f5a63118eecdb58c8ca28ab31007da79437e0a1ef
95727793842437774ad9ae27b8154a6f37f208b75a03d3a4d4a2443422bb6bc85efcfa92aa4376926ea89a8f5a63118eecdb58c8ca28ab31007da79437e0a1ef
95727793842437774ad9ae27b8154a6f37f208b75a03d3a4d4a2443422bb6bc85efcfa92aa4376926ea89a8f5a63118eecdb58c8ca28ab31007da79437e0a1ef
EXPECTED 1a7e02e8ba0e357269a55642024b85738b95238d6cdc49bc440204995aefeff499e22cba76d4c7e96b7d4a9596a70e744f53fa94f3547e7dc506fcaf16ceb4a2
1a7e02e8ba0e357269a55642024b85738b95238d6cdc49bc440204995aefeff499e22cba76d4c7e96b7d4a9596a70e744f53fa94f3547e7dc506fcaf16ceb4a2
1a7e02e8ba0e357269a55642024b85738b95238d6cdc49bc440204995aefeff499e22cba76d4c7e96b7d4a9596a70e744f53fa94f3547e7dc506fcaf16ceb4a2

[indutny]

Yikes, I thought about binary to utf8 migration. Just didn't expect it to surface here just now.

[indutny]

So, just in case, binary encoding strips non-ascii bits of utf8 characters and acts pretty similarly to latin. I would suggest just porting tests to utf8.

[dcousens]

Welp, hope that doesn't impact cause issues in the wild.

[indutny]

@dcousens actually, binary was causing problems in the wild. Many didn't know that it was default and passed utf8 stuff to it, so incorrect hashes were produced.

[dominictarr]

@indutny it might help if the crypto library exported some thing to mark that it uses binary or utf8. this change could easily cause weird problems... say, you can't decrypt your stored private key (i.e. bitcoin wallet anymore)

in secure-scuttlebutt we didn't realize it was binary, but to keep everything running now we must explicitly convert to buffers via binary... utf8 was the right decision, problem is it should have been like that since the start.

[dominictarr]

@indutny so does parsing binary actually throw some bits away? meaning that now there are now collisions?

[dcousens]

@dominictarr I think the binary/utf8 issue was that binary was parsing strings by character (not by byte), then % 256, hence the truncation. AFAIK.

[indutny]

@dominictarr exactly. Collisions for utf8 texts.

[dominictarr]

oh dear. that is actually a lot worse than I thought...

[indutny]

Depends, this was documented since crypto inception.

[dcousens]

@dominictarr well, I think we make the move to utf8 as the default, even if its inconsistent with the previous versions? It may break things, but it'll probably protect more often then not.

We kind of sit in a hard place with crypto-browserify versioning and backwards compatibility heh.

edit: major version?

[dominictarr]

@indutny I'm certainly not saying we should continue to use binary. I'm trying to figure out the best way to migrate an application that didn't realize it used binary encoding to utf8.

secure-scuttlebutt has this for hashes but not signatures (since it uses libsodium for sigs) you could return a message that seems to hash correctly but is has an invalid signature. it would be difficult to weaponize that, but it creates wiggle room for someone to try and I'd be much happier knowing it wasn't possible.

[calvinmetcalf]

the main issue is just a lot of people get this library as a dep of a dep, so if we just upgrade code will break we could do something like create a depreciation warning first and then upgrade it maybe ?

[dominictarr]

hmm, we should probably contact the authors of the those modules, and make sure they know. but the terrible thing is that crypto-browserify is just included transparently in browserify, so we have no idea who is actually using this! This situation would be better if you actually had to manually install the browserify shim you wanted...

another option is we could print a warning if someone passes a string in a way that would have used binary encoding (including a link to this issue, or similar?)

[dcousens]

another option is we could print a warning if someone passes a string in a way that would have used binary encoding (including a link to this issue, or similar?)

So, maybe a temporary version that throws a warning if input is given which if run through "binary" does not equate byte-for-byte the "utf8" representation?