Skip to content
Permalink
Browse files Browse the repository at this point in the history
Don't allow remote shell execution
Kernel#open accepts a string of format "| <shell command>" which
executes the specified shell command and otherwise presumably acts as
IO.popen. The open-uri standard library overrides Kernel#open to also
accept URLs.

However, the overridden Kernel#open just delegates to URI#open, so we
switch to using that directly and avoid the remote shell execution
vulnerability. For files we just use File.open, which should have the
same behaviour as Kernel#open.
  • Loading branch information
janko committed May 26, 2019
1 parent 152d33a commit 4cd5081
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 8 deletions.
14 changes: 6 additions & 8 deletions lib/mini_magick/image.rb
Expand Up @@ -82,17 +82,15 @@ def self.import_pixels(blob, columns, rows, depth, map, format = 'png')
def self.open(path_or_url, ext = nil, options = {})
options, ext = ext, nil if ext.is_a?(Hash)

ext ||=
if File.exist?(path_or_url)
File.extname(path_or_url)
else
File.extname(URI(path_or_url).path)
end
uri = URI(path_or_url.to_s)

ext ||= File.extname(uri.path)
ext.sub!(/:.*/, '') # hack for filenames or URLs that include a colon

Kernel.open(path_or_url, "rb", options) do |file|
read(file, ext)
if uri.is_a?(URI::HTTP) || uri.is_a?(URI::FTP)
uri.open(options) { |file| read(file, ext) }
else
File.open(uri.to_s, "rb", options) { |file| read(file, ext) }
end
end

Expand Down
8 changes: 8 additions & 0 deletions spec/lib/mini_magick/image_spec.rb
Expand Up @@ -76,6 +76,14 @@
expect(File.extname(image.path)).to eq ".jpg"
end

it "doesn't allow remote shell execution" do
expect {
described_class.open("| touch file.txt") # Kernel#open accepts this
}.to raise_error(URI::InvalidURIError)

expect(File.exist?("file.txt")).to eq(false)
end

it "accepts open-uri options" do
stub_request(:get, "http://example.com/image.jpg")
.with(headers: {"Foo" => "Bar"})
Expand Down

3 comments on commit 4cd5081

@greysteil
Copy link

@greysteil greysteil commented on 4cd5081 Jul 18, 2019

Choose a reason for hiding this comment

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

First of all, thanks for mini_magick (I'm a user on an app that tracked a big cycle I did) and for your work on this security fix.

I work on GitHub's security workflows team and am looking into how we can make it easier for maintainers to publicise security vulnerabilities like this one. I'd love your feedback if you have 5 mins?

Specifically:

  • did you consider using the "Maintainer Security Advisories" feature to publicise this vulnerability (it's in the "Security" tab on this repo)? What would make that feature useful to you?
  • do you have any other thoughts on the process of handling a security vulnerability, and how GitHub can help?

We're trying to make the process easier so if you have any feedback at all please let me know. You can email me on greysteil@github.com if you'd like to discuss anything privately.

@janko
Copy link
Member Author

@janko janko commented on 4cd5081 Jul 18, 2019

Choose a reason for hiding this comment

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

did you consider using the "Maintainer Security Advisories" feature to publicise this vulnerability (it's in the "Security" tab on this repo)? What would make that feature useful to you?

The main reason was that I don't have admin access to the minimagick organization, so I don't have the permission to edit security advisories.

do you have any other thoughts on the process of handling a security vulnerability, and how GitHub can help?

I've had a surprisingly hard time requesting a CVE for this vulnerability. Trying directly on https://cve.mitre.org/ obviously wasn't a good user experience, but I even couldn't figure it out on HackerOne (we've had to email them for instructions). It would be nice if GitHub had a straightforward guide for how to request a CVE.

@greysteil
Copy link

Choose a reason for hiding this comment

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

It would be nice if GitHub had a straightforward guide for how to request a CVE.

Really useful feedback - thanks. We can do that!

The main reason was that I don't have admin access to the minimagick organization, so I don't have the permission to edit security advisories.

Oh interesting, thanks for that. I think requiring admin permission there makes sense, but will have a think on that one.

Please sign in to comment.