Do not use open-uri to fetch encoded content for paperclip

OpenURI module introduces several vulnerabilities. In short, it accepts
any string and calls Kernel#open. Because ruby executes commands if it
starts with a pipe, an attacker could pass strings like '|ls' or '|cat
/etc/passwd'. In other words, an attacker could execute any script.

More info here: http://sakurity.com/blog/2015/02/28/openuri.html

Now, we use URI#parse to fetch the URI and let paperclip make its magic
to fetch the encoded content and store it. Although paperclip uses
OpenURI too (UriAdapater class), it tries to create a valid URI from the
string (HttpUrlProxyAdapter class). If the string does not conform with
rules defined on RFC2396, it will raise an exception. That should be
enough to mitigate the attacks mentioned (paperclip also raises errors
if it cannot access the URI).

Of course, if Ruby's implementation of RFC2396 has security issues, we
are still vulnerable (I don't think that's the case).
Signed-off-by: marcheing's avatarHeitor Reis <marcheing@gmail.com>
parent 44c0b365
Pipeline #3671569 passed with stage
in 11 minutes and 9 seconds
require 'open-uri'
class ContentsController < ApplicationController
before_action :set_content, only: [:show]
protect_from_forgery with: :null_session, only: :zencoder_callback
......@@ -38,8 +36,8 @@ class ContentsController < ApplicationController
@encoded_content.height = params[:outputs].first[:height]
@encoded_content.duration = params[:outputs].first[:duration_in_ms]
@encoded_content.file_size = params[:outputs].first[:file_size_in_bytes]
@encoded_content.video = open(params[:outputs].first[:url])
@encoded_content.thumbnail = open(params[:outputs].first[:thumbnails].first[:images].first[:url])
@encoded_content.video = URI.parse(params[:outputs].first[:url])
@encoded_content.thumbnail = URI.parse(params[:outputs].first[:thumbnails].first[:images].first[:url])
@encoded_content.save
end
......
......@@ -97,8 +97,8 @@ RSpec.describe ContentsController, type: :controller do
let(:encoded_video) { build(:encoded_video) }
before do
expect(EncodedContent).to receive(:find_by).with(job_id: job_id).and_return(encoded_video)
expect(subject).to receive(:open).with(outputs.first[:url]).and_return(video_file_mock)
expect(subject).to receive(:open).with(thumb_output.first[:images].first[:url]).and_return(thumb_file_mock)
expect(URI).to receive(:parse).with(outputs.first[:url]).and_return(video_file_mock)
expect(URI).to receive(:parse).with(thumb_output.first[:images].first[:url]).and_return(thumb_file_mock)
expect(encoded_video).to receive(:save).and_return(true)
expect(encoded_video).to receive(:output_id=).with(output[:id])
expect(encoded_video).to receive(:state=).with(output[:state])
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment