Possible use-after-free of JS Buffers captured by the async WriteMultiVars worker
I found a possible borrowed-buffer-across-async use-after-free in S7Client::WriteMultiVars.
For every item in the input array, WriteMultiVars stores the raw backing-store pointer of that
item's Data Buffer directly into Items[i].pdata via
node::Buffer::Data(... "Data" ...). When a completion callback is supplied, the whole Items
array (holding those borrowed pointers) is handed to a libuv-threadpool IOWorker
(caller WRITEMULTI). No napi_reference/Nan::Persistent is created for any of the input
buffers, so after the synchronous call returns undefined, the garbage collector may free or
relocate any of the Data buffers while the worker thread is still reading them inside
snap7Client->WriteMultiVars(Items, len) → use-after-free.
File: src/node_snap7_client.cpp
Function: S7Client::WriteMultiVars
Items[i].pdata = node::Buffer::Data(Nan::Get(data_obj,
Nan::New<v8::String>("Data").ToLocalChecked()).ToLocalChecked().As<v8::Object>());
...
} else {
Nan::Callback *callback = new Nan::Callback(info[1].As<v8::Function>());
Nan::AsyncQueueWorker(new IOWorker(callback, s7client, WRITEMULTI
, Items, len));
info.GetReturnValue().SetUndefined();
}
The worker dereferences those borrowed pointers on the threadpool thread
(IOWorker::Execute()):
case WRITEMULTI:
returnValue = s7client->snap7Client->WriteMultiVars(
static_cast<PS7DataItem>(pData), int1);
Path:
- Each
Items[i].pdata is a pointer into the i-th input Buffer's V8 backing store.
- The
Items array is queued to the worker; the input Buffers themselves are never
referenced/pinned (unlike the addon-owned pdata buffers in ReadMultiVars, these point
into caller-owned JS memory).
- The sync call returns immediately; if JS drops the buffers, a GC frees/moves them.
Execute() then reads freed memory when building the S7 write request → UAF.
JS trigger (if applicable):
const snap7 = require('node-snap7');
const c = new snap7.S7Client();
c.WriteMultiVars([
{ Area: c.S7AreaDB, WordLen: c.S7WLByte, DBNumber: 1, Start: 0, Amount: 4,
Data: Buffer.alloc(4) } // buffer not retained by the caller
], (err) => {});
for (let i = 0; i < 1e6; i++) ({}); // provoke GC while the worker runs
Suggested fix: pin each input Data buffer for the worker's lifetime (store Nan::Persistent
references — e.g. via SaveToPersistent on the IOWorker), or memcpy each buffer into
worker-owned storage before queuing, releasing/freeing them in HandleOKCallback.
Additional defect: unchecked item buffer length vs Amount (out-of-bounds read)
Independently of the UAF, for each item WriteMultiVars records Items[i].Amount (from the JS
Amount field) and Items[i].pdata = node::Buffer::Data(... "Data" ...), but never checks that
the item's Data buffer is at least Amount * bytesPerWordLen(WordLen) bytes. The per-item
validation only calls node::Buffer::HasInstance(...) on the Data field — its length is never
consulted:
Items[i].Amount = Nan::To<int32_t>(Nan::Get(data_obj,
Nan::New<v8::String>("Amount").ToLocalChecked()).ToLocalChecked()).FromJust();
Items[i].pdata = node::Buffer::Data(Nan::Get(data_obj,
Nan::New<v8::String>("Data").ToLocalChecked()).ToLocalChecked().As<v8::Object>());
// no check: Buffer::Length(Data) vs Items[i].Amount * bytesPerWordLen(Items[i].WordLen)
snap7's WriteMultiVars then reads Amount * bytesPerWordLen bytes from each pdata, so any item
whose Data buffer is smaller than its declared transfer amount triggers a deterministic OOB read
of that buffer's backing store (bytes sent to the PLC) — no GC race required.
Additional fix (OOB read): for each item, reject when
node::Buffer::Length(Data) < Amount * GetByteCountFromWordLen(WordLen) before storing pdata.
Possible use-after-free of JS Buffers captured by the async
WriteMultiVarsworkerI found a possible borrowed-buffer-across-async use-after-free in
S7Client::WriteMultiVars.For every item in the input array,
WriteMultiVarsstores the raw backing-store pointer of thatitem's
DataBufferdirectly intoItems[i].pdatavianode::Buffer::Data(... "Data" ...). When a completion callback is supplied, the wholeItemsarray (holding those borrowed pointers) is handed to a libuv-threadpool
IOWorker(caller
WRITEMULTI). Nonapi_reference/Nan::Persistentis created for any of the inputbuffers, so after the synchronous call returns
undefined, the garbage collector may free orrelocate any of the
Databuffers while the worker thread is still reading them insidesnap7Client->WriteMultiVars(Items, len)→ use-after-free.File:
src/node_snap7_client.cppFunction:
S7Client::WriteMultiVarsThe worker dereferences those borrowed pointers on the threadpool thread
(
IOWorker::Execute()):Path:
Items[i].pdatais a pointer into the i-th inputBuffer's V8 backing store.Itemsarray is queued to the worker; the inputBuffers themselves are neverreferenced/pinned (unlike the addon-owned
pdatabuffers inReadMultiVars, these pointinto caller-owned JS memory).
Execute()then reads freed memory when building the S7 write request → UAF.JS trigger (if applicable):
Suggested fix: pin each input
Databuffer for the worker's lifetime (storeNan::Persistentreferences — e.g. via
SaveToPersistenton theIOWorker), or memcpy each buffer intoworker-owned storage before queuing, releasing/freeing them in
HandleOKCallback.Additional defect: unchecked item buffer length vs
Amount(out-of-bounds read)Independently of the UAF, for each item
WriteMultiVarsrecordsItems[i].Amount(from the JSAmountfield) andItems[i].pdata = node::Buffer::Data(... "Data" ...), but never checks thatthe item's
Databuffer is at leastAmount * bytesPerWordLen(WordLen)bytes. The per-itemvalidation only calls
node::Buffer::HasInstance(...)on theDatafield — its length is neverconsulted:
snap7's
WriteMultiVarsthen readsAmount * bytesPerWordLenbytes from eachpdata, so any itemwhose
Databuffer is smaller than its declared transfer amount triggers a deterministic OOB readof that buffer's backing store (bytes sent to the PLC) — no GC race required.
Additional fix (OOB read): for each item, reject when
node::Buffer::Length(Data) < Amount * GetByteCountFromWordLen(WordLen)before storingpdata.