Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove protection on CA file after upgrade #48

Merged
merged 3 commits into from
Nov 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 73 additions & 42 deletions src/certificate-authority.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,20 @@
import path from 'path';
import {
unlinkSync as rm,
readFileSync as readFile,
writeFileSync as writeFile,
existsSync as exists
writeFileSync as writeFile
} from 'fs';
import { sync as rimraf } from 'rimraf';
import createDebug from 'debug';

import {
domainsDir,
rootCADir,
ensureConfigDirs,
getLegacyConfigDir,
rootCAKeyPath,
rootCACertPath,
caSelfSignConfig,
opensslSerialFilePath,
opensslDatabaseFilePath,
isWindows,
isLinux,
caVersionFile
} from './constants';
import currentPlatform from './platforms';
Expand All @@ -30,10 +29,11 @@ const debug = createDebug('devcert:certificate-authority');
* per-app certs.
*/
export default async function installCertificateAuthority(options: Options = {}): Promise<void> {
debug(`Checking if older devcert install is present`);
scrubOldInsecureVersions();
debug(`Uninstalling existing certificates, which will be void once any existing CA is gone`);
uninstall();
ensureConfigDirs();

debug(`Generating a root certificate authority`);
debug(`Making a temp working directory for files to copied in`);
let rootKeyPath = mktmp();

debug(`Generating the OpenSSL configuration needed to setup the certificate authority`);
Expand All @@ -52,39 +52,6 @@ export default async function installCertificateAuthority(options: Options = {})
await currentPlatform.addToTrustStores(rootCACertPath, options);
}

/**
* Older versions of devcert left the root certificate keys unguarded and
* accessible by userland processes. Here, we check for evidence of this older
* version, and if found, we delete the root certificate keys to remove the
* attack vector.
*/
function scrubOldInsecureVersions() {
// Use the old verion's logic for determining config directory
let configDir: string;
if (isWindows && process.env.LOCALAPPDATA) {
configDir = path.join(process.env.LOCALAPPDATA, 'devcert', 'config');
} else {
let uid = process.getuid && process.getuid();
let userHome = (isLinux && uid === 0) ? path.resolve('/usr/local/share') : require('os').homedir();
configDir = path.join(userHome, '.config', 'devcert');
}

// Delete the root certificate keys, as well as the generated app certificates
debug(`Checking ${ configDir } for legacy files ...`);
[
path.join(configDir, 'openssl.conf'),
path.join(configDir, 'devcert-ca-root.key'),
path.join(configDir, 'devcert-ca-root.crt'),
path.join(configDir, 'devcert-ca-version'),
path.join(configDir, 'certs')
].forEach((filepath) => {
if (exists(filepath)) {
debug(`Removing legacy file: ${ filepath }`)
rimraf(filepath);
}
});
}

