From 2a4ec45ab1085e7d00c39f3f8ce5e4850c9b9cb9 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Thu, 18 Jun 2015 14:07:22 -0600 Subject: [PATCH] buffer: prevent abort on bad proto If an object's prototype is munged it's possible to bypass the instanceof check and cause the application to abort. Instead now use HasInstance() to verify that the object is a Buffer, and throw if not. This check will not work for JS only methods. So while the application won't abort, it also won't throw. In order to properly throw in all cases with toString() the JS optimization of checking that length is zero has been removed. In its place the native methods will now return early if a zero length string is detected. --- lib/buffer.js | 2 -- src/node_buffer.cc | 36 ++++++++++++++++--- test/parallel/test-buffer-fakes.js | 56 ++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 6 deletions(-) create mode 100644 test/parallel/test-buffer-fakes.js diff --git a/lib/buffer.js b/lib/buffer.js index 4625515c63c91b..6732ecdee127ce 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -380,8 +380,6 @@ function slowToString(encoding, start, end) { Buffer.prototype.toString = function() { const length = this.length | 0; - if (length === 0) - return ''; if (arguments.length === 0) return this.utf8Slice(0, length); return slowToString.apply(this, arguments); diff --git a/src/node_buffer.cc b/src/node_buffer.cc index fca08599e50feb..5133140a8d0d23 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -18,6 +18,12 @@ if (!(r)) return env->ThrowRangeError("out of range index"); \ } while (0) +#define THROW_AND_RETURN_UNLESS_BUFFER(env, obj) \ + do { \ + if (!HasInstance(obj)) \ + return env->ThrowTypeError("argument should be a Buffer"); \ + } while (0) + #define ARGS_THIS(argT) \ Local obj = argT; \ size_t obj_length = obj->GetIndexedPropertiesExternalArrayDataLength(); \ @@ -223,7 +229,12 @@ template void StringSlice(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_UNLESS_BUFFER(env, args.This()); ARGS_THIS(args.This()) + + if (obj_length == 0) + return args.GetReturnValue().SetEmptyString(); + SLICE_START_END(args[0], args[1], obj_length) args.GetReturnValue().Set( @@ -235,7 +246,12 @@ template <> void StringSlice(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_UNLESS_BUFFER(env, args.This()); ARGS_THIS(args.This()) + + if (obj_length == 0) + return args.GetReturnValue().SetEmptyString(); + SLICE_START_END(args[0], args[1], obj_length) length /= 2; @@ -306,8 +322,9 @@ void Copy(const FunctionCallbackInfo &args) { if (!HasInstance(args[0])) return env->ThrowTypeError("first arg should be a Buffer"); - Local target = args[0]->ToObject(env->isolate()); + Local target = args[0].As(); + THROW_AND_RETURN_UNLESS_BUFFER(env, args.This()); ARGS_THIS(args.This()) size_t target_length = target->GetIndexedPropertiesExternalArrayDataLength(); char* target_data = static_cast( @@ -340,6 +357,7 @@ void Copy(const FunctionCallbackInfo &args) { void Fill(const FunctionCallbackInfo& args) { + THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); ARGS_THIS(args[0].As()) size_t start = args[2]->Uint32Value(); @@ -383,6 +401,7 @@ template void StringWrite(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_UNLESS_BUFFER(env, args.This()); ARGS_THIS(args.This()) if (!args[0]->IsString()) @@ -459,6 +478,7 @@ static inline void Swizzle(char* start, unsigned int len) { template void ReadFloatGeneric(const FunctionCallbackInfo& args) { + THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); ARGS_THIS(args[0].As()); uint32_t offset = args[1]->Uint32Value(); @@ -522,21 +542,25 @@ uint32_t WriteFloatGeneric(const FunctionCallbackInfo& args) { void WriteFloatLE(const FunctionCallbackInfo& args) { + THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); args.GetReturnValue().Set(WriteFloatGeneric(args)); } void WriteFloatBE(const FunctionCallbackInfo& args) { + THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); args.GetReturnValue().Set(WriteFloatGeneric(args)); } void WriteDoubleLE(const FunctionCallbackInfo& args) { + THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); args.GetReturnValue().Set(WriteFloatGeneric(args)); } void WriteDoubleBE(const FunctionCallbackInfo& args) { + THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); args.GetReturnValue().Set(WriteFloatGeneric(args)); } @@ -550,6 +574,10 @@ void ByteLengthUtf8(const FunctionCallbackInfo &args) { void Compare(const FunctionCallbackInfo &args) { + Environment* env = Environment::GetCurrent(args); + THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]); + THROW_AND_RETURN_UNLESS_BUFFER(env, args[1]); + Local obj_a = args[0].As(); char* obj_a_data = static_cast(obj_a->GetIndexedPropertiesExternalArrayData()); @@ -599,10 +627,10 @@ int32_t IndexOf(const char* haystack, void IndexOfString(const FunctionCallbackInfo& args) { - ASSERT(args[0]->IsObject()); ASSERT(args[1]->IsString()); ASSERT(args[2]->IsNumber()); + THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); ARGS_THIS(args[0].As()); node::Utf8Value str(args.GetIsolate(), args[1]); int32_t offset_i32 = args[2]->Int32Value(); @@ -630,10 +658,10 @@ void IndexOfString(const FunctionCallbackInfo& args) { void IndexOfBuffer(const FunctionCallbackInfo& args) { - ASSERT(args[0]->IsObject()); ASSERT(args[1]->IsObject()); ASSERT(args[2]->IsNumber()); + THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); ARGS_THIS(args[0].As()); Local buf = args[1].As(); int32_t offset_i32 = args[2]->Int32Value(); @@ -667,10 +695,10 @@ void IndexOfBuffer(const FunctionCallbackInfo& args) { void IndexOfNumber(const FunctionCallbackInfo& args) { - ASSERT(args[0]->IsObject()); ASSERT(args[1]->IsNumber()); ASSERT(args[2]->IsNumber()); + THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); ARGS_THIS(args[0].As()); uint32_t needle = args[1]->Uint32Value(); int32_t offset_i32 = args[2]->Int32Value(); diff --git a/test/parallel/test-buffer-fakes.js b/test/parallel/test-buffer-fakes.js new file mode 100644 index 00000000000000..d473d16e92159f --- /dev/null +++ b/test/parallel/test-buffer-fakes.js @@ -0,0 +1,56 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const Buffer = require('buffer').Buffer; +const Bp = Buffer.prototype; + +function FakeBuffer() { } +FakeBuffer.__proto__ = Buffer; +FakeBuffer.prototype.__proto__ = Buffer.prototype; + +const fb = new FakeBuffer(); + +assert.throws(function() { + new Buffer(fb); +}, TypeError); + +assert.throws(function() { + +Buffer.prototype; +}, TypeError); + +assert.throws(function() { + Buffer.compare(fb, new Buffer(0)); +}, TypeError); + +assert.throws(function() { + fb.write('foo'); +}, TypeError); + +assert.throws(function() { + Buffer.concat([fb, fb]); +}, TypeError); + +assert.throws(function() { + fb.toString(); +}, TypeError); + +assert.throws(function() { + fb.equals(new Buffer(0)); +}, TypeError); + +assert.throws(function() { + fb.indexOf(5); +}, TypeError); + +assert.throws(function() { + fb.readFloatLE(0); +}, TypeError); + +assert.throws(function() { + fb.writeFloatLE(0); +}, TypeError); + +assert.throws(function() { + fb.fill(0); +}, TypeError);