Fix remote execution vulnerability by switching from execSync to exec…

…FileSync (#55) * use execFileSync * add regex validation * fixup logging and interpolation
davewasmer · Jun 10, 2020 · e0e8ae3 · e0e8ae3
1 parent 2b1b8d4
commit e0e8ae3
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 45 deletions.
diff --git a/src/certificate-authority.ts b/src/certificate-authority.ts
@@ -43,7 +43,7 @@ export default async function installCertificateAuthority(options: Options = {})
   generateKey(rootKeyPath);
 
   debug(`Generating a CA certificate`);
-  openssl(`req -new -x509 -config "${ caSelfSignConfig }" -key "${ rootKeyPath }" -out "${ rootCACertPath }" -days 825`);
+  openssl(['req', '-new', '-x509', '-config', caSelfSignConfig, '-key', rootKeyPath, '-out', rootCACertPath, '-days', '825']);
 
   debug('Saving certificate authority credentials');
   await saveCertificateAuthorityCredentials(rootKeyPath);
@@ -82,7 +82,7 @@ async function saveCertificateAuthorityCredentials(keypath: string) {
 
 function certErrors(): string {
   try {
-    openssl(`x509 -in "${ rootCACertPath }" -noout`);
+    openssl(['x509', '-in', rootCACertPath, '-noout']);
     return '';
   } catch (e) {
     return e.toString();

diff --git a/src/certificates.ts b/src/certificates.ts
@@ -25,22 +25,22 @@ export default async function generateDomainCertificate(domain: string): Promise
   debug(`Generating certificate signing request for ${ domain }`);
   let csrFile = pathForDomain(domain, `certificate-signing-request.csr`);
   withDomainSigningRequestConfig(domain, (configpath) => {
-    openssl(`req -new -config "${ configpath }" -key "${ domainKeyPath }" -out "${ csrFile }"`);
+    openssl(['req', '-new', '-config', configpath, '-key', domainKeyPath, '-out', csrFile]);
   });
 
   debug(`Generating certificate for ${ domain } from signing request and signing with root CA`);
   let domainCertPath = pathForDomain(domain, `certificate.crt`);
 
   await withCertificateAuthorityCredentials(({ caKeyPath, caCertPath }) => {
     withDomainCertificateConfig(domain, (domainCertConfigPath) => {
-      openssl(`ca -config "${ domainCertConfigPath }" -in "${ csrFile }" -out "${ domainCertPath }" -keyfile "${ caKeyPath }" -cert "${ caCertPath }" -days 825 -batch`)
+      openssl(['ca', '-config', domainCertConfigPath, '-in', csrFile, '-out', domainCertPath, '-keyfile', caKeyPath, '-cert', caCertPath, '-days', '825', '-batch'])
     });
   });
 }
 
 // Generate a cryptographic key, used to sign certificates or certificate signing requests.
 export function generateKey(filename: string): void {
   debug(`generateKey: ${ filename }`);
-  openssl(`genrsa -out "${ filename }" 2048`);
+  openssl(['genrsa', '-out', filename, '2048']);
   chmod(filename, 400);
 }
diff --git a/src/constants.ts b/src/constants.ts
@@ -6,6 +6,8 @@ import applicationConfigPath = require('application-config-path');
 import eol from 'eol';
 import { mktmp } from './utils';
 
+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]/;
+
 // Platform shortcuts
 export const isMac = process.platform === 'darwin';
 export const isLinux = process.platform === 'linux';

diff --git a/src/index.ts b/src/index.ts
@@ -9,7 +9,8 @@ import {
   pathForDomain,
   domainsDir,
   rootCAKeyPath,
-  rootCACertPath
+  rootCACertPath,
+  VALID_DOMAIN
 } from './constants';
 import currentPlatform from './platforms';
 import installCertificateAuthority, { ensureCACertReadable, uninstall } from './certificate-authority';
@@ -65,6 +66,9 @@ type IReturnData<O extends Options = {}> = (IDomainData) & (IReturnCa<O>) & (IRe
  * as { caPath: string }
  */
 export async function certificateFor<O extends Options>(domain: string, options: O = {} as O): Promise<IReturnData<O>> {
+  if (!VALID_DOMAIN.test(domain)) {
+    throw new Error(`"${domain}" is not a valid domain name.`);
+  }
   debug(`Certificate requested for ${ domain }. Skipping certutil install: ${ Boolean(options.skipCertutilInstall) }. Skipping hosts file: ${ Boolean(options.skipHostsFile) }`);
 
   if (options.ui) {

diff --git a/src/platforms/darwin.ts b/src/platforms/darwin.ts
@@ -2,14 +2,14 @@ import path from 'path';
 import { writeFileSync as writeFile, existsSync as exists, readFileSync as read } from 'fs';
 import createDebug from 'debug';
 import { sync as commandExists } from 'command-exists';
-import { run } from '../utils';
+import { run, sudoAppend } from '../utils';
 import { Options } from '../index';
 import { addCertificateToNSSCertDB, assertNotTouchingFiles, openCertificateInFirefox, closeFirefox, removeCertificateFromNSSCertDB } from './shared';
 import { Platform } from '.';
 
 const debug = createDebug('devcert:platforms:macos');
 
-const getCertUtilPath = () => path.join(run('brew --prefix nss').toString().trim(), 'bin', 'certutil');
+const getCertUtilPath = () => path.join(run('brew', ['--prefix', 'nss']).toString().trim(), 'bin', 'certutil');
 
 export default class MacOSPlatform implements Platform {
 
@@ -30,7 +30,20 @@ export default class MacOSPlatform implements Platform {
 
     // Chrome, Safari, system utils
     debug('Adding devcert root CA to macOS system keychain');
-    run(`sudo security add-trusted-cert -d -r trustRoot -k /Library/Keychains/System.keychain -p ssl -p basic "${ certificatePath }"`);
+    run('sudo', [
+      'security',
+      'add-trusted-cert',
+      '-d',
+      '-r',
+      'trustRoot',
+      '-k',
+      '/Library/Keychains/System.keychain',
+      '-p',
+      'ssl',
+      '-p',
+      'basic',
+      certificatePath
+    ]);
 
     if (this.isFirefoxInstalled()) {
       // Try to use certutil to install the cert automatically
@@ -39,9 +52,13 @@ export default class MacOSPlatform implements Platform {
         if (!options.skipCertutilInstall) {
           if (commandExists('brew')) {
             debug(`certutil is not already installed, but Homebrew is detected. Trying to install certutil via Homebrew...`);
-            run('brew install nss');
+            try {
+              run('brew', ['install', 'nss'], { stdio: 'ignore' });
+            } catch (e) {
+              debug(`brew install nss failed`);
+            }
           } else {
-            debug(`Homebrew isn't installed, so we can't try to install certutil. Falling back to manual certificate install`);
+            debug(`Homebrew didn't work, so we can't try to install certutil. Falling back to manual certificate install`);
             return await openCertificateInFirefox(this.FIREFOX_BIN_PATH, certificatePath);
           }
         } else {
@@ -59,7 +76,14 @@ export default class MacOSPlatform implements Platform {
   removeFromTrustStores(certificatePath: string) {
     debug('Removing devcert root CA from macOS system keychain');
     try {
-      run(`sudo security remove-trusted-cert -d "${ certificatePath }"`);
+      run('sudo', [
+        'security',
+        'remove-trusted-cert',
+        '-d',
+        certificatePath
+      ], {
+        stdio: 'ignore'
+      });
     } catch(e) {
       debug(`failed to remove ${ certificatePath } from macOS cert store, continuing. ${ e.toString() }`);
     }
@@ -70,30 +94,31 @@ export default class MacOSPlatform implements Platform {
   }
 
   async addDomainToHostFileIfMissing(domain: string) {
+    const trimDomain = domain.trim().replace(/[\s;]/g,'')
     let hostsFileContents = read(this.HOST_FILE_PATH, 'utf8');
-    if (!hostsFileContents.includes(domain)) {
-      run(`echo '\n127.0.0.1 ${ domain }' | sudo tee -a "${ this.HOST_FILE_PATH }" > /dev/null`);
+    if (!hostsFileContents.includes(trimDomain)) {
+      sudoAppend(this.HOST_FILE_PATH, `127.0.0.1 ${trimDomain}\n`);
     }
   }
-  
+
   deleteProtectedFiles(filepath: string) {
     assertNotTouchingFiles(filepath, 'delete');
-    run(`sudo rm -rf "${filepath}"`);
+    run('sudo', ['rm', '-rf', filepath]);
   }
 
   async readProtectedFile(filepath: string) {
     assertNotTouchingFiles(filepath, 'read');
-    return (await run(`sudo cat "${filepath}"`)).toString().trim();
+    return (await run('sudo', ['cat', filepath])).toString().trim();
   }
 
   async writeProtectedFile(filepath: string, contents: string) {
     assertNotTouchingFiles(filepath, 'write');
     if (exists(filepath)) {
-      await run(`sudo rm "${filepath}"`);
+      await run('sudo', ['rm', filepath]);
     }
     writeFile(filepath, contents);
-    await run(`sudo chown 0 "${filepath}"`);
-    await run(`sudo chmod 600 "${filepath}"`);
+    await run('sudo', ['chown', '0', filepath]);
+    await run('sudo', ['chmod', '600', filepath]);
   }
 
   private isFirefoxInstalled() {
@@ -102,7 +127,7 @@ export default class MacOSPlatform implements Platform {
 
   private isNSSInstalled() {
     try {
-      return run('brew list -1').toString().includes('\nnss\n');
+      return run('brew', ['list', '-1']).toString().includes('\nnss\n');
     } catch (e) {
       return false;
     }

diff --git a/src/platforms/linux.ts b/src/platforms/linux.ts
@@ -3,7 +3,7 @@ import { existsSync as exists, readFileSync as read, writeFileSync as writeFile
 import createDebug from 'debug';
 import { sync as commandExists } from 'command-exists';
 import { addCertificateToNSSCertDB, assertNotTouchingFiles, openCertificateInFirefox, closeFirefox, removeCertificateFromNSSCertDB } from './shared';
-import { run } from '../utils';
+import { run, sudoAppend } from '../utils';
 import { Options } from '../index';
 import UI from '../user-interface';
 import { Platform } from '.';
@@ -32,9 +32,9 @@ export default class LinuxPlatform implements Platform {
 
     debug('Adding devcert root CA to Linux system-wide trust stores');
     // run(`sudo cp ${ certificatePath } /etc/ssl/certs/devcert.crt`);
-    run(`sudo cp "${ certificatePath }" /usr/local/share/ca-certificates/devcert.crt`);
+    run('sudo', ['cp', certificatePath, '/usr/local/share/ca-certificates/devcert.crt']);
     // run(`sudo bash -c "cat ${ certificatePath } >> /etc/ssl/certs/ca-certificates.crt"`);
-    run(`sudo update-ca-certificates`);
+    run('sudo', ['update-ca-certificates']);
 
     if (this.isFirefoxInstalled()) {
       // Firefox
@@ -45,7 +45,7 @@ export default class LinuxPlatform implements Platform {
           openCertificateInFirefox(this.FIREFOX_BIN_PATH, certificatePath);
         } else {
           debug('NSS tooling is not already installed. Trying to install NSS tooling now with `apt install`');
-          run('sudo apt install libnss3-tools');
+          run('sudo',  ['apt', 'install', 'libnss3-tools']);
           debug('Installing certificate into Firefox trust stores using NSS tooling');
           await closeFirefox();
           await addCertificateToNSSCertDB(this.FIREFOX_NSS_DIR, certificatePath, 'certutil');
@@ -70,8 +70,8 @@ export default class LinuxPlatform implements Platform {
 
   removeFromTrustStores(certificatePath: string) {
     try {
-      run(`sudo rm /usr/local/share/ca-certificates/devcert.crt`);
-      run(`sudo update-ca-certificates`); 
+      run('sudo', ['rm', '/usr/local/share/ca-certificates/devcert.crt']);
+      run('sudo', ['update-ca-certificates']);
     } catch (e) {
       debug(`failed to remove ${ certificatePath } from /usr/local/share/ca-certificates, continuing. ${ e.toString() }`);
     }
@@ -86,30 +86,31 @@ export default class LinuxPlatform implements Platform {
   }
 
   async addDomainToHostFileIfMissing(domain: string) {
+    const trimDomain = domain.trim().replace(/[\s;]/g,'')
     let hostsFileContents = read(this.HOST_FILE_PATH, 'utf8');
-    if (!hostsFileContents.includes(domain)) {
-      run(`echo '127.0.0.1  ${ domain }' | sudo tee -a "${ this.HOST_FILE_PATH }" > /dev/null`);
+    if (!hostsFileContents.includes(trimDomain)) {
+      sudoAppend(this.HOST_FILE_PATH, `127.0.0.1 ${trimDomain}\n`);
     }
   }
 
   deleteProtectedFiles(filepath: string) {
     assertNotTouchingFiles(filepath, 'delete');
-    run(`sudo rm -rf "${filepath}"`);
+    run('sudo', ['rm', '-rf', filepath]);
   }
 
   async readProtectedFile(filepath: string) {
     assertNotTouchingFiles(filepath, 'read');
-    return (await run(`sudo cat "${filepath}"`)).toString().trim();
+    return (await run('sudo', ['cat', filepath])).toString().trim();
   }
 
   async writeProtectedFile(filepath: string, contents: string) {
     assertNotTouchingFiles(filepath, 'write');
     if (exists(filepath)) {
-      await run(`sudo rm "${filepath}"`);
+      await run('sudo', ['rm', filepath]);
     }
     writeFile(filepath, contents);
-    await run(`sudo chown 0 "${filepath}"`);
-    await run(`sudo chmod 600 "${filepath}"`);
+    await run('sudo', ['chown', '0', filepath]);
+    await run('sudo', ['chmod', '600', filepath]);
   }
 
   private isFirefoxInstalled() {

diff --git a/src/platforms/shared.ts b/src/platforms/shared.ts
@@ -39,7 +39,7 @@ export function addCertificateToNSSCertDB(nssDirGlob: string, certPath: string,
   debug(`trying to install certificate into NSS databases in ${ nssDirGlob }`);
   doForNSSCertDB(nssDirGlob, (dir, version) => {
     const dirArg = version === 'modern' ? `sql:${ dir }` : dir;
-    run(`${ certutilPath } -A -d "${ dirArg }" -t 'C,,' -i "${ certPath }" -n devcert`)
+      run(certutilPath, ['-A', '-d', dirArg, '-t', 'C,,', '-i', certPath, '-n', 'devcert']);
   });
   debug(`finished scanning & installing certificate in NSS databases in ${ nssDirGlob }`);
 }
@@ -49,7 +49,7 @@ export function removeCertificateFromNSSCertDB(nssDirGlob: string, certPath: str
   doForNSSCertDB(nssDirGlob, (dir, version) => {
     const dirArg = version === 'modern' ? `sql:${ dir }` : dir;
     try {
-      run(`${ certutilPath } -A -d "${ dirArg }" -t 'C,,' -i "${ certPath }" -n devcert`)
+      run(certutilPath, ['-A', '-d', dirArg, '-t', 'C,,', '-i', certPath, '-n', 'devcert']);
     } catch (e) {
       debug(`failed to remove ${ certPath } from ${ dir }, continuing. ${ e.toString() }`)
     }
@@ -124,7 +124,7 @@ export async function openCertificateInFirefox(firefoxPath: string, certPath: st
   }).listen(port);
   debug('Certificate server is up. Printing instructions for user and launching Firefox with hosted certificate URL');
   await UI.startFirefoxWizard(`http://localhost:${ port }`);
-  run(`${ firefoxPath } http://localhost:${ port }`);
+  run(firefoxPath, [`http://localhost:${ port }`]);
   await UI.waitForFirefoxWizard();
   server.close();
 }

diff --git a/src/platforms/win32.ts b/src/platforms/win32.ts
@@ -28,7 +28,7 @@ export default class WindowsPlatform implements Platform {
     // IE, Chrome, system utils
     debug('adding devcert root to Windows OS trust store')
     try {
-      run(`certutil -addstore -user root "${ certificatePath }"`);
+      run('certutil', ['-addstore', '-user', 'root', certificatePath]);
     } catch (e) {
       e.output.map((buffer: Buffer) => {
         if (buffer) {
@@ -49,7 +49,7 @@ export default class WindowsPlatform implements Platform {
     debug('removing devcert root from Windows OS trust store');
     try {
       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.');
-      run(`certutil -delstore -user root devcert`);
+      run('certutil', ['-delstore', '-user', 'root', 'devcert']);
     } catch (e) {
       debug(`failed to remove ${ certificatePath } from Windows OS trust store, continuing. ${ e.toString() }`)
     }

diff --git a/src/utils.ts b/src/utils.ts
@@ -1,4 +1,4 @@
-import { execSync, ExecSyncOptions } from 'child_process';
+import { execFileSync, ExecFileSyncOptions } from 'child_process';
 import tmp from 'tmp';
 import createDebug from 'debug';
 import path from 'path';
@@ -8,18 +8,24 @@ import { configPath } from './constants';
 
 const debug = createDebug('devcert:util');
 
-export function openssl(cmd: string) {
-  return run(`openssl ${ cmd }`, {
+export function openssl(args: string[]) {
+  return run('openssl', args, {
     stdio: 'pipe',
     env: Object.assign({
       RANDFILE: path.join(configPath('.rnd'))
     }, process.env)
   });
 }
 
-export function run(cmd: string, options: ExecSyncOptions = {}) {
-  debug(`exec: \`${ cmd }\``);
-  return execSync(cmd, options);
+export function run(cmd: string, args: string[], options: ExecFileSyncOptions = {}) {
+  debug(`execFileSync: \`${ cmd } ${args.join(' ')}\``);
+  return execFileSync(cmd, args, options);
+}
+
+export function sudoAppend(file: string, input: ExecFileSyncOptions["input"]) {
+  run('sudo', ['tee', '-a', file], {
+    input
+  });
 }
 
 export function waitForUser() {