/**
* Initializes the files OpenSSL needs to sign certificates as a certificate
* authority, as well as our CA setup version
Expand All @@ -111,3 +78,67 @@ async function saveCertificateAuthorityCredentials(keypath: string) {
let key = readFile(keypath, 'utf-8');
await currentPlatform.writeProtectedFile(rootCAKeyPath, key);
}


function certErrors(): string {
try {
openssl(`x509 -in "${ rootCACertPath }" -noout`);
return '';
} catch (e) {
return e.toString();
}
}

// This function helps to migrate from v1.0.x to >= v1.1.0.
/**
* Smoothly migrate the certificate storage from v1.0.x to >= v1.1.0.
* In v1.1.0 there are new options for retrieving the CA cert directly,
* to help third-party Node apps trust the root CA.
*
* If a v1.0.x cert already exists, then devcert has written it with
* platform.writeProtectedFile(), so an unprivileged readFile cannot access it.
* Pre-detect and remedy this; it should only happen once per installation.
*/
export async function ensureCACertReadable(options: Options = {}): Promise<void> {
if (!certErrors()) {
return;
}
/**
* on windows, writeProtectedFile left the cert encrypted on *nix, the cert
* has no read permissions either way, openssl will fail and that means we
* have to fix it
*/
try {
const caFileContents = await currentPlatform.readProtectedFile(rootCACertPath);
currentPlatform.deleteProtectedFiles(rootCACertPath);
writeFile(rootCACertPath, caFileContents);
} catch (e) {
return installCertificateAuthority(options);
}

// double check that we have a live one
const remainingErrors = certErrors();
if (remainingErrors) {
return installCertificateAuthority(options);
}
Comment on lines +120 to +123
Copy link
Contributor

@Js-Brecht Js-Brecht Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be misreading this
I did misread this, sorry.

Irrelevant comments

Should this condition be this?

- if (remainingErrors) {
+ if (!remainingErrors) {
      return installCertificateAuthority(options);
  }

from line 119

return '' // Success returns a falsy value, yeah?

Copy link
Contributor

@Js-Brecht Js-Brecht Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I know why you're doing this now. Just kinda popped into my head 😆 Need to recreate the certificate authority if it is invalid.

So, now my question becomes: shouldn't the old domain certificates, the old CA files, and the old CA from the trust stores be removed? If creating a new CA, then all of that stuff will be invalid.

I know that the installCertificateAuthority routine checks for older versions.... maybe a new function that can be run in various circumstances when it's desirable to wipe out the current devcert CA state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it would be easier to read if certErrors(): string was like certIsValid(): boolean, but I did it this way to lay the groundwork for more sophisticated error reporting in the future.

As for removing the old certificates, you're right--and some of those will also be protected. Time to do even more scrubbing!

Copy link
Contributor

@Js-Brecht Js-Brecht Nov 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was all me. I don't think it's necessarily hard to read; I just had it in my head for some reason that the next reasonable step to take upon successful conversion was to run the install routine. Somehow I forgot that the certificate would have already been installed previously if it got to this point 🤷‍♂. Doubting whether I was reading it right was my subconscious telling me I absolutely was not, I think. 😄

I did it this way to lay the groundwork for more sophisticated error reporting in the future.

Out of curiosity, how would you feel about extending this cert checking & scrubbing process to check for an expired CA + renewing it (and its trusts/signed certs)? I could write up a PR when I have some more free time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that sounds great, @Js-Brecht . But yeah, it can be a subsequent PR.

Because of the breaking change in #41, I've had to revamp the installation/uninstallation process, so I've just updated this PR a fair bit. Please take another look!

}

/**
* Remove as much of the devcert files and state as we can. This is necessary
* when generating a new root certificate, and should be available to API
* consumers as well.
*
* Not all of it will be removable. If certutil is not installed, we'll leave
* Firefox alone. We try to remove files with maximum permissions, and if that
* fails, we'll silently fail.
*
* It's also possible that the command to untrust will not work, and we'll
* silently fail that as well; with no existing certificates anymore, the
* security exposure there is minimal.
*/
export function uninstall(): void {
currentPlatform.removeFromTrustStores(rootCACertPath);
currentPlatform.deleteProtectedFiles(domainsDir);
currentPlatform.deleteProtectedFiles(rootCADir);
currentPlatform.deleteProtectedFiles(getLegacyConfigDir());
}
23 changes: 20 additions & 3 deletions src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,23 @@ export const rootCADir = configPath('certificate-authority');
export const rootCAKeyPath = configPath('certificate-authority', 'private-key.key');
export const rootCACertPath = configPath('certificate-authority', 'certificate.cert');

mkdirp(configDir);
mkdirp(domainsDir);
mkdirp(rootCADir);


// Exposed for uninstallation purposes.
export function getLegacyConfigDir(): string {
if (isWindows && process.env.LOCALAPPDATA) {
return path.join(process.env.LOCALAPPDATA, 'devcert', 'config');
} else {
let uid = process.getuid && process.getuid();
let userHome = (isLinux && uid === 0) ? path.resolve('/usr/local/share') : require('os').homedir();
return path.join(userHome, '.config', 'devcert');
}
}

export function ensureConfigDirs() {
mkdirp(configDir);
mkdirp(domainsDir);
mkdirp(rootCADir);
}

ensureConfigDirs();
6 changes: 5 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ import {
rootCACertPath
} from './constants';
import currentPlatform from './platforms';
import installCertificateAuthority from './certificate-authority';
import installCertificateAuthority, { ensureCACertReadable, uninstall } from './certificate-authority';
import generateDomainCertificate from './certificates';
import UI, { UserInterface } from './user-interface';
export { uninstall };

const debug = createDebug('devcert');

Expand Down Expand Up @@ -84,6 +85,9 @@ export async function certificateFor<O extends Options>(domain: string, options:
if (!exists(rootCAKeyPath)) {
debug('Root CA is not installed yet, so it must be our first run. Installing root CA ...');
await installCertificateAuthority(options);
} else if (options.getCaBuffer || options.getCaPath) {
debug('Root CA is not readable, but it probably is because an earlier version of devcert locked it. Trying to fix...');
await ensureCACertReadable(options);
}

if (!exists(pathForDomain(domain, `certificate.crt`))) {
Expand Down
26 changes: 23 additions & 3 deletions src/platforms/darwin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ import createDebug from 'debug';
import { sync as commandExists } from 'command-exists';
import { run } from '../utils';
import { Options } from '../index';
import { addCertificateToNSSCertDB, openCertificateInFirefox, closeFirefox } from './shared';
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');

export default class MacOSPlatform implements Platform {

Expand Down Expand Up @@ -48,26 +49,45 @@ export default class MacOSPlatform implements Platform {
return await openCertificateInFirefox(this.FIREFOX_BIN_PATH, certificatePath);
}
}
let certutilPath = path.join(run('brew --prefix nss').toString().trim(), 'bin', 'certutil');
await closeFirefox();
await addCertificateToNSSCertDB(this.FIREFOX_NSS_DIR, certificatePath, certutilPath);
await addCertificateToNSSCertDB(this.FIREFOX_NSS_DIR, certificatePath, getCertUtilPath());
} else {
debug('Firefox does not appear to be installed, skipping Firefox-specific steps...');
}
}

removeFromTrustStores(certificatePath: string) {
debug('Removing devcert root CA from macOS system keychain');
try {
run(`sudo security remove-trusted-cert -d "${ certificatePath }"`);
} catch(e) {
debug(`failed to remove ${ certificatePath } from macOS cert store, continuing. ${ e.toString() }`);
}
if (this.isFirefoxInstalled() && this.isNSSInstalled()) {
debug('Firefox install and certutil install detected. Trying to remove root CA from Firefox NSS databases');
removeCertificateFromNSSCertDB(this.FIREFOX_NSS_DIR, certificatePath, getCertUtilPath());
}
}

async addDomainToHostFileIfMissing(domain: string) {
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`);
}
}

deleteProtectedFiles(filepath: string) {
assertNotTouchingFiles(filepath, 'delete');
run(`sudo rm -rf "${filepath}"`);
}

async readProtectedFile(filepath: string) {
assertNotTouchingFiles(filepath, 'read');
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}"`);
}
Expand Down
2 changes: 2 additions & 0 deletions src/platforms/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import { Options } from '../index';

