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: PBKDF2 works with int not ssize_t #5397

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 26 additions & 19 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4953,12 +4953,12 @@ class PBKDF2Request : public AsyncWrap {
PBKDF2Request(Environment* env,
Local<Object> object,
const EVP_MD* digest,
ssize_t passlen,
int passlen,
char* pass,
ssize_t saltlen,
int saltlen,
char* salt,
ssize_t iter,
ssize_t keylen)
int iter,
int keylen)
: AsyncWrap(env, object, AsyncWrap::PROVIDER_CRYPTO),
digest_(digest),
error_(0),
Expand Down Expand Up @@ -4987,31 +4987,31 @@ class PBKDF2Request : public AsyncWrap {
return digest_;
}

inline ssize_t passlen() const {
inline int passlen() const {
return passlen_;
}

inline char* pass() const {
return pass_;
}

inline ssize_t saltlen() const {
inline int saltlen() const {
return saltlen_;
}

inline char* salt() const {
return salt_;
}

inline ssize_t keylen() const {
inline int keylen() const {
return keylen_;
}

inline char* key() const {
return key_;
}

inline ssize_t iter() const {
inline int iter() const {
return iter_;
}

Expand Down Expand Up @@ -5044,13 +5044,13 @@ class PBKDF2Request : public AsyncWrap {
private:
const EVP_MD* digest_;
int error_;
ssize_t passlen_;
int passlen_;
char* pass_;
ssize_t saltlen_;
int saltlen_;
char* salt_;
ssize_t keylen_;
int keylen_;
char* key_;
ssize_t iter_;
int iter_;
};


Expand Down Expand Up @@ -5107,10 +5107,11 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {
const char* type_error = nullptr;
char* pass = nullptr;
char* salt = nullptr;
ssize_t passlen = -1;
ssize_t saltlen = -1;
double keylen = -1;
ssize_t iter = -1;
int passlen = -1;
int saltlen = -1;
double raw_keylen = -1;
int keylen = -1;
int iter = -1;
PBKDF2Request* req = nullptr;
Local<Object> obj;

Expand Down Expand Up @@ -5162,8 +5163,14 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {
goto err;
}

keylen = args[3]->NumberValue();
if (keylen < 0 || isnan(keylen) || isinf(keylen)) {
raw_keylen = args[3]->NumberValue();
if (raw_keylen < 0.0 || isnan(raw_keylen) || isinf(raw_keylen)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably write this as:

if (!std::isfinite(raw_keylen) || raw_keylen < 0 || raw_keylen > INT_MAX) {
  // ...
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

type_error = "Bad key length";
goto err;
}

keylen = static_cast<int>(raw_keylen);
if (keylen < 0) {
type_error = "Bad key length";
goto err;
}
Expand All @@ -5190,7 +5197,7 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {
saltlen,
salt,
iter,
static_cast<ssize_t>(keylen));
keylen);

if (args[5]->IsFunction()) {
obj->Set(env->ondone_string(), args[5]);
Expand Down
7 changes: 7 additions & 0 deletions test/parallel/test-crypto-pbkdf2.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,10 @@ assert.throws(function() {
}, function(err) {
return err instanceof Error && err.message === 'Bad key length';
});

// Should not work with key length that does not fit into 32 signed bits
assert.throws(function() {
crypto.pbkdf2('password', 'salt', 1, 4073741824, 'sha256', common.fail);
}, function(err) {
return err instanceof Error && err.message === 'Bad key length';
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can condense the check callback to just /Bad key length/.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.