Skip to content

Commit 9d4c5a1

Browse files
felixgery
felixge
authored and
ry
committed
Crypto update should only accept strings / buffers
I have seen a lot of people trying to pass objects to crypto's update functions, assuming that it would somehow serialize the object before hashing. In reality, the object was converted to '[object Object]' which was then hashed, without any error message showing. This patch modifies the DecodeBytes function (used exclusively by crypto at this point) to complain when receiving anything but a string or buffer. Overall this should be a less-suprising, more robust behavior.
1 parent 2a05fe7 commit 9d4c5a1

2 files changed

Lines changed: 28 additions & 2 deletions

File tree

‎src/node_crypto.cc‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@
1616
# define OPENSSL_CONST
1717
#endif
1818

19+
#define ASSERT_IS_STRING_OR_BUFFER(val) \
20+
if (!val->IsString() && !Buffer::HasInstance(val)) { \
21+
return ThrowException(Exception::TypeError(String::New("Not a string or buffer"))); \
22+
}
23+
1924
namespace node {
2025
namespace crypto {
2126

@@ -1420,6 +1425,7 @@ class Cipher : public ObjectWrap {
14201425
"Must give cipher-type, key")));
14211426
}
14221427

1428+
ASSERT_IS_STRING_OR_BUFFER(args[1]);
14231429
ssize_t key_buf_len = DecodeBytes(args[1], BINARY);
14241430

14251431
if (key_buf_len < 0) {
@@ -1456,13 +1462,16 @@ class Cipher : public ObjectWrap {
14561462
return ThrowException(Exception::Error(String::New(
14571463
"Must give cipher-type, key, and iv as argument")));
14581464
}
1465+
1466+
ASSERT_IS_STRING_OR_BUFFER(args[1]);
14591467
ssize_t key_len = DecodeBytes(args[1], BINARY);
14601468

14611469
if (key_len < 0) {
14621470
Local<Value> exception = Exception::TypeError(String::New("Bad argument"));
14631471
return ThrowException(exception);
14641472
}
14651473

1474+
ASSERT_IS_STRING_OR_BUFFER(args[2]);
14661475
ssize_t iv_len = DecodeBytes(args[2], BINARY);
14671476

14681477
if (iv_len < 0) {
@@ -1492,12 +1501,13 @@ class Cipher : public ObjectWrap {
14921501
return args.This();
14931502
}
14941503

1495-
14961504
static Handle<Value> CipherUpdate(const Arguments& args) {
14971505
Cipher *cipher = ObjectWrap::Unwrap<Cipher>(args.This());
14981506

14991507
HandleScope scope;
15001508

1509+
ASSERT_IS_STRING_OR_BUFFER(args[0]);
1510+
15011511
enum encoding enc = ParseEncoding(args[1]);
15021512
ssize_t len = DecodeBytes(args[0], enc);
15031513

@@ -1781,6 +1791,7 @@ class Decipher : public ObjectWrap {
17811791
"Must give cipher-type, key as argument")));
17821792
}
17831793

1794+
ASSERT_IS_STRING_OR_BUFFER(args[1]);
17841795
ssize_t key_len = DecodeBytes(args[1], BINARY);
17851796

17861797
if (key_len < 0) {
@@ -1818,13 +1829,15 @@ class Decipher : public ObjectWrap {
18181829
"Must give cipher-type, key, and iv as argument")));
18191830
}
18201831

1832+
ASSERT_IS_STRING_OR_BUFFER(args[1]);
18211833
ssize_t key_len = DecodeBytes(args[1], BINARY);
18221834

18231835
if (key_len < 0) {
18241836
Local<Value> exception = Exception::TypeError(String::New("Bad argument"));
18251837
return ThrowException(exception);
18261838
}
18271839

1840+
ASSERT_IS_STRING_OR_BUFFER(args[2]);
18281841
ssize_t iv_len = DecodeBytes(args[2], BINARY);
18291842

