CVE-2019-13574: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection') (CWE-78)
value: In lib/mini_magick/image.rb in MiniMagick before 4.9.4, a fetched remote image filename could cause remote command execution because Image.open input is directly passed to Kernel#open, which accepts a '|' character followed by a command.
CWE description: The product constructs all or part of an OS command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended OS command when it is sent to a downstream component.
CVSS v3 base score: 7.8
Published: 2019-07-12T03:15Z
Repository: https://github.com/minimagick/minimagick
Language: Ruby
Files changed: lib/mini_magick/image.rb, spec/lib/mini_magick/image_spec.rb
Commit date: 2019-05-26 17:47:24 +0200
Commit message: 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.

Diff: diff --git a/lib/mini_magick/image.rb b/lib/mini_magick/image.rb
index a1f47c6..0ac4780 100644
--- a/lib/mini_magick/image.rb
+++ b/lib/mini_magick/image.rb
@@ -80,21 +80,19 @@ module MiniMagick
     # @return [MiniMagick::Image] The loaded image
     #
     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
 
     ##
     # Used to create a new Image object data-copy. Not used to "paint" or
diff --git a/spec/lib/mini_magick/image_spec.rb b/spec/lib/mini_magick/image_spec.rb
index 192d834..00f9cb0 100644
--- a/spec/lib/mini_magick/image_spec.rb
+++ b/spec/lib/mini_magick/image_spec.rb
@@ -74,10 +74,18 @@ require "webmock/rspec"
           image = described_class.open("http://example.com/image.jpg")
           expect(image).to be_valid
           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"})
             .to_return(body: File.read(image_path))
           described_class.open("http://example.com/image.jpg", {"Foo" => "Bar"})

Vulnerable code:       ext ||=
        if File.exist?(path_or_url)
          File.extname(path_or_url)
        else
          File.extname(URI(path_or_url).path)
        end
      Kernel.open(path_or_url, "rb", options) do |file|
        read(file, ext)
Fixed code:       uri = URI(path_or_url.to_s)
      ext ||= File.extname(uri.path)
      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) }
        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