Feature #19630
closed[RFC] Deprecate `Kernel#open("|command-here")` due to frequent security issues
Description
Kernel.open()
is the source of numerous 1 security 2 issues 3, due to the fact that it can be used to execute commands if given a String argument of the form "|command-here"
. However, in most uses of Kernel.open()
the developer appears to either want to open a local file, or if 'open-uri' was explicitly required open a remote URI. We should deprecate calling Kernel.open()
with a "|command-here"
style arguments, with a warning message instructing the developer to use IO.popen()
instead. Eventually, support for Kernel.open("|command-here")
could be removed completely, in favor of having the developer explicitly call IO.popen()
or URI.open()
.
Updated by postmodern (Hal Brodigan) over 1 year ago
A more complete list of the CVEs related to Kernel.open
:
- CVE-2017-17405 (ruby, net-ftp)
- CVE-2017-17790 (ruby, resolv)
- CVE-2019-10780 (bibtex-ruby)
- CVE-2021-21289 (mechanize)
- CVE-2019-5477 (nokogiri)
- CVE-2021-31799 (rdoc)
- CVE-2019-5477 (rexical)
Updated by mdalessio (Mike Dalessio) over 1 year ago
I enthusiastically support this suggestion. This is something that even experienced Ruby developers frequently forget about. I think it would be wise to evolve this API towards being "secure by default", even if that means forcing users to be explicit about the class or module.
Updated by byroot (Jean Boussier) over 1 year ago
- Related to Misc #15893: open-uri: URI.open status added
Updated by hsbt (Hiroshi SHIBATA) over 1 year ago
This proposal make sense to me. But I'm not sure how impact existing code for this incompatibility.
Do you have any deprecated process for this?
Updated by mdalessio (Mike Dalessio) over 1 year ago
@hsbt (Hiroshi SHIBATA) Because this functionality has existed in Ruby for such a long time, maybe we should target the next major release for removal of this functionality, and for now just print a deprecation warning.
@postmodern (Hal Brodigan) Just to clarify, you're only suggesting deprecating this in Kernel#open
. It's also possible for commands to be injected into:
- IO.binread
- IO.foreach
- IO.readlines
- IO.read
- IO.write
but my understanding is that you're proposing to leave these methods alone, is that correct?
If noone has objections, I'll create a pull request so we have something concrete to discuss.
Updated by byroot (Jean Boussier) over 1 year ago
for now just print a deprecation warning.
My worry is that since deprecation warnings are disabled by default, many people might not notice.
Recent examples show that things like File.exists?
was deprecated for a decade, and some people were still surprised by its removal.
I know it's a distinct issue, but it impacts this one.
Updated by mdalessio (Mike Dalessio) over 1 year ago
I've created https://github.com/ruby/ruby/pull/7915 for review.
Updated by postmodern (Hal Brodigan) over 1 year ago
@mdalessio (Mike Dalessio) (Mike Dalessio) wrote in #note-5:
@hsbt (Hiroshi SHIBATA) Because this functionality has existed in Ruby for such a long time, maybe we should target the next major release for removal of this functionality, and for now just print a deprecation warning.
@postmodern (Hal Brodigan) Just to clarify, you're only suggesting deprecating this in
Kernel#open
. It's also possible for commands to be injected into:
- IO.binread
- IO.foreach
- IO.readlines
- IO.read
- IO.write
but my understanding is that you're proposing to leave these methods alone, is that correct?
If noone has objections, I'll create a pull request so we have something concrete to discuss.
I was unaware that these methods can accept |command
style inputs. Based on the stdlib documentation, the first argument is called name
and the examples show reading from testfile
, which implies to me they should only read from files. We could at first deprecate Kernel.open
and see how much impact it has on users complaining about deprecation warnings, or we could start with the other IO
methods?
Updated by Eregon (Benoit Daloze) over 1 year ago
IIRC IO
methods all have an equivalent under File
, and those do not accept pipes.
So e.g. RuboCop warns about them and suggest to use File.some_method
instead: https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Security/IoMethods
And there is already a cop too for Kernel#open it seems: https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Security/Open
But I agree for security reasons I think it makes sense to deprecate them in Ruby too, not everyone uses RuboCop or these cops in particular.
Updated by nobu (Nobuyoshi Nakada) over 1 year ago
postmodern (Hal Brodigan) wrote in #note-8:
I was unaware that these methods can accept
|command
style inputs. Based on the stdlib documentation, the first argument is calledname
and the examples show reading fromtestfile
, which implies to me they should only read from files. We could at first deprecateKernel.open
and see how much impact it has on users complaining about deprecation warnings, or we could start with the otherIO
methods?
I'm against deprecating pipe in IO
methods.
It is intentionally kept quiet, unlike File
.
Updated by mdalessio (Mike Dalessio) over 1 year ago
If we all agree that deprecating this behavior in Kernel#open
is a good idea, is there any objection to something like https://github.com/ruby/ruby/pull/7915 ?
@byroot (Jean Boussier) I agree with your concerns about people ignoring deprecation warnings, but I don't think that's a good reason to stop deprecating behavior that we all agree should be deprecated.
Updated by kosaki (Motohiro KOSAKI) over 1 year ago
- Related to Feature #19723: [RFC] Deprecate/disallow passing `"|command..." values to open-uri's URI.open() method added
Updated by byroot (Jean Boussier) over 1 year ago
is there any objection
Not from me, we should add this ticket to the next dev meeting.
However I feel like other IO methods (IO.binread
, etc) should do the same otherwise it's a bit inconsistent.
Updated by matz (Yukihiro Matsumoto) over 1 year ago
OK. Probably we should remove pipe notation from all open methods, with deprecation process.
Matz.
Updated by hsbt (Hiroshi SHIBATA) over 1 year ago
@mdalessio (Mike Dalessio) Could you also deprecate the following methods in your pull request?
IO.binread
IO.foreach
IO.readlines
IO.read
IO.write
Updated by mdalessio (Mike Dalessio) over 1 year ago
@hsbt (Hiroshi SHIBATA) Yes, I'll update the pull request.
Updated by mdalessio (Mike Dalessio) over 1 year ago
@hsbt (Hiroshi SHIBATA) Do you think I should also deprecate pipe commends in URI.open
as suggested in https://bugs.ruby-lang.org/issues/19723 ?
It seems like @matz (Yukihiro Matsumoto) may be encouraging this by saying "all open methods" above.
Updated by hsbt (Hiroshi SHIBATA) over 1 year ago
@mdalessio (Mike Dalessio) Deprecated URI.open
is also accepted. We should deprecate it in same time.
Updated by ko1 (Koichi Sasada) over 1 year ago
memo:
rb_warn_deprecated_to_remove("4.0", "Calling Kernel#open with a leading '|'", "IO.popen");
- warning until 4.0
- delete at 4.0
Updated by nobu (Nobuyoshi Nakada) over 1 year ago
- Subject changed from [RFC] Deprecate `Kernel.open("|command-here")` due to frequent security issues to [RFC] Deprecate `Kernel#open("|command-here")` due to frequent security issues
Updated by mdalessio (Mike Dalessio) over 1 year ago
The pull request is ready for review: https://github.com/ruby/ruby/pull/7915
Updated by mdalessio (Mike Dalessio) about 1 year ago
- Status changed from Open to Closed
Applied in changeset git|d2343368ab7e270118ea6baa9c6418bfed83135c.
Deprecate Kernel#open and IO support for subprocess creation/forking
Deprecate Kernel#open and IO support for subprocess creation and
forking. This deprecates subprocess creation and forking in
- Kernel#open
- URI.open
- IO.binread
- IO.foreach
- IO.readlines
- IO.read
- IO.write
This behavior is slated to be removed in Ruby 4.0
[Feature #19630]