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.read over IO.read #11138

Merged
merged 1 commit into from Apr 14, 2021
Merged

Use File.read over IO.read #11138

merged 1 commit into from Apr 14, 2021

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Apr 14, 2021

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

IO.read can allow arbitrary command execution. Since we know we only want to read files, File.read is safer here for security.

The usage of IO.read in the cases here weren't particularly exploitable - they mostly required there to be JSON files in the working directory with maliciously crafted filenames. Not knowing what's in your working directory when running brew pr-upload or brew bottle --merge isn't a situation I expect anyone to ever be in and it would require some extreme levels of social engineering to trick someone into thinking a funky looking JSON file is ok. A formula file with a maliciously crafted name should also never get any further than the CI, where exploits have negligible impact due to the fact you can just run arbitrary Ruby code.

@Bo98 Bo98 added the critical Critical change which should be shipped as soon as possible. label Apr 14, 2021
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

Copy link
Member

@Rylan12 Rylan12 left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@MikeMcQuaid
Copy link
Member

IO.read can allow arbitrary command execution.

TIL. Suggestion: global rubocop to stop us using it. This change makes sense to me as-is 👍🏻

@MikeMcQuaid MikeMcQuaid merged commit b153620 into Homebrew:master Apr 14, 2021
@Bo98
Copy link
Member Author

Bo98 commented Apr 14, 2021

Suggestion: global rubocop to stop us using it.

I'll have a look. I'm somewhat surprised Security/Open doesn't cover this already.

@Bo98 Bo98 deleted the io-read branch April 14, 2021 12:32
@github-actions github-actions bot added the outdated PR was locked due to age label May 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants