From 82df345fbba15ba42eeb9955f3bc76609c86e677 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sun, 21 Oct 2012 23:55:52 +0200 Subject: [PATCH 1/6] test: add diffie-hellman regression test Exercises the error path in DiffieHellman::ComputeSecret() in src/node_crypto.cc --- test/simple/test-crypto.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/simple/test-crypto.js b/test/simple/test-crypto.js index de8c1a9d638..535af760fac 100644 --- a/test/simple/test-crypto.js +++ b/test/simple/test-crypto.js @@ -530,6 +530,10 @@ var secret3 = dh3.computeSecret(key2, 'hex', 'base64'); assert.equal(secret1, secret3); +assert.throws(function() { + dh3.computeSecret(''); +}, /key is too small/i); + // https://github.com/joyent/node/issues/2338 assert.throws(function() { var p = 'FFFFFFFFFFFFFFFFC90FDAA22168C234C4C6628B80DC1CD129024E088A67CC74' + From de18e29784b5f10f695d0859b43c81f41d743e85 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 22 Oct 2012 00:03:47 +0200 Subject: [PATCH 2/6] crypto: fix DH 1 byte buffer underflow Passing a bad key to DiffieHellman::ComputeSecret() made it zero the byte before the heap allocated buffer due to an erroneous size calculation. --- src/node_crypto.cc | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 04954328dfc..69ccfd26280 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3960,17 +3960,6 @@ class DiffieHellman : public ObjectWrap { key, diffieHellman->dh); BN_free(key); - Local outString; - - // DH_size returns number of bytes in a prime number - // DH_compute_key returns number of bytes in a remainder of exponent, which - // may have less bytes than a prime number. Therefore add 0-padding to the - // allocated buffer. - if (size != dataSize) { - assert(dataSize > size); - memset(data + size, 0, dataSize - size); - } - if (size == -1) { int checkResult; if (!DH_check_pub_key(diffieHellman->dh, key, &checkResult)) { @@ -3988,14 +3977,27 @@ class DiffieHellman : public ObjectWrap { } else { return ThrowException(Exception::Error(String::New("Invalid key"))); } + } + + assert(size >= 0); + + // DH_size returns number of bytes in a prime number + // DH_compute_key returns number of bytes in a remainder of exponent, which + // may have less bytes than a prime number. Therefore add 0-padding to the + // allocated buffer. + if (size != dataSize) { + assert(dataSize > size); + memset(data + size, 0, dataSize - size); + } + + Local outString; + + if (args.Length() > 2 && args[2]->IsString()) { + outString = EncodeWithEncoding(args[2], data, dataSize); + } else if (args.Length() > 1 && args[1]->IsString()) { + outString = EncodeWithEncoding(args[1], data, dataSize); } else { - if (args.Length() > 2 && args[2]->IsString()) { - outString = EncodeWithEncoding(args[2], data, dataSize); - } else if (args.Length() > 1 && args[1]->IsString()) { - outString = EncodeWithEncoding(args[1], data, dataSize); - } else { - outString = Encode(data, dataSize, BINARY); - } + outString = Encode(data, dataSize, BINARY); } delete[] data; From 844a0058d0c6815308a0e68192df6a1d6213c68e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 22 Oct 2012 00:18:14 +0200 Subject: [PATCH 3/6] crypto: fix DH use-after-free and memory leak Fix a use-after-free bug and a memory leak in the error path of DiffieHellman::ComputeSecret(). * the BIGNUM key was used after being freed with BN_free(). * the output buffer was not freed --- src/node_crypto.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 69ccfd26280..b85e5e57e77 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3958,11 +3958,16 @@ class DiffieHellman : public ObjectWrap { int size = DH_compute_key(reinterpret_cast(data), key, diffieHellman->dh); - BN_free(key); if (size == -1) { int checkResult; - if (!DH_check_pub_key(diffieHellman->dh, key, &checkResult)) { + int checked; + + checked = DH_check_pub_key(diffieHellman->dh, key, &checkResult); + BN_free(key); + delete[] data; + + if (!checked) { return ThrowException(Exception::Error(String::New("Invalid key"))); } else if (checkResult) { if (checkResult & DH_CHECK_PUBKEY_TOO_SMALL) { @@ -3979,6 +3984,7 @@ class DiffieHellman : public ObjectWrap { } } + BN_free(key); assert(size >= 0); // DH_size returns number of bytes in a prime number From 9fa953d3e7a864cfce4a69f47cca27d21c124672 Mon Sep 17 00:00:00 2001 From: "yangguo@chromium.org" Date: Tue, 23 Oct 2012 13:04:05 +0000 Subject: [PATCH 4/6] v8: use correct timezone information on Solaris MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `timezone` variable contains the difference, in seconds, between UTC and local standard time (see `man 3 localtime` on Solaris). Call to `tzset` is required to apply contents of `TZ` variable to `timezone` variable. BUG=v8:2064 Review URL: https://chromiumcodereview.appspot.com/10967066 Patch from Maciej Małecki . This is a back-port of upstream commit r12802. --- deps/v8/src/platform-solaris.cc | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/deps/v8/src/platform-solaris.cc b/deps/v8/src/platform-solaris.cc index 4248ea214fa..07718fe50b9 100644 --- a/deps/v8/src/platform-solaris.cc +++ b/deps/v8/src/platform-solaris.cc @@ -125,12 +125,8 @@ const char* OS::LocalTimezone(double time) { double OS::LocalTimeOffset() { - // On Solaris, struct tm does not contain a tm_gmtoff field. - time_t utc = time(NULL); - ASSERT(utc != -1); - struct tm* loc = localtime(&utc); - ASSERT(loc != NULL); - return static_cast((mktime(loc) - utc) * msPerSecond); + tzset(); + return -static_cast(timezone * msPerSecond); } From 49f0f618a92bf27caad000545bc1f460574a7504 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Tue, 23 Oct 2012 16:43:51 +1100 Subject: [PATCH 5/6] typed arrays: use `signed char` for signed int8s The C standard allows plain `char` to be unsigned. The build environment at Google trips this issue. --- src/v8_typed_array.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/v8_typed_array.cc b/src/v8_typed_array.cc index 4a76f970598..d1437cdcdf0 100644 --- a/src/v8_typed_array.cc +++ b/src/v8_typed_array.cc @@ -497,7 +497,7 @@ v8::Handle cTypeToValue(unsigned char val) { } template <> -v8::Handle cTypeToValue(char val) { +v8::Handle cTypeToValue(signed char val) { return v8::Integer::New(val); } @@ -543,7 +543,7 @@ unsigned char valueToCType(v8::Handle value) { } template <> -char valueToCType(v8::Handle value) { +signed char valueToCType(v8::Handle value) { return value->Int32Value(); } @@ -758,7 +758,7 @@ class DataView { } static v8::Handle getInt8(const v8::Arguments& args) { - return getGeneric(args); + return getGeneric(args); } static v8::Handle getUint16(const v8::Arguments& args) { @@ -790,7 +790,7 @@ class DataView { } static v8::Handle setInt8(const v8::Arguments& args) { - return setGeneric(args); + return setGeneric(args); } static v8::Handle setUint16(const v8::Arguments& args) { From b6b881378aec979efb3859ceb3b025d6cb262f25 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 23 Oct 2012 17:07:23 +0200 Subject: [PATCH 6/6] test: add typed arrays regression test Ensure that uint8 values >= 128 are correctly promoted to int8 <= -1. --- test/simple/test-typed-arrays.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/simple/test-typed-arrays.js b/test/simple/test-typed-arrays.js index 803f18a7293..00bf613543a 100644 --- a/test/simple/test-typed-arrays.js +++ b/test/simple/test-typed-arrays.js @@ -174,3 +174,11 @@ uint8c.set(1, 260); assert.equal(uint8c[0], 0); assert.equal(uint8c[1], 255); + +(function() { + var numbers = []; + for (var i = 128; i <= 255; ++i) numbers.push(i); + var array = new Uint8Array(numbers); + var view = new DataView(array.buffer); + for (var i = 128; i <= 255; ++i) assert.equal(view.getInt8(i - 128), i - 256); +})();