Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new `Security/Open` cop #5319

Merged
merged 1 commit into from Dec 28, 2017

Conversation

Projects
None yet
4 participants
@mame
Copy link
Contributor

mame commented Dec 26, 2017

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.

@mame mame force-pushed the mame:add_new_security_open_cop branch from 200f1d6 to bd9fb67 Dec 26, 2017

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

## master (unreleased)

### New features

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

This comment has been minimized.

@pocke

pocke Dec 27, 2017

Member

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

This comment has been minimized.

@mame

mame Dec 27, 2017

Author Contributor

git push --force done!

@mame mame force-pushed the mame:add_new_security_open_cop branch from bd9fb67 to b8c2413 Dec 27, 2017

@pocke

pocke approved these changes Dec 27, 2017

Copy link
Member

pocke left a comment

Looks good to me except the comments, thank you!


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

This comment has been minimized.

@pocke

pocke Dec 27, 2017

Member

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

This comment has been minimized.

@mame

mame Dec 27, 2017

Author Contributor

Indeed! Fixed and pushed.

#
# # bad
#
# open(something)

This comment has been minimized.

@pocke

pocke Dec 27, 2017

Member

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)

This comment has been minimized.

@mame

mame Dec 27, 2017

Author Contributor

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

@mame mame force-pushed the mame:add_new_security_open_cop branch 2 times, most recently from 2037b61 to 3829ec5 Dec 27, 2017

@mame

This comment has been minimized.

Copy link
Contributor Author

mame commented Dec 27, 2017

Rebased.

@pocke

This comment has been minimized.

Copy link
Member

pocke commented Dec 27, 2017

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

@mame

This comment has been minimized.

Copy link
Contributor Author

mame commented Dec 27, 2017

No problem! Thank you for your work.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Dec 27, 2017

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

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

This comment has been minimized.

@bbatsov

bbatsov Dec 27, 2017

Collaborator

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.

# @example
#
# # bad
#

This comment has been minimized.

@bbatsov

bbatsov Dec 27, 2017

Collaborator

I'd remove this blank line.

# open(something)
#
# # good
#

This comment has been minimized.

@bbatsov

bbatsov Dec 27, 2017

Collaborator

Here as well.

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

This comment has been minimized.

@bbatsov

bbatsov Dec 27, 2017

Collaborator

open -> Kernel#open

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Dec 27, 2017

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 mame force-pushed the mame:add_new_security_open_cop branch from 3829ec5 to 556ddeb Dec 28, 2017

@mame

This comment has been minimized.

Copy link
Contributor Author

mame commented Dec 28, 2017

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 :-)

@@ -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 comment has been minimized.

@bbatsov

bbatsov Dec 28, 2017

Collaborator

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

This comment has been minimized.

@mame

mame Dec 28, 2017

Author Contributor

Sorry! I left it to git merge. Fixed.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Dec 28, 2017

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

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

@mame mame force-pushed the mame:add_new_security_open_cop branch from 556ddeb to f398a97 Dec 28, 2017

@Drenmi

This comment has been minimized.

Copy link
Collaborator

Drenmi commented Dec 28, 2017

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 bbatsov merged commit 59c4dcf into rubocop-hq:master Dec 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Dec 28, 2017

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

This comment has been minimized.

Copy link
Contributor Author

mame commented Dec 28, 2017

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

This comment has been minimized.

Copy link
Collaborator

Drenmi commented Dec 28, 2017

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. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.