Security vulnerability in version 5.1.0

[abadfish]

Just got this message from GitHub:
We found a potential security vulnerability in one of your dependencies.
The paperclip dependency defined in Gemfile.lock has a known high severity security vulnerability in version range >= 3.1.4, <= 5.1.0 and should be updated.
I am using the latest version of paperclip. Is there a fix out there in the works? New version?

[guihatano]

I think this #2435 should work
But it needs to be merged yet, we will need to wait

[robbkidd]

Until a new version of the gem is released, would it be awful to add the following to an initializer?

(Updated with a probably-more-expressive single loop.)

worrisome_adapters = [Paperclip::UriAdapter, Paperclip::HttpUrlProxyAdapter, Paperclip::DataUriAdapter]

Paperclip.io_adapters
         .registered_handlers
         .delete_if do |_block, handler_class|
           worrisome_adapters.include?(handler_class)
         end

Results in:

> Paperclip.io_adapters.registered_handlers.map(&:second)
=> [Paperclip::EmptyStringAdapter,
 Paperclip::IdentityAdapter: ,
 Paperclip::FileAdapter,
 Paperclip::StringioAdapter,
 Paperclip::NilAdapter,
 Paperclip::AttachmentAdapter,
 Paperclip::UploadedFileAdapter]

[jhulford]

I'm curious how this CVE is exploitable by a bad actor? There are no real details given in the CVE description or the PR referenced.

Don't you have to actively attach a URI object or http(s) String for the unsafe adapters to be utilized? For those of us simply using Paperclip to handle uploaded files from an HTTP file upload, we should ok, right?

[blimmer]

Agreed - a writeup in this repo would be really helpful now that GitHub is warning everyone. It's hard to tell who is affected, or the recommended workaround.

[edslocomb]

@jhulford I've been digging around for that myself; the description of the current vulnerability is apparently not public yet: https://hackerone.com/reports/209430

@guihatano that PR disables the Uri adapter by default, but users who still want the feature and re-enable it will presumably still be vulnerable, so I'm not convinced that the patch closes the security hole (whatever it might actually be, see above).

[reedloden]

This is a Server-Side Request Forgery (SSRF) vulnerability, FYI. https://hackerone.com/reports/713 is an example of a way to exploit this (though the reporter wasn't aware of it at the time).

[edslocomb]

@reedloden that report is 4 years old, and a patch of some sort was merged 4 years ago. Are you saying the fix missed a particular category of attack? What do you mean by "though the reporter wasn't aware of it at the time?"

[reedloden]

@edslocomb Seems like some history would be helpful here.

Four years ago, we (HackerOne) received a report (https://hackerone.com/reports/713) alerting us to this SSRF issue (the reporter didn't seem to be aware it was an SSRF issue, but our standard root-cause analysis picked that up). We monkey-patched our version of paperclip by disabling Paperclip::HttpUrlProxyAdapter. However, balls were dropped (predates my time... sorry!), and upstream (Thoughtbot) was not notified properly, meaning that current paperclip has still been vulnerable since then.

Thoughtbot was finally notified in early 2017 as the result of this appearing somewhere else, which is where #2435 comes from. However, it seems that PR was never landed and released.

I finally assigned the CVE for this last November, and I guess one of the services that GitHub uses for monitoring for Ruby gem vulnerabilities took that CVE assignment and put it into some database, resulting in this GitHub-wide notification for anybody who uses paperclip.

Hope that helps provide some missing historical context behind this issue.

[edslocomb]

@reedloden thanks for the history, that does clear things up!

From the sound of it, users who do enable upload via URI will still be vulnerable after #2435 is merged.

[reedloden]

Perhaps incorporating https://github.com/jtdowney/private_address_check would be helpful towards a complete fix?

[paul-at]

@reedloden thank you for the detailed explanation!

My perception has always been that this is a widely known feature (even if undocumented) rather than a vulnerability. I first found it when one of the integrations to my app used it to copy attachments from one cloud to another (managed by paperclip).

[guihatano]

Hope they fix it soon

[mike-burns]

Fixing in #2531.

[mike-burns]

We have merged and released the fix from #2435 as Paperclip 5.2.0. Please upgrade as you see fit.

[rongutierrez]

I've released a blog post providing some more details on the vulnerability and some recommendations for handling uri_adapter validation when an app is using it - https://medium.com/in-the-weeds/all-about-paperclips-cve-2017-0889-server-side-request-forgery-ssrf-vulnerability-8cb2b1c96fe8

[dbelling]

Thanks for fixing this.

[pouellet]

Sorry for hijacking this closed thread, but it seems the best place to ask without reopening a new issue.

The fix for this issue resulted in the removal of DataUriAdapter from the default list of registered adapters. After having a quick look at the code, I do not understand how this adapter could be vulnerable to SSRF. It is not a subclass of the UriAdapter class and, unless I'm missing something, doesn't do any network requests:

def extract_target(uri)
  data_uri_parts = uri.match(REGEXP) || []
  StringIO.new(Base64.decode64(data_uri_parts[2] || ""))
end

It's debatable wether it should be part of the default registered adapters, but I think it might be good to clarify in the documentation if it is actually vulnerable to SSFR.

[mike-burns]

@pouellet you are right, a data URI is not vulnerable to SSRF attack. It can pose other issues (this is a fun one from today, unrelated to Paperclip: https://thatoddmailbox.github.io/2017/01/28/iotaseed.html ), but that is unrelated to this specific CVE.