Using YAML.load for the FileDataSource

[julik]

Describe the bug

FileDataSource uses YAML.load to load the feature flags file. YAML is quite elaborate, and it can demarshal arbitrary Ruby objects. If the data source is used in production/development, and the feature flag store is compromised, demarshaling of arbitrary objects can be achieved.

To reproduce

Take a look at https://gist.github.com/niklasb/df9dba3097df536820888aeb4de3284 and similar YAML-vector based exploits. Then a YAML file can be crafted that causes arbitrary Ruby objects to get initialised during feature flag loading from the FileDataSource.

Expected behavior
Since JSON is already used in the library source I would also expect it to be used for loading the stored flag data.

SDK version
master from this GH repository.

Language version, developer tools
For instance, Go 1.11 or Ruby 2.5.3. If you are using a language that requires a separate compiler, such as C, please include the name and version of the compiler too.

OS/platform
MRI any

Additional context
Using a richer marshaling format than strictly necessary might not be the best idea.

[eli-darkly]

Thanks for pointing this out. Rather than disabling the YAML capability completely, what if we just used YAML.safe_load() instead, which disallows all of the complex features that cause the vulnerability? It is supported in all of the Ruby versions that we support, and it should be possible to unit-test the vulnerability.

[eli-darkly]

By the way, that Gist URL is broken, but I'm assuming you mean this kind of thing.

[julik]

That could be an option. I would actually rename the method to something like load_unsafe_yaml to make the tradeoff more obvious, and change the existing one to JSON. I also don't know how safe the safe_load is in practice

[eli-darkly]

I would actually rename the method to something like load_unsafe_yaml to make the tradeoff more obvious

I don't know which method you mean. When you're using FileDataSource, you don't call any method to load an individual file, you just specify a bunch of file paths.

I also don't know how safe the safe_load is in practice

Since it was added specifically because of this vulnerability, and it explicitly disallows all data types other than primitives and normal hashes and arrays, I can't think why it would not be safe. All of the following articles describe safe_load as an effective workaround. The only articles I've found that do not mention it are from before it was added in Ruby 2.0.0.

http://www.benjaminfleischer.com/2013/03/20/yaml-and-security-in-ruby/
https://justi.cz/security/2017/10/07/rubygems-org-rce.html
https://arp242.net/yaml-config.html

I reproduced one version of the attack using a file like this:

--- !ruby/hash:BadClassWeShouldNotInstantiate

Without the fix, this does indeed create an instance of that class. With the fix, YAML.safe_load refuses to parse the file. This is easily reproducible in a unit test. I'm sure there are many variations on the attack, but they all depend on !ruby directives which are disallowed by safe_load.

[eli-darkly]

I should also mention that FileDataSource is not intended for production use, but rather as a testing tool. There isn't really a use case for it in production, since if you deployed an app that doesn't actually connect to LaunchDarkly at all to get feature flags, then there wouldn't be much reason to use LaunchDarkly in the first place. We're updating our docs to emphasize this point.

[eli-darkly]

Well, while there may still be further investigation needed into whether safe_load is a rock-solid solution, it's certainly better than what we have now, so we've released v5.5.10 with this fix. Developers are much more likely to upgrade to a patch release than a major release, so that at least greatly mitigates the problem and then if we have to entirely remove this functionality in the next major release, we will. So we'll continue looking into it.

[julik]

👍 and thanks for the quick fix.

I should also mention that FileDataSource is not intended for production use, but rather as a testing tool. There isn't really a use case for it in production, since if you deployed an app that doesn't actually connect to LaunchDarkly at all to get feature flags, then there wouldn't be much reason to use LaunchDarkly in the first place.

I understand. However, it is plausible to assume that even if something is not intended for production use someone, somewhere, would use it in this manner anyway since it is available functionality. Also, there is a use case for it: if you want to use a "frozen" version of the feature flags - for example with short-lived immutable deploys which do not connect to the Internet to retrieve the feature flags - you might want to use this storage model as a file you inject "at build time". And that's where, if this file is maliciously crafted, it could bite.

But once more: thanks for addressing it so expediently ❤️

[eli-darkly]

Actually, I'm sorry, the 5.5.10 release was a mistake and did not contain this fix - it is now in 5.5.11.