Merge pull request #180 from browserstack/locsec/WI-82ae3d6d

Keep the access key out of child argv; honour useCaCertificate without a proxy

Author: 07souravkunda

.github/workflows/Semgrep.yml

@@ -27,7 +27,10 @@ jobs:
 
     container:
       # A Docker image with Semgrep installed. Do not change this.
-      image: returntocorp/semgrep:1.166.0
+      # Pinned to an immutable digest so a mutated tag cannot redirect CI to a
+      # different image. Refresh with:
+      #   docker manifest inspect returntocorp/semgrep:<tag>
+      image: returntocorp/semgrep:1.166.0@sha256:c180f0c93a17b420c0af5006214a29d3c747c5459c732b740191adf657dd0068
     # Skip any PR created by dependabot to avoid permission issues:
     if: (github.actor != 'dependabot[bot]')
 

lib/LocalBinary.js

@@ -46,7 +46,9 @@ function LocalBinary(){
 
     let cmd, opts;
     cmd = 'node';
-    opts = [path.join(__dirname, 'fetchDownloadSourceUrl.js'), this.key, this.bsHost];
+    /* The auth token is handed to the child through its environment, not argv —
+       argv is readable by any local user via `ps` / /proc/<pid>/cmdline. */
+    opts = [path.join(__dirname, 'fetchDownloadSourceUrl.js'), this.bsHost];
 
     if (retries == 4 || (this.downloadState.fallbackEnabled && this.parentRetries == 4)) {
       opts.push(true, this.downloadErrorMessage || this.downloadState.errorMessage);
@@ -65,6 +67,9 @@ function LocalBinary(){
 
     const userAgent = [packageName, version].join('/');
     const env = Object.assign({ 'USER_AGENT': userAgent }, process.env);
+    if (this.key) {
+      env.BROWSERSTACK_LOCAL_AUTH_TOKEN = this.key;
+    }
     const obj = childProcess.spawnSync(cmd, opts, { env: env });
     if(obj.stdout.length > 0) {
       this.sourceURL = obj.stdout.toString().replace(/\n+$/, '');
@@ -147,10 +152,11 @@ function LocalBinary(){
     var that = this;
     if(retries > 0) {
       console.log('Retrying Download. Retries left', retries);
-      fs.stat(binaryPath, function(err) {
-        if(err == null) {
-          fs.unlinkSync(binaryPath);
-        }
+      /* Single unlink instead of stat-then-unlinkSync: the gap between the two
+         let a concurrent writer swap the file, and a failing unlinkSync threw
+         out of the stat callback where it could not be caught. A missing file
+         is the expected case here, so any error is ignored. */
+      fs.unlink(binaryPath, function() {
         if(!callback) {
           return that.downloadSync(conf, destParentDir, retries - 1);
         }
@@ -322,18 +328,38 @@ function LocalBinary(){
   this.getAvailableDirs = function(){
     for(var i=0; i < this.orderedPaths.length; i++){
       var path = this.orderedPaths[i];
-      if(this.makePath(path))
+      // the last entry lives under the shared temp dir — it must be ours alone
+      var requirePrivate = (i === this.orderedPaths.length - 1);
+      if(this.makePath(path, requirePrivate))
         return path;
     }
     throw new LocalError('Error trying to download BrowserStack Local binary');
   };
 
-  this.makePath = function(path){
+  this.makePath = function(path, requirePrivate){
     try {
       if(!this.checkPath(path)){
-        fs.mkdirSync(path);
+        fs.mkdirSync(path, { mode: 0o700 });
       }
-      return true;
+      return requirePrivate ? this.isUserPrivateDir(path) : true;
+    } catch(e){
+      return false;
+    }
+  };
+
+  /* Only applied to the shared-temp fallback. The binary is written there and
+     then executed, so that directory must not be writable by anyone but us —
+     otherwise another local user can swap the binary between the download and
+     the exec, or pre-create the path as a symlink. Windows has no POSIX mode
+     bits; there this is a no-op. */
+  this.isUserPrivateDir = function(dirPath){
+    if(process.platform === 'win32' || typeof process.getuid !== 'function') return true;
+    try {
+      var stats = fs.lstatSync(dirPath);
+      if(!stats.isDirectory()) return false;
+      if(stats.uid !== process.getuid()) return false;
+      // reject group- or world-writable
+      return (stats.mode & 0o022) === 0;
     } catch(e){
       return false;
     }
@@ -361,10 +387,18 @@ function LocalBinary(){
     return home || null;
   };
 
+  /* The last entry is a per-user subdirectory of the temp dir rather than the
+     temp dir itself: os.tmpdir() is /tmp on Linux, which is world-writable, and
+     the binary name below it is fixed and predictable. */
+  this.tmpDirPath = function(){
+    var suffix = (typeof process.getuid === 'function') ? String(process.getuid()) : 'user';
+    return path.join(os.tmpdir(), 'browserstack-local-' + suffix);
+  };
+
   this.orderedPaths = [
     path.join(this.homedir(), '.browserstack'),
     process.cwd(),
-    os.tmpdir()
+    this.tmpDirPath()
   ];
 }
 

lib/download.js

@@ -2,24 +2,33 @@ const https = require('https'),
   fs = require('fs'),
   HttpsProxyAgent = require('https-proxy-agent'),
   url = require('url'),
-  zlib = require('zlib');
+  zlib = require('zlib'),
+  { isUndefined } = require('./util');
 
 const binaryPath = process.argv[2], httpPath = process.argv[3], proxyHost = process.argv[4], proxyPort = process.argv[5], useCaCertificate = process.argv[6];
 
 var fileStream = fs.createWriteStream(binaryPath);
 
 var options = url.parse(httpPath);
-if(proxyHost && proxyPort) {
+/* isUndefined, not plain truthiness: the parent passes literal `undefined`
+   placeholders for the proxy slots when only a CA is configured, and those
+   arrive here as the *string* "undefined" — which is truthy, and previously
+   built a proxy agent pointing at the host "undefined". */
+if(!isUndefined(proxyHost) && !isUndefined(proxyPort)) {
   options.agent = new HttpsProxyAgent({
     host: proxyHost,
     port: proxyPort
   });
-  if (useCaCertificate) {
-    try {
-      options.ca = fs.readFileSync(useCaCertificate);
-    } catch(err) {
-      console.log('failed to read cert file', err);
-    }
+}
+
+/* Applied regardless of whether a proxy is configured: this is the caller's TLS
+   trust anchor, and silently falling back to the system store when no proxy is
+   set ignored what they asked for. Mirrors LocalBinary.js's async download path. */
+if (!isUndefined(useCaCertificate)) {
+  try {
+    options.ca = fs.readFileSync(useCaCertificate);
+  } catch(err) {
+    console.log('failed to read cert file', err);
   }
 }
 

lib/fetchDownloadSourceUrl.js

@@ -3,7 +3,10 @@ const https = require('https'),
   HttpsProxyAgent = require('https-proxy-agent'),
   { isUndefined } = require('./util');
 
-const authToken = process.argv[2], bsHost = process.argv[3], proxyHost = process.argv[6], proxyPort = process.argv[7], useCaCertificate = process.argv[8], downloadFallback = process.argv[4], downloadErrorMessage = process.argv[5];
+/* The auth token is read from the environment, never from argv: argv is world-readable
+   via `ps` / /proc/<pid>/cmdline, whereas /proc/<pid>/environ is restricted to the
+   owning user. Keep it out of this argument list. */
+const authToken = process.env.BROWSERSTACK_LOCAL_AUTH_TOKEN, bsHost = process.argv[2], proxyHost = process.argv[5], proxyPort = process.argv[6], useCaCertificate = process.argv[7], downloadFallback = process.argv[3], downloadErrorMessage = process.argv[4];
 
 let body = '', data = {'auth_token': authToken};
 const options = {