Add explicit bounds checks

[davidje13]

As this is a compiled binary, it carries more risk than javascript code. Specifically: buffer overflows are possible if this is called with incorrect parameters.

Though I believe it's likely that these functions are always invoked safely by ws, it would be easier to audit (and give more confidence of future safety) if the C code checked the bounds explicitly. Since this is just a few operations per invocation, it will not have any noticeable impact on performance (akin to the existing status checks).

The checks that should be added:

• Both Mask and Unmask should confirm that the mask parameter has a length of at least 4 bytes
• Mask should confirm that both source.length and destination.length - offset are ≥ the length parameter (to avoid overflow issues the latter may need to be 2 checks: offset <= destination.length && destination.length - offset >= length)

It would be nice if any failures here resulted in napi_throw_error being called, rather than an assert failure (so that broken code does not cause the whole process to terminate, potentially allowing a denial-of-service attack).

---

test cases: (currently all execute without error or cause a segfault)

const bufferUtil = require('bufferutil');

// source not large enough
bufferUtil.mask(Buffer.alloc(9), Buffer.alloc(4), Buffer.alloc(10), 0, 10);

// destination not large enough
bufferUtil.mask(Buffer.alloc(10), Buffer.alloc(4), Buffer.alloc(9), 0, 10);

// destination not large enough (due to offset)
bufferUtil.mask(Buffer.alloc(10), Buffer.alloc(4), Buffer.alloc(10), 1, 10);

// destination not large enough (due to negative offset interpreted as unsigned int - causes a segfault)
bufferUtil.mask(Buffer.alloc(10), Buffer.alloc(4), Buffer.alloc(10), -1, 10);

// mask not large enough
bufferUtil.mask(Buffer.alloc(10), Buffer.alloc(3), Buffer.alloc(10), 0, 10);

// empty buffers (causes a segfault because `null` is passed)
bufferUtil.mask(Buffer.alloc(0), Buffer.alloc(4), Buffer.alloc(10), 0, 10);
bufferUtil.mask(Buffer.alloc(10), Buffer.alloc(0), Buffer.alloc(10), 0, 10);
bufferUtil.mask(Buffer.alloc(10), Buffer.alloc(4), Buffer.alloc(0), 0, 10);

// length out-of-range (negative length interpreted as an unsigned int - causes a segfault)
bufferUtil.mask(Buffer.alloc(10), Buffer.alloc(4), Buffer.alloc(10), 0, -1);

All of these cases could be caught with the bounds checks above.

[lpinca]

Input validation is outside of the scope of the module. Parameters should be validated before calling the utility functions provided by this module.

This has already been reported before.

[davidje13]

Is there a particular reason against doing these checks? I believe the handful of bounds checks I listed above would be enough to fully guarantee safety here, and since it's just a few operations at the start of the call (not part of the loop), performance won't be impacted.

Given the potential for abuse if a mistake is made in the code that calls this (in the right circumstances this could be part of an RCE, especially since the whole use-case is to handle untrusted data received from the network), it seems that some amount of layered security would be beneficial.

(I had a look through the way ws uses these function and — as expected — I found no obvious issues, but it did take quite a bit of analysis of various paths to be sure that it was always invoked safely, and it's the sort of thing that can easily go awry in a future update)

[lpinca]

The main reason is performance. There is no need to validate parameters twice. The first time in JavaScript in and then again in C. The overhead is probably not noticeable but the functions are called for every received and sent frame.

I'm happy to document that no input validation is done in this module.

[davidje13]

If that level of performance is important to you and you're happy to put the validation purely on the caller, you should probably remove the existing status assertions as well, since between them those will be costing you approximately 2x more operations per call than the bounds checks I'm suggesting here, and more than 2x the number of branches. They are not presently compiled out as no-ops in the release builds, despite using the assert macro.

Though I personally think that is the wrong approach to take, and goes against the direction of the industry as a whole towards layered security protections.

[lpinca]

The difference is that the work done by the status assertions can't be and is not already done by the caller. It is done only once. ~Anyway, yes, assert() calls should be removed or converted to no-ops in the release build.~

[lpinca]

The assert() calls should actually be kept even in the release builds to document "impossible conditions". Replacing them with napi_throw_error() is not very useful unless it is faster.

Also, to properly validate the input, the type of the parameters should also be checked. Bounds checks are not sufficient.

[davidje13]

The parameter types are actually checked automatically as part of calling napi_get_buffer_info so you get no choice there (presumably because it's simply unable to do anything meaningful if given the wrong parameter type):

node[72510]: ../src/node_buffer.cc:248:char *node::Buffer::Data(Local<v8::Value>): Assertion `val->IsArrayBufferView()' failed.
 1: 0x10163bb15 node::Abort() [/usr/local/bin/node]
 2: 0x10163b861 node::Assert(node::AssertionInfo const&) [/usr/local/bin/node]
 3: 0x10160edd2 node::Buffer::Data(v8::Local<v8::Value>) [/usr/local/bin/node]
 4: 0x101605323 napi_get_buffer_info [/usr/local/bin/node]
 5: 0x107567463 Mask […/node.napi.node]
 6: 0x1015f4350 v8impl::(anonymous namespace)::FunctionCallbackWrapper::Invoke(v8::FunctionCallbackInfo<v8::Value> const&) [/usr/local/bin/node]
 7: 0x101857f78 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, unsigned long*, int) [/usr/local/bin/node]
 8: 0x10185753a v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [/usr/local/bin/node]
 9: 0x102204176 Builtins_CEntry_Return1_ArgvOnStack_BuiltinExit [/usr/local/bin/node]

(that happens when passing 0 as one of the buffers, for example).

[lpinca]

The parameter types are actually checked automatically as part of calling napi_get_buffer_info so you get no choice there (presumably because it's simply unable to do anything meaningful if given the wrong parameter type):

No, with proper validation the abort should not happen. The argument type is checked and napi_get_buffer_info() is not called at all if it is not a buffer, but again the function parameters validation is not done by design.

[lpinca]

I'm closing this as answered.