Skip to content

Commit e0e8ae3

Browse files
zetlenzetlen
authored
Fix remote execution vulnerability by switching from execSync to execFileSync (#55)
* use execFileSync * add regex validation * fixup logging and interpolation
1 parent 2b1b8d4 commit e0e8ae3

File tree

9 files changed

+83
-45
lines changed

9 files changed

+83
-45
lines changed

‎src/certificate-authority.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ export default async function installCertificateAuthority(options: Options = {})
4343
generateKey(rootKeyPath);
4444

4545
debug(`Generating a CA certificate`);
46-
openssl(`req -new -x509 -config "${ caSelfSignConfig }" -key "${ rootKeyPath }" -out "${ rootCACertPath }" -days 825`);
46+
openssl(['req', '-new', '-x509', '-config', caSelfSignConfig, '-key', rootKeyPath, '-out', rootCACertPath, '-days', '825']);
4747

4848
debug('Saving certificate authority credentials');
4949
await saveCertificateAuthorityCredentials(rootKeyPath);
@@ -82,7 +82,7 @@ async function saveCertificateAuthorityCredentials(keypath: string) {
8282

8383
function certErrors(): string {
8484
try {
85-
openssl(`x509 -in "${ rootCACertPath }" -noout`);
85+
openssl(['x509', '-in', rootCACertPath, '-noout']);
8686
return '';
8787
} catch (e) {
8888
return e.toString();

‎src/certificates.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,22 +25,22 @@ export default async function generateDomainCertificate(domain: string): Promise
2525
debug(`Generating certificate signing request for ${ domain }`);
2626
let csrFile = pathForDomain(domain, `certificate-signing-request.csr`);
2727
withDomainSigningRequestConfig(domain, (configpath) => {
28-
openssl(`req -new -config "${ configpath }" -key "${ domainKeyPath }" -out "${ csrFile }"`);
28+
openssl(['req', '-new', '-config', configpath, '-key', domainKeyPath, '-out', csrFile]);
2929
});
3030

3131
debug(`Generating certificate for ${ domain } from signing request and signing with root CA`);
3232
let domainCertPath = pathForDomain(domain, `certificate.crt`);
3333

3434
await withCertificateAuthorityCredentials(({ caKeyPath, caCertPath }) => {
3535
withDomainCertificateConfig(domain, (domainCertConfigPath) => {
36-
openssl(`ca -config "${ domainCertConfigPath }" -in "${ csrFile }" -out "${ domainCertPath }" -keyfile "${ caKeyPath }" -cert "${ caCertPath }" -days 825 -batch`)
36+
openssl(['ca', '-config', domainCertConfigPath, '-in', csrFile, '-out', domainCertPath, '-keyfile', caKeyPath, '-cert', caCertPath, '-days', '825', '-batch'])
3737
});
3838
});
3939
}
4040

4141
// Generate a cryptographic key, used to sign certificates or certificate signing requests.
4242
export function generateKey(filename: string): void {
4343
debug(`generateKey: ${ filename }`);
44-
openssl(`genrsa -out "${ filename }" 2048`);
44+
openssl(['genrsa', '-out', filename, '2048']);
4545
chmod(filename, 400);
4646
}

‎src/constants.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import applicationConfigPath = require('application-config-path');
66
import eol from 'eol';
77
import { mktmp } from './utils';
88

9+
export const VALID_DOMAIN = /(?:[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?\.)+[a-z0-9][a-z0-9-]{0,61}[a-z0-9]/;
10+
911
// Platform shortcuts
1012
export const isMac = process.platform === 'darwin';
1113
export const isLinux = process.platform === 'linux';

‎src/index.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@ import {
99
pathForDomain,
1010
domainsDir,
1111
rootCAKeyPath,
12-
rootCACertPath
12+
rootCACertPath,
13+
VALID_DOMAIN
1314
} from './constants';
1415
import currentPlatform from './platforms';
1516
import installCertificateAuthority, { ensureCACertReadable, uninstall } from './certificate-authority';
@@ -65,6 +66,9 @@ type IReturnData<O extends Options = {}> = (IDomainData) & (IReturnCa<O>) & (IRe
6566
* as { caPath: string }
6667
*/
6768
export async function certificateFor<O extends Options>(domain: string, options: O = {} as O): Promise<IReturnData<O>> {
69+
if (!VALID_DOMAIN.test(domain)) {
70+
throw new Error(`"${domain}" is not a valid domain name.`);
71+
}
6872
debug(`Certificate requested for ${ domain }. Skipping certutil install: ${ Boolean(options.skipCertutilInstall) }. Skipping hosts file: ${ Boolean(options.skipHostsFile) }`);
6973

7074
if (options.ui) {

‎src/platforms/darwin.ts

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@ import path from 'path';
22
import { writeFileSync as writeFile, existsSync as exists, readFileSync as read } from 'fs';
33
import createDebug from 'debug';
44
import { sync as commandExists } from 'command-exists';
5-
import { run } from '../utils';
5+
import { run, sudoAppend } from '../utils';
66
import { Options } from '../index';
77
import { addCertificateToNSSCertDB, assertNotTouchingFiles, openCertificateInFirefox, closeFirefox, removeCertificateFromNSSCertDB } from './shared';
88
import { Platform } from '.';
99

1010
const debug = createDebug('devcert:platforms:macos');
1111

12-
const getCertUtilPath = () => path.join(run('brew --prefix nss').toString().trim(), 'bin', 'certutil');
12+
const getCertUtilPath = () => path.join(run('brew', ['--prefix', 'nss']).toString().trim(), 'bin', 'certutil');
1313

1414
export default class MacOSPlatform implements Platform {
1515

@@ -30,7 +30,20 @@ export default class MacOSPlatform implements Platform {
3030

3131
// Chrome, Safari, system utils
3232
debug('Adding devcert root CA to macOS system keychain');
33-
run(`sudo security add-trusted-cert -d -r trustRoot -k /Library/Keychains/System.keychain -p ssl -p basic "${ certificatePath }"`);
33+
run('sudo', [
34+
'security',
35+
'add-trusted-cert',
36+
'-d',
37+
'-r',
38+
'trustRoot',
39+
'-k',
40+
'/Library/Keychains/System.keychain',
41+
'-p',
42+
'ssl',
43+
'-p',
44+
'basic',
45+
certificatePath
46+
]);
3447

3548
if (this.isFirefoxInstalled()) {
3649
// Try to use certutil to install the cert automatically
@@ -39,9 +52,13 @@ export default class MacOSPlatform implements Platform {
3952
if (!options.skipCertutilInstall) {
4053
if (commandExists('brew')) {
4154
debug(`certutil is not already installed, but Homebrew is detected. Trying to install certutil via Homebrew...`);
42-
run('brew install nss');
55+
try {
56+
run('brew', ['install', 'nss'], { stdio: 'ignore' });
57+
} catch (e) {
58+
debug(`brew install nss failed`);
59+
}
4360
} else {
44-
debug(`Homebrew isn't installed, so we can't try to install certutil. Falling back to manual certificate install`);
61+
debug(`Homebrew didn't work, so we can't try to install certutil. Falling back to manual certificate install`);
4562
return await openCertificateInFirefox(this.FIREFOX_BIN_PATH, certificatePath);
4663
}
4764
} else {
@@ -59,7 +76,14 @@ export default class MacOSPlatform implements Platform {
5976
removeFromTrustStores(certificatePath: string) {
6077
debug('Removing devcert root CA from macOS system keychain');
6178
try {
62-
run(`sudo security remove-trusted-cert -d "${ certificatePath }"`);
79+
run('sudo', [
80+
'security',
81+
'remove-trusted-cert',
82+
'-d',
83+
certificatePath
84+
], {
85+
stdio: 'ignore'
86+
});
6387
} catch(e) {
6488
debug(`failed to remove ${ certificatePath } from macOS cert store, continuing. ${ e.toString() }`);
6589
}
@@ -70,30 +94,31 @@ export default class MacOSPlatform implements Platform {
7094
}
7195

7296
async addDomainToHostFileIfMissing(domain: string) {
97+
const trimDomain = domain.trim().replace(/[\s;]/g,'')
7398
let hostsFileContents = read(this.HOST_FILE_PATH, 'utf8');
74-
if (!hostsFileContents.includes(domain)) {
75-
run(`echo '\n127.0.0.1 ${ domain }' | sudo tee -a "${ this.HOST_FILE_PATH }" > /dev/null`);
99+
if (!hostsFileContents.includes(trimDomain)) {
100+
sudoAppend(this.HOST_FILE_PATH, `127.0.0.1 ${trimDomain}\n`);
76101
}
77102
}
78-
103+
79104
deleteProtectedFiles(filepath: string) {
80105
assertNotTouchingFiles(filepath, 'delete');
81-
run(`sudo rm -rf "${filepath}"`);
106+
run('sudo', ['rm', '-rf', filepath]);
82107
}
83108

84109
async readProtectedFile(filepath: string) {
85110
assertNotTouchingFiles(filepath, 'read');
86-
return (await run(`sudo cat "${filepath}"`)).toString().trim();
111+
return (await run('sudo', ['cat', filepath])).toString().trim();
87112
}
88113

89114
async writeProtectedFile(filepath: string, contents: string) {
90115
assertNotTouchingFiles(filepath, 'write');
91116
if (exists(filepath)) {
92-
await run(`sudo rm "${filepath}"`);
117+
await run('sudo', ['rm', filepath]);
93118
}
94119
writeFile(filepath, contents);
95-
await run(`sudo chown 0 "${filepath}"`);
96-
await run(`sudo chmod 600 "${filepath}"`);
120+
await run('sudo', ['chown', '0', filepath]);
121+
await run('sudo', ['chmod', '600', filepath]);
97122
}
98123

99124
private isFirefoxInstalled() {
@@ -102,7 +127,7 @@ export default class MacOSPlatform implements Platform {
102127

103128
private isNSSInstalled() {
104129
try {
105-
return run('brew list -1').toString().includes('\nnss\n');
130+
return run('brew', ['list', '-1']).toString().includes('\nnss\n');
106131
} catch (e) {
107132
return false;
108133
}

‎src/platforms/linux.ts

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { existsSync as exists, readFileSync as read, writeFileSync as writeFile
33
import createDebug from 'debug';
44
import { sync as commandExists } from 'command-exists';
55
import { addCertificateToNSSCertDB, assertNotTouchingFiles, openCertificateInFirefox, closeFirefox, removeCertificateFromNSSCertDB } from './shared';
6-
import { run } from '../utils';
6+
import { run, sudoAppend } from '../utils';
77
import { Options } from '../index';
88
import UI from '../user-interface';
99
import { Platform } from '.';
@@ -32,9 +32,9 @@ export default class LinuxPlatform implements Platform {
3232

3333
debug('Adding devcert root CA to Linux system-wide trust stores');
3434
// run(`sudo cp ${ certificatePath } /etc/ssl/certs/devcert.crt`);
35-
run(`sudo cp "${ certificatePath }" /usr/local/share/ca-certificates/devcert.crt`);
35+
run('sudo', ['cp', certificatePath, '/usr/local/share/ca-certificates/devcert.crt']);
3636
// run(`sudo bash -c "cat ${ certificatePath } >> /etc/ssl/certs/ca-certificates.crt"`);
37-
run(`sudo update-ca-certificates`);
37+
run('sudo', ['update-ca-certificates']);
3838

3939
if (this.isFirefoxInstalled()) {
4040
// Firefox
@@ -45,7 +45,7 @@ export default class LinuxPlatform implements Platform {
4545
openCertificateInFirefox(this.FIREFOX_BIN_PATH, certificatePath);
4646
} else {
4747
debug('NSS tooling is not already installed. Trying to install NSS tooling now with `apt install`');
48-
run('sudo apt install libnss3-tools');
48+
run('sudo', ['apt', 'install', 'libnss3-tools']);
4949
debug('Installing certificate into Firefox trust stores using NSS tooling');
5050
await closeFirefox();
5151
await addCertificateToNSSCertDB(this.FIREFOX_NSS_DIR, certificatePath, 'certutil');
@@ -70,8 +70,8 @@ export default class LinuxPlatform implements Platform {
7070

7171
removeFromTrustStores(certificatePath: string) {
7272
try {
73-
run(`sudo rm /usr/local/share/ca-certificates/devcert.crt`);
74-
run(`sudo update-ca-certificates`);
73+
run('sudo', ['rm', '/usr/local/share/ca-certificates/devcert.crt']);
74+
run('sudo', ['update-ca-certificates']);
7575
} catch (e) {
7676
debug(`failed to remove ${ certificatePath } from /usr/local/share/ca-certificates, continuing. ${ e.toString() }`);
7777
}
@@ -86,30 +86,31 @@ export default class LinuxPlatform implements Platform {
8686
}
8787

8888
async addDomainToHostFileIfMissing(domain: string) {
89+
const trimDomain = domain.trim().replace(/[\s;]/g,'')
8990
let hostsFileContents = read(this.HOST_FILE_PATH, 'utf8');
90-
if (!hostsFileContents.includes(domain)) {
91-
run(`echo '127.0.0.1 ${ domain }' | sudo tee -a "${ this.HOST_FILE_PATH }" > /dev/null`);
91+
if (!hostsFileContents.includes(trimDomain)) {
92+
sudoAppend(this.HOST_FILE_PATH, `127.0.0.1 ${trimDomain}\n`);
9293
}
9394
}
9495

9596
deleteProtectedFiles(filepath: string) {
9697
assertNotTouchingFiles(filepath, 'delete');
97-
run(`sudo rm -rf "${filepath}"`);
98+
run('sudo', ['rm', '-rf', filepath]);
9899
}
99100

100101
async readProtectedFile(filepath: string) {
101102
assertNotTouchingFiles(filepath, 'read');
102-
return (await run(`sudo cat "${filepath}"`)).toString().trim();
103+
return (await run('sudo', ['cat', filepath])).toString().trim();
103104
}
104105

105106
async writeProtectedFile(filepath: string, contents: string) {
106107
assertNotTouchingFiles(filepath, 'write');
107108
if (exists(filepath)) {
108-
await run(`sudo rm "${filepath}"`);
109+
await run('sudo', ['rm', filepath]);
109110
}
110111
writeFile(filepath, contents);
111-
await run(`sudo chown 0 "${filepath}"`);
112-
await run(`sudo chmod 600 "${filepath}"`);
112+
await run('sudo', ['chown', '0', filepath]);
113+
await run('sudo', ['chmod', '600', filepath]);
113114
}
114115

115116
private isFirefoxInstalled() {

‎src/platforms/shared.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ export function addCertificateToNSSCertDB(nssDirGlob: string, certPath: string,
3939
debug(`trying to install certificate into NSS databases in ${ nssDirGlob }`);
4040
doForNSSCertDB(nssDirGlob, (dir, version) => {
4141
const dirArg = version === 'modern' ? `sql:${ dir }` : dir;
42-
run(`${ certutilPath } -A -d "${ dirArg }" -t 'C,,' -i "${ certPath }" -n devcert`)
42+
run(certutilPath, ['-A', '-d', dirArg, '-t', 'C,,', '-i', certPath, '-n', 'devcert']);
4343
});
4444
debug(`finished scanning & installing certificate in NSS databases in ${ nssDirGlob }`);
4545
}
@@ -49,7 +49,7 @@ export function removeCertificateFromNSSCertDB(nssDirGlob: string, certPath: str
4949
doForNSSCertDB(nssDirGlob, (dir, version) => {
5050
const dirArg = version === 'modern' ? `sql:${ dir }` : dir;
5151
try {
52-
run(`${ certutilPath } -A -d "${ dirArg }" -t 'C,,' -i "${ certPath }" -n devcert`)
52+
run(certutilPath, ['-A', '-d', dirArg, '-t', 'C,,', '-i', certPath, '-n', 'devcert']);
5353
} catch (e) {
5454
debug(`failed to remove ${ certPath } from ${ dir }, continuing. ${ e.toString() }`)
5555
}
@@ -124,7 +124,7 @@ export async function openCertificateInFirefox(firefoxPath: string, certPath: st
124124
}).listen(port);
125125
debug('Certificate server is up. Printing instructions for user and launching Firefox with hosted certificate URL');
126126
await UI.startFirefoxWizard(`http://localhost:${ port }`);
127-
run(`${ firefoxPath } http://localhost:${ port }`);
127+
run(firefoxPath, [`http://localhost:${ port }`]);
128128
await UI.waitForFirefoxWizard();
129129
server.close();
130130
}

‎src/platforms/win32.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ export default class WindowsPlatform implements Platform {
2828
// IE, Chrome, system utils
2929
debug('adding devcert root to Windows OS trust store')
3030
try {
31-
run(`certutil -addstore -user root "${ certificatePath }"`);
31+
run('certutil', ['-addstore', '-user', 'root', certificatePath]);
3232
} catch (e) {
3333
e.output.map((buffer: Buffer) => {
3434
if (buffer) {
@@ -49,7 +49,7 @@ export default class WindowsPlatform implements Platform {
4949
debug('removing devcert root from Windows OS trust store');
5050
try {
5151
console.warn('Removing old certificates from trust stores. You may be prompted to grant permission for this. It\'s safe to delete old devcert certificates.');
52-
run(`certutil -delstore -user root devcert`);
52+
run('certutil', ['-delstore', '-user', 'root', 'devcert']);
5353
} catch (e) {
5454
debug(`failed to remove ${ certificatePath } from Windows OS trust store, continuing. ${ e.toString() }`)
5555
}

‎src/utils.ts

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { execSync, ExecSyncOptions } from 'child_process';
1+
import { execFileSync, ExecFileSyncOptions } from 'child_process';
22
import tmp from 'tmp';
33
import createDebug from 'debug';
44
import path from 'path';
@@ -8,18 +8,24 @@ import { configPath } from './constants';
88

99
const debug = createDebug('devcert:util');
1010

11-
export function openssl(cmd: string) {
12-
return run(`openssl ${ cmd }`, {
11+
export function openssl(args: string[]) {
12+
return run('openssl', args, {
1313
stdio: 'pipe',
1414
env: Object.assign({
1515
RANDFILE: path.join(configPath('.rnd'))
1616
}, process.env)
1717
});
1818
}
1919

20-
export function run(cmd: string, options: ExecSyncOptions = {}) {
21-
debug(`exec: \`${ cmd }\``);
22-
return execSync(cmd, options);
20+
export function run(cmd: string, args: string[], options: ExecFileSyncOptions = {}) {
21+
debug(`execFileSync: \`${ cmd } ${args.join(' ')}\``);
22+
return execFileSync(cmd, args, options);
23+
}
24+
25+
export function sudoAppend(file: string, input: ExecFileSyncOptions["input"]) {
26+
run('sudo', ['tee', '-a', file], {
27+
input
28+
});
2329
}
2430

2531
export function waitForUser() {

0 commit comments

Comments
 (0)