Merge pull request #184 from browserstack/release_1.5.14

Release 1.5.14

Author: AdityaHirapara

.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/Local.js

@@ -23,6 +23,16 @@ function Local(){
   this.logfile = this.sanitizePath(path.join(process.cwd(), 'local.log'));
   this.opcode = 'start';
   this.exitCallback;
+  /*
+   * Binary-download fallback signalling, scoped to THIS Local instance. Replaces
+   * the former process.env.BINARY_DOWNLOAD_* globals, which bled retry/fallback
+   * state (and the cached source URL) across every concurrent Local instance in
+   * the process and let a pre-set env var steer the download to an arbitrary
+   * host. This single object is shared with each LocalBinary the retry loop
+   * creates, so the fallback URL is still cached across retries of THIS instance
+   * only.
+   */
+  this.binaryDownloadState = { fallbackEnabled: false, errorMessage: null, sourceURL: null };
 
   this.errorRegex = /\*\*\* Error: [^\r\n]*/i;
   this.doneRegex = /Press Ctrl-C to exit/i;
@@ -57,7 +67,7 @@ function Local(){
       else
         return new LocalError('No output received');
       if(data['state'] != 'connected'){
-        return new LocalError(data['message']['message']);
+        return new LocalError(that.getErrorMessage(data));
       } else {
         that.pid = data['pid'];
         that.isProcessRunning = true;
@@ -71,8 +81,8 @@ function Local(){
         that.retriesLeft -= 1;
         fs.unlinkSync(that.binaryPath);
         delete(that.binaryPath);
-        process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
-        process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
+        that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
+        that.binaryDownloadState.fallbackEnabled = true;
         return that.startSync(options);
       } else {
         throw new LocalError(error.toString());
@@ -106,25 +116,31 @@ function Local(){
             that.retriesLeft -= 1;
             fs.unlinkSync(that.binaryPath);
             delete(that.binaryPath);
-            process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
-            process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
+            that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
+            that.binaryDownloadState.fallbackEnabled = true;
             that.start(options, callback);
             return;
           } else {
             callback(new LocalError(error.toString()));
+            return;
           }
         }
 
         var data = {};
-        if(stdout)
-          data = JSON.parse(stdout);
-        else if(stderr)
-          data = JSON.parse(stderr);
-        else
+        var output = stdout || stderr;
+        if(!output) {
           callback(new LocalError('No output received'));
+          return;
+        }
+        try {
+          data = JSON.parse(output);
+        } catch(parseError) {
+          callback(new LocalError('Invalid output received: ' + parseError.message, output));
+          return;
+        }
 
         if(data['state'] != 'connected'){
-          callback(new LocalError(data['message']['message']));
+          callback(new LocalError(that.getErrorMessage(data)));
         } else {
           that.pid = data['pid'];
           that.isProcessRunning = true;
@@ -134,6 +150,17 @@ function Local(){
     }, options['bs-host']);
   };
 
+  // The binary reports failures as {"state": "...", "message": {"message": "..."}},
+  // but not every non-connected payload carries a message key. Dereferencing it
+  // blindly throws, and inside the execFile callback that throw is an
+  // uncaughtException the caller cannot catch. See LOC-7325.
+  this.getErrorMessage = function(data){
+    var message = data && data['message'];
+    if(message && typeof message === 'object')
+      message = message['message'];
+    return message || 'Failed to start BrowserStack Local';
+  };
+
   this.isRunning = function(){
     return this.pid && running(this.pid) && this.isProcessRunning;
   };
@@ -260,6 +287,10 @@ function Local(){
   this.getBinaryPath = function(callback, bsHost){
     if(typeof(this.binaryPath) == 'undefined'){
       this.binary = new LocalBinary();
+      /* Share THIS instance's download-fallback state so it survives across the
+       * LocalBinary objects recreated during the retry loop, without ever
+       * touching process-global state. */
+      this.binary.downloadState = this.binaryDownloadState;
       var conf = {};
       if(this.proxyHost && this.proxyPort){
         conf.proxyHost = this.proxyHost;

lib/LocalBinary.js

@@ -20,24 +20,38 @@ function LocalBinary(){
   this.baseRetries = 9;
   this.sourceURL = null;
   this.downloadErrorMessage = null;
+  /*
+   * Per-instance binary-download signalling. Historically these three fields were
+   * carried on process.env (BINARY_DOWNLOAD_FALLBACK_ENABLED / _ERROR_MESSAGE /
+   * _SOURCE_URL), which is a process-global mutable store: a failure on one Local
+   * instance bled into every other instance in the same process, and an attacker
+   * who could set the env before boot could force this instance to download from
+   * an arbitrary host. Keep the state on the instance instead. The owning Local
+   * object shares ONE downloadState object across the LocalBinary instances it
+   * recreates during a retry loop, so the fallback URL is still cached within a
+   * single Local instance without leaking across sibling instances.
+   */
+  this.downloadState = { fallbackEnabled: false, errorMessage: null, sourceURL: null };
 
   this.getSourceUrlSync = function(conf, retries) {
     /* Request for an endpoint to download the local binary from Rails no more than twice with 5 retries each */
     if (![4, 9].includes(retries) && this.sourceURL != null) {
       return this.sourceURL;
     }
 
-    if (process.env.BINARY_DOWNLOAD_SOURCE_URL !== undefined && process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED == 'true' && this.parentRetries != 4) {
+    if (this.downloadState.sourceURL != null && this.downloadState.fallbackEnabled && this.parentRetries != 4) {
       /* This is triggered from Local.js if there's an error executing the downloaded binary */
-      return process.env.BINARY_DOWNLOAD_SOURCE_URL;
+      return this.downloadState.sourceURL;
     }
 
     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 || (process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED == 'true' && this.parentRetries == 4)) {
-      opts.push(true, this.downloadErrorMessage || process.env.BINARY_DOWNLOAD_ERROR_MESSAGE);
+    if (retries == 4 || (this.downloadState.fallbackEnabled && this.parentRetries == 4)) {
+      opts.push(true, this.downloadErrorMessage || this.downloadState.errorMessage);
     } else {
       opts.push(false, null);
     }
@@ -53,10 +67,13 @@ 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+$/, '');
-      process.env.BINARY_DOWNLOAD_SOURCE_URL = this.sourceURL;
+      this.downloadState.sourceURL = this.sourceURL;
       return this.sourceURL;
     } else if(obj.stderr.length > 0) {
       let output = Buffer.from(JSON.parse(JSON.stringify(obj.stderr)).data).toString();
@@ -70,23 +87,23 @@ function LocalBinary(){
       return callback(null, this.sourceURL);
     }
 
-    if (process.env.BINARY_DOWNLOAD_SOURCE_URL !== undefined && process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED == 'true' && this.parentRetries != 4) {
+    if (this.downloadState.sourceURL != null && this.downloadState.fallbackEnabled && this.parentRetries != 4) {
       /* This is triggered from Local.js if there's an error executing the downloaded binary */
-      return callback(null, process.env.BINARY_DOWNLOAD_SOURCE_URL);
+      return callback(null, this.downloadState.sourceURL);
     }
 
     let downloadFallback = false;
     let downloadErrorMessage = null;
 
-    if (retries == 4 || (process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED == 'true' && this.parentRetries == 4)) {
+    if (retries == 4 || (this.downloadState.fallbackEnabled && this.parentRetries == 4)) {
       downloadFallback = true;
-      downloadErrorMessage = this.downloadErrorMessage || process.env.BINARY_DOWNLOAD_ERROR_MESSAGE;
+      downloadErrorMessage = this.downloadErrorMessage || this.downloadState.errorMessage;
     }
 
     fetchDownloadSourceUrlAsync(this.key, this.bsHost, downloadFallback, downloadErrorMessage, conf.proxyHost, conf.proxyPort, conf.useCaCertificate, (err, sourceURL) => {
       if (err) return callback(err);
       this.sourceURL = sourceURL;
-      process.env.BINARY_DOWNLOAD_SOURCE_URL = sourceURL;
+      this.downloadState.sourceURL = sourceURL;
       callback(null, sourceURL);
     });
   };
@@ -135,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);
         }
@@ -310,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;
     }
@@ -349,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 = {