Skip to content

Commit 1964cb1

Browse files
07souravkundaclaude
andcommitted
LOC-6805: allowlist option keys forwarded to the BrowserStackLocal binary
addArgs() prefixed ANY unrecognised key in the caller-supplied options object with '--' and pushed it, with its value, onto the daemon argv. Any caller — or upstream code merging untrusted input into options — could inject arbitrary flags into the native binary (CWE-88). Only documented BrowserStackLocal modifiers are forwarded now. The allowlist mirrors the binary's own CLI definition (COMMAND_CONFIGURATION in browserStackTunnel), long names and aliases, so every documented modifier without an explicit switch case — localProxyHost/Port/User/Pass, pac-file, custom-repeater, bs-host — keeps working. Also refused: - daemon / log-file / source, which getBinaryArgs() sets itself, so a caller cannot append a conflicting second copy (e.g. '--daemon stop' after our '--daemon start') - a value beginning with '-'. The binary's parser does not consume such a value, it reads it as another flag, so a legitimate key could still smuggle one in. These values are already mis-parsed today, so this is not a regression. - onlyCommand, a wrapper-internal key, no longer leaks into the argv. addArgs returns a LocalError, delivered through the existing paths: callback(err) for start(), returned for startSync(). Closes the entry step of chain C-008. LOC-6790 (binarypath traversal) and LOC-6777 (no binary integrity check) are unaffected and remain open — the chain description's claim that this gates binarypath is inaccurate, that value is an explicit switch case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 0d29261 commit 1964cb1

3 files changed

Lines changed: 167 additions & 19 deletions

File tree

