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

Use file name whitelist to prevent RCE #4866

Merged
merged 4 commits into from
Aug 22, 2018

Conversation

acdlite
Copy link
Contributor

@acdlite acdlite commented Aug 8, 2018

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.
@acdlite acdlite force-pushed the file-name-whitelist branch from 4328730 to f928fee Compare August 8, 2018 18:47
@acdlite acdlite changed the title Use file name whitelist to prevent CSRF Use file name whitelist to prevent RCE Aug 8, 2018
@Timer
Copy link
Contributor

Timer commented Aug 8, 2018

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

@gaearon
Copy link
Contributor

gaearon commented Aug 8, 2018

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
Copy link
Contributor

Timer commented Aug 8, 2018

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
Copy link
Contributor

gaearon commented Aug 8, 2018

I think this is fine as a stop measure.

@acdlite
Copy link
Contributor Author

acdlite commented Aug 8, 2018

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

@gaearon
Copy link
Contributor

gaearon commented Aug 8, 2018

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.

acdlite added 3 commits August 8, 2018 14:14
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 acdlite force-pushed the file-name-whitelist branch from 5abec23 to c1ef946 Compare August 8, 2018 21:19
'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.'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaearon How's this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good

@gaearon
Copy link
Contributor

gaearon commented Aug 8, 2018

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

@acdlite
Copy link
Contributor Author

acdlite commented Aug 16, 2018

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

@gaearon
Copy link
Contributor

gaearon commented Aug 16, 2018

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

@gaearon gaearon merged commit 577a274 into facebook:next Aug 22, 2018
gaearon pushed a commit that referenced this pull request Aug 22, 2018
* 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
Copy link
Contributor

gaearon commented Aug 24, 2018

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
Copy link
Contributor

gaearon commented Aug 24, 2018

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.

cloud-walker pushed a commit to cloud-walker/create-react-app that referenced this pull request Aug 27, 2018
* Add modes to our Babel preset (1.x) (facebook#4668)

* babel-preset-react-app@3.1.2

* add react-testing-library documentation/examples (facebook#4679)

* add react-testing-library documentation/examples

* make react-testing-library a heading

* fix typo

* Fix link to the article about BEM (facebook#4858)

* Use file name whitelist to prevent RCE (facebook#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
Copy link
Contributor

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

xs9627 pushed a commit to xs9627/create-react-app that referenced this pull request Aug 29, 2018
* facebook-master:
  Add 1.1.5 release notes
  Bump release
  Bump versions
  Use file name whitelist to prevent RCE (facebook#4866)
  Fix link to the article about BEM (facebook#4858)
@gaearon
Copy link
Contributor

gaearon commented Aug 29, 2018

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
Copy link
Contributor

gaearon commented Aug 29, 2018

@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.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Sep 4, 2018
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: facebook/create-react-app#4866

See s163726 for more details

Reviewed By: LukasReschke

Differential Revision: D9504148

fbshipit-source-id: e3c7587f1b7f93bec90a58a38d5f6d58f1f59275
gengjiawen pushed a commit to gengjiawen/react-native that referenced this pull request Sep 14, 2018
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: facebook/create-react-app#4866

See s163726 for more details

Reviewed By: LukasReschke

Differential Revision: D9504148

fbshipit-source-id: e3c7587f1b7f93bec90a58a38d5f6d58f1f59275
@facebook facebook deleted a comment Sep 23, 2018
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
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: facebook/create-react-app#4866

See s163726 for more details

Reviewed By: LukasReschke

Differential Revision: D9504148

fbshipit-source-id: e3c7587f1b7f93bec90a58a38d5f6d58f1f59275
@gaearon
Copy link
Contributor

gaearon commented Oct 14, 2018

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

@gaearon
Copy link
Contributor

gaearon commented Oct 14, 2018

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
Copy link
Contributor

gaearon commented Oct 14, 2018

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

adammockor added a commit to lightingbeetle/lighter that referenced this pull request Oct 23, 2018
* commit 'dc74990b89b5c6e143b522c759be3dac2c286514':
  Add 1.1.5 release notes
  Bump release
  Bump versions
  Use file name whitelist to prevent RCE (facebook#4866)
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants