Add new Security/Open cop

`Kernel#open` is considered harmful for production use.  Some programs use `Kernel#open` with untrusted input, but it allows command injection by prefixing a pipe (`|`).  [An actual vulnerability](https://www.ruby-lang.org/en/news/2017/12/14/net-ftp-command-injection-cve-2017-17405/) is found, and deprecating `open("|...")` is [proposed in bugs.ruby-lang.org](https://bugs.ruby-lang.org/issues/14239).  I'm unsure if the deprecation is really good, but at least it would be a good idea for Rubocop to prevent such a bad usage of `Kernel#open`.

```console
% cat /tmp/test.rb

open(something)
```

```console
% bundle exec bin/rubocop /tmp/test.rb
Inspecting 1 file
C

Offenses:

/tmp/test.rb:3:1: C: Security/Open: The use of open is a serious security risk.
open(something)
^^^^

1 file inspected, 1 offense detected
```

CHANGELOG.md

@@ -6,6 +6,7 @@
 
 * [#3394](https://github.com/bbatsov/rubocop/issues/3394): Add new `Style/TrailingCommmaInArrayLiteral` cop. ([@garettarrowood][])
 * [#3394](https://github.com/bbatsov/rubocop/issues/3394): Add new `Style/TrailingCommmaInHashLiteral` cop. ([@garettarrowood][])
+* [#5319](https://github.com/bbatsov/rubocop/pull/5319): Add new `Security/Open` cop. ([@mame][])
 
 ### Changes
 
@@ -3130,3 +3131,4 @@
 [@melch]: https://github.com/melch
 [@flyerhzm]: https://github.com/flyerhzm
 [@ybiquitous]: https://github.com/ybiquitous
+[@mame]: https://github.com/mame

config/enabled.yml

@@ -1253,6 +1253,10 @@ Security/MarshalLoad:
   Reference: 'http://ruby-doc.org/core-2.3.3/Marshal.html#module-Marshal-label-Security+considerations'
   Enabled: true
 
+Security/Open:
+  Description: 'The use of Kernel#open represents a serious security risk.'
+  Enabled: true
+
 Security/YAMLLoad:
   Description: >-
                  Prefer usage of `YAML.safe_load` over `YAML.load` due to potential

lib/rubocop.rb

@@ -557,6 +557,7 @@
 require_relative 'rubocop/cop/security/eval'
 require_relative 'rubocop/cop/security/json_load'
 require_relative 'rubocop/cop/security/marshal_load'
+require_relative 'rubocop/cop/security/open'
 require_relative 'rubocop/cop/security/yaml_load'
 
 require_relative 'rubocop/cop/team'

lib/rubocop/cop/security/open.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Security
+      # This cop checks for the use of `Kernel#open`.
+      # `Kernel#open` enables not only file access but also process invocation
+      # by prefixing a pipe symbol (e.g., `open("| ls")`).  So, it may lead to
+      # a serious security risk by using variable input to the argument of
+      # `Kernel#open`.  It would be better to use `File.open` or `IO.popen`
+      # explicitly.
+      #
+      # @example
+      #   # bad
+      #   open(something)
+      #
+      #   # good
+      #   File.open(something)
+      #   IO.popen(something)
+      class Open < Cop
+        MSG = 'The use of `Kernel#open` is a serious security risk.'.freeze
+
+        def_node_matcher :open?, <<-PATTERN
+          (send nil? :open $!str ...)
+        PATTERN
+
+        def safe?(node)
+          if node.str_type?
+            !node.str_content.empty? && !node.str_content.start_with?('|')
+          elsif node.dstr_type?
+            safe?(node.child_nodes.first)
+          elsif node.send_type? && node.method_name == :+
+            safe?(node.child_nodes.first)
+          else
+            false
+          end
+        end
+
+        def on_send(node)
+          open?(node) do |code|
+            return if safe?(code)
+            add_offense(node, location: :selector)
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/remote_config.rb

@@ -21,7 +21,7 @@ def file
       request do |response|
         next if response.is_a?(Net::HTTPNotModified)
         next if response.is_a?(SocketError)
-        open cache_path, 'w' do |io|
+        File.open cache_path, 'w' do |io|
           io.write response.body
         end
       end

manual/cops.md

@@ -359,6 +359,7 @@ In the following section you find all available cops:
 * [Security/Eval](cops_security.md#securityeval)
 * [Security/JSONLoad](cops_security.md#securityjsonload)
 * [Security/MarshalLoad](cops_security.md#securitymarshalload)
+* [Security/Open](cops_security.md#securityopen)
 * [Security/YAMLLoad](cops_security.md#securityyamlload)
 
 #### Department [Style](cops_style.md)

manual/cops_security.md

@@ -83,6 +83,30 @@ Marshal.load(Marshal.dump({}))
 
 * [http://ruby-doc.org/core-2.3.3/Marshal.html#module-Marshal-label-Security+considerations](http://ruby-doc.org/core-2.3.3/Marshal.html#module-Marshal-label-Security+considerations)
 
+## Security/Open
+
+Enabled by default | Supports autocorrection
+--- | ---
+Enabled | No
+
+This cop checks for the use of `Kernel#open`.
+`Kernel#open` enables not only file access but also process invocation
+by prefixing a pipe symbol (e.g., `open("| ls")`).  So, it may lead to
+a serious security risk by using variable input to the argument of
+`Kernel#open`.  It would be better to use `File.open` or `IO.popen`
+explicitly.
+
+### Examples
+
+```ruby
+# bad
+open(something)
+
+# good
+File.open(something)
+IO.popen(something)
+```
+
 ## Security/YAMLLoad
 
 Enabled by default | Supports autocorrection

spec/rubocop/cop/security/open_spec.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Security::Open do
+  subject(:cop) { described_class.new }
+
+  it 'registers an offense for open' do
+    expect_offense(<<-RUBY.strip_indent)
+      open(something)
+      ^^^^ The use of `Kernel#open` is a serious security risk.
+    RUBY
+  end
+
+  it 'registers an offense for open with mode argument' do
+    expect_offense(<<-RUBY.strip_indent)
+      open(something, "r")
+      ^^^^ The use of `Kernel#open` is a serious security risk.
+    RUBY
+  end
+
+  it 'registers an offense for open with dynamic string that is not prefixed' do
+    expect_offense(<<-'RUBY'.strip_indent)
+      open("#{foo}.txt")
+      ^^^^ The use of `Kernel#open` is a serious security risk.
+    RUBY
+  end
+
+  it 'registers an offense for open with string that starts with a pipe' do
+    expect_offense(<<-'RUBY'.strip_indent)
+      open("| #{foo}")
+      ^^^^ The use of `Kernel#open` is a serious security risk.
+    RUBY
+  end
+
+  it 'accepts open as variable' do
+    expect_no_offenses('open = something')
+  end
+
+  it 'accepts File.open as method' do
+    expect_no_offenses('File.open(something)')
+  end
+
+  it 'accepts open on a literal string' do
+    expect_no_offenses('open("foo.txt")')
+  end
+
+  it 'accepts open with no arguments' do
+    expect_no_offenses('open')
+  end
+
+  it 'accepts open with string that has a prefixed interpolation' do
+    expect_no_offenses('open "prefix_#{foo}"')
+  end
+
+  it 'accepts open with prefix string literal plus something' do
+    expect_no_offenses('open "prefix" + foo')
+  end
+
+  it 'accepts open with a string that interpolates a literal' do
+    expect_no_offenses('open "foo#{2}.txt"')
+  end
+end