Project

General

Profile

Actions

Feature #19630

closed

[RFC] Deprecate `Kernel#open("|command-here")` due to frequent security issues

Added by postmodern (Hal Brodigan) over 1 year ago. Updated over 1 year ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:113407]

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().


Related issues 2 (0 open2 closed)

Related to Ruby master - Misc #15893: open-uri: URI.open statusClosedakr (Akira Tanaka)Actions
Related to Ruby master - Feature #19723: [RFC] Deprecate/disallow passing `"|command..." values to open-uri's URI.open() methodClosedActions

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.

Actions #3

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 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 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?

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.

Actions #12

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
Actions #20

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
Actions #22

Updated by mdalessio (Mike Dalessio) over 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]

Actions

Also available in: Atom PDF

Like4
Like0Like1Like0Like0Like1Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like1Like0Like0Like1