Add new `Security/Open` cop #5319

mame · 2017-12-26T17:16:05Z

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 is found, and deprecating open("|...") is proposed in bugs.ruby-lang.org. 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.

% cat /tmp/test.rb

open(something)

% 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

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

pocke · 2017-12-27T01:23:18Z

CHANGELOG.md

@@ -2,6 +2,10 @@

 ## master (unreleased)

+### New features
+
+* [#5319](https://github.com/bbatsov/rubocop/pull/5319): Add new `Security/Open` cop. ([@mame][])


Can you add [@mame]: https://github.com/mame to bottom of the file?

git push --force done!

pocke

Looks good to me except the comments, thank you!

pocke · 2017-12-27T04:27:36Z

lib/rubocop/cop/security/open.rb

+
+        def on_send(node)
+          open?(node) do |code|
+            return if code.str_type? || safe?(code)


Probably code.str_type? is unnecessary because open? pattern already checks the argument type ($!str).

Indeed! Fixed and pushed.

pocke · 2017-12-27T04:45:55Z

lib/rubocop/cop/security/open.rb

+      #
+      #   # bad
+      #
+      #   open(something)


I think the comment also needs a good example(s). And don't forget to execute rake generate_cops_documentation to re-generate the documentation (The task is included in rake default/parallel).

# bad open(something) # good File.open(something)

I added File.open and IO.popen as good examples. Thanks!

mame · 2017-12-27T10:11:24Z

Rebased.

pocke · 2017-12-27T10:34:07Z

Thank you. Sorry probably merging the pull-request will be delayed. Because we prior bug fix release over new feature.
See #5248 (comment)

mame · 2017-12-27T11:30:56Z

No problem! Thank you for your work.

bbatsov · 2017-12-27T14:53:09Z

Well, I just cut 0.52.1, so this just needs to be rebased and can be merged.

bbatsov · 2017-12-27T14:53:55Z

lib/rubocop/cop/security/open.rb

+module RuboCop
+  module Cop
+    module Security
+      # This cop checks for the use of `Kernel#open`.


I'd add more explanations here, as to why this is a security risk. After all this comment is going to become the cop's description in the manual.

bbatsov · 2017-12-27T14:54:16Z

lib/rubocop/cop/security/open.rb

+      # @example
+      #
+      #   # bad
+      #


I'd remove this blank line.

bbatsov · 2017-12-27T14:54:24Z

lib/rubocop/cop/security/open.rb

+      #   open(something)
+      #
+      #   # good
+      #


Here as well.

bbatsov · 2017-12-27T14:54:52Z

lib/rubocop/cop/security/open.rb

+      #   File.open(something)
+      #   IO.popen(something)
+      class Open < Cop
+        MSG = 'The use of `open` is a serious security risk.'.freeze


open -> Kernel#open

bbatsov · 2017-12-27T14:58:36Z

Btw, some of the tests are bit strange - e.g. we don't really need a test for open something and open(something) and two different interpolation tests. Ultimately you're testing the same thing in those tests.

You should probably add a test for calling open with more than one param, though.

mame · 2017-12-28T01:35:37Z

Thank you for the reviewing my PR!

Added more explanations
Removed extra blank lines
Improved some tests

FYI, I wrote this code by copying and modifying Security/Eval. The extra blank lines and "strange" tests are due to it :-)

bbatsov · 2017-12-28T07:32:54Z

CHANGELOG.md

@@ -12,6 +12,7 @@
 * [#3394](https://github.com/bbatsov/rubocop/issues/3394): Remove `Style/TrailingCommmaInLiteral` in favor of two new cops. ([@garettarrowood][])

 ## 0.52.1 (2017-12-27)
+* [#5319](https://github.com/bbatsov/rubocop/pull/5319): Add new `Security/Open` cop. ([@mame][])


This should be under New Feature higher up in the file.

Sorry! I left it to git merge. Fixed.

bbatsov · 2017-12-28T07:33:50Z

FYI, I wrote this code by copying and modifying Security/Eval. The extra blank lines and "strange" tests are due to it :-)

I guess I didn't pay it much attention or something. It should be improved. :-)

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

Drenmi · 2017-12-28T09:12:38Z

The overloading of Kernel#open is pretty flabbergasting. Feel free to link to my talk about a vulnerability I found in Refile, that came from using #open. 😛

bbatsov · 2017-12-28T09:55:18Z

And thinking of the false positives we're going to get on this cop down the road I'm reminded that eventually we need to add some cop to discourage naming methods like something in Kernel. :-)

