Skip to content

crypto.pbkdf2Sync can't handle non ASCII character in Node 6.9.2 #10265

Description

@hacker112
  • Version: v6.0.0, v6.9.2, v7.2.1
  • Platform: Mac OS X 10.11.6 and Ubuntu 16.04
  • Subsystem: crypto.pbkdf2Sync

After upgrading our servers from Node 4 to Node 6. I was not able to login with my password anymore, while my colleagues had no problem logging in. After some research I found that since my password contained the letter 'ö' the crypto.pbkdf2Sync failed to create the same hash in Node 6 as in Node 4. So it is broken for non ASCII characters like 'åäö'.

I made the following test that shows that the error started in version 6.0.0 of node.

it('should return same hash in node 6 and 4 for non ASCII characters', function () {
    var crypto = require('crypto');

    var iterations = 1000,
        keylen = 64,
        salt = '8320c789c1869574c159c9758db370855a00cf987ebefaa240649139e53f8066',
        password;

    //
    // ASCII Characters
    //
    password = 'abc';
    var passwordHash1 = crypto.pbkdf2Sync(password, salt, iterations, keylen, 'sha1').toString('hex');
    console.log('hash:', passwordHash1);

    // GOOD
    // v4.7.0
    // hash: eb29636dc841231b3300a66da04c1e46007a63e5933783daca0e96ed6e4a98431a7c8d59b29146edca0aa8d40a8381e0de72a9a857993a3283494c93db33967b
    // v6.0.0
    // hash: eb29636dc841231b3300a66da04c1e46007a63e5933783daca0e96ed6e4a98431a7c8d59b29146edca0aa8d40a8381e0de72a9a857993a3283494c93db33967b
    // v6.9.2
    // hash: eb29636dc841231b3300a66da04c1e46007a63e5933783daca0e96ed6e4a98431a7c8d59b29146edca0aa8d40a8381e0de72a9a857993a3283494c93db33967b

    expect(passwordHash1).toBe('eb29636dc841231b3300a66da04c1e46007a63e5933783daca0e96ed6e4a98431a7c8d59b29146edca0aa8d40a8381e0de72a9a857993a3283494c93db33967b');

    //
    // Non ASCII Characters
    //
    password = 'åäö';
    var passwordHash2 = crypto.pbkdf2Sync(password, salt, iterations, keylen, 'sha1').toString('hex');

    console.log('hash:', passwordHash2);
    // GOOD
    // v4.7.0
    // hash: d29871ab324d9bbcd868185d74d205253acc45620585a44cd3e95cd53769fb3cff88f4df3dc971adf32acd25b9ec5dde3e43c7ef50d59865db6458897d9d22ee
    // v5.12.0
    // hash: d29871ab324d9bbcd868185d74d205253acc45620585a44cd3e95cd53769fb3cff88f4df3dc971adf32acd25b9ec5dde3e43c7ef50d59865db6458897d9d22ee

    // BAD
    // v6.0.0
    // hash: fdb431352dd40e3ffe8e9e6fb725cd150d85ea3e41bb34fb3b3b6355324660a97cd63251628c30219ad9707dcabc316c22e4dda7a7b44ed61f43a252bee5595b
    // v6.9.2
    // hash: fdb431352dd40e3ffe8e9e6fb725cd150d85ea3e41bb34fb3b3b6355324660a97cd63251628c30219ad9707dcabc316c22e4dda7a7b44ed61f43a252bee5595b
    // v7.2.1
    // hash: fdb431352dd40e3ffe8e9e6fb725cd150d85ea3e41bb34fb3b3b6355324660a97cd63251628c30219ad9707dcabc316c22e4dda7a7b44ed61f43a252bee5595b

    expect(passwordHash2).toBe('d29871ab324d9bbcd868185d74d205253acc45620585a44cd3e95cd53769fb3cff88f4df3dc971adf32acd25b9ec5dde3e43c7ef50d59865db6458897d9d22ee');
});

Activity

  1. added
    cryptoIssues and PRs related to the crypto subsystem.
    on Dec 14, 2016
  2. added
    duplicateIssues and PRs that are duplicates of other issues or PRs.
    questionIssues asking questions about Node.js.
    on Dec 14, 2016
  3. addaleax commented on Dec 14, 2016

    @addaleax
    Member

    Yes, the default encoding for the crypto methods changed in v6.x from latin1/binary to utf8. If you know that you rely on a specific encoding, you might want to pass in a Buffer to pbkdf2Sync (e.g. pbkdf2Sync(Buffer.from(password, "latin1"), …)).

    I’m closing this as this is expected behaviour, but please feel free to ask follow-up questions!

  4. sam-github commented on Dec 14, 2016

    @sam-github
    Contributor

    @addaleax shouldn't this be documented? https://nodejs.org/api/crypto.html#crypto_crypto_pbkdf2_password_salt_iterations_keylen_digest_callback says it was added in 0.5.5, it doesn't say it was changed in 6.x.

  5. added
    docIssues and PRs related to Node.js documentation.
    on Dec 14, 2016
  6. addaleax commented on Dec 14, 2016

    @addaleax
    Member

    @sam-github Yeah, maybe there should be a mention of that. It applies to virtually all crypto functions, though.

  7. sam-github commented on Dec 14, 2016

    @sam-github
    Contributor

    Then docs are needed for all crypto functions, either in each one, or once at the top of the docs.

  8. hacker112 commented on Dec 14, 2016

    @hacker112
    Author

    I looked in the changelog and tried to find if any changes was made to "pbkdf2Sync", but I did not look for crypto.

  9. addaleax commented on Apr 29, 2017

    @addaleax
    Member

    Information about the crypto encoding change has been added in d27c983, I’ll closed this as a fixed issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    cryptoIssues and PRs related to the crypto subsystem.docIssues and PRs related to Node.js documentation.duplicateIssues and PRs that are duplicates of other issues or PRs.questionIssues asking questions about Node.js.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions