Is safe_yaml safe from CVE-2014-2525?

[konklone]

Does safe_yaml, by default, fall prey to the buffer overflow exploit described (and fixed) below?

Looks like it's not about "smart" parsing, but just about basic string parsing -- yet it also relates to URIs specifically, somehow, and I don't know if safe_yaml makes that distinction?

Announcement: https://www.ruby-lang.org/en/news/2014/03/29/heap-overflow-in-yaml-uri-escape-parsing-cve-2014-2525/
Technical description: http://www.ocert.org/advisories/ocert-2014-003.html

It's fixed in libyaml 1.6.

/cc @drinks

[dtao]

I saw this announcement and will look into the issue today. My hunch is that SafeYAML could very well be vulnerable to this exploit, since it still uses Psych (which in turn uses libyaml) internally. I will update this issue when I know more later this afternoon.

[dtao]

Yeah, SafeYAML has the vulnerability. I've got a PoC but (obviously) I'd rather not share it here.

Probably the safest bet is to explicitly require psych >= 2.0.5 since that has a backport of the fix to libyaml. I will see if I can find a different solution; but if not, I will release a new version by EOD w/ the updated psych dependency.

[konklone]

Thank you, @dtao!

[dtao]

On further investigation I think I have a better approach, which won't necessarily require forcing an update to the psych gem. It will take a bit more time to implement, though. But for reasons I'd rather not get into (yet), I think this alternate approach is preferable.

Naturally this is a high priority, and I will update this issue when I've hammered out a fix. It probably won't be done today.

[dtao]

Update: my genius idea turned out to be a dead end. (Actually, it was just way too complicated to implement in a reasonable time frame.) What I intend to do now is just detect, if using Psych, when the libyaml version is older than 0.1.6. And I'll have SafeYAML issue a warning in that case. Unfortunately it seems that's really the best I can do, short of adding a C extension to automatically upgrade libyaml for you (which, even to me, seems like overkill).

[konklone]

That makes sense to me.

[dtao]

FYI I've released 1.0.2, which detects your libyaml version and warns you if it's vulnerable to CVE-2014-2525.

I've also asked @tenderlove for his opinion about the recommendation to do this (if necessary):

gem install psych -- --enable-bundled-libyaml

If I get a thumbs up or he just never gets back to me, I'll close this issue. If he's like, "NO, that is a bad idea," I will (obviously) revisit the warning message.

[dtao]

Also, if you're looking at this issue and aren't sure what to do: another option is to switch to Syck as the underlying YAML parser. If you're on Ruby 1.9.x, Syck is automatically available. If you're on Ruby 2+ you'd need to install it manually.

# necessary if using Ruby 2+
gem install syck

Then:

require 'syck'
YAML::ENGINE.yamler = 'syck'

# note: this should be done *after* setting the YAML parser above
require 'safe_yaml' # or safe_yaml/load

Based on my tests, Syck doesn't have the same vulnerability as Psych w/ libyaml < 0.1.6 (makes sense, as Syck doesn't use libyaml).

[parkr]

@dtao You rock. Thanks for doing this!

[wlipa]

On the current Ubuntu LTS release, this change results in a loud, incorrect warning, because the bug fix was backported to the 0.1.4 version of the library.

See http://people.canonical.com/~ubuntu-security/cve/2014/CVE-2014-2525.html

[dtao]

@wlipa In this particular case I'm a bit at a loss, as I don't know a good way to test if the current libyaml installation is safe other than testing the version. I'm sure the Ubuntu devs had their reasons for backporting w/o updating the version number; but it puts SafeYAML in an awkward situation. Aside from actually trying to run an exploit (which will crash the program and potentially corrupt memory), there's no way to know if the local libyaml installation is safe.

The goal of SafeYAML is to protect you from YAML-related vulnerabilities. I'd rather it be too cautious than not cautious enough. Of course, an inaccurate warning is not helpful. So I'm not sure what the right course of action is.

At the moment I'm thinking the least of all evils would be:

