security,unix: audit use of process.env in lib/ for setuid binary #18511

j0t3x · 2018-02-01T16:44:17Z

Wrapped SafeGetenv() in util binding with the
purpose of protecting the cases when env vars are
accessed with the privileges of another user in jsland.

Fixes: #9160

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

os, module, util binding

addaleax · 2018-02-01T17:06:08Z

src/node_util.cc

+  String::Utf8Value strenvtag(envtag);
+  std::string text;
+  bool isEnvSafe = false;
+  isEnvSafe = SafeGetenv(*strenvtag, &text);


Okay, doing it this way does make my TODO comment relevant – this is going to break on Windows if the environment variable is not pure ASCII.

I think I'm not using it on any windows part of the code, right?

bnoordhuis · 2018-02-01T18:27:34Z

lib/os.js

@@ -123,13 +123,14 @@ function tmpdir() {
  if (isWindows) {
    path = process.env.TEMP ||
           process.env.TMP ||
-           (process.env.SystemRoot || process.env.windir) + '\\temp';
+           (process.env.SystemRoot ||
+            process.env.windir) + '\\temp';


No unrelated stylistic changes, please.

🤦🏽‍♂️ Will do.

bnoordhuis · 2018-02-01T18:28:07Z

lib/module.js

@@ -35,6 +35,7 @@ const {
  internalModuleReadJSON,
  internalModuleStat
 } = process.binding('fs');
+const { safeGetenvForJs } = process.binding('util');


Just safeGetenv? The ForJs part is self-evident.

Its alright if I let the SafeGetenvForJs name in c++, I think it's useful there.

It's also self-evident for C++ code because of the function prototype; if it takes a const FunctionCallbackInfo<Value>&, you know it's a JS -> C++ API method.

but here I have same name functions in the same namespace.

bnoordhuis · 2018-02-01T18:30:33Z

src/node_util.cc

@@ -174,6 +176,20 @@ void PromiseReject(const FunctionCallbackInfo<Value>& args) {
  args.GetReturnValue().Set(ret.FromMaybe(false));
 }

+void SafeGetenvForJs(const FunctionCallbackInfo<Value>& args) {
+  Isolate* isolate = args.GetIsolate();
+  Local<String> envtag = args[0]->ToString();


Can you use the overload that takes a Local<Context>?

Or simply use args[0].As<String>() and CHECK(args[0]->IsString()) first. It's an internal API, it's reasonable to enforce strict types.

Can you point me where can I find different overload options?

Yep, search deps/v8/include/v8.h for ToString.

@bnoordhuis as a side benefit, can you point me somewhere to read about v8(guides are out of date or poor), mostly to improve my contributions.

There is some documentation on the V8 wiki and a few samples in the V8 repo but that's about it. Working with V8 means reading its source (or at least its header files.)

At least the headers have pretty good doc comments now. It wasn't anything like that when I started working on node.

will have that in mind, thanks!

bnoordhuis · 2018-02-01T18:31:02Z

src/node_util.cc

+  Local<String> envtag = args[0]->ToString();
+  String::Utf8Value strenvtag(envtag);
+  std::string text;
+  bool isEnvSafe = false;


Locals should use snake_case, i.e., is_env_safe.

That said, you don't need a separate local, just do if (!SafeGetenv(...)) return;

bnoordhuis · 2018-02-01T18:33:26Z

src/node_util.cc

+  if (isEnvSafe) {
+    args.GetReturnValue()
+    .Set(String::NewFromUtf8(isolate, text.c_str()));
+    return;


Superfluous return statements.

so no return statements are needed given the case i have 1 value to return?

Yep. You set the return value and then just fall through. If you use the guard style I mentioned above, the args.GetReturnValue().Set(...) will also be the last statement in this function.

bnoordhuis · 2018-02-01T18:35:24Z

test/parallel/test-util-internal.js

 } = process.binding('util');

+for (const oneEnv in process.env) {
+  console.log(oneEnv, safeGetenvForJs(oneEnv));


Tests should, as a rule of thumb, be silent. Don't print the inputs, the exception from assert.strictEqual() takes care of that (it's in the error message.)

bnoordhuis · 2018-02-01T18:38:04Z

test/parallel/test-util-internal.js

+assert.strictEqual(
+  safeGetenvForJs(FAKEENVVAR),
+  undefined
+);


I guess it's unlikely that anyone has this set in the environment but it's less robust than I'd like.

I'll remove it then.

BridgeAR · 2018-02-02T09:54:28Z

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

j0t3x · 2018-02-02T15:05:19Z

lib/module.js

@@ -735,7 +736,7 @@ Module._initPaths = function() {
    paths.unshift(path.resolve(homeDir, '.node_modules'));
  }

-  var nodePath = process.env.NODE_PATH;
+  var nodePath = safeGetenv('NODE_PATH');


@addaleax here I'm touching windows and indeed it's tests are failing. 👍
I see the value of changing NODE_PATH in a test,
Ill consider windows vs Unix on that too, Is it ok?

jdalton · 2018-02-02T16:55:50Z

Is this something user code has to worry about? Should this be tackled in the accessors of the magic env object itself so that user code gets this too?

addaleax · 2018-02-02T19:01:16Z

Is this something user code has to worry about?

Yes, but I don’t think Node can make a blanket decision for all user code to block out environment variables in this (already very rarely occurring) situation.

I would be okay with requiring an explicit opt-in by the userland code in some way, I guess.

addaleax

Looks good!

addaleax · 2018-02-02T19:04:04Z

src/node_util.cc

+  std::string text;
+  if (!SafeGetenv(*strenvtag, &text)) return;
+  args.GetReturnValue()
+  .Set(String::NewFromUtf8(args.GetIsolate(), text.c_str()));


Can you indent this line by 4 spaces? We generally do that for statement continuations, see https://github.com/nodejs/node/blob/master/CPP_STYLE_GUIDE.md#4-spaces-of-indentation-for-statement-continuations

bnoordhuis · 2018-02-03T10:25:46Z

I don’t think Node can make a blanket decision for all user code to block out environment variables in this (already very rarely occurring) situation.

That's more of an argument for an opt-out. JS code can't even properly detect it's running as setuid root right now, let alone protect against it.

edit: nuance: process.geteuid() and process.getegid() have been back-ported but there's no way to detect AT_SECURE.

j0t3x · 2018-02-07T00:53:07Z

process.geteuid() and process.getegid() have been back-ported but there's no way to detect AT_SECURE.

@bnoordhuis This is something we can raise in another pr, right? I would love to do that too.

BridgeAR · 2018-02-12T19:27:40Z

Ping @bnoordhuis

bnoordhuis

Is the ping w.r.t. @j0t3x's question? Yes, can be done in another PR.

bnoordhuis · 2018-02-12T20:23:22Z

src/node_util.cc

+  std::string text;
+  if (!SafeGetenv(*strenvtag, &text)) return;
+  args.GetReturnValue()
+      .Set(String::NewFromUtf8(args.GetIsolate(), text.c_str()));


Can you use the String::NewFromUtf8 overload that returns a MaybeLocal?

Apropos the function name, I remember discussing renaming it to just SafeGetenv?

bnoordhuis · 2018-02-12T20:28:58Z

src/node_util.cc

@@ -8,12 +8,14 @@ using v8::Array;
 using v8::Context;
 using v8::FunctionCallbackInfo;
 using v8::Integer;
+using v8::Isolate;


Unused import.

bnoordhuis · 2018-02-12T20:30:27Z

src/node_util.cc

+void SafeGetenvForJs(const FunctionCallbackInfo<Value>& args) {
+  CHECK(args[0]->IsString());
+  Local<String> envtag = args[0].As<String>();
+  String::Utf8Value strenvtag(envtag);


Node has its own slightly more efficient Utf8Value implementation; just drop the String:: part and pass in args.GetIsolate() as the first argument. You can pass in args[0] as-is, no need to cast.

done, thanks for the tip!

Wrapped SafeGetenv() in util binding with the purpose of protecting the cases when env vars are accessed with the privileges of another user in jsland. Fixes: #9160

bnoordhuis

Thanks, LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/13150/

j0t3x · 2018-02-13T19:34:15Z

@bnoordhuis is the node-test-linter failing for something out of this pr?

bnoordhuis · 2018-02-13T19:50:34Z

Nope, it's something else. Can be ignored.

Wrap SafeGetenv() in util binding with the purpose of protecting the cases when env vars are accessed with the privileges of another user in jsland. PR-URL: nodejs#18511 Fixes: nodejs#9160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

BridgeAR · 2018-02-16T16:43:14Z

Landed in e200db1b2d8e6b25bb273d5a058957b07fc0166b 🎉

I fixed the commit message while landing.

j0t3x · 2018-02-18T00:15:04Z

Thanks a lot :)

richardlau · 2018-02-18T00:35:31Z

Landed in e200db1b2d8e6b25bb273d5a058957b07fc0166b 🎉

This looks to have actually landed in 916cfec.

BridgeAR · 2018-02-18T01:04:09Z

Uh, thanks @richardlau. I sometimes post the commit id while pushing. If someone else has landed something in the meanwhile, it is rejected and I have to rebase. Seems like I forgot to update the commit it.

richardlau · 2018-02-18T01:51:44Z

@BridgeAR easily done 😁. Just making sure the information is accurate just in case backports are needed to earlier release lines. And on that note, @bnoordhuis what's the semver-ness of this PR?

MylesBorins · 2018-02-21T15:02:16Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Wrap SafeGetenv() in util binding with the purpose of protecting the cases when env vars are accessed with the privileges of another user in jsland. PR-URL: nodejs#18511 Fixes: nodejs#9160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins · 2018-08-07T17:50:13Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. os Issues and PRs related to the os subsystem. util Issues and PRs related to the built-in util module. labels Feb 1, 2018

addaleax reviewed Feb 1, 2018

View reviewed changes

bnoordhuis reviewed Feb 1, 2018

View reviewed changes

ChALkeR added the security Issues and PRs related to security. label Feb 1, 2018

j0t3x commented Feb 2, 2018

View reviewed changes

jasnell approved these changes Feb 2, 2018

View reviewed changes

addaleax approved these changes Feb 2, 2018

View reviewed changes

bnoordhuis reviewed Feb 12, 2018

View reviewed changes

security,unix: audit use of process.env in lib/ for setuid binary

258f2e8

Wrapped SafeGetenv() in util binding with the purpose of protecting the cases when env vars are accessed with the privileges of another user in jsland. Fixes: #9160

bnoordhuis approved these changes Feb 13, 2018

View reviewed changes

Merge branch 'master' into envsecurityaudit

88e19c5

bnoordhuis added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 15, 2018

BridgeAR closed this Feb 16, 2018

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

MylesBorins added the backport-requested-v8.x label Aug 7, 2018

richardlau mentioned this pull request Sep 4, 2018

NODE_PATH not recognized when cap_net_bind_service is set #22648

Closed

yorkie mentioned this pull request Oct 18, 2019

os: add method tmpdir() yodaos-project/ShadowNode#569

Closed

3 tasks

security,unix: audit use of process.env in lib/ for setuid binary #18511

security,unix: audit use of process.env in lib/ for setuid binary #18511

Conversation

j0t3x commented Feb 1, 2018

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Feb 2, 2018

j0t3x Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

jdalton commented Feb 2, 2018 • edited Loading

addaleax commented Feb 2, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Feb 3, 2018 • edited Loading

j0t3x commented Feb 7, 2018 • edited Loading

BridgeAR commented Feb 12, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

j0t3x commented Feb 13, 2018 • edited Loading

bnoordhuis commented Feb 13, 2018

BridgeAR commented Feb 16, 2018

j0t3x commented Feb 18, 2018

richardlau commented Feb 18, 2018

BridgeAR commented Feb 18, 2018

richardlau commented Feb 18, 2018

MylesBorins commented Feb 21, 2018

MylesBorins commented Aug 7, 2018

j0t3x Feb 2, 2018 •

edited

Loading

jdalton commented Feb 2, 2018 •

edited

Loading

bnoordhuis commented Feb 3, 2018 •

edited

Loading

j0t3x commented Feb 7, 2018 •

edited

Loading

j0t3x commented Feb 13, 2018 •

edited

Loading