Skip to content

Commit

Permalink
crypto: PBKDF2 works with int not ssize_t
Browse files Browse the repository at this point in the history
Change types of all PBKDF2 params to `int` as they are `int` in `evp.h`.

Check that `raw_keylen` fits into `int` before passing it to OpenSSL.

Fix: #5396
PR-URL: #5397
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Reviewed-By: Ben Noorhduis <info@bnoordhuis.nl>
  • Loading branch information
indutny committed Mar 1, 2016
1 parent 610bd8d commit da3f425
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 31 deletions.
43 changes: 24 additions & 19 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "CNNICHashWhitelist.inc"

#include <errno.h>
#include <limits.h> // INT_MAX
#include <math.h>
#include <stdlib.h>
#include <string.h>
Expand Down Expand Up @@ -4955,12 +4956,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 @@ -4989,31 +4990,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 @@ -5046,13 +5047,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 @@ -5109,10 +5110,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 @@ -5164,12 +5166,15 @@ 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) ||
raw_keylen > INT_MAX) {
type_error = "Bad key length";
goto err;
}

keylen = static_cast<int>(raw_keylen);

if (args[4]->IsString()) {
node::Utf8Value digest_name(env->isolate(), args[4]);
digest = EVP_get_digestbyname(*digest_name);
Expand All @@ -5192,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
21 changes: 9 additions & 12 deletions test/parallel/test-crypto-pbkdf2.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,27 +63,24 @@ assert.throws(function() {
// Should not work with Infinity key length
assert.throws(function() {
crypto.pbkdf2('password', 'salt', 1, Infinity, 'sha256', common.fail);
}, function(err) {
return err instanceof Error && err.message === 'Bad key length';
});
}, /Bad key length/);

// Should not work with negative Infinity key length
assert.throws(function() {
crypto.pbkdf2('password', 'salt', 1, -Infinity, 'sha256', common.fail);
}, function(err) {
return err instanceof Error && err.message === 'Bad key length';
});
}, /Bad key length/);

// Should not work with NaN key length
assert.throws(function() {
crypto.pbkdf2('password', 'salt', 1, NaN, 'sha256', common.fail);
}, function(err) {
return err instanceof Error && err.message === 'Bad key length';
});
}, /Bad key length/);

// Should not work with negative key length
assert.throws(function() {
crypto.pbkdf2('password', 'salt', 1, -1, 'sha256', common.fail);
}, function(err) {
return err instanceof Error && err.message === 'Bad key length';
});
}, /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);
}, /Bad key length/);

5 comments on commit da3f425

@ronkorving
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @bnoordhuis

Reviewed-By: Ben Noorhduis <info@bnoordhuis.nl>

(typo in your name)

@indutny
Copy link
Member Author

@indutny indutny commented on da3f425 Mar 1, 2016

Choose a reason for hiding this comment

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

That came from 1411e0b. I usually copy-paste Reviewed-By

@indutny
Copy link
Member Author

@indutny indutny commented on da3f425 Mar 1, 2016

Choose a reason for hiding this comment

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

And from 4e77a7c

@indutny
Copy link
Member Author

@indutny indutny commented on da3f425 Mar 1, 2016

Choose a reason for hiding this comment

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

And the first one was c98d159

@indutny
Copy link
Member Author

@indutny indutny commented on da3f425 Mar 1, 2016

Choose a reason for hiding this comment

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

Ah no, it was actually myself who did it ef46ca6

Please sign in to comment.