1. Update the warning message to explain that on Ubuntu, the libyaml package was patched at 0.1.4.
2. Give instructions to suppress the warning, if the user is sure their system is patched (or they're sure there is no vulnerability in their application—e.g. they're not parsing arbitrary user-supplied YAML anywhere).

Re: the first item, it seems like it would be helpful to provide a simple test case for the user to run. Something like, "Try running this script; if it doesn't crash, then your system is safe." However I don't actually know if that's a good idea. Is it already widely known how to exploit this vulnerability (i.e. has it been written about somewhere)? It's not really complicated; but all the same I'd feel more comfortable providing a test script knowing that I'm not putting potentially harmful new information out there.

[konklone]

Is there any way to determine if it's the backport-fixed version of libyaml 1.4? I'm not sure how Ubuntu versioning works - surely there's a patchlevel or some equivalent?

[wlipa]

If Ubuntu and the Version returned from dpkg -s libyaml-0-2 >= 0.1.4-2 then it's OK.

[dtao]

Maybe I'll do that. It still seems there should just be a suppress option, though. Because honestly this seems like it could lead down quite a whack-a-mole path, where different parties have patched the vulnerability in their own ways (e.g., some cowboy dev could have even compiled libyaml himself from modified source on his company's server) and SafeYAML can't be aware of all the ways it's patched.

Ubuntu is a pretty legitimate special case, though, as a very widely-used OS. But the code to test this is going to be hacky—something like:

def is_patched_ubuntu?
  return false if (`which dpkg` rescue '').empty?
  libyaml_version = `dpkg -s libyaml-0-2`.match(/^Version: (.*)$/)
  return false if libyaml_version.nil?
  return libyaml_version[1] >= '0.1.4-2'
end

Ugh 😫

[wlipa]

Better that than an incorrect warning, IMHO! I think you'd need to be smarter about the version check than lexical string comparison though.

[konklone]

@wlipa could you provide any guidance on how to be smarter about the version check?

[wlipa]

What I had in mind was something like:

# Treat ubuntu patch level as extra-teeny
gv = Gem::Version.new(libyaml_version[1].gsub(/ubuntu.*/, '').gsub('-', '.'))
# 0.1.4 with backport, or safe later version
gv == Gem::Version.new('0.1.4.2') || gv >= Gem::Version.new('0.1.6')

[wlipa]

To handle future patches, the last line would be better as:

Gem::Requirement.new('~> 0.1.4.2') === gv || gv >= Gem::Version.new('0.1.6')

[fgi]

Hi,
Ubuntu is a Debian distribution and many Debian boxes run yaml.
I'm afraid that testing >= '0.1.4-2' would not be enough.
Here are versions that could be returned by libyaml_version[1]
Debian 6 (squeeze), vulnerable: "0.1.3-1"
Debian 6 (squeeze), fixed: "0.1.3-1+deb6u4"
Debian 7 (whezzy), vulnerable: "0.1.4-2+deb7u2"
Debian 7(whezzy), fixed: "0.1.4-2+deb7u4"
Debian 8 (jessie, unstable): "0.1.4-3.2"
You can check vulnerable and fixed libyaml versions for Debian here: https://security-tracker.debian.org/tracker/CVE-2014-2525

I don't have better to suggest than this hugly test:
(libyaml_version[1].include?("deb6") && libyaml_version[1] >= "0.1.3-1+deb6u4") || (libyaml_version[1].include?("deb7") && libyaml_version[1] >= "0.1.4-2+deb7u4") || libyaml_version[1] >= "0.1.4-3.2"

Thanks.

[wlipa]

Yeah, I can see that this quickly gets untenable. Does safe_yaml want to be responsible for knowing this much about the versions of libyaml out there in the world? And yet, a loud, incorrect warning could easily be worse than no warning at all, because it teaches us to ignore warnings.

Personally I think the warning should be removed, since it's so hard to make it correct. It should not be safe_yaml's responsibility to warn about bugs in underlying libraries. Safe_yaml should concern itself with how yaml is used in ruby.

[dtao]

