Skip to content

Possible use-after-free of JS Buffers captured by the async WriteMultiVars worker #109

Description

@OvOhao

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:

  1. Each Items[i].pdata is a pointer into the i-th input Buffer's V8 backing store.
  2. 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).
  3. The sync call returns immediately; if JS drops the buffers, a GC frees/moves them.
  4. 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions