lib,src: audit process.env in lib/ for setuid binary

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: #18511 Fixes: #9160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
nodejs · Feb 16, 2018 · 916cfec · 916cfec
1 parent ec9e792
commit 916cfec
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 7 deletions.
diff --git a/lib/module.js b/lib/module.js
@@ -34,6 +34,7 @@ const {
   internalModuleReadJSON,
   internalModuleStat
 } = process.binding('fs');
+const { safeGetenv } = process.binding('util');
 const internalModule = require('internal/module');
 const preserveSymlinks = !!process.binding('config').preserveSymlinks;
 const experimentalModules = !!process.binding('config').experimentalModules;
@@ -697,10 +698,13 @@ Module._initPaths = function() {
   const isWindows = process.platform === 'win32';
 
   var homeDir;
+  var nodePath;
   if (isWindows) {
     homeDir = process.env.USERPROFILE;
+    nodePath = process.env.NODE_PATH;
   } else {
-    homeDir = process.env.HOME;
+    homeDir = safeGetenv('HOME');
+    nodePath = safeGetenv('NODE_PATH');
   }
 
   // $PREFIX/lib/node, where $PREFIX is the root of the Node.js installation.
@@ -719,7 +723,6 @@ Module._initPaths = function() {
     paths.unshift(path.resolve(homeDir, '.node_modules'));
   }
 
-  var nodePath = process.env.NODE_PATH;
   if (nodePath) {
     paths = nodePath.split(path.delimiter).filter(function(path) {
       return !!path;

diff --git a/lib/os.js b/lib/os.js
@@ -21,7 +21,7 @@
 
 'use strict';
 
-const { pushValToArrayMax } = process.binding('util');
+const { pushValToArrayMax, safeGetenv } = process.binding('util');
 const constants = process.binding('constants').os;
 const { deprecate } = require('internal/util');
 const { getCIDRSuffix } = require('internal/os');
@@ -127,9 +127,9 @@ function tmpdir() {
     if (path.length > 1 && path.endsWith('\\') && !path.endsWith(':\\'))
       path = path.slice(0, -1);
   } else {
-    path = process.env.TMPDIR ||
-           process.env.TMP ||
-           process.env.TEMP ||
+    path = safeGetenv('TMPDIR') ||
+           safeGetenv('TMP') ||
+           safeGetenv('TEMP') ||
            '/tmp';
     if (path.length > 1 && path.endsWith('/'))
       path = path.slice(0, -1);

diff --git a/src/node_util.cc b/src/node_util.cc
@@ -14,6 +14,7 @@ using v8::Object;
 using v8::Private;
 using v8::Promise;
 using v8::Proxy;
+using v8::String;
 using v8::Value;
 
 
@@ -174,6 +175,16 @@ void PromiseReject(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(ret.FromMaybe(false));
 }
 
+void SafeGetenv(const FunctionCallbackInfo<Value>& args) {
+  CHECK(args[0]->IsString());
+  Utf8Value strenvtag(args.GetIsolate(), args[0]);
+  std::string text;
+  if (!node::SafeGetenv(*strenvtag, &text)) return;
+  args.GetReturnValue()
+      .Set(String::NewFromUtf8(
+            args.GetIsolate(), text.c_str(),
+            v8::NewStringType::kNormal).ToLocalChecked());
+}
 
 void Initialize(Local<Object> target,
                 Local<Value> unused,
@@ -225,6 +236,8 @@ void Initialize(Local<Object> target,
   env->SetMethod(target, "createPromise", CreatePromise);
   env->SetMethod(target, "promiseResolve", PromiseResolve);
   env->SetMethod(target, "promiseReject", PromiseReject);
+
+  env->SetMethod(target, "safeGetenv", SafeGetenv);
 }
 
 }  // namespace util

diff --git a/test/parallel/test-util-internal.js b/test/parallel/test-util-internal.js
@@ -8,9 +8,17 @@ const fixtures = require('../common/fixtures');
 const {
   getHiddenValue,
   setHiddenValue,
-  arrow_message_private_symbol: kArrowMessagePrivateSymbolIndex
+  arrow_message_private_symbol: kArrowMessagePrivateSymbolIndex,
+  safeGetenv
 } = process.binding('util');
 
+for (const oneEnv in process.env) {
+  assert.strictEqual(
+    safeGetenv(oneEnv),
+    process.env[oneEnv]
+  );
+}
+
 assert.strictEqual(
   getHiddenValue({}, kArrowMessagePrivateSymbolIndex),
   undefined);