diff --git a/lib/internal/crypto/cipher.js b/lib/internal/crypto/cipher.js index 34507dd157b378..20ca1454c871e2 100644 --- a/lib/internal/crypto/cipher.js +++ b/lib/internal/crypto/cipher.js @@ -12,7 +12,7 @@ const { ERR_INVALID_ARG_TYPE, ERR_INVALID_OPT_VALUE } = require('internal/errors').codes; -const { validateString } = require('internal/validators'); +const { validateEncoding, validateString } = require('internal/validators'); const { preparePrivateKey, @@ -161,6 +161,8 @@ Cipher.prototype.update = function update(data, inputEncoding, outputEncoding) { throw invalidArrayBufferView('data', data); } + validateEncoding(data, inputEncoding); + const ret = this[kHandle].update(data, inputEncoding); if (outputEncoding && outputEncoding !== 'buffer') { diff --git a/lib/internal/crypto/hash.js b/lib/internal/crypto/hash.js index 38b125e5f22c2d..7e4a83ca9346ae 100644 --- a/lib/internal/crypto/hash.js +++ b/lib/internal/crypto/hash.js @@ -25,7 +25,8 @@ const { ERR_CRYPTO_HASH_UPDATE_FAILED, ERR_INVALID_ARG_TYPE } = require('internal/errors').codes; -const { validateString, validateUint32 } = require('internal/validators'); +const { validateEncoding, validateString, validateUint32 } = + require('internal/validators'); const { normalizeEncoding } = require('internal/util'); const { isArrayBufferView } = require('internal/util/types'); const LazyTransform = require('internal/streams/lazy_transform'); @@ -61,6 +62,8 @@ Hash.prototype._flush = function _flush(callback) { }; Hash.prototype.update = function update(data, encoding) { + encoding = encoding || getDefaultEncoding(); + const state = this[kState]; if (state[kFinalized]) throw new ERR_CRYPTO_HASH_FINALIZED(); @@ -74,7 +77,9 @@ Hash.prototype.update = function update(data, encoding) { data); } - if (!this[kHandle].update(data, encoding || getDefaultEncoding())) + validateEncoding(data, encoding); + + if (!this[kHandle].update(data, encoding)) throw new ERR_CRYPTO_HASH_UPDATE_FAILED(); return this; }; diff --git a/lib/internal/validators.js b/lib/internal/validators.js index 8f5605dad25fd1..f74338ebcb8cef 100644 --- a/lib/internal/validators.js +++ b/lib/internal/validators.js @@ -9,6 +9,7 @@ const { ERR_UNKNOWN_SIGNAL } } = require('internal/errors'); +const { normalizeEncoding } = require('internal/util'); const { isArrayBufferView } = require('internal/util/types'); @@ -142,11 +143,24 @@ const validateBuffer = hideStackFrames((buffer, name = 'buffer') => { } }); +function validateEncoding(data, encoding) { + const normalizedEncoding = normalizeEncoding(encoding); + const length = data.length; + + if (normalizedEncoding === 'hex' && length % 2 !== 0) { + throw new ERR_INVALID_ARG_VALUE('encoding', encoding, + `is invalid for data of length ${length}`); + } + + // TODO(bnoordhuis) Add BASE64 check? +} + module.exports = { isInt32, isUint32, parseMode, validateBuffer, + validateEncoding, validateInteger, validateInt32, validateUint32, diff --git a/src/node_buffer.cc b/src/node_buffer.cc index e6a88f649895e8..d3a9abc571c42b 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -843,7 +843,7 @@ void IndexOfString(const FunctionCallbackInfo& args) { if (IsBigEndian()) { StringBytes::InlineDecoder decoder; - if (decoder.Decode(env, needle, args[3], UCS2).IsNothing()) return; + if (decoder.Decode(env, needle, enc).IsNothing()) return; const uint16_t* decoded_string = reinterpret_cast(decoder.out()); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 70da2e310ea15c..f1c7d1796acc58 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4317,8 +4317,9 @@ void CipherBase::Update(const FunctionCallbackInfo& args) { // Only copy the data if we have to, because it's a string if (args[0]->IsString()) { StringBytes::InlineDecoder decoder; - if (!decoder.Decode(env, args[0].As(), args[1], UTF8) - .FromMaybe(false)) + enum encoding enc = ParseEncoding(env->isolate(), args[1], UTF8); + + if (decoder.Decode(env, args[0].As(), enc).IsNothing()) return; r = cipher->Update(decoder.out(), decoder.size(), &out); } else { @@ -4501,8 +4502,9 @@ void Hmac::HmacUpdate(const FunctionCallbackInfo& args) { bool r = false; if (args[0]->IsString()) { StringBytes::InlineDecoder decoder; - if (decoder.Decode(env, args[0].As(), args[1], UTF8) - .FromMaybe(false)) { + enum encoding enc = ParseEncoding(env->isolate(), args[1], UTF8); + + if (!decoder.Decode(env, args[0].As(), enc).IsNothing()) { r = hmac->HmacUpdate(decoder.out(), decoder.size()); } } else { @@ -4626,8 +4628,9 @@ void Hash::HashUpdate(const FunctionCallbackInfo& args) { bool r = true; if (args[0]->IsString()) { StringBytes::InlineDecoder decoder; - if (!decoder.Decode(env, args[0].As(), args[1], UTF8) - .FromMaybe(false)) { + enum encoding enc = ParseEncoding(env->isolate(), args[1], UTF8); + + if (decoder.Decode(env, args[0].As(), enc).IsNothing()) { args.GetReturnValue().Set(false); return; } diff --git a/src/string_bytes.cc b/src/string_bytes.cc index 131a0333be22dc..f8d7243e5d6c36 100644 --- a/src/string_bytes.cc +++ b/src/string_bytes.cc @@ -392,15 +392,6 @@ size_t StringBytes::Write(Isolate* isolate, } -bool StringBytes::IsValidString(Local string, - enum encoding enc) { - if (enc == HEX && string->Length() % 2 != 0) - return false; - // TODO(bnoordhuis) Add BASE64 check? - return true; -} - - // Quick and dirty size calculation // Will always be at least big enough, but may have some extra // UTF8 can be as much as 3x the size, Base64 can have 1-2 extra bytes diff --git a/src/string_bytes.h b/src/string_bytes.h index 62a67247375518..6ffdc47db799ae 100644 --- a/src/string_bytes.h +++ b/src/string_bytes.h @@ -37,14 +37,7 @@ class StringBytes { public: inline v8::Maybe Decode(Environment* env, v8::Local string, - v8::Local encoding, - enum encoding _default) { - enum encoding enc = ParseEncoding(env->isolate(), encoding, _default); - if (!StringBytes::IsValidString(string, enc)) { - env->ThrowTypeError("Bad input string"); - return v8::Nothing(); - } - + enum encoding enc) { size_t storage; if (!StringBytes::StorageSize(env->isolate(), string, enc).To(&storage)) return v8::Nothing(); @@ -60,12 +53,6 @@ class StringBytes { inline size_t size() const { return length(); } }; - // Does the string match the encoding? Quick but non-exhaustive. - // Example: a HEX string must have a length that's a multiple of two. - // FIXME(bnoordhuis) IsMaybeValidString()? Naming things is hard... - static bool IsValidString(v8::Local string, - enum encoding enc); - // Fast, but can be 2 bytes oversized for Base64, and // as much as triple UTF-8 strings <= 65536 chars in length static v8::Maybe StorageSize(v8::Isolate* isolate, diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index 9337621d37fcfb..c029ab8c77ad25 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -167,42 +167,66 @@ testImmutability(crypto.getCurves); // Regression tests for https://github.com/nodejs/node-v0.x-archive/pull/5725: // hex input that's not a power of two should throw, not assert in C++ land. -assert.throws(function() { - crypto.createCipher('aes192', 'test').update('0', 'hex'); -}, (err) => { - const errorMessage = - common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/; - // Throws general Error, so there is no opensslErrorStack property. - if ((err instanceof Error) && - errorMessage.test(err) && - err.opensslErrorStack === undefined) { - return true; - } -}); -assert.throws(function() { - crypto.createDecipher('aes192', 'test').update('0', 'hex'); -}, (err) => { - const errorMessage = - common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/; - // Throws general Error, so there is no opensslErrorStack property. - if ((err instanceof Error) && - errorMessage.test(err) && - err.opensslErrorStack === undefined) { - return true; +common.expectsError( + () => crypto.createCipher('aes192', 'test').update('0', 'hex'), + Object.assign( + common.hasFipsCrypto ? + { + code: undefined, + type: Error, + message: /not supported in FIPS mode/, + } : + { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError, + message: "The argument 'encoding' is invalid for data of length 1." + + " Received 'hex'", + }, + { opensslErrorStack: undefined } + ) +); + +common.expectsError( + () => crypto.createDecipher('aes192', 'test').update('0', 'hex'), + Object.assign( + common.hasFipsCrypto ? + { + code: undefined, + type: Error, + message: /not supported in FIPS mode/, + } : + { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError, + message: "The argument 'encoding' is invalid for data of length 1." + + " Received 'hex'", + }, + { opensslErrorStack: undefined } + ) +); + +common.expectsError( + () => crypto.createHash('sha1').update('0', 'hex'), + { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError, + message: "The argument 'encoding' is invalid for data of length 1." + + " Received 'hex'", + opensslErrorStack: undefined } -}); +); -assert.throws(function() { - crypto.createHash('sha1').update('0', 'hex'); -}, (err) => { - // Throws TypeError, so there is no opensslErrorStack property. - if ((err instanceof Error) && - /^TypeError: Bad input string$/.test(err) && - err.opensslErrorStack === undefined) { - return true; +common.expectsError( + () => crypto.createHmac('sha256', 'a secret').update('0', 'hex'), + { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError, + message: "The argument 'encoding' is invalid for data of length 1." + + " Received 'hex'", + opensslErrorStack: undefined } -}); +); assert.throws(function() { const priv = [