From 0d5f47a97609643f491e1ff523dfbf0082d16ce0 Mon Sep 17 00:00:00 2001
From: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Date: Wed, 11 Nov 2015 20:54:03 +0530
Subject: [PATCH] buffer: throw if string is not a valid HEX string

As it is, if an invalid HEX string is passed to `Buffer` constructor,
it will only use the valid HEX values and ignore the rest. But, it also
throws an error when the length of the string is odd in length. This
patch throws an error if the string is not a valid HEX string.

Fixes: https://github.com/nodejs/node/issues/3770
---
 src/node_buffer.cc           |  8 +++++---
 src/string_bytes.cc          | 27 ++++++++++++++++++++++++++-
 test/parallel/test-buffer.js |  9 +++++++++
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/src/node_buffer.cc b/src/node_buffer.cc
index 988e41dbc9aa22..4312ed4f1cd872 100644
--- a/src/node_buffer.cc
+++ b/src/node_buffer.cc
@@ -622,9 +622,6 @@ void StringWrite(const FunctionCallbackInfo<Value>& args) {
 
   Local<String> str = args[0]->ToString(env->isolate());
 
-  if (encoding == HEX && str->Length() % 2 != 0)
-    return env->ThrowTypeError("Invalid hex string");
-
   size_t offset;
   size_t max_length;
 
@@ -639,6 +636,11 @@ void StringWrite(const FunctionCallbackInfo<Value>& args) {
   if (offset >= ts_obj_length)
     return env->ThrowRangeError("Offset is out of bounds");
 
+  if (StringBytes::IsValidString(env->isolate(), str, encoding) == false) {
+    if (encoding == HEX)
+      return env->ThrowTypeError("Invalid hex string");
+  }
+
   uint32_t written = StringBytes::Write(env->isolate(),
                                         ts_obj_data + offset,
                                         max_length,
diff --git a/src/string_bytes.cc b/src/string_bytes.cc
index a916caf75e8960..d49bf337afe87e 100644
--- a/src/string_bytes.cc
+++ b/src/string_bytes.cc
@@ -451,11 +451,36 @@ size_t StringBytes::Write(Isolate* isolate,
   return nbytes;
 }
 
+template <typename Type>
+bool IsValidHexString(Isolate* isolate, const Type* str, const size_t len) {
+  for (size_t i = 0; i < len; i += 1) {
+    if (!((str[i] >= '0' && str[i] <= '9') ||
+          (str[i] >= 'a' && str[i] <= 'f') ||
+          (str[i] >= 'A' && str[i] <= 'F')))
+      return false;
+  }
+  return true;
+}
+
+bool IsValidHexString(Isolate* isolate, Local<String> string) {
+  if (string->Length() % 2 != 0)
+    return false;
+
+  const char* str = nullptr;
+  size_t n = 0;
+  bool external = StringBytes::GetExternalParts(isolate, string, &str, &n);
+
+  if (external)
+    return IsValidHexString(isolate, str, n);
+
+  String::Value value(string);
+  return IsValidHexString(isolate, *value, value.length());
+}
 
 bool StringBytes::IsValidString(Isolate* isolate,
                                 Local<String> string,
                                 enum encoding enc) {
-  if (enc == HEX && string->Length() % 2 != 0)
+  if (enc == HEX && IsValidHexString(isolate, string) == false)
     return false;
   // TODO(bnoordhuis) Add BASE64 check?
   return true;
diff --git a/test/parallel/test-buffer.js b/test/parallel/test-buffer.js
index 6ab84fd8809289..6d4a6e20b34b3e 100644
--- a/test/parallel/test-buffer.js
+++ b/test/parallel/test-buffer.js
@@ -741,6 +741,15 @@ for (let i = 0; i < 256; i++) {
   assert.equal(b2, b4);
 }
 
+// invalid HEX strings should throw an error
+assert.throws(() => new Buffer('xx', 'hex'), /TypeError: Invalid hex string/);
+// following will fail even though it has zero as the first character, because
+// `x` is an invalid hexa-decimal value
+assert.throws(() => new Buffer('0x', 'hex'), /TypeError: Invalid hex string/);
+// following invalid HEX strings will fail because of the odd length
+assert.throws(() => new Buffer('0', 'hex'), /TypeError: Invalid hex string/);
+assert.throws(() => new Buffer('000', 'hex'), /TypeError: Invalid hex string/);
+
 function buildBuffer(data) {
   if (Array.isArray(data)) {
     var buffer = new Buffer(data.length);