Skip to content

Commit

Permalink
Add new Security/IoMethods cop
Browse files Browse the repository at this point in the history
## Summary

Follow up to #9695 (comment).

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.

- rubygems/rubygems#4530
- ruby/ruby#4579
  • Loading branch information
koic committed Sep 28, 2021
1 parent 85d67c9 commit a88c43c
Show file tree
Hide file tree
Showing 7 changed files with 155 additions and 1 deletion.
1 change: 1 addition & 0 deletions 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][])
8 changes: 8 additions & 0 deletions config/default.yml
Expand Up @@ -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: '<<next>>'

Security/JSONLoad:
Description: >-
Prefer usage of `JSON.parse` over `JSON.load` due to potential
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -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'
Expand Down
49 changes: 49 additions & 0 deletions 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.%<method_name>s` is safer than `IO.%<method_name>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
2 changes: 2 additions & 0 deletions spec/rubocop/cli/options_spec.rb
Expand Up @@ -507,6 +507,8 @@ class SomeCop < Base
Enabled: false
Naming:
Enabled: false
Security:
Enabled: false
Style/SomeCop:
Description: Something
Expand Down
3 changes: 2 additions & 1 deletion spec/rubocop/cop/generator_spec.rb
Expand Up @@ -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
Expand Down
92 changes: 92 additions & 0 deletions 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

0 comments on commit a88c43c

Please sign in to comment.