Safe load 'included' yaml files

[FabioRosado]

Description

This is the first baby-step to replace yaml.load with yaml.safe_load. Currently, I have changed how the command !include loads a new file into the configuration - mostly because this is the first bit where people could exploit the vulnerabilities of yaml.load.

When I tried to replace load with safe_load on lines loader:229-232 the texts failed as everything was returning None, so I will need to do some further testing to see why safe_load is not loading the file properly.

Another thing I need to do will be testing opsdroid with an included file and see if it works without any issue.

Fixes #580

Status

READY | UNDER DEVELOPMENT | ON HOLD

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

• Tox - All Green
• Travis - All Green
• !Include doesn't load python code related to the vulnerability
• Tested with an !include file - returns NoneType (needs further investigation)

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[codecov]

Codecov Report

Merging #589 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #589   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          20     20           
  Lines        1369   1369           
=====================================
  Hits         1369   1369

Impacted Files	Coverage Δ
opsdroid/loader.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0536cc7...04198a5. Read the comment docs.

[jacobtomlinson]

Thanks for starting on this. It looks like this test already exists which is reassuring.

Feel free to merge this and raise a new PR with your next changes or continue working here.

It might be interesting to see if we can add a test which fails if the exploit can be abused, then #580 can be closed when it passes.

[FabioRosado]

I'm wondering why safe_load doesn't work when we run the yaml validator method, so I'll need to investigate further to try and figure out why this happens.

As for your suggestion on creating a test to see if the exploit can be abused, sounds like a good thing to have so I'll try to write something like that, thanks for the suggestion 👍

--EDIT--
So I have been playing around with the exploit to try and understand how to use it in a test, I tried to load the following code on a yaml file !!python/object/apply:os.system ["echo 'Oops!';"] this should make stdout echo "Oops" but when I load this file as config, the config turns into a normal configuration file, see below:

    def test_yaml_load_exploit(self):
        opsdroid, loader = self.setup()
        config = loader.load_config_file("tests/configs/exploit.yaml")
        self.assertIsNotNone(config)
>       self.assertFalse(config)
E       AssertionError: {'welcome-message': True, 'connectors': [{'name': 'shell', 'bot-name': 'mybot'}, {'name': 'websocket', 'bot-name': 'mybot', 'max-connections': 10, 'connection-timeout': 10}], 'skills': [{'name': 'dance'}, {'name': 'hello'}, {'name': 'loudnoises'}, {'name': 'seen'}]} is not false

The issue is when we use !include exploit.yaml from a yaml file. Then the code will run and the message is echoed on stdout then returns 0.

For the sake of consistency I tried to import the code straight from the loader ( config = loader.load_config_file(!!python/object/apply:os.system ["echo \'Oops!\';])) just to see if the command would run when opsdroid loads the config. I have got the same result as when exploit.yaml is used as the config file.

I assume that opsdroid will just use the config example file if the file used for the configuration file doesn't have a specific name (I believe this is the case if memory doesn't fail me).

Finally, I am going to suppose that the biggest issue to opsdroid is indeed the !include command. By using safe_load the code simply isn't run and it returns None (instead of 0).

[FabioRosado]

I'm not sure if you saw my second edit in which I mentioned a NoneType, but I figured out the issue. It was because I forgot to delete one empty line (doh!)

I have tested on my end to include another yaml file to the skill-chat and see if both safe_load and load work the same way. Both of them worked as expected so I'm happy with it.

@jacobtomlinson I would just like your opinion about the other yaml.load (loader:232) since safe_load seems to break the envvar and include constructors. Also, I would like to know if you are happy with my tests or if I should try something else that I haven't thought of. 😄 👍

[jacobtomlinson]

These changes look good.

I'm not entirely sure what to do about the other safe load. I would recommend merging this now and then investigating it further in a new PR.