From 7852a152b8674c9f4594d7b43363f1c7f58d8a6e Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Fri, 14 Aug 2026 16:21:49 +0530 Subject: [PATCH] Keep the access key out of child argv; honour useCaCertificate without a proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four hardening fixes to the binary download path, all in the same area: - The access key was passed to lib/fetchDownloadSourceUrl.js as a positional argv element, so it was readable by any local user via `ps` or /proc//cmdline for the lifetime of the spawn. It now travels in the child's environment (/proc//environ is restricted to the owning user) and the remaining argv slots shift down by one accordingly. (CWE-214) - lib/download.js only applied `useCaCertificate` inside the `if (proxyHost && proxyPort)` branch, so a caller-supplied TLS trust anchor was ignored whenever no proxy was configured. Worse, the parent passes the literal `undefined` placeholders for the proxy slots in exactly that case, which arrive as the truthy *string* "undefined" — so the sync download built a proxy agent for host "undefined" and failed outright with `getaddrinfo ENOTFOUND undefined`. Both are fixed: the CA is applied unconditionally, and the proxy slots are compared with the existing `isUndefined` helper (as lib/fetchDownloadSourceUrl.js already does). (CWE-295) - retryBinaryDownload did an async fs.stat followed by a synchronous fs.unlinkSync inside the callback. Collapsed to a single ENOENT-tolerant fs.unlink, removing the window between the two and the uncatchable throw a failing unlinkSync raised from within the stat callback. (CWE-362) - getAvailableDirs fell back to os.tmpdir() itself — /tmp on Linux, which is world-writable — under a fixed, predictable binary name. It now uses a per-uid subdirectory created 0700, and that fallback is rejected unless it is a real directory owned by us and not group/world-writable, so a pre-created symlink or shared directory cannot be used as the destination for a binary we are about to execute. Only the temp fallback is subjected to this check; $HOME/.browserstack and cwd are unchanged. (CWE-377) Also pins the Semgrep CI container to an immutable digest so a mutated tag cannot redirect the workflow to a different image. (CWE-829) --- .github/workflows/Semgrep.yml | 5 +++- lib/LocalBinary.js | 54 ++++++++++++++++++++++++++++------- lib/download.js | 25 ++++++++++------ lib/fetchDownloadSourceUrl.js | 5 +++- 4 files changed, 69 insertions(+), 20 deletions(-) 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 = {