Sidekiq class methods dont work with safe_yaml

Question

Sidekiq class methods dont work with safe_yaml

snmaynard opened this issue 11 years ago · 11 comments

When using sidekiq and safe_yaml, the class methods i.e Class.delay.method dont work.

Here is the relevant line in sidekiq.

Answer 1 · 2014-01-23T18:24:03.000Z

Did you try setting:

SafeYAML::OPTIONS[:deserialize_symbols] = true

As indicated in the Known Issues section?

Answer 2 · 2014-01-23T18:27:13.000Z

Yeah, that works for most things, but not this.

It actually serializes entire objects and even the target class. So you get an error message like undefined method 'method' for "Class":String when calling Class.delay.method.

Answer 3 · 2014-01-23T18:40:23.000Z

Oh right, I vaguely remember that now (delayed_job does the same thing as I recall).

Honestly, this one is a bit tricky. What this means is that sidekiq relies on being able to deserialize arbitrary objects to work. Which is exactly the security vulnerability that SafeYAML is meant to avoid.

I see a few possible solutions here, none of them particularly attractive...

Submit a PR to Sidekiq to do something like YAML.load(yml, :safe => false) if SafeYAML is loaded (seems a bit far fetched as I doubt they'd want that in their code).
In the application using Sidekiq, set SafeYAML::OPTIONS[:default_mode] = :unsafe. This should get Sidekiq working. Then manually figure out where you might be deserializing user-supplied YAML data and only there manually switch from YAML.load to SafeYAML.load.
Figure out all the classes you're using delay with and whitelist these using SafeYAML.whitelist!.

The problem w/ no. 1 is more of a philosophical one. I can even try submitting it myself; I just think if I were the author of sidekiq, I'd probably resist it.

The problem w/ no. 2 is pretty significant. One of the whole points of patching YAML.load is so that you secure yourself along boundaries you don't even control (i.e., your library dependencies). So changing the default mode opens up potential vulnerabilities you might not even know about.

The problem w/ no. 3 is simply that it's tedious.

Of these three approaches, I'm honestly not sure I'd go for any of them. Well, I'll try no. 1 at least. But it's also possible I'm not thinking of a more reasonable/better solution (it's happened before).

Answer 4 · 2014-01-23T19:02:01.000Z

Yeah, I'm unsure of the best way forward myself. The issue took a while to diagnose as safe_yaml was actually a dependency of crack and so we hadn't even installed it ourselves, took a while to figure it out!

Perhaps @mperham can offer his opinion? We could detect the presence of SafeYAML in sidekiq and pass the safe option in only if its needed? I've gone for option 2 for now, just to get it working.

In the sort term we could potentially add a note to the readme of safe_yaml explicitly mentioning the issues with calling Class.delay.method so others dont have to spend so long diagnosing the problem.

Answer 5 · 2014-01-23T19:04:47.000Z

I definitely agree with your last remark (about adding this to the README).

I also relatively recently added a way for libraries to depend on SafeYAML
in a way that doesn't patch YAML globally (so the library can be safe
without having such a ripple effect on consuming applications). It would
probably be worth reaching out to crack to get them to adopt this feature.

I'll have an update on this later today.

On Thu, Jan 23, 2014 at 11:02 AM, Simon Maynard notifications@github.comwrote:

Yeah, I'm unsure of the best way forward myself. The issue took a while to
diagnose as safe_yaml was actually a dependency of crack and so we hadn't
even installed it ourselves, took a while to figure it out!

Perhaps @mperham https://github.com/mperham can offer his opinion? We
could detect the presence of SafeYAML in sidekiq and pass the safe option
in only if its needed? I've gone for option 2 for now, just to get it
working.

In the sort term we could potentially add a note to the readme of
safe_yaml explicitly mentioning the issues with calling Class.delay.methodso others dont have to spend so long diagnosing the problem.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/53#issuecomment-33156425
.

Answer 6 · 2014-01-23T22:08:35.000Z

OK, now I'm really confused. How did you trace the issue back to crack? I ask because I just looked at that project, and it looks to me like it was never actually requiring safe_yaml anywhere (only in the gemspec, which doesn't actually load the gem).

I've actually submitted a pull request to GET crack to use safe_yaml; but for now, I'm scratching my head over here.

Answer 7 · 2014-01-24T00:18:21.000Z

I think the confusion is my fault, we added Jekyll and that requires safe_yaml, and I saw it was required by crack in our Gemfile.lock and so assumed that the issue was caused by crack. It was actually caused by jekyll.

Apologies for that.

Answer 8 · 2014-01-24T00:30:20.000Z

No problem, that helps clear that up.

The more I think about it, I'm starting to feel that the best way to handle this is actually to just whitelist the types that you're serializing with Sidekiq when calling delay. How many classes are you doing this with in your code? Not all that many, I would assume?

If you have time, try calling SafeYAML.whitelist! on the types you want to allow before starting up Sidekiq; and let me know how that works out.

Answer 9 · 2014-01-31T21:20:04.000Z

OK, I think I'll close this now.

For projects that require 'safe_yaml' but also need to use Sidekiq, I think the right thing to do is to whitelist the classes you need (for calling delay).
More importantly (re: this specific issue), the Jekyll project has merged my PR to require 'safe_yaml/load', which does not patch the global YAML module. So Jekyll should no longer break Sidekiq.

Let me know if you have any lingering concerns.

Answer 10 · 2014-01-31T21:21:32.000Z

@dtao Thank you for taking care of this and submitting a PR to Jekyll!

Answer 11 · 2014-01-31T21:30:02.000Z

That seems more than reasonable, I think we found a good solution!