donfig.config_obj.collect_yaml method is vulnerable

[bigbigliang-malwarebenchmark]

#coding=utf-8
import donfig.config_obj as test
test.collect_yaml('C:\simple.yaml')
#'C:\Users\Administrator\Desktop\simple.yaml':!!python/object/apply:os.system ["calc.exe"]

Hi, there is a vulnerability in collect_yaml(paths) method in config_obj.py,, please see PoC above. It can execute arbitrary python commands resulting in command execution.

[djhoese]

@bigbigliang-malwarebenchmark I'm guessing you are pointing out the unsafe reading of yaml files. The configs loaded by donfig are provided by the user or by the package they are using. Ultimately it is up to the user and package developers to decide what goes in these YAML files. If that happens to be loadable/executable code then so be it.

@mrocklin @jcrist @jhamman Thoughts?

[bigbigliang-malwarebenchmark]

@ bigbigliang-malwarebenchmark我猜你在指出yaml文件的不安全读取。donfig加载的配置由用户或他们使用的包提供。最终，由用户和软件包开发人员决定这些YAML文件中的内容。如果这恰好是可加载/可执行代码那么就是这样。

@mrocklin @jcrist @jhamman想法？

Yes, very clear, then why don't you change the method?

[bigbigliang-malwarebenchmark]

@ bigbigliang-malwarebenchmark我猜你在指出yaml文件的不安全读取。donfig加载的配置由用户或他们使用的包提供。最终，由用户和软件包开发人员决定这些YAML文件中的内容。如果这恰好是可加载/可执行代码那么就是这样。
@mrocklin @jcrist @jhamman想法？

Yes, very clear, then why don't you change the method?

yaml.safe.load might be better.

[djhoese]

I'm saying that due to the generic use cases that donfig may be used for, users may want to load in an unsafe way (to load python classes/functions for example). Since the configs being read are either provided by the library they are using or by the user themselves, there is theoretically no way for unwanted things to occur from reading the YAML file. No more than running a random software projects code on your system. You, the user, should understand what you are downloading from the internet (don't run random scripts unless you know what they are doing, don't use configs that you don't know what they are doing). We could replace the load with a safe load, but I also don't see a reason to limit the user right now. This is just my opinion and I don't feel super strong about it. We could give the option to load unsafe but default to safe loading.

Lastly, this project is based on the configuration system used by dask (https://github.com/dask/dask) so it won't be changed until the dask devs (mentioned above) give their opinion. My eventual goal is to replace dask's internal config code with this project.