LinusU / LinusU/fs-xattr

Possible use-after-free of a borrowed Buffer in async `xattr_set`

Open
#46 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
71
Forks
20
PR merge metrics
No merged PRs in 30d

Description

# Possible use-after-free of a borrowed Buffer in async `xattr_set`

I found a possible borrowed-buffer-across-async use-after-free in `xattr_set`. The function
captures the raw backing-store pointer and length of the caller's value `Buffer` via
`napi_get_buffer_info` and stores them in a heap struct, then hands that struct to a libuv
worker thread through `napi_queue_async_work`. Unlike the two string arguments (filename,
attribute), which are copied into freshly `malloc`'d memory, the value buffer is **not** copied
and **no reference is pinned** on it (`napi_create_reference` is never called). Between the time
`xattr_set` returns the promise and the time the worker thread runs `xattr_set_execute`, V8's GC
is free to collect or move the JS `Buffer`, freeing its backing store. The worker then calls
`setxattr(..., data->value, data->value_length, ...)` on a dangling pointer.

File: `src/async.c`

Function: `xattr_set` / `xattr_set_execute`

```c
napi_value xattr_set(napi_env env, napi_callback_info info) {
...
XattrSetData* data = malloc(sizeof(XattrSetData));
...
// value + value_length are borrowed straight from the JS Buffer, no copy, no ref
assert(napi_get_buffer_info(env, args[2], (void**) &data->value, (size_t*) &data->value_length) == napi_ok);
...
napi_async_work work;
assert(napi_create_async_work(env, NULL, work_name, xattr_set_execute, xattr_set_complete, (void*) data, &work) == napi_ok);
assert(napi_queue_async_work(env, work) == napi_ok); // runs later, on a worker thread
return promise;
}

void xattr_set_execute(napi_env env, void* _data) {
XattrSetData* data = _data;
// data->value is the borrowed backing store; may already be freed/moved by GC
int res = setxattr(data->filename, data->attribute, data->value, data->value_length, 0);
...
}
```

1. `xattr_set` obtains `data->value` (raw pointer) and `data->value_length` directly from the
JS `Buffer` `args[2]` with `napi_get_buffer_info`. This pointer is owned by V8, not by the
addon.
2. No `napi_create_reference(env, args[2], 1, ...)` is taken on the buffer, so nothing keeps the
`Buffer` (or its `ArrayBuffer` backing store) alive. The returned `promise` does not reference
the buffer either.
3. `napi_queue_async_work` schedules `xattr_set_execute` to run on a libuv thread-pool thread at
an unspecified later time. Control returns to JS immediately.
4. If the caller drops its reference to the buffer (e.g. an inline `Buffer.from(...)`), the buffer
becomes collectible. A GC cycle before the worker runs frees/relocates the backing store.
5. `xattr_set_execute` then reads `data->value_length` bytes from the freed `data->value` via
`setxattr` — a use-after-free read (potential crash or disclosure of reused heap memory to the
filesystem xattr).

Note the contrast with `filename`/`attribute`, which are deep-copied into `malloc`'d buffers and
are safe, and with the synchronous `xattr_set_sync` in `src/sync.c`, which is safe because the
buffer cannot be collected during a synchronous call.

JS trigger (if applicable):

```js
const xattr = require('fs-xattr');
// The value Buffer is unreferenced after the call; GC may free it before the
// worker thread runs setxattr().
xattr.set('/tmp/somefile', 'user.test', Buffer.alloc(4096, 0x41))
.then(() => { /* ... */ });
// force allocations / GC pressure here to reclaim the buffer before the worker runs
```

Suggested fix: pin the value buffer for the duration of the async work. Either take
`napi_create_reference(env, args[2], 1, &data->ref_value)` in `xattr_set` and
`napi_delete_reference` it in `xattr_set_complete`, or (simplest) `malloc` a copy of the buffer
bytes in `xattr_set` and free it in the completion callback, mirroring how `filename`/`attribute`
are already handled.

## Additional defects: per-call leaks in `xattr_set_complete`

Independently of the UAF, `xattr_set_complete` leaks on two counts:

```c
void xattr_set_complete(napi_env env, napi_status status, void* _data) {
XattrSetData* data = _data;
free(data->filename);
free(data->attribute);
if (data->e != 0) {
napi_value error;
assert(create_xattr_error(env, data->e, &error) == napi_ok);
assert(napi_reject_deferred(env, data->deferred, error) == napi_ok);
return; // <-- error path: free(_data) never reached -> struct leaks
}
napi_value undefined;
assert(napi_get_undefined(env, &undefined) == napi_ok);
assert(napi_resolve_deferred(env, data->deferred, undefined) == napi_ok);
free(_data); // reached only on success
}
```

1. **`data` struct leak on the error path.** When `setxattr` failed (`data->e != 0`), the callback
rejects the deferred and `return`s *before* `free(_data)`, so the entire `XattrSetData` heap
struct is leaked on every failed `set` (e.g. `ENOTSUP`, `EPERM`, missing file).
2. **Async-work handle never deleted.** Neither `xattr_set` nor `xattr_set_complete` ever calls
`napi_delete_async_work` on the `work` handle created by `napi_create_async_work`. N-API requires
the work object to be freed with `napi_delete_async_work` once complete; it is missing on **every**
call (success and error), leaking the async-work object per invocation. (The same omission exists
in `xattr_get`/`xattr_list`/`xattr_remove`.)

Additional fix: call `free(_data)` before the early `return` on the error path (or restructure so
both paths fall through to a single `free`), and add `napi_delete_async_work(env, work)` after the
work completes (store the `work` handle in `data` so the completion callback can delete it).

Contributor guide

No contributing guide indexed for this repository

Research direction

Read src/async.c, starting with xattr_set, xattr_set_execute, and xattr_set_complete, and compare their ownership and cleanup with the synchronous path in src/sync.c. Trace the async-work handle and value buffer from queuing through completion, including success and error paths. Done means the async operation cannot use released buffer storage and does not leak its per-call allocations or work handle.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, node.js
Domain
backend, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.