README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ bs_local.start(bs_local_args, function() {
4040

4141
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 -
4242

43+
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.
44+
4345
#### Verbose Logging
4446
To enable verbose logging -
4547
```js

lib/Local.js

Lines changed: 95 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,60 @@ var childProcess = require('child_process'),
99
version = require('../package.json').version,
1010
treeKill = require('tree-kill');
1111

12+
// Option keys this wrapper is allowed to forward verbatim to the
13+
// BrowserStackLocal daemon. Mirrors the binary's own CLI definition
14+
// (COMMAND_CONFIGURATION in browserStackTunnel, extensions/node/config/constants.js)
15+
// — long names and their aliases — so every documented modifier keeps working
16+
// while an unrecognised key can no longer reach the daemon argv.
17+
var PASSTHROUGH_OPTIONS = [
18+
'key', 'folder', 'help', 'version', 'force', 'only',
19+
'forcelocal', 'force-local',
20+
'verbose',
21+
'onlyAutomate', 'only-automate',
22+
'proxyHost', 'proxy-host',
23+
'proxyPort', 'proxy-port',
24+
'proxyUser', 'proxy-user',
25+
'proxyPass', 'proxy-pass',
26+
'localIdentifier', 'local-identifier',
27+
'forceproxy', 'force-proxy',
28+
'region',
29+
'localProxyHost', 'local-proxy-host',
30+
'localProxyPort', 'local-proxy-port',
31+
'localProxyUser', 'local-proxy-user',
32+
'localProxyPass', 'local-proxy-pass',
33+
'enableLoggingForAPI', 'enable-logging-for-api',
34+
'logFile',
35+
'pacFile', 'pac-file',
36+
'parallelRuns', 'parallel-runs',
37+
'disableProxyDiscovery', 'disable-proxy-discovery',
38+
'enableUTCLogging', 'enable-utc-logging',
39+
'no-container',
40+
'include-hosts', 'exclude-hosts',
41+
'bsHost', 'bs-host',
42+
'debug-utility', 'debug-url',
43+
'customRepeater', 'custom-repeater',
44+
'enterprise',
45+
'use-system-installed-ca', 'use-ca-certificate',
46+
'https-ports',
47+
'ntlm-username', 'ntlm-password', 'ntlm-domain', 'ntlm-workstation',
48+
'connect-timeout',
49+
'public-interface-services',
50+
'disableDashboard', 'disable-dashboard',
51+
'config-file',
52+
'client-protocol',
53+
'identifier',
54+
'trusted-hosts'
55+
];
56+
57+
// Flags getBinaryArgs() always puts on the argv itself. Accepting them from
58+
// the options object too would let a caller append a second, conflicting copy
59+
// — e.g. '--daemon stop' after our '--daemon start'. ('logFile' has its own
60+
// supported option; only the raw binary alias is reserved.)
61+
var RESERVED_OPTIONS = ['daemon', 'log-file', 'source'];
62+
63+
// Keys consumed by this wrapper and never meant for the binary.
64+
var INTERNAL_OPTIONS = ['onlyCommand'];
65+
1266
function Local(){
1367
this.sanitizePath = function(rawPath) {
1468
var doubleQuoteIfRequired = this.windows && !rawPath.match(/"[^"]+"/) ? '"' : '';
@@ -30,7 +84,9 @@ function Local(){
3084
this.startSync = function(options) {
3185
this.userArgs = [];
3286
var that = this;
33-
this.addArgs(options);
87+
const argsError = this.addArgs(options);
88+
if(argsError)
89+
return argsError;
3490

3591
if(typeof options['onlyCommand'] !== 'undefined')
3692
return;
@@ -83,7 +139,9 @@ function Local(){
83139
this.start = function(options, callback){
84140
this.userArgs = [];
85141
var that = this;
86-
this.addArgs(options);
142+
const argsError = this.addArgs(options);
143+
if(argsError)
144+
return callback(argsError);
87145

88146
if(typeof options['onlyCommand'] !== 'undefined')
89147
return callback();
@@ -245,18 +303,46 @@ function Local(){
245303
this.binaryPath = value;
246304
break;
247305

248-
default:
249-
if(value.toString().toLowerCase() == 'true'){
250-
this.userArgs.push('--' + key);
251-
} else {
252-
this.userArgs.push('--' + key);
253-
this.userArgs.push(value);
254-
}
306+
default: {
307+
var error = this.addUserArg(key, value);
308+
if(error)
309+
return error;
255310
break;
256311
}
312+
}
257313
}
258314
};
259315

316+
// Forwards one caller-supplied option to the daemon argv, or returns a
317+
// LocalError describing why it was refused. Only documented modifiers get
318+
// through: an unknown key used to be prefixed with '--' and pushed blindly,
319+
// which let any caller inject arbitrary flags into the native binary.
320+
this.addUserArg = function(key, value){
321+
if(INTERNAL_OPTIONS.indexOf(key) !== -1)
322+
return;
323+
324+
if(RESERVED_OPTIONS.indexOf(key) !== -1)
325+
return new LocalError('Option \'' + key + '\' is set by browserstack-local itself and cannot be passed in');
326+
327+
if(PASSTHROUGH_OPTIONS.indexOf(key) === -1)
328+
return new LocalError('Unknown option \'' + key + '\'. Only documented BrowserStack Local modifiers are forwarded to the binary, see https://www.browserstack.com/local-testing#modifiers');
329+
330+
var stringValue = value === undefined || value === null ? '' : value.toString();
331+
332+
if(stringValue.toLowerCase() == 'true'){
333+
this.userArgs.push('--' + key);
334+
return;
335+
}
336+
337+
// The binary's argv parser will not consume a value that begins with '-';
338+
// it reads it as another flag instead. Refuse rather than smuggle one in.
339+
if(stringValue.charAt(0) === '-')
340+
return new LocalError('Invalid value for option \'' + key + '\': values starting with \'-\' are not allowed');
341+
342+
this.userArgs.push('--' + key);
343+
this.userArgs.push(value);
344+
};
345+
260346
this.getBinaryPath = function(callback, bsHost){
261347
if(typeof(this.binaryPath) == 'undefined'){
262348
this.binary = new LocalBinary();

test/local.js

Lines changed: 70 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -124,20 +124,80 @@ describe('Local', function () {
124124
});
125125
});
126126

127-
it('should enable custom boolean args', function (done) {
128-
bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'boolArg1': true, 'boolArg2': true }, function(){
129-
expect(bsLocal.getBinaryArgs().indexOf('--boolArg1')).to.not.equal(-1);
130-
expect(bsLocal.getBinaryArgs().indexOf('--boolArg2')).to.not.equal(-1);
127+
// LOC-6805 / LOC-6783 (F-007, CWE-88): addArgs used to prefix ANY unknown
128+
// option key with '--' and push it onto the daemon argv, letting a caller
129+
// — or upstream code merging untrusted input into `options` — inject
130+
// arbitrary flags into the native binary. Only documented BrowserStackLocal
131+
// modifiers may be forwarded now.
132+
133+
it('should reject unknown boolean args', function (done) {
134+
bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'boolArg1': true, 'boolArg2': true }, function(error){
135+
expect(error).to.be.an(Error);
136+
expect(error.toString()).to.contain('Unknown option \'boolArg1\'');
137+
expect(bsLocal.getBinaryArgs().indexOf('--boolArg1')).to.equal(-1);
138+
expect(bsLocal.getBinaryArgs().indexOf('--boolArg2')).to.equal(-1);
131139
done();
132140
});
133141
});
134142

135-
it('should enable custom keyval args', function (done) {
136-
bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'customKey1': 'custom value1', 'customKey2': 'custom value2' }, function(){
137-
expect(bsLocal.getBinaryArgs().indexOf('--customKey1')).to.not.equal(-1);
138-
expect(bsLocal.getBinaryArgs().indexOf('custom value1')).to.not.equal(-1);
139-
expect(bsLocal.getBinaryArgs().indexOf('--customKey2')).to.not.equal(-1);
140-
expect(bsLocal.getBinaryArgs().indexOf('custom value2')).to.not.equal(-1);
143+
it('should reject unknown keyval args', function (done) {
144+
bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'customKey1': 'custom value1', 'customKey2': 'custom value2' }, function(error){
145+
expect(error).to.be.an(Error);
146+
expect(error.toString()).to.contain('Unknown option \'customKey1\'');
147+
expect(bsLocal.getBinaryArgs().indexOf('--customKey1')).to.equal(-1);
148+
expect(bsLocal.getBinaryArgs().indexOf('custom value1')).to.equal(-1);
149+
done();
150+
});
151+
});
152+
153+
it('should reject the reported flag-injection payload', function (done) {
154+
bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'config': '/tmp/attacker.conf', 'daemon': 'stop' }, function(error){
155+
expect(error).to.be.an(Error);
156+
const args = bsLocal.getBinaryArgs();
157+
expect(args.indexOf('--config')).to.equal(-1);
158+
expect(args.indexOf('/tmp/attacker.conf')).to.equal(-1);
159+
// the wrapper's own '--daemon start' must be the only daemon flag
160+
expect(args.indexOf('stop')).to.equal(-1);
161+
done();
162+
});
163+
});
164+
165+
it('should reject options the wrapper sets itself', function (done) {
166+
bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'log-file': '/tmp/attacker-owned' }, function(error){
167+
expect(error).to.be.an(Error);
168+
expect(error.toString()).to.contain('set by browserstack-local itself');
169+
expect(bsLocal.getBinaryArgs().indexOf('/tmp/attacker-owned')).to.equal(-1);
170+
done();
171+
});
172+
});
173+
174+
it('should reject a value that would be parsed as another flag', function (done) {
175+
// bs-minimist does not consume a value beginning with '-'; it reads it as
176+
// a separate flag, so a legitimate key can still smuggle one in.
177+
bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'region': '--pac-file' }, function(error){
178+
expect(error).to.be.an(Error);
179+
expect(error.toString()).to.contain('values starting with \'-\' are not allowed');
180+
expect(bsLocal.getBinaryArgs().indexOf('--pac-file')).to.equal(-1);
181+
done();
182+
});
183+
});
184+
185+
it('should not forward wrapper-internal keys to the binary', function (done) {
186+
bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true }, function(error){
187+
expect(error).to.equal(undefined);
188+
expect(bsLocal.getBinaryArgs().indexOf('--onlyCommand')).to.equal(-1);
189+
done();
190+
});
191+
});
192+
193+
it('should still forward documented modifiers that have no explicit case', function (done) {
194+
bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'localProxyHost': '127.0.0.1', 'pac-file': '/tmp/proxy.pac' }, function(error){
195+
expect(error).to.equal(undefined);
196+
const args = bsLocal.getBinaryArgs();
197+
expect(args.indexOf('--localProxyHost')).to.not.equal(-1);
198+
expect(args.indexOf('127.0.0.1')).to.not.equal(-1);
199+
expect(args.indexOf('--pac-file')).to.not.equal(-1);
200+
expect(args.indexOf('/tmp/proxy.pac')).to.not.equal(-1);
141201
done();
142202
});
143203
});

0 commit comments

Comments
 (0)