Use file name whitelist to prevent RCE by acdlite · Pull Request #4866 · react/create-react-app

acdlite · 2018-08-08T18:42:01Z

Use a whitelist to validate user-provided file names. This doesn't cover the entire range of valid filenames but should cover almost all of them in practice. Allows letters, numbers, periods, dashes, and underscores. Opting to use a whitelist instead of a blacklist because getting this wrong leaves us vulnerable to a RCE attack.

Should I submit a separate PR to the master branch?

Use a whitelist to validate user-provided file names. This doesn't cover the entire range of valid filenames but should cover almost all of them in practice. Allows letters, numbers, periods, dashes, and underscores. Opting to use a whitelist instead of a blacklist because getting this wrong leaves us vulnerable to a RCE attack.

Timer · 2018-08-08T19:11:28Z

We're probably cutting a 1.x soon, so a separate PR for master would be great (unless if this cherry picks cleanly)!

gaearon · 2018-08-08T19:25:00Z

A few things I’d like to see

if this is windows specific let’s add a platform check
lets whitelist non-English alphabet characters too (don’t forget Cyrillic too!)

Timer · 2018-08-08T19:37:37Z

What if we opened a file explaining why their file didn't open when a request is rejected? This would encourage them to file an issue, rename their file, or check their system and package tree for integrity.

Something like rce.md in our package internals.

Is there a npm package that checks this with greater accuracy?

gaearon · 2018-08-08T20:06:07Z

I think this is fine as a stop measure.

acdlite · 2018-08-08T20:22:01Z

Updated. Once y'all are happy with this I'll open a duplicate PR against master.

gaearon · 2018-08-08T20:29:19Z

Looks reasonable to me. Can we print something to the console when it happens? Like we do in this file when env variable isn’t set.

Updated the whitelist to /^[\p{L}0-9/.\-_]+$/u, which matches alphanumeric characters, periods, dashes, and underscores. Unicode property support is stage 4 so I've inlined the transpiled version.

acdlite · 2018-08-08T22:06:41Z

+ 'When running on Windows, file names are checked against a whitelist ' +
+ 'to protect against remote code execution attacks. File names may ' +
+ 'consist only of alphanumeric characters (all languages), periods, ' +
+ 'dashes, slashes, and underscores.'


@gaearon How's this?

gaearon · 2018-08-08T22:09:12Z

@acdlite Can you do some testing to confirm it works on intended on Windows (and that it mitigates the example you used)?

acdlite · 2018-08-16T16:10:02Z

Sorry, neglected to check back on this. Yeah I tested it on a VM.

gaearon · 2018-08-16T16:27:23Z

Cool. I'll release today. Can you prepare a master PR please too?

* Use file name whitelist to prevent RCE Use a whitelist to validate user-provided file names. This doesn't cover the entire range of valid filenames but should cover almost all of them in practice. Allows letters, numbers, periods, dashes, and underscores. Opting to use a whitelist instead of a blacklist because getting this wrong leaves us vulnerable to a RCE attack. * Allow alphabet characters from all languages Updated the whitelist to /^[\p{L}0-9/.\-_]+$/u, which matches alphanumeric characters, periods, dashes, and underscores. Unicode property support is stage 4 so I've inlined the transpiled version. * Only use file name whitelist on Windows * Log error message if file name does not pass whitelist

gaearon · 2018-08-24T15:02:42Z

I released react-scripts@1.1.5 which enforces a version of react-dev-utils with this change.
For react-dev-utils, I cut patches for every major version:

react-dev-utils@1.0.4 contains the fix
react-dev-utils@2.0.2 contains the fix
react-dev-utils@3.1.2 contains the fix
react-dev-utils@4.2.2 contains the fix
react-dev-utils@5.0.2 contains the fix

I'll also cut a new release from the next branch since react-dev-utils is pinned in alphas.

gaearon · 2018-08-24T15:14:34Z

Also released react-dev-utils@6.0.0-next.a671462c (and react-scripts@2.0.0-next.a671462c requiring it) with the fix for the next branch.

