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

Ensure File.open applies default umask on gem extract #7300

Merged
merged 2 commits into from Dec 20, 2023

Conversation

martinemde
Copy link
Member

@martinemde martinemde commented Dec 17, 2023

This is an alternate to #7299

What was the end-user or developer problem that led to this PR?

Global and group write permissions are possible in gems.

What is your fix for the problem, implemented in this PR?

Allow File.open to handle applying the default umask.

Non-backwards-compatible change: This drops previously tested persistence of extended file modes.

Make sure the following tasks are checked

@martinemde martinemde merged commit 2cdf374 into master Dec 20, 2023
72 checks passed
@martinemde martinemde deleted the gem-extract-umask-default branch December 20, 2023 00:09
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

This looks good to me!

The only thing intentionally being reverted is 3211b16. I agree it's best to respect umask by default, and I would treat this as a security fix, but let me ask @nobu in case there was some reason we are missing for that change.

@nobu Is it ok for us to revert 3211b16? Do you remember of any issues with the default umask being respected? Perhaps it was just for consistency with it not being respected in other places?

@martinemde
Copy link
Member Author

martinemde commented Dec 20, 2023

@deivid-rodriguez @nobu If there's a problem with this PR, #7299 would be the right one to switch to, with maybe an intentional mode & 0o700 added.

Is there a test we could add?

@deivid-rodriguez
Copy link
Member

mmm so in few words, #7299 is more backwards compatible (defining that by our existing tests) but less secure?

@martinemde
Copy link
Member Author

martinemde commented Dec 20, 2023

Yes. On my system #7299 doesn't clear setuid/setgid bits on files. Allowing File.open to do its default clears those bits. This implies that File.umask (usually 0o022 or 0o077) is a limited version (9 bits?) of the actual umask that is applied by File.open (which is something more like 0o7022 but is probably much more platform dependent)

@deivid-rodriguez
Copy link
Member

Thanks for explaining. Are you comfortable then with shipping this as a security fix? I think I am.

@martinemde
Copy link
Member Author

martinemde commented Dec 20, 2023

I prefer this version, but there's an untested implied behavior in the original commit by @nobu that may mean some gems on some systems don't get their 0700 set.

(Edit: I take that back, the chmod will set to prog_mode or 0755 no matter what. I'm not sure about the 0700...)

@deivid-rodriguez
Copy link
Member

I see. Let's wait for @nobu's feedback then.

@hsbt
Copy link
Member

hsbt commented Dec 25, 2023

This change break some of our CI https://app.travis-ci.com/github/ruby/ruby/jobs/615527743

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants