Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
hacker112 opened this issue Dec 14, 2016 · 6 comments
Closed

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

hacker112 opened this issue Dec 14, 2016 · 6 comments
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. duplicate Issues and PRs that are duplicates of other issues or PRs. question Issues that look for answers.

Comments

@hacker112
Copy link

hacker112 commented Dec 14, 2016

  • 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');
});
@sam-github sam-github added the crypto Issues and PRs related to the crypto subsystem. label Dec 14, 2016
@addaleax addaleax added duplicate Issues and PRs that are duplicates of other issues or PRs. question Issues that look for answers. labels Dec 14, 2016
@addaleax
Copy link
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!

@sam-github
Copy link
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.

@sam-github sam-github reopened this Dec 14, 2016
@sam-github sam-github added the doc Issues and PRs related to the documentations. label Dec 14, 2016
@addaleax
Copy link
Member

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

@sam-github
Copy link
Contributor

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

@hacker112
Copy link
Author

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

@addaleax
Copy link
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
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. duplicate Issues and PRs that are duplicates of other issues or PRs. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

3 participants