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 remote execution vulnerability by switching from execSync to execFileSync #55

Merged
merged 3 commits into from
Jun 10, 2020

Conversation

zetlen
Copy link
Collaborator

@zetlen zetlen commented Jun 9, 2020

Changes

  • Change run() to use execFileSync
  • Refactor codebase to use new signature of run()
  • Add an extra sanitizing step: test arguments passed to certificateFor with a (fairly permissive) regular expression limiting them to legal domain name chars

Fixes

Currently the run() command in utils.js does not sanitize its input, and other modules invoke run() with string-concatenated arguments including user input.

A downstream dependency that uses devcert with public input might unwittingly permit remote execution on their servers by passing shell commands.

This PR changes all "shell commands" to use Node child_process.execFileSync, which can only invoke specific executables with an array of arguments, rather than passing a full string to a shell to be evaluated.

@zetlen zetlen requested a review from davewasmer June 9, 2020 18:17
@zetlen zetlen merged commit e0e8ae3 into master Jun 10, 2020
@zetlen zetlen deleted the zetlen/fix-remoteexec branch June 10, 2020 02:47
Comment on lines +69 to +71
if (!VALID_DOMAIN.test(domain)) {
throw new Error(`"${domain}" is not a valid domain name.`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@zetlen FYI, this looks like it is causing #56. I can open a PR to fix it when I have time, but that might not be for a few days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants