Skip to content

Instantly share code, notes, and snippets.

@joernchen
Last active January 9, 2017 01:38
Show Gist options
  • Star 1 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save joernchen/f28ec01de20b22bbbee1622a41deb601 to your computer and use it in GitHub Desktop.
Save joernchen/f28ec01de20b22bbbee1622a41deb601 to your computer and use it in GitHub Desktop.
DIscourse

The root cause lies in app/models/optimized_image.rb. I'll use the downsize method here as an example to illustrate the general problem also within the other conversion methods:

  def self.downsize_instructions(from, to, dimensions, opts={})
    %W{
      convert
      #{from}[0]
      -gravity center
      -background transparent
      -resize #{dimensions}#{!!opts[:force_aspect_ratio] ? "\\!" : "\\>"}
      -profile #{File.join(Rails.root, 'vendor', 'data', 'RT_sRGB.icm')}
      #{to}
    }
  end

  def self.optimize(operation, from, to, dimensions, opts={})
    method_name = "#{operation}_instructions"
    if !!opts[:allow_animation] && (from =~ /\.GIF$/i || opts[:filename] =~ /\.GIF$/i)
      method_name += "_animated"
    end
    instructions = self.send(method_name.to_sym, from, to, dimensions, opts)
    convert_with(instructions, to)
  end

  def self.convert_with(instructions, to)
    `#{instructions.join(" ")} &> /dev/null`
    return false if $?.exitstatus != 0

    ImageOptim.new.optimize_image!(to)
    true
  rescue

In the above code none of the parameters to downsize are properly validated before they are used in the shell via the backtick operators in convert_with. Therefore if an attacker can control any parameter to downsize this would lead directly to command execution.

The above code is indeed reachable via user input from HTTP parameters. Observe the UploadsController, within the create_upload method OptimizedImage.downsize is being called:

  def create_upload(type, file, url)
    begin
      maximum_upload_size = [SiteSetting.max_image_size_kb, SiteSetting.max_attachment_size_kb].max.kilobytes

      # ensure we have a file
      if file.nil?
        # API can provide a URL
        if url.present? && is_api?
          tempfile = FileHelper.download(url, maximum_upload_size, "discourse-upload-#{type}") rescue nil
          filename = File.basename(URI.parse(url).path)
        end
      else
        tempfile = file.tempfile
        filename = file.original_filename
        content_type = file.content_type
      end

     [...] 
OptimizedImage.downsize(tempfile.path, tempfile.path, dimensions, filename: filename, allow_animation: allow_animation)

Here tempfile.path is partially user controlled via the type parameter from within the create method. This partial control is enough to trigger command execution via a type parameter like "`some command`".

Further constraints are that the request has to be an API request with an URL, additionally the JPG image delivered from that URL needs to be big enough to trigger the downsize.

The constraints to the image are pretty easy to be solved. A bit harder is the API key constraint, but with (in the default setting) a user trust level of 1, this can be fulfilled as well. This is due to that a request with UserApiKey is sufficient to pass the is_api? check in the UploadsController.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment