Skip to content

permission: fix some vulnerabilities in fs - #47091

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
tniessen:permission-fix-some-fatal-flaws-in-fs-open
Mar 19, 2023
Merged

permission: fix some vulnerabilities in fs#47091
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
tniessen:permission-fix-some-fatal-flaws-in-fs-open

Conversation

@tniessen

@tniessentniessen commented Mar 14, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Without this patch, any restrictions imposed by the permission model can be easily bypassed, granting full read and write access to any file. On Windows, this could even be used to delete files that are supposed to be write-protected.

This likely also fixes a separate bug in fsPromises.open(), which currently incorrectly requires read permissions even for write-only access. (Unless that was somehow intentional?)

I'm not very confident in my understanding of the permission model. Please review carefully.

Fixes: #47090

@tniessentniessen added fs Issues and PRs related to the fs subsystem / file system. security Issues and PRs related to security. labels Mar 14, 2023
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 14, 2023
@tniessentniessen mentioned this pull request Mar 14, 2023
Closed
@tniessen
tniessen requested a review from RafaelGSSMarch 14, 2023 20:44
@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 14, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50382/

RafaelGSS
RafaelGSS reviewed Mar 14, 2023
View reviewed changes

@RafaelGSSRafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch. I wasn't aware of write_as_side_effects. That's indeed bizarre.

Comment threadsrc/node_file.cc Outdated
Comment threadsrc/node_file.cc Outdated
Comment threadtest/parallel/test-permission-deny-fs-read.js Outdated
RafaelGSS
RafaelGSS approved these changes Mar 15, 2023
View reviewed changes

@RafaelGSSRafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@tniessentniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 15, 2023
@RafaelGSSRafaelGSS mentioned this pull request Mar 15, 2023
Merged
@tniessen
permission: fix some vulnerabilities in fs
233eb62
Without this patch, any restrictions imposed by the permission model can
be easily bypassed, granting full read and write access to any file. On
Windows, this could even be used to delete files that are supposed to be
write-protected.
Fixes: nodejs#47090
@tniessen
tniessenforce-pushed the permission-fix-some-fatal-flaws-in-fs-open branch from c291faa to 233eb62CompareMarch 15, 2023 19:05
@tniessen

tniessen commented Mar 15, 2023

Copy link
Copy Markdown
MemberAuthor

I changed the implementation to follow V8 conventions as suggested by @targos and @addaleax. The diff is in c291faa8f7a78aff418a9784b98647b69f651c08.

@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 15, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 15, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen

tniessen commented Mar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Both Jenkins CI runs failed on Windows due to test-http-max-sockets, which also failed in #47108, so it seems unrelated.

@github-actionsgithub-actionsBot mentioned this pull request Mar 16, 2023
Open
22 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessentniessen mentioned this pull request Mar 16, 2023
Closed
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actionsgithub-actionsBot mentioned this pull request Mar 17, 2023
Open
19 tasks
@tniessen

tniessen commented Mar 17, 2023

Copy link
Copy Markdown
MemberAuthor

It might be good to get more eyes on this, perhaps from @nodejs/libuv since especially the behavior on Windows depends more on how libuv translates flags than on Windows itself.

@tniessentniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 17, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 17, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actionsgithub-actionsBot mentioned this pull request Mar 18, 2023
Open
23 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actionsgithub-actionsBot mentioned this pull request Mar 19, 2023
Open
19 tasks
@nodejs-github-bot

nodejs-github-bot commented Mar 19, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50454/

@tniessentniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 19, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 19, 2023
@nodejs-github-bot
nodejs-github-bot merged commit aa30e16 into nodejs:mainMar 19, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 19, 2023

Copy link
Copy Markdown
Collaborator

Landed in aa30e16

This was referenced Mar 20, 2023
Open
Open
Open
@RafaelGSSRafaelGSS mentioned this pull request Apr 3, 2023
Merged
@tniessentniessen added the permission Issues and PRs related to the Permission Model label Aug 10, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax left review comments
@targostargostargos left review comments
@RafaelGSSRafaelGSSRafaelGSS approved these changes
@TrottTrottTrott approved these changes
@marco-ippolitomarco-ippolitomarco-ippolito approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to the fs subsystem / file system.needs-ciPRs that need a full CI run.permissionIssues and PRs related to the Permission ModelsecurityIssues and PRs related to security.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Permission logic in fs.open(), fs.openSync(), and fsPromises.open() can easily be bypassed

7 participants

@tniessen@nodejs-github-bot@Trott@addaleax@targos@RafaelGSS@marco-ippolito