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/LocalBinary.js b/lib/LocalBinary.js index 8d694b2..0ac0a65 100644 --- a/lib/LocalBinary.js +++ b/lib/LocalBinary.js @@ -34,7 +34,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//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); @@ -53,6 +55,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+$/, ''); @@ -135,10 +140,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 +316,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 +375,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 = {