Parse YAML safely #157

timrogers · 2017-06-14T09:12:54Z

An issue from June 2014, #92, raises the risks of the current FaradayMiddleware::ParseYaml middleware which uses YAML.load. This method is very unsafe and exposes you to remote code execution - see ruby/psych#119 for discussion.

At the time, @mislav decided not to make this change to avoid messing with backwards compatability.

I would suggest that we should revisit this decision - the risks of this are very high, very few people are using this middleware most likely, and it doesn't seem unreasonable to break this
as long as we are clear on the change in the changelog.

This does that by using Ruby's built-in Psych.safe_load for Ruby 2.0.0 and onwards, or otherwise uses the safe_yaml gem for earlier versions.

iMacTia · 2017-06-14T10:36:21Z

spec/unit/parse_yaml_spec.rb

-    it "returns false for empty body" do
-      expect(process('').body).to be false
-    end
-


My understanding is that the new safe_yaml is supposed to return nil. Can we test that rather than deleting the test?

YAML.safe_load (which is part of Psych in Ruby from 2.x onwards) returns nil.

But the safe_yaml gem returns false still, unfortunately. Rather than have a conditional test, I elected to remove this. We have to use safe_yaml because Psych.safe_load doesn't exist in Ruby 1.x. The better solution would be to stop supporting Ruby 1.x, which would vastly improve this code, and makes sense given that 1.9 reached end of life more than 2 years ago.

iMacTia · 2017-06-14T10:37:30Z

lib/faraday_middleware/response/parse_yaml.rb

+        Psych.safe_load body
+      else
+        SafeYAML.load body
+      end


I'm a bit short on time so can you please expose the difference between one and the other?
How would this code change, assuming we might drop support for some ruby versions?

Psych (which is included in Ruby), from Ruby 2.x onwards, includes the safe_load method. Using SafeYAML is a way of backporting this for Ruby 1.x.

If we dropped support for old Ruby versions, then we wouldn't need an if statement, we could remove SafeYAML and just use Psych.

timrogers · 2017-06-14T10:46:01Z

@iMacTia Thanks for getting back to me on this - really appreciate it!

timrogers · 2017-06-14T10:57:03Z

Actually, ignore my previous comments - I have a better plan! A version of Psych has been included in Ruby's STDLIB since 1.9.2. Psych.safe_load was only added in the version of Psych bundled with Ruby 2.1 (Psych v2.0.0).

However, and fortunately, you can install a newer version of Psych using RubyGems, which supports .safe_load, to replace the STDLIB version.

I'm going to implement this approach now. __The only slight cost is it will require us to drop support for Ruby 1.8.x, but that isn't supported by Faraday anyway, and is way out of date, having been EOL'd in early 2013.

iMacTia · 2017-06-14T11:04:15Z

Thanks for the detailed explanation @timrogers!
As you probably guessed, my main objectives here are:

Find a solution that works for every case, avoiding if/else branches on .gemspec and elsewhere.
Keep backward compatibility.
If 2 is not possible, than drop ruby versions support (but this could mean waiting for a future release).

After reading your comments, my understanding is that SafeYAML gem is available for all ruby versions we support and has the same identical behaviour as the current release (return false in case of empty body). That makes it, for me, the best option we have.

iMacTia · 2017-06-14T11:06:05Z

Going for psych usage instead, will cause 2 direct effects:

Drop support for ruby 1.8.7 (not that much of a deal, as you said)
Break backward compatibility

Second point is extremely important and makes me looking at the SafeYAML solution.
If we decide to go down this way, then we'll need to postpone the release for this change

timrogers · 2017-06-14T11:17:06Z

@iMacTia That makes sense. Let me try with safe_yaml.

timrogers · 2017-06-14T11:31:55Z

This is now using safe_yaml - this maintains compatability with all supported Ruby versions, and as far as possible, should behave the same. This, I think, is the best we can do whilst still having a secure default behaviour. Just waiting for the tests to run now

We should still make sure people are aware that this is breaking in some sense (it'll no longer unsafely parse by default!), but I think it's highly likely that few or zero people are actually doing this.

@iMacTia Does this look good to you?

iMacTia · 2017-06-14T11:36:48Z

I agree this is the best solution we could go for :) I'll review the code one last time if tests are all green.
Agree with you that people are not (or SHOULD not) "relying" on the unsafe parser. If the safe one works exactly the same way, apart from code injections or similar, then it should be OK 👍

timrogers · 2017-06-14T13:06:59Z

@iMacTia Passing now!

mislav · 2017-06-14T14:07:18Z

@timrogers Please don't @-mention someone in a commit and then rebase the heck out of that commit repeatedly. 😉

iMacTia · 2017-06-14T14:23:19Z

ahaha he wanted to be sure you got notified. I guess that worked 😃

iMacTia · 2017-06-14T14:27:33Z

approved and marked for v0.12

timrogers · 2017-06-19T08:52:55Z

@iMacTia Do you have a view on when v0.12 is likely to go live? 🚀

iMacTia · 2017-06-19T09:04:24Z

Unfortunately not at the moment.
I need some time to get rid of ruby 1.8.7 first.
Moreover, there are a couple of bugs on Faraday as well that I would like to address first, in order to release an hotfix version there.
It's hard to plan something with certainty due to the busy period at work, I'll just get to it as soon as I find a couple of hours free.

Sorry!

* Partially resolves #272 * Full fix: wait for lostisland/faraday_middleware#157 to be merged

iMacTia

Hi @timrogers, I'm trying to get release 0.12 ready to ship.
I realised you added safe_yaml to the gemspec and this made it dependency that will be installed with faraday_middleware. However, we only want that dependency to be considered if you use the Yaml middleware, so can you please remove it from there and move it to the Gemfile (for rspec)?

iMacTia · 2017-07-27T11:55:31Z

lib/faraday_middleware/response/parse_yaml.rb

-    dependency 'yaml'
+    dependency do
+      require 'safe_yaml/load'
+    end


Shouldn't we specify the actual dependency here?

dependency 'safe_yaml' do require 'safe_yaml/load' end

It looks from Faraday::Middleware.dependency (https://github.com/lostisland/faraday/blob/a712938071bb0997410b5de4a4f2517d30fc4d3d/lib/faraday/middleware.rb#L13) that you either supply a block or a string to require. I'll change this to dependency 'safe_yaml/load'.

I got mad looking for that method, and it was in Faraday::Middleware 🤦‍♂️
Agree with you that `dependency 'safe_yaml/load' is better then

iMacTia · 2017-07-27T11:59:08Z

faraday_middleware.gemspec

@@ -14,6 +14,7 @@ Gem::Specification.new do |spec|
  spec.licenses = ['MIT']

  spec.add_dependency 'faraday', ['>= 0.7.4', '< 1.0']
+  spec.add_dependency 'safe_yaml'


Don't add the dependency here, add it on the Gemfile so the gem won't be installed with Faraday

@mislav

An issue from June 2014, #92, raises the risks of the current `FaradayMiddleware::ParseYaml` middleware which uses `YAML.load`. This method is very unsafe and exposes you to remote code execution - see ruby/psych#119 for discussion. At the time, @mislav decided not to make this change to avoid messing with backwards compatability. I would suggest that we should revisit this decision - the risks of this are very high, very few people are using this middleware most likely, and it doesn't seem unreasonable to break this as long as we are clear on the change in the changelog. This does that by installing the `safe_yaml` gem, which is compatible with all Ruby versions we support.

timrogers · 2017-07-28T08:44:41Z

@iMacTia Done!

iMacTia · 2017-07-28T08:53:27Z

Github is not showing any new commit, are you sure you pushed your latest changes?

timrogers · 2017-07-28T09:00:15Z

Sorry! I thought I'd pushed but the connection failed. Should be there now.

* lostisland/faraday_middleware#157 * Now merged, so lets update the gem!

timrogers changed the title ~~Parse YAML safely with Psych.safe_load~~ Parse YAML safely Jun 14, 2017

iMacTia requested changes Jun 14, 2017

View reviewed changes

iMacTia added this to the 0.12.0 milestone Jun 14, 2017

iMacTia approved these changes Jun 14, 2017

View reviewed changes

petems added a commit to petems/tugboat that referenced this pull request Jul 3, 2017

Switch to safe_yaml fork

067ef30

* Partially resolves #272 * Full fix: wait for lostisland/faraday_middleware#157 to be merged

petems mentioned this pull request Jul 3, 2017

Switch to safe_yaml fork petems/tugboat#274

Merged

iMacTia requested changes Jul 27, 2017

View reviewed changes

iMacTia approved these changes Jul 28, 2017

View reviewed changes

iMacTia merged commit 84be491 into lostisland:master Jul 28, 2017

petems added a commit to petems/tugboat that referenced this pull request Nov 22, 2017

Updates Faraday middleware gem

b5cee3d

* lostisland/faraday_middleware#157 * Now merged, so lets update the gem!

petems added a commit to petems/tugboat that referenced this pull request Nov 22, 2017

Updates Faraday middleware gem

c7f5fb7

* lostisland/faraday_middleware#157 * Now merged, so lets update the gem!

petems mentioned this pull request Nov 22, 2017

Updates Faraday middleware gem petems/tugboat#289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse YAML safely #157

Parse YAML safely #157

timrogers commented Jun 14, 2017 •

edited

Loading

iMacTia Jun 14, 2017

timrogers Jun 14, 2017 •

edited

Loading

iMacTia Jun 14, 2017

timrogers Jun 14, 2017

timrogers commented Jun 14, 2017

timrogers commented Jun 14, 2017

iMacTia commented Jun 14, 2017

iMacTia commented Jun 14, 2017

timrogers commented Jun 14, 2017

timrogers commented Jun 14, 2017

iMacTia commented Jun 14, 2017

timrogers commented Jun 14, 2017

mislav commented Jun 14, 2017

iMacTia commented Jun 14, 2017

iMacTia commented Jun 14, 2017

timrogers commented Jun 19, 2017

iMacTia commented Jun 19, 2017

iMacTia left a comment

iMacTia Jul 27, 2017

timrogers Jul 28, 2017

iMacTia Jul 28, 2017

iMacTia Jul 27, 2017

timrogers commented Jul 28, 2017

iMacTia commented Jul 28, 2017

timrogers commented Jul 28, 2017

Parse YAML safely #157

Parse YAML safely #157

Conversation

timrogers commented Jun 14, 2017 • edited Loading

iMacTia Jun 14, 2017

Choose a reason for hiding this comment

timrogers Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

iMacTia Jun 14, 2017

Choose a reason for hiding this comment

timrogers Jun 14, 2017

Choose a reason for hiding this comment

timrogers commented Jun 14, 2017

timrogers commented Jun 14, 2017

iMacTia commented Jun 14, 2017

iMacTia commented Jun 14, 2017

timrogers commented Jun 14, 2017

timrogers commented Jun 14, 2017

iMacTia commented Jun 14, 2017

timrogers commented Jun 14, 2017

mislav commented Jun 14, 2017

iMacTia commented Jun 14, 2017

iMacTia commented Jun 14, 2017

timrogers commented Jun 19, 2017

iMacTia commented Jun 19, 2017

iMacTia left a comment

Choose a reason for hiding this comment

iMacTia Jul 27, 2017

Choose a reason for hiding this comment

timrogers Jul 28, 2017

Choose a reason for hiding this comment

iMacTia Jul 28, 2017

Choose a reason for hiding this comment

iMacTia Jul 27, 2017

Choose a reason for hiding this comment

timrogers commented Jul 28, 2017

iMacTia commented Jul 28, 2017

timrogers commented Jul 28, 2017

timrogers commented Jun 14, 2017 •

edited

Loading

timrogers Jun 14, 2017 •

edited

Loading