From 6ae8d950f4c7324efd3ca5f7ef49e1378c17f0e7 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 21 Sep 2021 00:59:14 +0900 Subject: [PATCH] Add new `Security/IoMethods` cop ## Summary Follow up to https://github.com/rubocop/rubocop/pull/9695#issuecomment-820151271. This PR adds new `Security/IoMethods` cop. It checks for the first argument to `IO.read`, `IO.binread`, `IO.write`, `IO.binwrite`, `IO.foreach`, and `IO.readlines`. If argument starts with a pipe character (`'|'`) and the receiver is the `IO` class, a subprocess is created in the same way as `Kernel#open`, and its output is returned. Consider to use `File.read` to disable the behavior of subprocess invocation. This cop is unsafe because false positive will occur if the variable passed as the first argument is a command that is not a file path. ```ruby # bad IO.read(path) IO.read('path') # good File.read(path) File.read('path') IO.read('| command') # Allow intentional command invocation. ``` ## Other Information Below are links related to this cop. - https://github.com/rubygems/rubygems/pull/4530 - https://github.com/ruby/ruby/pull/4579 --- changelog/new_add_new_security_io_read_cop.md | 1 + config/default.yml | 8 ++ lib/rubocop.rb | 1 + lib/rubocop/cop/security/io_methods.rb | 49 ++++++++++ spec/rubocop/cli/options_spec.rb | 2 + spec/rubocop/cop/generator_spec.rb | 3 +- spec/rubocop/cop/security/io_methods_spec.rb | 92 +++++++++++++++++++ 7 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 changelog/new_add_new_security_io_read_cop.md create mode 100644 lib/rubocop/cop/security/io_methods.rb create mode 100644 spec/rubocop/cop/security/io_methods_spec.rb diff --git a/changelog/new_add_new_security_io_read_cop.md b/changelog/new_add_new_security_io_read_cop.md new file mode 100644 index 00000000000..f293862f81c --- /dev/null +++ b/changelog/new_add_new_security_io_read_cop.md @@ -0,0 +1 @@ +* [#10102](https://github.com/rubocop/rubocop/pull/10102): Add new `Security/IoMethods` cop. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index f4cf3427f38..b9dcc96101a 100644 --- a/config/default.yml +++ b/config/default.yml @@ -2739,6 +2739,14 @@ Security/Eval: Enabled: true VersionAdded: '0.47' +Security/IoMethods: + Description: >- + Checks for the first argument to `IO.read`, `IO.binread`, `IO.write`, `IO.binwrite`, + `IO.foreach`, and `IO.readlines`. + Enabled: pending + Safe: false + VersionAdded: '<>' + Security/JSONLoad: Description: >- Prefer usage of `JSON.parse` over `JSON.load` due to potential diff --git a/lib/rubocop.rb b/lib/rubocop.rb index c1d105d6c57..c6930cfc346 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -639,6 +639,7 @@ require_relative 'rubocop/cop/style/zero_length_predicate' require_relative 'rubocop/cop/security/eval' +require_relative 'rubocop/cop/security/io_methods' require_relative 'rubocop/cop/security/json_load' require_relative 'rubocop/cop/security/marshal_load' require_relative 'rubocop/cop/security/open' diff --git a/lib/rubocop/cop/security/io_methods.rb b/lib/rubocop/cop/security/io_methods.rb new file mode 100644 index 00000000000..89d0a6ce12c --- /dev/null +++ b/lib/rubocop/cop/security/io_methods.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Security + # Checks for the first argument to `IO.read`, `IO.binread`, `IO.write`, `IO.binwrite`, + # `IO.foreach`, and `IO.readlines`. + # + # If argument starts with a pipe character (`'|'`) and the receiver is the `IO` class, + # a subprocess is created in the same way as `Kernel#open`, and its output is returned. + # `Kernel#open` may allow unintentional command injection, which is the reason these + # `IO` methods are a security risk. + # Consider to use `File.read` to disable the behavior of subprocess invocation. + # + # @safety + # This cop is unsafe because false positive will occur if the variable passed as + # the first argument is a command that is not a file path. + # + # @example + # + # # bad + # IO.read(path) + # IO.read('path') + # + # # good + # File.read(path) + # File.read('path') + # IO.read('| command') # Allow intentional command invocation. + # + class IoMethods < Base + extend AutoCorrector + + MSG = '`File.%s` is safer than `IO.%s`.' + RESTRICT_ON_SEND = %i[read binread write binwrite foreach readlines].freeze + + def on_send(node) + return unless (receiver = node.receiver) && receiver.source == 'IO' + + argument = node.first_argument + return if argument.respond_to?(:value) && argument.value.strip.start_with?('|') + + add_offense(node, message: format(MSG, method_name: node.method_name)) do |corrector| + corrector.replace(receiver, 'File') + end + end + end + end + end +end diff --git a/spec/rubocop/cli/options_spec.rb b/spec/rubocop/cli/options_spec.rb index 22a4c31dcdf..1ed2a3fa7b0 100644 --- a/spec/rubocop/cli/options_spec.rb +++ b/spec/rubocop/cli/options_spec.rb @@ -507,6 +507,8 @@ class SomeCop < Base Enabled: false Naming: Enabled: false + Security: + Enabled: false Style/SomeCop: Description: Something diff --git a/spec/rubocop/cop/generator_spec.rb b/spec/rubocop/cop/generator_spec.rb index 607949a54db..85b5fb2ef29 100644 --- a/spec/rubocop/cop/generator_spec.rb +++ b/spec/rubocop/cop/generator_spec.rb @@ -183,7 +183,8 @@ def on_send(node) end before do - IO.write(path, <<~YAML) + # It is hacked to use `IO.write` to avoid mocking `File.write` for testing. + IO.write(path, <<~YAML) # rubocop:disable Security/IoMethods Style/Alias: Enabled: true diff --git a/spec/rubocop/cop/security/io_methods_spec.rb b/spec/rubocop/cop/security/io_methods_spec.rb new file mode 100644 index 00000000000..8705869c5a2 --- /dev/null +++ b/spec/rubocop/cop/security/io_methods_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Security::IoMethods, :config do + shared_examples 'offense' do |current, preferred, method_name| + it "registers and corrects an offense when using `#{method_name}`" do + expect_offense(<<~RUBY, current: current) + #{current} + ^{current} `File.#{method_name}` is safer than `IO.#{method_name}`. + RUBY + + expect_correction(<<~RUBY) + #{preferred} + RUBY + end + end + + shared_examples 'accepts' do |code| + it "does not register an offense when using `#{code}`" do + expect_no_offenses(<<~RUBY) + #{code} + RUBY + end + end + + context 'when using `IO` receiver and variable argument' do + it_behaves_like 'offense', 'IO.read(path)', 'File.read(path)', 'read' + it_behaves_like 'offense', 'IO.write(path, "hi")', 'File.write(path, "hi")', 'write' + it_behaves_like 'offense', 'IO.binread(path)', 'File.binread(path)', 'binread' + it_behaves_like 'offense', 'IO.binwrite(path, "hi")', 'File.binwrite(path, "hi")', 'binwrite' + it_behaves_like 'offense', 'IO.readlines(path)', 'File.readlines(path)', 'readlines' + it 'registers and corrects an offense when using `foreach`' do + expect_offense(<<~RUBY) + IO.foreach(path) { |x| puts x } + ^^^^^^^^^^^^^^^^ `File.foreach` is safer than `IO.foreach`. + RUBY + + expect_correction(<<~RUBY) + File.foreach(path) { |x| puts x } + RUBY + end + end + + context 'when using `IO` receiver and string argument' do + it_behaves_like 'offense', 'IO.read("command")', 'File.read("command")', 'read' + it_behaves_like 'offense', 'IO.write("command", "hi")', 'File.write("command", "hi")', 'write' + it_behaves_like 'offense', 'IO.binwrite("command", "hi")', 'File.binwrite("command", "hi")', 'binwrite' + it_behaves_like 'offense', 'IO.binwrite(path, "hi")', 'File.binwrite(path, "hi")', 'binwrite' + it_behaves_like 'offense', 'IO.readlines("command")', 'File.readlines("command")', 'readlines' + it 'registers and corrects an offense when using `foreach`' do + expect_offense(<<~RUBY) + IO.foreach("command") { |x| puts x } + ^^^^^^^^^^^^^^^^^^^^^ `File.foreach` is safer than `IO.foreach`. + RUBY + + expect_correction(<<~RUBY) + File.foreach("command") { |x| puts x } + RUBY + end + end + + context 'when using `File` receiver' do + it_behaves_like 'accepts', 'File.read(path)' + it_behaves_like 'accepts', 'File.binread(path)' + it_behaves_like 'accepts', 'File.binwrite(path, "hi")' + it_behaves_like 'accepts', 'File.readlines(path)' + it_behaves_like 'accepts', 'File.foreach(path) { |x| puts x }' + end + + context 'when using no receiver' do + it_behaves_like 'accepts', 'read("command")' + it_behaves_like 'accepts', 'write("command", "hi")' + it_behaves_like 'accepts', 'binwrite("command", "hi")' + it_behaves_like 'accepts', 'readlines("command")' + it_behaves_like 'accepts', 'foreach("command") { |x| puts x }' + end + + context 'when using `IO` receiver and string argument that starts with a pipe character (`"| command"`)' do + it_behaves_like 'accepts', 'IO.read("| command")' + it_behaves_like 'accepts', 'IO.write("| command", "hi")' + it_behaves_like 'accepts', 'IO.binwrite("| command", "hi")' + it_behaves_like 'accepts', 'IO.readlines("| command")' + it_behaves_like 'accepts', 'IO.foreach("| command") { |x| puts x }' + end + + context 'when using `IO` receiver and string argument that starts with a pipe character (`" | command"`)' do + it_behaves_like 'accepts', 'IO.read(" | command")' + it_behaves_like 'accepts', 'IO.write(" | command", "hi")' + it_behaves_like 'accepts', 'IO.binwrite(" | command", "hi")' + it_behaves_like 'accepts', 'IO.readlines(" | command")' + it_behaves_like 'accepts', 'IO.foreach(" | command") { |x| puts x }' + end +end