mame · 2017-12-28T10:00:42Z

Thank you for the merge!

Honestly, I like the current behavior of Kernel#open. After understanding its danger, I believe that it is actually useful in daily programming (not in production). But unfortunately, people tend to use it blindly even in production. So I expect this cop to educate people. When the behavior is really desired, people can disable it by rubocop:disable comment.

Drenmi · 2017-12-28T12:29:35Z

After understanding its danger, I believe that it is actually useful [...]

I think this is the deceptive part of Ruby. It is beginner-friendly and expert-friendly (not a paradox) at the same time. It gives you a lot of freedom, and with that near infinite opportunities to do harm to yourself. 😅

So I expect this cop to educate people.

This is what keeps me interested in RuboCop. In the end, the answer to every problem is "better people." Let's keep thinking about ways to improve the educational aspect. 🙂

mame force-pushed the add_new_security_open_cop branch from 200f1d6 to bd9fb67 Compare December 26, 2017 17:17

pocke reviewed Dec 27, 2017

View reviewed changes

mame force-pushed the add_new_security_open_cop branch from bd9fb67 to b8c2413 Compare December 27, 2017 01:24

pocke approved these changes Dec 27, 2017

View reviewed changes

mame force-pushed the add_new_security_open_cop branch 2 times, most recently from 2037b61 to 3829ec5 Compare December 27, 2017 08:01

bbatsov reviewed Dec 27, 2017

View reviewed changes

lib/rubocop/cop/security/open.rb Outdated

# @example

#

# # bad

#

Copy link

Collaborator

bbatsov Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove this blank line.

bbatsov reviewed Dec 27, 2017

View reviewed changes

lib/rubocop/cop/security/open.rb Outdated

# open(something)

#

# # good

#

Copy link

Collaborator

bbatsov Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well.

bbatsov reviewed Dec 27, 2017

View reviewed changes

mame force-pushed the add_new_security_open_cop branch from 3829ec5 to 556ddeb Compare December 28, 2017 01:33

bbatsov reviewed Dec 28, 2017

View reviewed changes

mame force-pushed the add_new_security_open_cop branch from 556ddeb to f398a97 Compare December 28, 2017 07:41

bbatsov merged commit 59c4dcf into rubocop:master Dec 28, 2017

dependencies bot mentioned this pull request Mar 5, 2018

Update rubocop in / from 0.52.1 to 0.53.0 chauncey-garrett/dotfiles#103

Open

dependabot-preview bot mentioned this pull request Mar 19, 2018

Bump rubocop from 0.52.1 to 0.53.0 tnir/gitlabhq#95

Closed

jobertabma mentioned this pull request May 9, 2018

Cop for detecting use of Kernel.open and IO.read methods #4516

Closed

mame deleted the add_new_security_open_cop branch August 20, 2024 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `Security/Open` cop #5319

Add new `Security/Open` cop #5319

mame commented Dec 26, 2017 •

edited

Loading

pocke Dec 27, 2017

mame Dec 27, 2017

pocke left a comment

pocke Dec 27, 2017

mame Dec 27, 2017

pocke Dec 27, 2017

mame Dec 27, 2017

mame commented Dec 27, 2017

pocke commented Dec 27, 2017

mame commented Dec 27, 2017

bbatsov commented Dec 27, 2017

bbatsov Dec 27, 2017

bbatsov Dec 27, 2017

bbatsov Dec 27, 2017

bbatsov Dec 27, 2017

bbatsov commented Dec 27, 2017

mame commented Dec 28, 2017 •

edited

Loading

bbatsov Dec 28, 2017

mame Dec 28, 2017

bbatsov commented Dec 28, 2017

Drenmi commented Dec 28, 2017

bbatsov commented Dec 28, 2017

mame commented Dec 28, 2017

Drenmi commented Dec 28, 2017

Add new Security/Open cop #5319

Add new Security/Open cop #5319

Conversation

mame commented Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pocke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mame commented Dec 27, 2017

pocke commented Dec 27, 2017

mame commented Dec 27, 2017

bbatsov commented Dec 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Dec 27, 2017

mame commented Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Dec 28, 2017

Drenmi commented Dec 28, 2017

bbatsov commented Dec 28, 2017

mame commented Dec 28, 2017

Drenmi commented Dec 28, 2017

Add new `Security/Open` cop #5319

Add new `Security/Open` cop #5319

mame commented Dec 26, 2017 •

edited

Loading

mame commented Dec 28, 2017 •

edited

Loading