Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bandit, use to catch known vulnerable XML parsing #28341

Merged
merged 3 commits into from Nov 18, 2019

Conversation

@scop
Copy link
Contributor

scop commented Oct 30, 2019

Description:

https://bandit.readthedocs.io/

Bandit has quite a few useful security related checks. This introduces it, and configures to use known vulnerable XML parsing use only for now. We can add more checks in the future.

Note that in its current state the build will expectedly fail, this is in order to demonstrate the found issues.

Related issue (if applicable): #28302 (vulnerable etree usage sidetrack)

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@scop scop mentioned this pull request Oct 30, 2019
3 of 9 tasks complete
@MartinHjelmare MartinHjelmare added this to Needs review in Dev Oct 30, 2019
@frenck

This comment has been minimized.

Copy link
Member

frenck commented Nov 3, 2019

Bandit is way more useful than just checking for XML vurns. IMHO, this could use a broader implementation. (For example, B101, which blocks the use of assert, to prevent surprises when using optimized bytecode. We do have those cases in our code-base.)

Considering your goal here, you might want to consider adding the range B405 - B411, as well.

@scop

This comment has been minimized.

Copy link
Contributor Author

scop commented Nov 3, 2019

I know it is way more useful, but I want to limit the introduction to something that should be entirely uncontroversial in order to just get it in. We can talk about enabling more stuff later, and your comment kind of proves that this approach is probably a good one :)

You mentioned the use of assert: if we want to ban it, there's some work ahead with getting mypy checks to pass as we have a bunch of places where something is Optional[Foo] but we "know" the None part of that won't happen but mypy doesn't and thus will give errors. assert foo is not None is one, kind of good if you ask me, way to let mypy know what's up too for now until we avoid those Optional in some cleaner way.

@scop

This comment has been minimized.

Copy link
Contributor Author

scop commented Nov 3, 2019

I'm going to fix the flagged issues to get CI green now, but here they are for reference:

/__w/1/s/tests/components/rss_feed_template/test_init.py:50: B314[bandit]: MEDIUM: Using xml.etree.ElementTree.fromstring to parse untrusted XML data is known to be vulnerable to XML attacks. Replace xml.etree.ElementTree.fromstring with its defusedxml equivalent function or make sure defusedxml.defuse_stdlib() is called
/__w/1/s/homeassistant/components/ssdp/__init__.py:152: B314[bandit]: MEDIUM: Using xml.etree.ElementTree.fromstring to parse untrusted XML data is known to be vulnerable to XML attacks. Replace xml.etree.ElementTree.fromstring with its defusedxml equivalent function or make sure defusedxml.defuse_stdlib() is called
/__w/1/s/tests/components/emulated_hue/test_upnp.py:64: B314[bandit]: MEDIUM: Using xml.etree.ElementTree.fromstring to parse untrusted XML data is known to be vulnerable to XML attacks. Replace xml.etree.ElementTree.fromstring with its defusedxml equivalent function or make sure defusedxml.defuse_stdlib() is called
@scop

This comment has been minimized.

Copy link
Contributor Author

scop commented Nov 3, 2019

Also, related to B405-B411, at least some of those are not good advice. For example, defusedxml.ElementTree cannot be used to generate XML, and it's ok to use plain xml.etree.ElementTree for that, so IMO it doesn't make sense to flat out forbid xml.etree.ElementTree imports, and even offer advice that will break the code, for example:

$ cat /tmp/t.py 
from xml.etree import ElementTree as ET
a = ET.Element("a")
ET.dump(a)
$ python /tmp/t.py
<a />
$ bandit --format custom /tmp/t.py 2>&1 | tail -n 1
/tmp/t.py:1: B405[bandit]: LOW: Using ElementTree to parse untrusted XML data is known to be vulnerable to XML attacks. Replace ElementTree with the equivalent defusedxml package, or make sure defusedxml.defuse_stdlib() is called.
$ sed -i -e s/xml\.etree/defusedxml/ /tmp/t.py
$ python /tmp/t.py
Traceback (most recent call last):
  File "/tmp/t.py", line 2, in <module>
    a = ET.Element("a")
AttributeError: module 'defusedxml.ElementTree' has no attribute 'Element'

It's the parsing that needs to be avoided, and that's handled by the now enabled B313-

@scop

This comment has been minimized.

Copy link
Contributor Author

scop commented Nov 3, 2019

CI failure seems unrelated. I would try to re-run the tests, but either I get asked for a password which I don't think I have, or I click the various re-run buttons at https://github.com/home-assistant/home-assistant/pull/28341/checks?check_run_id=286683001 and get a message that I have requested stuff from azure pipelines and that checks are being re-run, but nothing suggests to me that they are actually being re-run.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Nov 4, 2019

If it's an unrelated test failure, we can ignore that here. We've fixed some flaky tests recently on dev branch so hopefully it's already fixed.

Regarding this change, I'm in favor of small steps.

@scop scop force-pushed the scop:bandit branch from ed3cc06 to 33a6bb7 Nov 9, 2019
@scop

This comment has been minimized.

Copy link
Contributor Author

scop commented Nov 9, 2019

Rebased against current dev, moved test configs to tests/bandit.yaml

Copy link
Member

MartinHjelmare left a comment

Looks good!

A core member should approve too as we're extending CI.

Dev automation moved this from Needs review to Reviewer approved Nov 9, 2019
@scop scop force-pushed the scop:bandit branch from 33a6bb7 to 70f1712 Nov 16, 2019
@frenck frenck requested a review from home-assistant/core Nov 17, 2019
@pvizeli pvizeli merged commit d4c80f1 into home-assistant:dev Nov 18, 2019
11 checks passed
11 checks passed
CI Build #20191116.35 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing d6e99db...70f1712
Details
codecov/project 94.44% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Nov 18, 2019
@scop scop deleted the scop:bandit branch Nov 18, 2019
@lock lock bot locked and limited conversation to collaborators Nov 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
5 participants
You can’t perform that action at this time.