diff --git a/README.md b/README.md index 6166a8f..24c30ad 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,8 @@ bs_local.start(bs_local_args, function() { Apart from the key, all other BrowserStack Local modifiers are optional. For the full list of modifiers, refer [BrowserStack Local modifiers](https://www.browserstack.com/local-testing#modifiers). For examples, refer below - +Only documented modifiers are forwarded to the binary. An unrecognised option key, an option the wrapper sets itself (`daemon`, `log-file`), or a value beginning with `-` is refused with an error rather than passed through to the `BrowserStackLocal` argv — otherwise any code that merges untrusted input into the options object could inject arbitrary flags into the binary. + #### Verbose Logging To enable verbose logging - ```js diff --git a/lib/Local.js b/lib/Local.js index 8f783d7..ea19edb 100644 --- a/lib/Local.js +++ b/lib/Local.js @@ -9,6 +9,68 @@ var childProcess = require('child_process'), version = require('../package.json').version, treeKill = require('tree-kill'); +// Option keys this wrapper is allowed to forward verbatim to the +// BrowserStackLocal daemon. Mirrors the binary's own CLI definition +// (COMMAND_CONFIGURATION in browserStackTunnel, extensions/node/config/constants.js) +// — long names and their aliases — so every documented modifier keeps working +// while an unrecognised key can no longer reach the daemon argv. +// +// Deliberately a COMPLETE mirror, so it can be diffed against the binary's CLI +// when that gains a flag. Some entries (key, folder, force, only, forcelocal, +// verbose, onlyAutomate, proxyHost/Port/User/Pass, localIdentifier, forceproxy, +// logFile, parallelRuns) are handled by an explicit case in addArgs and so never +// reach this list at runtime; they are listed for completeness, not effect. +var PASSTHROUGH_OPTIONS = [ + 'key', 'folder', 'help', 'version', 'force', 'only', + 'forcelocal', 'force-local', + 'verbose', + 'onlyAutomate', 'only-automate', + 'proxyHost', 'proxy-host', + 'proxyPort', 'proxy-port', + 'proxyUser', 'proxy-user', + 'proxyPass', 'proxy-pass', + 'localIdentifier', 'local-identifier', + 'forceproxy', 'force-proxy', + 'region', + 'localProxyHost', 'local-proxy-host', + 'localProxyPort', 'local-proxy-port', + 'localProxyUser', 'local-proxy-user', + 'localProxyPass', 'local-proxy-pass', + 'enableLoggingForAPI', 'enable-logging-for-api', + 'logFile', + 'pacFile', 'pac-file', + 'parallelRuns', 'parallel-runs', + 'disableProxyDiscovery', 'disable-proxy-discovery', + 'enableUTCLogging', 'enable-utc-logging', + 'no-container', + 'include-hosts', 'exclude-hosts', + 'bsHost', 'bs-host', + 'debug-utility', 'debug-url', + 'customRepeater', 'custom-repeater', + 'enterprise', + 'use-system-installed-ca', 'use-ca-certificate', + 'https-ports', + 'ntlm-username', 'ntlm-password', 'ntlm-domain', 'ntlm-workstation', + 'connect-timeout', + 'public-interface-services', + 'disableDashboard', 'disable-dashboard', + 'config-file', + 'client-protocol', + 'identifier', + 'trusted-hosts' +]; + +// Flags getBinaryArgs() always puts on the argv itself. Accepting them from +// the options object too would let a caller append a second, conflicting copy +// — e.g. '--daemon stop' after our '--daemon start'. The log file is settable, +// but only through the wrapper's own 'logfile'/'logFile' case above, which +// routes it into getBinaryArgs' single '--log-file'; the binary's raw +// 'log-file' alias is reserved so it cannot add a second one. +var RESERVED_OPTIONS = ['daemon', 'log-file', 'source']; + +// Keys consumed by this wrapper and never meant for the binary. +var INTERNAL_OPTIONS = ['onlyCommand']; + function Local(){ this.sanitizePath = function(rawPath) { var doubleQuoteIfRequired = this.windows && !rawPath.match(/"[^"]+"/) ? '"' : ''; @@ -30,7 +92,9 @@ function Local(){ this.startSync = function(options) { this.userArgs = []; var that = this; - this.addArgs(options); + const argsError = this.addArgs(options); + if(argsError) + return argsError; if(typeof options['onlyCommand'] !== 'undefined') return; @@ -83,7 +147,9 @@ function Local(){ this.start = function(options, callback){ this.userArgs = []; var that = this; - this.addArgs(options); + const argsError = this.addArgs(options); + if(argsError) + return callback(argsError); if(typeof options['onlyCommand'] !== 'undefined') return callback(); @@ -150,6 +216,15 @@ function Local(){ for(var key in options){ var value = options[key]; + // Runs for EVERY key, including the ones with an explicit case below. + // A value is only ever safe as a value: the binary's parser will not + // consume one that begins with '-', it reads it as another flag — and it + // accepts the '--flag=value' form, so a value like '--log-file=/tmp/x' + // smuggles a complete flag in through an otherwise legitimate option. + var valueError = this.rejectFlagLikeValue(key, value); + if(valueError) + return valueError; + switch(key){ case 'key': if(value) @@ -245,15 +320,61 @@ function Local(){ this.binaryPath = value; break; - default: - if(value.toString().toLowerCase() == 'true'){ - this.userArgs.push('--' + key); - } else { - this.userArgs.push('--' + key); - this.userArgs.push(value); - } + default: { + var error = this.addUserArg(key, value); + if(error) + return error; break; } + } + } + }; + + // Returns a LocalError if any part of the value would be read as a flag + // rather than as this option's value. List-typed options (--include-hosts, + // --exclude-hosts) take an array, so every element is checked. + this.rejectFlagLikeValue = function(key, value){ + var values = Array.isArray(value) ? value : [value]; + for(var i = 0; i < values.length; i++){ + if(values[i] === undefined || values[i] === null) + continue; + if(values[i].toString().charAt(0) === '-') + return new LocalError('Invalid value for option \'' + key + '\': values starting with \'-\' are not allowed'); + } + }; + + // Forwards one caller-supplied option to the daemon argv, or returns a + // LocalError describing why it was refused. Only documented modifiers get + // through: an unknown key used to be prefixed with '--' and pushed blindly, + // which let any caller inject arbitrary flags into the native binary. + // The value has already been checked by rejectFlagLikeValue in addArgs. + this.addUserArg = function(key, value){ + if(INTERNAL_OPTIONS.indexOf(key) !== -1) + return; + + if(RESERVED_OPTIONS.indexOf(key) !== -1) + return new LocalError('Option \'' + key + '\' is set by browserstack-local itself and cannot be passed in'); + + if(PASSTHROUGH_OPTIONS.indexOf(key) === -1) + return new LocalError('Unknown option \'' + key + '\'. Only documented BrowserStack Local modifiers are forwarded to the binary, see https://www.browserstack.com/local-testing#modifiers'); + + // Match the explicit cases above, which all guard with `if(value)`. + if(value === undefined || value === null) + return; + + if(value.toString().toLowerCase() == 'true'){ + this.userArgs.push('--' + key); + return; + } + + // argv elements must be strings — execFile/spawnSync reject anything else. + this.userArgs.push('--' + key); + if(Array.isArray(value)){ + for(var i = 0; i < value.length; i++){ + this.userArgs.push(value[i].toString()); + } + } else { + this.userArgs.push(value.toString()); } }; diff --git a/test/local.js b/test/local.js index 79c10eb..e83bf95 100644 --- a/test/local.js +++ b/test/local.js @@ -124,20 +124,136 @@ describe('Local', function () { }); }); - it('should enable custom boolean args', function (done) { - bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'boolArg1': true, 'boolArg2': true }, function(){ - expect(bsLocal.getBinaryArgs().indexOf('--boolArg1')).to.not.equal(-1); - expect(bsLocal.getBinaryArgs().indexOf('--boolArg2')).to.not.equal(-1); + // Argument injection (CWE-88): addArgs used to prefix ANY unknown option key + // with '--' and push it onto the daemon argv, letting a caller — or upstream + // code merging untrusted input into `options` — inject arbitrary flags into + // the native binary. Only documented BrowserStackLocal modifiers may be + // forwarded now, and no value may pose as a flag. + + it('should reject unknown boolean args', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'boolArg1': true, 'boolArg2': true }, function(error){ + expect(error).to.be.an(Error); + expect(error.toString()).to.contain('Unknown option \'boolArg1\''); + expect(bsLocal.getBinaryArgs().indexOf('--boolArg1')).to.equal(-1); + expect(bsLocal.getBinaryArgs().indexOf('--boolArg2')).to.equal(-1); done(); }); }); - it('should enable custom keyval args', function (done) { - bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'customKey1': 'custom value1', 'customKey2': 'custom value2' }, function(){ - expect(bsLocal.getBinaryArgs().indexOf('--customKey1')).to.not.equal(-1); - expect(bsLocal.getBinaryArgs().indexOf('custom value1')).to.not.equal(-1); - expect(bsLocal.getBinaryArgs().indexOf('--customKey2')).to.not.equal(-1); - expect(bsLocal.getBinaryArgs().indexOf('custom value2')).to.not.equal(-1); + it('should reject unknown keyval args', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'customKey1': 'custom value1', 'customKey2': 'custom value2' }, function(error){ + expect(error).to.be.an(Error); + expect(error.toString()).to.contain('Unknown option \'customKey1\''); + expect(bsLocal.getBinaryArgs().indexOf('--customKey1')).to.equal(-1); + expect(bsLocal.getBinaryArgs().indexOf('custom value1')).to.equal(-1); + done(); + }); + }); + + it('should reject the reported flag-injection payload', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'config': '/tmp/attacker.conf', 'daemon': 'stop' }, function(error){ + expect(error).to.be.an(Error); + const args = bsLocal.getBinaryArgs(); + expect(args.indexOf('--config')).to.equal(-1); + expect(args.indexOf('/tmp/attacker.conf')).to.equal(-1); + // the wrapper's own '--daemon start' must be the only daemon flag + expect(args.indexOf('stop')).to.equal(-1); + done(); + }); + }); + + it('should reject options the wrapper sets itself', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'log-file': '/tmp/attacker-owned' }, function(error){ + expect(error).to.be.an(Error); + expect(error.toString()).to.contain('set by browserstack-local itself'); + expect(bsLocal.getBinaryArgs().indexOf('/tmp/attacker-owned')).to.equal(-1); + done(); + }); + }); + + it('should reject a value that would be parsed as another flag', function (done) { + // bs-minimist does not consume a value beginning with '-'; it reads it as + // a separate flag, so a legitimate key can still smuggle one in. + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'region': '--pac-file' }, function(error){ + expect(error).to.be.an(Error); + expect(error.toString()).to.contain('values starting with \'-\' are not allowed'); + expect(bsLocal.getBinaryArgs().indexOf('--pac-file')).to.equal(-1); + done(); + }); + }); + + // The value check must cover keys that have an explicit case too, not just + // the ones reaching the allowlist. bs-minimist accepts '--flag=value', so a + // smuggled flag carries its own value and needs no following argv slot. + it('should reject a flag-like value on an explicitly handled option', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'localIdentifier': '--log-file=/tmp/attacker-owned' }, function(error){ + expect(error).to.be.an(Error); + expect(error.toString()).to.contain('values starting with \'-\' are not allowed'); + const args = bsLocal.getBinaryArgs(); + expect(args.indexOf('--log-file=/tmp/attacker-owned')).to.equal(-1); + expect(args.indexOf('--local-identifier')).to.equal(-1); + done(); + }); + }); + + it('should reject a daemon-lifecycle smuggle through an explicitly handled option', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'only': '--daemon=stop' }, function(error){ + expect(error).to.be.an(Error); + expect(bsLocal.getBinaryArgs().indexOf('--daemon=stop')).to.equal(-1); + done(); + }); + }); + + it('should reject a flag-like element inside a list-valued option', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'include-hosts': ['localhost', '--config-file=/tmp/attacker.yml'] }, function(error){ + expect(error).to.be.an(Error); + expect(bsLocal.getBinaryArgs().indexOf('--config-file=/tmp/attacker.yml')).to.equal(-1); + done(); + }); + }); + + it('should forward a list-valued option as separate argv elements', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'include-hosts': ['localhost', '127.0.0.1'] }, function(error){ + expect(error).to.equal(undefined); + const args = bsLocal.getBinaryArgs(); + expect(args.indexOf('--include-hosts')).to.not.equal(-1); + expect(args.indexOf('localhost')).to.not.equal(-1); + expect(args.indexOf('127.0.0.1')).to.not.equal(-1); + done(); + }); + }); + + it('should skip a null or undefined passthrough value instead of pushing it raw', function (done) { + // execFile/spawnSync reject a non-string argv element, so a raw null here + // used to throw ERR_INVALID_ARG_TYPE out of start() instead of erroring. + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'region': null, 'connect-timeout': 30 }, function(error){ + expect(error).to.equal(undefined); + const args = bsLocal.getBinaryArgs(); + expect(args.indexOf('--region')).to.equal(-1); + expect(args.indexOf(null)).to.equal(-1); + // numbers are coerced, so every argv element is a string + expect(args.indexOf('30')).to.not.equal(-1); + expect(args.every(function(a){ return typeof a === 'string'; })).to.equal(true); + done(); + }); + }); + + it('should not forward wrapper-internal keys to the binary', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true }, function(error){ + expect(error).to.equal(undefined); + expect(bsLocal.getBinaryArgs().indexOf('--onlyCommand')).to.equal(-1); + done(); + }); + }); + + it('should still forward documented modifiers that have no explicit case', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'localProxyHost': '127.0.0.1', 'pac-file': '/tmp/proxy.pac' }, function(error){ + expect(error).to.equal(undefined); + const args = bsLocal.getBinaryArgs(); + expect(args.indexOf('--localProxyHost')).to.not.equal(-1); + expect(args.indexOf('127.0.0.1')).to.not.equal(-1); + expect(args.indexOf('--pac-file')).to.not.equal(-1); + expect(args.indexOf('/tmp/proxy.pac')).to.not.equal(-1); done(); }); });