diff --git a/.github/workflows/Semgrep.yml b/.github/workflows/Semgrep.yml index 95c5710..c5e4e21 100644 --- a/.github/workflows/Semgrep.yml +++ b/.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: + image: returntocorp/semgrep:1.166.0@sha256:c180f0c93a17b420c0af5006214a29d3c747c5459c732b740191adf657dd0068 # Skip any PR created by dependabot to avoid permission issues: if: (github.actor != 'dependabot[bot]') diff --git a/lib/Local.js b/lib/Local.js index 8f783d7..05d29e0 100644 --- a/lib/Local.js +++ b/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; diff --git a/lib/LocalBinary.js b/lib/LocalBinary.js index 8d694b2..00e1748 100644 --- a/lib/LocalBinary.js +++ b/lib/LocalBinary.js @@ -20,6 +20,18 @@ 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 */ @@ -27,17 +39,19 @@ function LocalBinary(){ 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//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() ]; } diff --git a/lib/download.js b/lib/download.js index 0b0e094..dde74a2 100644 --- a/lib/download.js +++ b/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); } } diff --git a/lib/fetchDownloadSourceUrl.js b/lib/fetchDownloadSourceUrl.js index df5c8f2..6b5d37c 100644 --- a/lib/fetchDownloadSourceUrl.js +++ b/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//cmdline, whereas /proc//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 = { diff --git a/test/local.js b/test/local.js index 576ad79..abe162c 100644 --- a/test/local.js +++ b/test/local.js @@ -490,3 +490,74 @@ describe('LocalBinary', function () { }); }); }); + +// Regression tests: the binary-download fallback signalling used to live on +// process.env, so (a) a value planted in process.env steered the download to an +// arbitrary host with no validation, and (b) a failure on one Local instance bled +// into every sibling instance in the same process. Both flip from FAIL on the +// pre-fix code to PASS once the state is per-instance. +describe('Binary download state isolation', function () { + var sandBox, childProcess; + var Local = require('../lib/Local'); + + beforeEach(function () { + sandBox = sinon.sandbox.create(); + childProcess = require('child_process'); + }); + + afterEach(function () { + sandBox.restore(); + delete process.env.BINARY_DOWNLOAD_SOURCE_URL; + delete process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED; + delete process.env.BINARY_DOWNLOAD_ERROR_MESSAGE; + }); + + it('does not honor a BINARY_DOWNLOAD_SOURCE_URL planted in process.env', function () { + // An attacker (CI secret injection, malicious dep, shared-workspace .env) or a + // sibling instance leaves these two vars set. + process.env.BINARY_DOWNLOAD_SOURCE_URL = 'https://attacker.example.com/evil'; + process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = 'true'; + + // Stub the endpoint API child process so nothing hits the network; the stub + // stands in for a legitimate BrowserStack endpoint response. + var spawnStub = sandBox.stub(childProcess, 'spawnSync', function () { + return { + stdout: Buffer.from('https://legit.browserstack.com/bs\n'), + stderr: Buffer.from('') + }; + }); + + var binary = new LocalBinary(); + binary.key = 'DUMMY'; + binary.bsHost = 'local.browserstack.com'; + binary.parentRetries = 9; + + var url = binary.getSourceUrlSync({}, 9); + + // Pre-fix: the env-shortcut returns the attacker URL and spawnSync is never + // reached. Post-fix: the shortcut is gone, so the real endpoint call runs. + expect(url).to.not.equal('https://attacker.example.com/evil'); + expect(url).to.equal('https://legit.browserstack.com/bs'); + expect(spawnStub.called).to.equal(true); + }); + + it('keeps download-fallback state per Local instance (no cross-instance bleed)', function () { + var a = new Local(); + var b = new Local(); + + // Instance A records a download failure (as its retry catch block does). + a.binaryDownloadState.fallbackEnabled = true; + a.binaryDownloadState.errorMessage = 'A private error: key=A_SECRET'; + a.binaryDownloadState.sourceURL = 'https://a-context.example/bs'; + + // Instance B, which never failed, must be unaffected. + expect(b.binaryDownloadState.fallbackEnabled).to.equal(false); + expect(b.binaryDownloadState.errorMessage).to.equal(null); + expect(b.binaryDownloadState.sourceURL).to.equal(null); + + // And nothing leaked to the process-global env. + expect(process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED).to.equal(undefined); + expect(process.env.BINARY_DOWNLOAD_SOURCE_URL).to.equal(undefined); + expect(process.env.BINARY_DOWNLOAD_ERROR_MESSAGE).to.equal(undefined); + }); +}); diff --git a/test/local_start_output_handling.js b/test/local_start_output_handling.js new file mode 100644 index 0000000..458759d --- /dev/null +++ b/test/local_start_output_handling.js @@ -0,0 +1,104 @@ +var expect = require('expect.js'), + fs = require('fs'), + os = require('os'), + path = require('path'), + browserstack = require('../index'); + +// Regression tests for LOC-7325. +// +// `Local.start` handles the binary's output inside an `execFile` callback. A +// throw there is raised by node's internal exithandler, so no try/catch around +// `start()` can intercept it — it surfaces as an uncaughtException and the +// blast radius is set by the host process's exception policy. These tests drive +// `start()` with stub binaries that reproduce each output shape and assert the +// callback fires exactly once with an error, and that nothing throws. +// +// Stubs are shell scripts, so these are skipped on Windows. +describe('Local.start output handling', function () { + var stubDir, bsLocal; + + function stub(name, body) { + var stubPath = path.join(stubDir, name); + fs.writeFileSync(stubPath, '#!/bin/sh\n' + body + '\n', { mode: 0o755 }); + return stubPath; + } + + // Drives start() with the given stub and collects every callback invocation + // plus any uncaughtException raised out of the execFile callback. + function run(stubPath, done) { + var calls = [], uncaught = []; + var existing = process.listeners('uncaughtException'); + process.removeAllListeners('uncaughtException'); + process.on('uncaughtException', function (err) { uncaught.push(err); }); + + bsLocal.binaryPath = stubPath; + bsLocal.start({ key: 'dummy-key', localIdentifier: 'loc-7325' }, function (error) { + calls.push(error); + }); + + // Settle past the execFile callback before asserting, so a second + // (throwing) invocation would have happened by now if it were going to. + setTimeout(function () { + process.removeAllListeners('uncaughtException'); + existing.forEach(function (listener) { process.on('uncaughtException', listener); }); + done(calls, uncaught); + }, 1000); + } + + before(function () { + stubDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bs-local-7325-')); + }); + + beforeEach(function () { + bsLocal = new browserstack.Local(); + // Keep the stubs from clobbering ./local.log in the repo root. + bsLocal.logfile = path.join(stubDir, 'local.log'); + }); + + if (os.platform().match(/win32/i)) { + it.skip('skipped on Windows (stub binaries are shell scripts)'); + return; + } + + it('reports an error exactly once when the binary exits with no output', function (done) { + this.timeout(10000); + run(stub('empty-output.sh', 'exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0]).to.be.an('object'); + expect(calls[0].message).to.equal('No output received'); + done(); + }); + }); + + it('reports an error exactly once when the binary emits non-JSON output', function (done) { + this.timeout(10000); + run(stub('garbage-output.sh', 'echo "segmentation fault"; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.match(/^Invalid output received: /); + expect(calls[0].extra).to.match(/segmentation fault/); + done(); + }); + }); + + it('reports a fallback message when a non-connected payload has no message key', function (done) { + this.timeout(10000); + run(stub('no-message-key.sh', 'echo \'{"state":"disconnected"}\'; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.equal('Failed to start BrowserStack Local'); + done(); + }); + }); + + it('surfaces the binary message when a non-connected payload carries one', function (done) { + this.timeout(10000); + run(stub('with-message.sh', 'echo \'{"state":"disconnected","message":{"message":"Invalid key"}}\'; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.equal('Invalid key'); + done(); + }); + }); +});