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

Questions about automatic "system gemrc" file creation #388

Closed
deivid-rodriguez opened this issue Jul 17, 2024 · 10 comments · Fixed by #393
Closed

Questions about automatic "system gemrc" file creation #388

deivid-rodriguez opened this issue Jul 17, 2024 · 10 comments · Fixed by #393

Comments

@deivid-rodriguez
Copy link
Contributor

What problems are you experiencing?

No problems, but I was investigating an issue about the etc gem being activated too soon on Windows, and eventually run into some code in the operating_system.rb file shipped by RubyInstaller2 that automatically creates an empty "system gemrc" file if it does not exist already:

begin
config_fname = Gem::ConfigFile::SYSTEM_WIDE_CONFIG_FILE
unless File.exist?(config_fname)
File.open(config_fname, File::CREAT | File::EXCL | File::WRONLY) do |fd|
fd.write <<-EOT
# This is the system wide config file for Rubygems.
# It is generated by RubyInstaller as a security measure.
# Feel free to add any rubygems config options as described on:
# https://docs.ruby-lang.org/en/3.1/Gem/ConfigFile.html
# But do not delete this file as otherwise it could be hijacked by
# another user in a multi-user environment.
---
{}
EOT
end
end
rescue => err
warn RubyInstaller::Runtime::Colors.yellow("Warning: Failed to create a system wide 'gemrc' file, making Rubygems possibly insecure: #{err}")
end

I got curious about the potential security issue. Does it affect more platforms other than Windows? The way I see it, system configuration is precisely intended for sharing a configuration with all users, so it's intended that a user with permissions to write files in Gem::ConfigFile::SYSTEM_WIDE_CONFIG_FILE can setup RubyGems configurations for all users. Also, it seems that the current implementation will still respect this global file if it's already there.

So I'm not fully clear about the security issue and I'm looking for a bit more insights.

Steps to reproduce

Create an empty Gemfile and observe that ruby -rbundler/setup -e 'puts Gem.loaded_specs["etc"]' activates the etc gem on Windows.

@larskanis
Copy link
Member

Gem::ConfigFile::SYSTEM_WIDE_CONFIG_FILE refers to C:/ProgramData on Windows. Everyone can create a new file in this directory, but the file can not be changed by anyone other than the owner subsequently. But everyone can read the file.

This is a security risk, since Rubygems reads this gemrc file, if it is present. When an attacker creates this file, it can hijack the configuration of some other user. It could inject different sources urls for instance.

I had a discussion with @hsbt and @unak about this topic on hackerone in 2022. They concluded about not to change anything in Rubygems. So I went further and implemented this simple solution, that the very first call of gem creates this gemrc file and closes the attack vector.

The issue doesn't affect platforms other than Windows.

@larskanis
Copy link
Member

larskanis commented Jul 17, 2024

We had an issue with fiddle in the past, that looks like this issue with etc loaded too soon: #251 . The way I solved it was to build a RubyInstaller specific Ruby C extension, which can call the Win32 API functions without fiddle.

Now that etc is gemified, we have the same issue here. The etc gem loading is triggered by accessing the constant Gem::ConfigFile::SYSTEM_WIDE_CONFIG_FILE. The etc gem reads the path here. But it is usually alternatively available in the environment variable ProgramData. So I think about switching to ENV['ProgramData'] in operating_system.rb.

@deivid-rodriguez
Copy link
Contributor Author

Gem::ConfigFile::SYSTEM_WIDE_CONFIG_FILE refers to C:/ProgramData on Windows. Everyone can create a new file in this directory, but the file can not be changed by anyone other than the owner subsequently. But everyone can read the file.

Thanks for the explanation, yeah, that's what I imagined. Are there paths on Windows that can be changed only by administrators ("root users")? I'd expect the system wide configuration to be writable only by administrators, and I'd say that's the main issue here. I wonder, for example, which path does git use for its system configuration.

Should we use an alternative secure path for this "system wide config"? Should we refuse to load it at all if it's parent directory is writable by the current user?

In any case, I still don't fully understand the solution. If the file is hijacked before RubyGems is loaded, then the solution won't be effective, right? Shouldn't the file be unconditionally overwritten for the hack to be effective? How do we distinguish between a well intended system configuration vs a malicious attempt to hijack configuration?

Now that etc is gemified, we have the same issue here. The etc gem loading is triggered by accessing the constant Gem::ConfigFile::SYSTEM_WIDE_CONFIG_FILE. The etc gem reads the path here. But it is usually alternatively available in the environment variable ProgramData. So I think about switching to ENV['ProgramData'] in operating_system.rb.