18301843
if (iv_len < 0) {
@@ -1859,6 +1872,8 @@ class Decipher : public ObjectWrap {
18591872

18601873
Decipher *cipher = ObjectWrap::Unwrap<Decipher>(args.This());
18611874

1875+
ASSERT_IS_STRING_OR_BUFFER(args[0]);
1876+
18621877
ssize_t len = DecodeBytes(args[0], BINARY);
18631878
if (len < 0) {
18641879
return ThrowException(Exception::Error(String::New(
@@ -2160,6 +2175,7 @@ class Hmac : public ObjectWrap {
21602175
"Must give hashtype string as argument")));
21612176
}
21622177

2178+
ASSERT_IS_STRING_OR_BUFFER(args[1]);
21632179
ssize_t len = DecodeBytes(args[1], BINARY);
21642180

21652181
if (len < 0) {
@@ -2189,6 +2205,7 @@ class Hmac : public ObjectWrap {
21892205

21902206
HandleScope scope;
21912207

2208+
ASSERT_IS_STRING_OR_BUFFER(args[0]);
21922209
enum encoding enc = ParseEncoding(args[1]);
21932210
ssize_t len = DecodeBytes(args[0], enc);
21942211

@@ -2336,6 +2353,7 @@ class Hash : public ObjectWrap {
23362353

23372354
Hash *hash = ObjectWrap::Unwrap<Hash>(args.This());
23382355

2356+
ASSERT_IS_STRING_OR_BUFFER(args[0]);
23392357
enum encoding enc = ParseEncoding(args[1]);
23402358
ssize_t len = DecodeBytes(args[0], enc);
23412359

@@ -2527,6 +2545,7 @@ class Sign : public ObjectWrap {
25272545

25282546
HandleScope scope;
25292547

2548+
ASSERT_IS_STRING_OR_BUFFER(args[0]);
25302549
enum encoding enc = ParseEncoding(args[1]);
25312550
ssize_t len = DecodeBytes(args[0], enc);
25322551

@@ -2573,6 +2592,7 @@ class Sign : public ObjectWrap {
25732592
md_len = 8192; // Maximum key size is 8192 bits
25742593
md_value = new unsigned char[md_len];
25752594

2595+
ASSERT_IS_STRING_OR_BUFFER(args[0]);
25762596
ssize_t len = DecodeBytes(args[0], BINARY);
25772597

25782598
if (len < 0) {
@@ -2740,6 +2760,7 @@ class Verify : public ObjectWrap {
27402760

27412761
Verify *verify = ObjectWrap::Unwrap<Verify>(args.This());
27422762

2763+
ASSERT_IS_STRING_OR_BUFFER(args[0]);
27432764
enum encoding enc = ParseEncoding(args[1]);
27442765
ssize_t len = DecodeBytes(args[0], enc);
27452766

@@ -2778,6 +2799,7 @@ class Verify : public ObjectWrap {
27782799

27792800
Verify *verify = ObjectWrap::Unwrap<Verify>(args.This());
27802801

2802+
ASSERT_IS_STRING_OR_BUFFER(args[0]);
27812803
ssize_t klen = DecodeBytes(args[0], BINARY);
27822804

27832805
if (klen < 0) {
@@ -2789,7 +2811,7 @@ class Verify : public ObjectWrap {
27892811
ssize_t kwritten = DecodeWrite(kbuf, klen, args[0], BINARY);
27902812
assert(kwritten == klen);
27912813

2792-
2814+
ASSERT_IS_STRING_OR_BUFFER(args[1]);
27932815
ssize_t hlen = DecodeBytes(args[1], BINARY);
27942816

27952817
if (hlen < 0) {

‎test/simple/test-crypto.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,3 +119,7 @@ txt += decipher.final('utf8');
119119

120120
assert.equal(txt, plaintext, 'encryption and decryption with key and iv');
121121

122+
// update() should only take buffers / strings
123+
assert.throws(function() {
124+
crypto.createHash('sha1').update({foo: 'bar'});
125+
}, /string or buffer/);

0 commit comments

Comments
 (0)