export interface Platform {
addToTrustStores(certificatePath: string, options?: Options): Promise<void>;
removeFromTrustStores(certificatePath: string): void;
addDomainToHostFileIfMissing(domain: string): Promise<void>;
deleteProtectedFiles(filepath: string): void;
readProtectedFile(filepath: string): Promise<string>;
writeProtectedFile(filepath: string, contents: string): Promise<void>;
}
Expand Down
27 changes: 25 additions & 2 deletions src/platforms/linux.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import path from 'path';
import { existsSync as exists, readFileSync as read, writeFileSync as writeFile } from 'fs';
import createDebug from 'debug';
import { sync as commandExists } from 'command-exists';
import { addCertificateToNSSCertDB, openCertificateInFirefox, closeFirefox } from './shared';
import { addCertificateToNSSCertDB, assertNotTouchingFiles, openCertificateInFirefox, closeFirefox, removeCertificateFromNSSCertDB } from './shared';
import { run } from '../utils';
import { Options } from '../index';
import UI from '../user-interface';
Expand Down Expand Up @@ -67,6 +67,23 @@ export default class LinuxPlatform implements Platform {
debug('Chrome does not appear to be installed, skipping Chrome-specific steps...');
}
}

removeFromTrustStores(certificatePath: string) {
try {
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() }`);
}
if (commandExists('certutil')) {
if (this.isFirefoxInstalled()) {
removeCertificateFromNSSCertDB(this.FIREFOX_NSS_DIR, certificatePath, 'certutil');
}
if (this.isChromeInstalled()) {
removeCertificateFromNSSCertDB(this.CHROME_NSS_DIR, certificatePath, 'certutil');
}
}
}

async addDomainToHostFileIfMissing(domain: string) {
let hostsFileContents = read(this.HOST_FILE_PATH, 'utf8');
Expand All @@ -75,11 +92,18 @@ export default class LinuxPlatform implements Platform {
}
}

deleteProtectedFiles(filepath: string) {
assertNotTouchingFiles(filepath, 'delete');
run(`sudo rm -rf "${filepath}"`);
}

async readProtectedFile(filepath: string) {
assertNotTouchingFiles(filepath, 'read');
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}"`);
}
Expand All @@ -88,7 +112,6 @@ export default class LinuxPlatform implements Platform {
await run(`sudo chmod 600 "${filepath}"`);
}


private isFirefoxInstalled() {
return exists(this.FIREFOX_BIN_PATH);
}
Expand Down
Loading