src: fix OOB reads in process.title getter by bnoordhuis · Pull Request #31633 · nodejs/node

bnoordhuis · 2020-02-04T15:24:14Z

The getter passed a stack-allocated, fixed-size buffer to
uv_get_process_title() but neglected to check the return value.

When the total length of the command line arguments exceeds the size of
the buffer, libuv returns UV_ENOBUFS and doesn't modify the contents of
the buffer. The getter then proceeded to return whatever garbage was on
the stack at the time of the call, quite possibly reading beyond the
end of the buffer.

Add a GetProcessTitle() helper that reads the process title into a
dynamically allocated buffer that is resized when necessary.

Fixes: #31631

this is technically a security vulnerability although probably impossible to exploit practically
the second commit I'm happy to leave out
there's also a minor libuv bug but this PR is still necessary

The getter passed a stack-allocated, fixed-size buffer to uv_get_process_title() but neglected to check the return value. When the total length of the command line arguments exceeds the size of the buffer, libuv returns UV_ENOBUFS and doesn't modify the contents of the buffer. The getter then proceeded to return whatever garbage was on the stack at the time of the call, quite possibly reading beyond the end of the buffer. Add a GetProcessTitle() helper that reads the process title into a dynamically allocated buffer that is resized when necessary. Fixes: nodejs#31631

Remove the version of GetHumanReadableProcessName() that operates on a fixed-size buffer. The only remaining caller is Assert() which might get called in contexts where dynamically allocating memory isn't possible but as Assert() calls printf(), which also allocates memory when necessary, this commit is unlikely to make matters much worse.

nodejs-github-bot · 2020-02-04T15:25:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28921/

jasnell · 2020-02-04T15:34:57Z

+  std::string buf(16, '\0');
+
+  for (;;) {
+    const int rc = uv_get_process_title(&buf[0], buf.size());


Not blocking for this PR, but... in the future, might it be possible to introduce a variant of uv_get_process_title that can optionally report the size of the buffer required? something along the lines of...

size_t actual = buf.size(); const int rc = uv_get_process_title2(&buf[0], &actual); if (rc == UV_ENOBUFS) { buf.resize(actual); // then try again } // ....

Definitely minor but a bit nicer to avoid the multiple resize operations

nodejs-github-bot · 2020-02-04T20:54:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28929/

jasnell · 2020-02-04T21:00:48Z

+// is necessary otherwise *its* process.title is whatever the last
+// SetConsoleTitle() call in our process tree set it to.
+if (common.isWindows)
+  process.title = process.execPath;


Using // Flags: --title=test to explicitly set the title with a flag would work here also, right? Might be better than having a special case here for Windows

See libuv/libuv#2667 - I believe this is a Windows-only libuv bug. Until it's fixed, it's best to call it out explicitly.

nodejs-github-bot · 2020-02-07T11:08:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28975/

nodejs-github-bot · 2020-02-07T14:35:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28985/ (:heavy_check_mark:)

The getter passed a stack-allocated, fixed-size buffer to uv_get_process_title() but neglected to check the return value. When the total length of the command line arguments exceeds the size of the buffer, libuv returns UV_ENOBUFS and doesn't modify the contents of the buffer. The getter then proceeded to return whatever garbage was on the stack at the time of the call, quite possibly reading beyond the end of the buffer. Add a GetProcessTitle() helper that reads the process title into a dynamically allocated buffer that is resized when necessary. Fixes: #31631 PR-URL: #31633 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Remove the version of GetHumanReadableProcessName() that operates on a fixed-size buffer. The only remaining caller is Assert() which might get called in contexts where dynamically allocating memory isn't possible but as Assert() calls printf(), which also allocates memory when necessary, this commit is unlikely to make matters much worse. PR-URL: #31633 Fixes: #31631 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Rich Trott <[email protected]>

addaleax · 2020-02-07T22:59:40Z

Landed in d394dd7 3271c40