Skip to content

Commit

Permalink
buffer: prevent abort on bad proto
Browse files Browse the repository at this point in the history
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.

Ref: #1486
Ref: #1922
Fixes: #1485
PR-URL: #2012
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
trevnorris committed Jun 25, 2015
1 parent 856c11f commit 1cd9eeb
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 6 deletions.
2 changes: 0 additions & 2 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
36 changes: 32 additions & 4 deletions src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object> obj = argT; \
size_t obj_length = obj->GetIndexedPropertiesExternalArrayDataLength(); \
Expand Down Expand Up @@ -223,7 +229,12 @@ template <encoding encoding>
void StringSlice(const FunctionCallbackInfo<Value>& 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(
Expand All @@ -235,7 +246,12 @@ template <>
void StringSlice<UCS2>(const FunctionCallbackInfo<Value>& 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;

Expand Down Expand Up @@ -306,8 +322,9 @@ void Copy(const FunctionCallbackInfo<Value> &args) {
if (!HasInstance(args[0]))
return env->ThrowTypeError("first arg should be a Buffer");

Local<Object> target = args[0]->ToObject(env->isolate());
Local<Object> target = args[0].As<Object>();

THROW_AND_RETURN_UNLESS_BUFFER(env, args.This());
ARGS_THIS(args.This())
size_t target_length = target->GetIndexedPropertiesExternalArrayDataLength();
char* target_data = static_cast<char*>(
Expand Down Expand Up @@ -340,6 +357,7 @@ void Copy(const FunctionCallbackInfo<Value> &args) {


void Fill(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
ARGS_THIS(args[0].As<Object>())

size_t start = args[2]->Uint32Value();
Expand Down Expand Up @@ -383,6 +401,7 @@ template <encoding encoding>
void StringWrite(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

THROW_AND_RETURN_UNLESS_BUFFER(env, args.This());
ARGS_THIS(args.This())

if (!args[0]->IsString())
Expand Down Expand Up @@ -459,6 +478,7 @@ static inline void Swizzle(char* start, unsigned int len) {

template <typename T, enum Endianness endianness>
void ReadFloatGeneric(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
ARGS_THIS(args[0].As<Object>());

uint32_t offset = args[1]->Uint32Value();
Expand Down Expand Up @@ -522,21 +542,25 @@ uint32_t WriteFloatGeneric(const FunctionCallbackInfo<Value>& args) {


void WriteFloatLE(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
args.GetReturnValue().Set(WriteFloatGeneric<float, kLittleEndian>(args));
}


void WriteFloatBE(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
args.GetReturnValue().Set(WriteFloatGeneric<float, kBigEndian>(args));
}


void WriteDoubleLE(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
args.GetReturnValue().Set(WriteFloatGeneric<double, kLittleEndian>(args));
}


void WriteDoubleBE(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
args.GetReturnValue().Set(WriteFloatGeneric<double, kBigEndian>(args));
}

Expand All @@ -550,6 +574,10 @@ void ByteLengthUtf8(const FunctionCallbackInfo<Value> &args) {


void Compare(const FunctionCallbackInfo<Value> &args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
THROW_AND_RETURN_UNLESS_BUFFER(env, args[1]);

Local<Object> obj_a = args[0].As<Object>();
char* obj_a_data =
static_cast<char*>(obj_a->GetIndexedPropertiesExternalArrayData());
Expand Down Expand Up @@ -599,10 +627,10 @@ int32_t IndexOf(const char* haystack,


void IndexOfString(const FunctionCallbackInfo<Value>& 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<Object>());
node::Utf8Value str(args.GetIsolate(), args[1]);
int32_t offset_i32 = args[2]->Int32Value();
Expand Down Expand Up @@ -630,10 +658,10 @@ void IndexOfString(const FunctionCallbackInfo<Value>& args) {


void IndexOfBuffer(const FunctionCallbackInfo<Value>& 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<Object>());
Local<Object> buf = args[1].As<Object>();
int32_t offset_i32 = args[2]->Int32Value();
Expand Down Expand Up @@ -667,10 +695,10 @@ void IndexOfBuffer(const FunctionCallbackInfo<Value>& args) {


void IndexOfNumber(const FunctionCallbackInfo<Value>& 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<Object>());
uint32_t needle = args[1]->Uint32Value();
int32_t offset_i32 = args[2]->Int32Value();
Expand Down
56 changes: 56 additions & 0 deletions test/parallel/test-buffer-fakes.js
Original file line number Diff line number Diff line change
@@ -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);

0 comments on commit 1cd9eeb

Please sign in to comment.