Gah! I don't know.

I have conflicting feelings about this whole notion of "responsibility". When the YAML vulnerability in Rails from 2013 was first disclosed, there was this whole huge thread over at Psych about offering a safe_load method (which now exists, in Psych >= 2.x I believe), and an attitude that got expressed from many parties was: "This isn't Psych's responsibility."

Well fine, but then what? We just leave these landmines lying around and let users get blown up? Part of me agrees that maybe SafeYAML is biting off more than it can chew by even trying to deal with this vulnerability at all. But another part of me feels like I'd rather at least give it a try—at least bring it to the users's attention. Because if SafeYAML doesn't, who will? Are all users just supposed to organically discover all CVEs that affect their code?

My inclination is this. Keep the warning, but add checks for well-known cases where the vulnerability is patched, like the Ubuntu and Debian versions that have been listed. In the warning, provide clear instructions:

• A script to test if the current system is vulnerable
• Guidance on whether the vulnerability is an issue (i.e., are you just using jekyll to create a blog?)
• How to suppress the warning in the future (and make this very easy)

I realize this is a bit of a slippery slope. Will SafeYAML always do this? Is the warning going to be way too huge? (I'll have to do my best to make it more succinct.) You also raise a good point about the danger of training users to ignore warnings.

So, to reiterate: I don't know. But that's what I'm thinking at the moment.

[konklone]

Proposal!

If libyaml <= 1.6 on any OS, warn: "We've detected that your system may be vulnerable to a libyaml bug. Run is_libyaml_safe to test if your system is vulnerable and remove this warning."

Running is_libyaml_safe: "Your system is not vulnerable to libyaml bugs. You won't receive any more warnings as this user." and this writes to ~/.safeyaml so that no more warnings are raised for that user.

Or if it is vulnerable: "Your system is vulnerable to a major libyaml bug! Update your version of libyaml, and run this test again."

[wlipa]

If you know how to write is_libyaml_safe, then why not use that check in the first place and avoid having a "rabbit dropping" file that requires manual work once per-user, per-instance?

[dtao]

@wlipa Because if libyaml is not safe then the check will crash the process.

[wlipa]

So far I've seen about 100 of these incorrect warnings go by (I see two of them for every deployment, plus every time I run the console, etc.) Any progress on a disablement?

[mkristian]

I have to add my 2 cents to it:
please just warn if you are are sure if the system is vulnerable everything
else seems to be a long running annoyance and the usefulness of such
"warnings" just vanishes.

[dtao]

Yeah, I hear you guys.

Having thought about this awhile (arguably too long), here's my plan:

1. Remove the warning entirely. Honestly I think it's served its
   purpose by now (and then some). I'm sure there is a long tail of users who
   still aren't aware of the issue, but by now it's clear the harm may have
   outweighed the good.
2. Add a project wiki and explain the libyaml issue there, so there's at
   least a place for users to read about and understand it.
3. To avoid vulnerabilities like this in the future, I'm now working on a
   pure-Ruby, Psych-compatible YAML parser. Obviously this is a pretty big
   undertaking so it will probably take a while.

Anyway, 1.0.3 will be out today with the warning gone.
On Apr 22, 2014 1:10 AM, "Christian Meier" [email protected] wrote:

I have to add my 2 cents to it:
please just warn if you are are sure if the system is vulnerable everything
else seems to be a long running annoyance and the usefulness of such
"warnings" just vanishes.

Reply to this email directly or view it on GitHubhttps://github.com//issues/56#issuecomment-41014072
.

[parkr]

@dtao Your solution sounds perfect, thank you for all your hard work!

[dtao]

1.0.3 is out, and the warning is gone.

I've added a CLI that lets you run a check to see if your version of libyaml is affected:

safe_yaml --libyaml-check

I've also added a wiki page explaining what you can do if your system is vulnerable.

I imagine most people are actually OK at this point. But at least the info is there, for whose who are interested.

[wlipa]

Thank you!

[nickrivadeneira]

👍

[konklone]

Thanks for working through all this, @dtao!