review: security patch to prevent code injection

[sadielbartholomew]

I have noticed a security flaw in the Cylc Review service, originating from its Rose Bush origin, which I raised & which has been discussed offline. Naturally, I will not provide details for recreation here (publicly) but I will email details across.

For general context it concerns a specific route for code injection which I demonstrated can execute malicious JavaScript to e.g. redirect the entire service upon viewing of any listing including one exploitative suite.

This PR:

Patch this known vulnerability:

• autoescape HTML & XML markup in the Jinja2 templates;
• use the Jinja2 safe filter to:
  • (now on a case-by-case basis, as required) unescape ampersands & to & to ensure URLs containing queries are still valid;
  • ensure the template formatting is otherwise unaffected, e.g. > still displays as > in textual arrows.

NB: the 'tags' file viewing tab will (as observed) still be processed to provide styling etc. after this change.

For reviewing:

See email for further info if required.

[kinow]

Related cylc/cylc-admin#11

[oliver-sanders]

This breaks the urilse function :(

[sadielbartholomew]

Well spotted. I'll have another look to work out how to make these lines safe without changing (breaking) the functionality.

[sadielbartholomew]

(I am not sure if anything can be done about the code coverage change, but I will have a look over the report as it will likely reveal something useful).

[sadielbartholomew]

PR updated & ready for re-review, pending CI passes.

Note: I haven't added relevant tests, i.e. for the handling of markup inputs, because it would not be simple to test whether various markup gets rendered. Plus the main test case would need to be for Rose inputs (e.g. see #2614 (comment)), so such tests would belong in the Rose codebase. Therefore unless anyone contests, I will not be incorporating tests in this PR.

[sadielbartholomew]

Codacy has outdone itself here! The one new issue flagged is that:

"using jinja2 templates with autoescape=False is dangerous and can lead to XSS. Ensure autoescape=True or use the select_autoescape function to mitigate XSS vulnerabilities."

Enabling autoescaping to prevent XSS and related attacks is exactly what this PR is doing 😑.

[kinow]

Hi @sadielbartholomew !

I lost some hair fighting Codacy issues some time ago. They are really annoyingly obscure in their web interface.

However, I learned later that Codacy is actually just a façade (love that I can use cedilla in English too!). So you can test why Codacy complains about security issues locally with the following steps:

$ cd cylc
$ pip install --user bandit
$ bandit lib/cylc/review.py

Which should report exactly the same error we are seeing on Codacy, but with bandit, which has a nice and clear output.

This issue was reported to bandit (which was later migrated to github) and fixed in bandit apparently in 2017.

Then, if you read the comments and the patch (commit 8f1b50b5cce2ea241dbee334c5f58234b8656849), you will see that they fixed it by checking if it is doing autoescape=True or autoescape=select_autoescape.

I believe the jinja2.select_autoescape is triggering the issue... a bit pedantic in my opinion. But as they are working with the abstract syntax tree, and matching word by word... I guess the check is doing what it promised.

To test that, I checked out your branch locally, then used a from jinja2 import select_autoescape, and changed to autoescape=select_autoescape(...). And voilà (I think I can use the grave accent too some times in English!), bandit/Codacy won't complain about that any longer.

Bug reported against bandit: PyCQA/bandit#453. And pull request as well: PyCQA/bandit#454. Hopefully that will be fixed later in Bandit and/or Codacy.

Anyway, agree there's something weird with Codacy :-) 👍 great work! 🎉

Cheers
Bruno

[kinow]

And regarding the test coverage @sadielbartholomew , I have to submit a pull request to fix that in Cylc after some discussion (not sure whether in Riot/next VC/etc).

I defined the numbers for coverage threshold in #2766 , but that is a bit low now, and I did not know what was the patch coverage exactly before.

We have two coverages in the checks in this pull request: codecov/patch and codecov/project. The project refers to the overall coverage in the project, which is 84.05 now, and we could set a threshold for something like 80 to make sure we never go below that, for example.

Now, the patch coverage is set to target 58%. This number was defined by me (so mea culpa), without much thinking. And now I know that this means "how much of the changes in this pull request are being covered by tests".

The issue is that the Cylc Review code includes a lot of HTML, JS, and even Python code that is not tested (whether necessary or not is another matter IMO, not important now). With the current threshold of 58%, it means that unless you cover at least 58% of the changes you pushed, Codecov will fail that check.

Hope that makes sense. I will try to remember to ask someone from @cylc/core to think about better thresholds for coverage. And 👍 for not bothering about this issue in this pull request for now. I think it would be really hard to test those changes in Jinja2. Just wanted to explain why the error 😁

Cheers
Bruno

[sadielbartholomew]

Wow, @kinow, excellent investigatory work! I was happy to just accept it was a facet of Codacy as a tool that 'tries its best' but given interpretative difficultly is often erroneous, but to have a detailed description of the context is really useful and interesting. Thank you so much for going out of your way to look into this & even file an external bug report 🌟.

you can test why Codacy complains about security issues locally

I've made a note to myself to have a try of this in future dev work, for general issues, to gain background knowledge.

Thank you also for clarifying on the code coverage. It seems to me to be really tricky to decide on one appropriate patch target percentage given the different contexts of files and the classes and methods etc within.

Best to discuss this elsewhere as part of a discussion on the coverage, but I am thinking it might be good to at least set a different patch target percentage (if that is possible) for non-Python files, or something similar to try to account somewhat for the fact certain logic is clearly less important to test to a certain level by line count than other logic. Reasonable, practicable targets are a challenge, for sure!

[sadielbartholomew]

PR updated & ready for re-review

This is still true, by the way, in case that wasn't clear after comments which concern Codacy & justify its failing.

[hjoliver]

This looks fine to me, but we should rely on experts on the former Rose Bush code for sanity checking.