Add cop forbidding `Kernel.open` method call
Description of the proposal
According to our security guidelines, the method open
or Kernel.open
can be used to execute commands as well. For example, with a simple method like:
def open_url(uri)
open(uri)
end
Without the necessary validation in the uri
variable, a malicious user could perform an attack by just setting the uri
to |cat /etc/passwd > /usr/share/nginx/html/copied
or any other kind. We'll be basically giving a console to the user.
Besides that, the Kernel.open
method can be used to open URLs. This is discouraged in favor of Gitlab::HTTP
because we don't have control over Kernel.open
and we can't validate if, for example, the request is made to localhost
.
I haven't found any reference in the codebase to this method so, yay!
Since version 0.53, Rubocop enables by default the cop Security/Open
, which does exactly this, but there are several cases not covered. It behaves like this:
# Bad
open(foo)
open("|foo #{bar}")
# Good
open("foo")
open("|foo")
open("foo #{bar}")
Kernel.open("foo")
Kernel.open("|foo")
Kernel.open("foo#{bar}")
Kernel.open("|foo #{bar}")
Kernel.open(foo)
The cop we're proposing will be more restrictive and it will behave like:
# Good
open # without arguments
# Bad
open(foo)
open("|foo #{bar}")
open("|foo")
open("foo #{bar}")
Kernel.open("|foo")
Kernel.open("foo#{bar}")
Kernel.open("|foo #{bar}")
Kernel.open(foo)
False Positives
The default Security/Open
can trigger some false positives:
class Foo
def open(file)
File.open(file)
end
def execute(file)
open(file) # false positive
end
end
The new cop will also have those false positives and also trigger some more in the scenarios commented above.
-
Mention the proposal in the next backend weekly call and the #backend channel to encourage contribution -
Proceed with the proposal once 50% of the maintainers have weighed in, and 80% of the votes are 👍 -
Once approved, mention it again in the next backend weekly call and the #backend channel