Prevent OpenURI redirection & private hosts during API image upload #3593

calebhaye · 2020-04-22T18:23:05Z

Description
Paperclip’s CVE-2017–0889 Server Side Request Forgery (SSRF)
vulnerability (https://medium.com/in-the-weeds/all-about-paperclips-cve-2017-0889-server-side-request-forgery-ssrf-vulnerability-8cb2b1c96fe8) was likely introduced in our attempts to allow API image uploads via URL. This patch ensures that redirection does not take place in the context of
uploading an image via the api. It also does a check to make sure the IP/host aren't private. Lastly, it uses VCR to stub out the http requests for images.

Ref #3573

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

api/spec/requests/spree/api/images_controller_spec.rb

api/app/controllers/spree/api/images_controller.rb

jarednorman

I took a quick look at the CVE and this seems like a good fix to me.

aldesantis · 2020-04-24T07:41:46Z

@calebhaye correct me if I'm wrong, but we still need to ensure the user cannot pass a private IP as the hostname, is that right?

Also, would it be possible to stub the HTTP requests with VCR? That way we're not calling live URLs in our tests, which may easily break/fail randomly.

Paperclip’s CVE-2017–0889 Server Side Request Forgery (SSRF) vulnerability (https://medium.com/in-the-weeds/all-about-paperclips-cve-2017-0889-server-side-request-forgery-ssrf-vulnerability-8cb2b1c96fe8) was likely introduced in solidusio#3573. This patch ensures that redirection does not take place in the context of uploading an image via the api, and ensures that a private address is not used for the attachment parameter.

calebhaye · 2020-04-24T10:43:49Z

@calebhaye correct me if I'm wrong, but we still need to ensure the user cannot pass a private IP as the hostname, is that right?

Thanks @aldesantis, I added a private IP/host check.

Also, would it be possible to stub the HTTP requests with VCR? That way we're not calling live URLs in our tests, which may easily break/fail randomly.

I also added VCR, but I left it as a separate commit, for now. Of course, if we're happy with the implementation I'll squash the commits and we can merge. But if we decide for some reason that VCR isn't the way we want to go, rebasing out the commit will be easy.

Thank you for your comment, I'm looking forward to hearing your feedback.

aldesantis · 2020-04-27T08:33:41Z

api/spec/requests/spree/api/images_controller_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true

 require 'spec_helper'
+require "private_address_check/tcpsocket_ext"


Just curious why this is needed?

This is the code that contains the error that will be raised, and that the spec tests for.

The gem itself seems pretty simple, and we could just copy or fork it, but it has undergone some review, so I figured maybe it's worth keeping. I'm open to a better solution, if anyone has any ideas.

I could require it only in the sole spec where it was needed

aldesantis · 2020-04-27T08:34:25Z

api/app/controllers/spree/api/images_controller.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true

+require 'private_address_check'


We should either add this to the gemspec or only require it when the attachment is a URI, making the functionality and dependency optional.

Yeah, I felt like I might be doing this wrong. Any advice on a proper implementation is welcomed. For now I'll do your latter suggestion.

aldesantis · 2020-04-27T08:34:46Z

@calebhaye left a couple of comments, thanks!

kennyadsl · 2020-04-28T08:42:31Z

I'm starting to think this shouldn't be handled by Solidus. What about keeping URL upload disabled and allow enabling it with a preference? As far as I got, it's just a matter of adding Paperclip::UriAdapter in a Rails initializer, as suggested here: https://github.com/thoughtbot/paperclip#io-adapters.

It may sound drastic but, what about reverting the code in the controller and just adding some documentation on how to enable this adapter in Paperclip?

Another thing that I prefer not to do is adding code specific for Paperclip which is deprecated and we are trying to slowly replace with ActiveStorage.

What are your thoughts about this?

aldesantis · 2020-04-28T10:02:27Z

@kennyadsl my 2 cents: I like the feature and would like to keep it for the time being, and I think we could easily replicate it with ActiveStorage when the time comes.

With that said, it would probably be a good idea to make the behavior optional by adding a configuration option, so that the user is fully aware of the security implications and we are not potentially slipping a vulnerability into the codebase without them knowing.

calebhaye · 2020-04-28T16:03:30Z

@kennyadsl @aldesantis

Honestly, I have mixed feelings about it. I'm obviously biased because a) I wrote the code, and b) it serves the needs of my business, but beyond my selfishness there may be a valid reason for keeping the feature in place.

I guess one relevant question would be can anyone show me an example of the image api working without the feature in place? I wasn't able to get it to work, there is no documentation on it, and when I asked the Slack channel no one was able to give me an example there either. My point is, proving that the alternative works and providing docs for that would be a good place to start.

Beyond that, I think we should consider non-ruby devs who want to use Solidus. Having this feature work out of the box without having to write a Rails initializer might be a good thing.

That being said, I think it is easy enough to write an initializer to get this sort of functionality, but honestly if I had, I probably would have exposed my app to vulnerabilities. Having the group-think we've had around these implementations and making a decision in the best interest of the end-user seems like the responsible thing to do.

kennyadsl · 2020-05-08T07:13:18Z

We discussed this via DM on Slack and we considered closing this and reverting the other PR that introduces this security concern (#3573).

The API image upload is working, we tried with:

curl -v -H "Content-Type: multipart/form-data" -H "Authorization: Bearer your_token" -i -F image[attachment]=@test_image.png http://localhost:3000/api/products/1/images

and the image is uploaded correctly.

Anyway, we agreed that we should improve the documentation around this with the following:

Add a better description in our API documentation for the endpoints that require "Content-Type: multipart/form-data".
Explain how to configure your app to allow other types of upload, like uploading via URLs (for paperclip we can link https://github.com/thoughtbot/paperclip/#io-adapters).

calebhaye · 2020-05-08T21:40:56Z

I personally still think we should keep it, but only enable it with a config flag. Just so that others don't introduce security vulnerabilities when they try to add the adapter as per the paperclip docs you referenced, and so we don't waste the thought we've put into this thus far. Showing how to do something safely seems like the best approach, plus we get a handy feature that many developers will use, and it doesn't require writing a custom adapter

@kennyadsl what do you think of doing that, rather than throwing away all the code and a feature that is arguably valuable?

aldesantis · 2020-05-11T07:35:19Z

@calebhaye how about extracting this into an extension? It should be fairly easy to do and would solve a lot of problems: users would have to explicitly install the extension, so they'd know the security implications of the feature, but they'd still be able to benefit from all the work and thought that went into the feature.

calebhaye · 2020-05-11T22:20:11Z

@calebhaye how about extracting this into an extension? It should be fairly easy to do and would solve a lot of problems: users would have to explicitly install the extension, so they'd know the security implications of the feature, but they'd still be able to benefit from all the work and thought that went into the feature.

Sounds good! I'll do that. Closing this now.

This reverts commit 4a1824a. This commit potentially introduces a security vulnerabilty, (CVE-2017–0889) as described here: https://medium.com/in-the-weeds/all-about-paperclips-cve-2017-0889-server-side-request-forgery-ssrf-vulnerability-8cb2b1c96fe8 as kindly reported and described by this PR author in solidusio#3593. Also, this PR has been introduced because there wasn't a clear way to upload images via API, but this can be done by setting the right Content-Type header in the request, as follow: curl -v -H "Content-Type: multipart/form-data" -H "Authorization: Bearer YOUR_TOKEN" http://your.host/api/products/1

houndci-bot reviewed Apr 22, 2020

View reviewed changes

api/spec/requests/spree/api/images_controller_spec.rb Show resolved Hide resolved

api/app/controllers/spree/api/images_controller.rb Outdated Show resolved Hide resolved

calebhaye force-pushed the prevent-openuri-redirect-for-api-upload-by-url branch 2 times, most recently from 64228ac to 348f8c1 Compare April 22, 2020 18:27

jarednorman approved these changes Apr 23, 2020

View reviewed changes

calebhaye force-pushed the prevent-openuri-redirect-for-api-upload-by-url branch from 348f8c1 to c4dfa2a Compare April 24, 2020 10:08

calebhaye changed the title ~~Prevent OpenURI redirection during API image upload~~ Prevent OpenURI redirection & private hosts during API image upload Apr 24, 2020

Add VCR for uploading images in tests.

66d53f9

calebhaye force-pushed the prevent-openuri-redirect-for-api-upload-by-url branch from 62d3e39 to 66d53f9 Compare April 24, 2020 18:46

aldesantis requested changes Apr 27, 2020

View reviewed changes

fixup: change location of require statements

9a72d19

calebhaye force-pushed the prevent-openuri-redirect-for-api-upload-by-url branch from af5c450 to 9a72d19 Compare April 28, 2020 16:35

kennyadsl added the Needs Core Team Decision label Apr 29, 2020

calebhaye closed this May 11, 2020

kennyadsl mentioned this pull request May 13, 2020

Revert "API uploads images via URL" (#3573) #3622

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent OpenURI redirection & private hosts during API image upload #3593

Prevent OpenURI redirection & private hosts during API image upload #3593

calebhaye commented Apr 22, 2020 •

edited

Loading

jarednorman left a comment

aldesantis commented Apr 24, 2020 •

edited

Loading

calebhaye commented Apr 24, 2020

aldesantis Apr 27, 2020

calebhaye Apr 28, 2020

calebhaye Apr 28, 2020

aldesantis Apr 27, 2020

calebhaye Apr 28, 2020

aldesantis commented Apr 27, 2020

kennyadsl commented Apr 28, 2020

aldesantis commented Apr 28, 2020

calebhaye commented Apr 28, 2020

kennyadsl commented May 8, 2020

calebhaye commented May 8, 2020

aldesantis commented May 11, 2020

calebhaye commented May 11, 2020

		@@ -1,5 +1,7 @@
		# frozen_string_literal: true

		require 'private_address_check'

Prevent OpenURI redirection & private hosts during API image upload #3593

Prevent OpenURI redirection & private hosts during API image upload #3593

Conversation

calebhaye commented Apr 22, 2020 • edited Loading

jarednorman left a comment

Choose a reason for hiding this comment

aldesantis commented Apr 24, 2020 • edited Loading

calebhaye commented Apr 24, 2020

aldesantis Apr 27, 2020

Choose a reason for hiding this comment

calebhaye Apr 28, 2020

Choose a reason for hiding this comment

calebhaye Apr 28, 2020

Choose a reason for hiding this comment

aldesantis Apr 27, 2020

Choose a reason for hiding this comment

calebhaye Apr 28, 2020

Choose a reason for hiding this comment

aldesantis commented Apr 27, 2020

kennyadsl commented Apr 28, 2020

aldesantis commented Apr 28, 2020

calebhaye commented Apr 28, 2020

kennyadsl commented May 8, 2020

calebhaye commented May 8, 2020

aldesantis commented May 11, 2020

calebhaye commented May 11, 2020

calebhaye commented Apr 22, 2020 •

edited

Loading

aldesantis commented Apr 24, 2020 •

edited

Loading