That makes sense although it'd mean having duplication with the etc gem, so maybe there'd be risk that the hack becomes no longer effective if the etc gem eventually changes?

@daipom
Copy link

daipom commented Sep 13, 2024

@larskanis
I understand that it prevents malicious users from creating or editing this file.

I have one question.
How should we edit this config?

Feel free to add any rubygems config options as described on:

This config file is YAML.
However, looks like the default content is not YAML.

So, I can't append a new setting to this file by simply using echo gem: --no-document >> c:\ProgramData\gemrc.

---
{}
gem: --no-document

This doesn't work.

Why is there the default content ---\n{}?

Do we need to overwrite this file completely?

echo gem: --no-document > c:\ProgramData\gemrc

I am not familiar with Windows permissions, but is this safe?

@daipom
Copy link

daipom commented Sep 13, 2024

Sorry, this is YAML.

---
{}

But I can't add key-value to this content because {} closes the hash.
Do we need {}?
=> I see. Without {}, it becomes nil... So we need {} here...

Feel free to add any rubygems config options

It prevents us from adding a config. We need to overwrite this file to do it.

Hmm, I see. We need {}. So, we can't simply append a new config to this file.
We need to overwrite this file to add some configs.
Is my understanding correct?

@daipom
Copy link

daipom commented Sep 17, 2024

I have confirmed that overwriting the file doesn't change the ACL.

fluent/fluentd-docker-image#398 (comment)

Just to be sure, I have confirmed that overwriting the file doesn't change the ACL.

Powershell with administrative privilege

> cp C:\ProgramData\gemrc C:\ProgramData\gemrc.cp
> Set-Acl C:\ProgramData\gemrc.cp -AclObject (Get-Acl C:\ProgramData\gemrc)
> Get-Acl .\gemrc.cp | Format-List Owner,Group,AccessToString


Owner          : NT AUTHORITY\SYSTEM
Group          : NT AUTHORITY\SYSTEM
AccessToString : NT AUTHORITY\SYSTEM Allow  FullControl
                 BUILTIN\Administrators Allow  FullControl
                 BUILTIN\Users Allow  ReadAndExecute, Synchronize

Command Prompt with administrative privilege

echo gem: --no-document > \ProgramData\gemrc.cp

Powershell with administrative privilege

> Get-Acl .\gemrc.cp | Format-List Owner,Group,AccessToString


Owner          : NT AUTHORITY\SYSTEM
Group          : NT AUTHORITY\SYSTEM
AccessToString : NT AUTHORITY\SYSTEM Allow  FullControl
                 BUILTIN\Administrators Allow  FullControl
                 BUILTIN\Users Allow  ReadAndExecute, Synchronize

@mohits
Copy link
Collaborator

mohits commented Oct 12, 2024

hi @daipom and @deivid-rodriguez - what's the next step here? Is this OK or are you still looking for some other change? Accordingly, I will leave this open or recommend to close it.

Thanks!

@deivid-rodriguez
Copy link
Contributor Author

I had some questions about this in #388 (comment), which I'd still be interested in, but I'm fine to close this since as of now, there's no specific action to be taken in RubyInstaller2. This issue was mainly a question to try understand things better.

@mohits
Copy link
Collaborator

mohits commented Oct 15, 2024

Closing this for now. Thanks!

@mohits mohits closed this as completed Oct 15, 2024
@daipom
Copy link

daipom commented Oct 15, 2024

Thanks! Sorry for my late response.

My question was resolved, so I'm OK with closing this.

If there could be other users who are as troubled as I am, it might be a good idea to improve this comment.
(Clarify how the option should be added. Overwriting the file or replacing lines? At least, appending is not possible.)

begin
config_fname = Gem::ConfigFile::SYSTEM_WIDE_CONFIG_FILE
unless File.exist?(config_fname)
File.open(config_fname, File::CREAT | File::EXCL | File::WRONLY) do |fd|
fd.write <<-EOT
# This is the system wide config file for Rubygems.
# It is generated by RubyInstaller as a security measure.
# Feel free to add any rubygems config options as described on:
# https://docs.ruby-lang.org/en/3.1/Gem/ConfigFile.html
# But do not delete this file as otherwise it could be hijacked by
# another user in a multi-user environment.
---
{}
EOT
end
end
rescue => err
warn RubyInstaller::Runtime::Colors.yellow("Warning: Failed to create a system wide 'gemrc' file, making Rubygems possibly insecure: #{err}")
end

larskanis added a commit that referenced this issue Oct 18, 2024
The previous "{}" literal closed the root hash, so that appending text to the file doesn't work easily.
But it's the only way to set an empty hash.
As a solution, we set a single key now.

This was raised in #388 (comment)
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 a pull request may close this issue.

4 participants