* Add modes to our Babel preset (1.x) (react#4668) * babel-preset-react-app@3.1.2 * add react-testing-library documentation/examples (react#4679) * add react-testing-library documentation/examples * make react-testing-library a heading * fix typo * Fix link to the article about BEM (react#4858) * Use file name whitelist to prevent RCE (react#4866) * Use file name whitelist to prevent RCE Use a whitelist to validate user-provided file names. This doesn't cover the entire range of valid filenames but should cover almost all of them in practice. Allows letters, numbers, periods, dashes, and underscores. Opting to use a whitelist instead of a blacklist because getting this wrong leaves us vulnerable to a RCE attack. * Allow alphabet characters from all languages Updated the whitelist to /^[\p{L}0-9/.\-_]+$/u, which matches alphanumeric characters, periods, dashes, and underscores. Unicode property support is stage 4 so I've inlined the transpiled version. * Only use file name whitelist on Windows * Log error message if file name does not pass whitelist * Bump versions * Bump release * Add 1.1.5 release notes

ForbesLindesay · 2018-08-27T21:31:24Z

I may be missing something. Shouldn't the fs.existsSync(fileName) check already prevent the RCE?

* facebook-master: Add 1.1.5 release notes Bump release Bump versions Use file name whitelist to prevent RCE (react#4866) Fix link to the article about BEM (react#4858)

gaearon · 2018-08-29T13:58:15Z

Hmm actually I completely forgot about this check. Maybe you’re right this mostly mitigates it. But doesn’t hurt to check more aggressively.

gaearon · 2018-08-29T16:01:34Z

@ForbesLindesay I double-checked in the vulnerability report, and indeed there is a way to fool this check by forcing a browser to download a file with a specially crafted name. It requires knowing some easily guessable things about the attack target but it's not implausible.

Summary: Use a whitelist to validate user-provided file names. This doesn't cover the entire range of valid filenames but should cover almost all of them in practice. Allows letters, numbers, periods, dashes, and underscores. Opting to use a whitelist instead of a blacklist because getting this wrong leaves us vulnerable to a RCE attack. This is the same patch I submitted to create-react-app: react/create-react-app#4866 See s163726 for more details Reviewed By: LukasReschke Differential Revision: D9504148 fbshipit-source-id: e3c7587f1b7f93bec90a58a38d5f6d58f1f59275

gaearon · 2018-10-14T17:59:28Z

It looks like this completely broke the click-to-open functionality on Windows.

gaearon · 2018-10-14T18:16:08Z

Seems like we'll need to release five patches again because with these patches, we broke it for all versions. No valid Windows file path can pass this regex because it always starts with X: but : is completely disallowed.

gaearon · 2018-10-14T18:49:25Z

Cut fixes in:

react-dev-utils@1.0.5
react-dev-utils@2.0.3
react-dev-utils@3.1.3
react-dev-utils@4.2.3
react-dev-utils@5.0.3

* commit 'dc74990b89b5c6e143b522c759be3dac2c286514': Add 1.1.5 release notes Bump release Bump versions Use file name whitelist to prevent RCE (react#4866)

facebook-github-bot added the CLA Signed label Aug 8, 2018

acdlite force-pushed the file-name-whitelist branch from 4328730 to f928feeCompare August 8, 2018 18:47

acdlite changed the title ~~Use file name whitelist to prevent CSRF~~Use file name whitelist to prevent RCEAug 8, 2018

acdlite added 3 commits August 8, 2018 14:14

Allow alphabet characters from all languages
a5f27ce
Updated the whitelist to /^[\p{L}0-9/.\-_]+$/u, which matches alphanumeric characters, periods, dashes, and underscores. Unicode property support is stage 4 so I've inlined the transpiled version.

Only use file name whitelist on Windows
7814826

Log error message if file name does not pass whitelist
c1ef946

acdlite force-pushed the file-name-whitelist branch from 5abec23 to c1ef946Compare August 8, 2018 21:19

acdlite commented Aug 8, 2018
View reviewed changes

gaearon approved these changes Aug 8, 2018
View reviewed changes

gaearon merged commit 577a274 into react:nextAug 22, 2018

gaearon mentioned this pull request Aug 24, 2018
Roadmap for react-scripts@2.0 #3815
Closed

react deleted a comment Sep 23, 2018

gaearon mentioned this pull request Oct 14, 2018
Fix click-to-open on Windows #5431
Merged

lockBot locked and limited conversation to collaborators Jan 18, 2019

Uh oh!

Uh oh!

Conversation

acdlite commented Aug 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Timer commented Aug 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Aug 8, 2018

Uh oh!

Timer commented Aug 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Aug 8, 2018

Uh oh!

acdlite commented Aug 8, 2018

Uh oh!

gaearon commented Aug 8, 2018

Uh oh!

acdliteAug 8, 2018

Choose a reason for hiding this comment

Uh oh!

gaearonAug 8, 2018

Choose a reason for hiding this comment

Uh oh!

gaearon commented Aug 8, 2018

Uh oh!

acdlite commented Aug 16, 2018

Uh oh!

gaearon commented Aug 16, 2018

Uh oh!

gaearon commented Aug 24, 2018

Uh oh!

gaearon commented Aug 24, 2018

Uh oh!

ForbesLindesay commented Aug 27, 2018

Uh oh!

gaearon commented Aug 29, 2018

Uh oh!

gaearon commented Aug 29, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Oct 14, 2018

Uh oh!

gaearon commented Oct 14, 2018

Uh oh!

gaearon commented Oct 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

acdlite commented Aug 8, 2018•
edited
Loading

Timer commented Aug 8, 2018•
edited
Loading

Timer commented Aug 8, 2018•
edited
Loading

gaearon commented Aug 29, 2018•
edited
Loading