chore: Handle all level 1 TiCS security violations

[TheRealFalcon]

Proposed Commit Message

chore: Handle all level 1 TiCS security violations

Ignore these bandit violations as we're not dealing with untrusted
input. Violations ignored in this commit are:
 B314: xml_bad_ElementTree
 B318: xml_bad_mindom
 B405: import_xml_etree
 B406: import_xml_sax
 B408: import_xml_minidom
 B603: subprocess_without_shell_equals_true

Additional Context

This PR addresses all of the level 1 warnings at: https://canonical.tiobe.com/tiobeweb/TICS/ViolationsDashboard.html#axes=Project(cloud-init)&sort=T(4,worst)&x=Ap(type),Ap(rule),Ap(level),Ap(impactAtProject),Ap(impactAtScope),Ap(message)

(Link is Canonical only)

I attempted to add a bandit config to ignore certain warnings, but TiCS won't pick it up. We have to go line by line.

As of right now, we're only concerned with Level 1 warnings. Running locally, I got this result:

SUMMARY
=======


Violations per rule
===================

 RULE ID                    | LEVEL |     # | DELTA | SYNOPSIS
----------------------------+-------+-------+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 BANDIT_PY_B103             |     2 |     1 |       | Setting bad file permissions.
 BANDIT_PY_B104             |     3 |    22 |    +1 | Binding to all network interfaces can potentially open up a service to traffic on unintended interfaces, that may not be properly documented or secured.
 BANDIT_PY_B105             |     2 |     4 |       | Possible hardcoded password in assigned variable or used in a comparison with a variable that looks like a password.
 BANDIT_PY_B107             |     2 |     1 |       | Possible hardcoded password.
 BANDIT_PY_B108             |     3 |     5 |       | Insecure usage of tmp file/directory.
 BANDIT_PY_B110             |     2 |    11 |       | Pass in the except block: Suppressing errors can be a potential security issue.
 BANDIT_PY_B112             |     2 |     1 |       | A continue in the except block: Suppressing errors can be a potential security issue.
 BANDIT_PY_B301             |     2 |     1 |       | Pickle and modules that wrap it can be unsafe when used to deserialize untrusted data.
 BANDIT_PY_B303             |     2 |     1 |       | Use of insecure MD2, MD4, MD5, or SHA1 hash function.
 BANDIT_PY_B311             |     5 |     3 |       | Standard pseudo-random generators are not suitable for security/cryptographic purposes.
 BANDIT_PY_B314             |     1 |     0 |    -4 | Using various XML methods (xml.etree.ElementTree.parse, xml.etree.ElementTree.iterparse, xml.etree.ElementTree.fromstring, xml.etree.ElementTree.XMLParser) to parse untrusted XML data is known to be vulnerable to XML attacks. Methods should be replaced with their defusedxml equivalents.
 BANDIT_PY_B318             |     1 |     0 |    -1 | Using various XML methods (xml.dom.minidom.parse, xml.dom.minidom.parseString) to parse untrusted XML data is known to be vulnerable to XML attacks. Methods should be replaced with their defusedxml equivalents.
 BANDIT_PY_B403             |     2 |     1 |       | Possible security implications associated with pickle modules (pickle, cPickle,dill, shelve).
 BANDIT_PY_B404             |     2 |     5 |    +1 | Consider possible security implications associated with subprocess modules.
 BANDIT_PY_B405             |     1 |     1 |    -1 | Using xml.sax to parse untrusted XML data is known to be vulnerable to XML attacks. Replace vulnerable imports with the equivalent defusedxml package, or make sure defusedxml.defuse_stdlib() is called.
 BANDIT_PY_B406             |     1 |     0 |    -1 | Using xml.sax to parse untrusted XML data is known to be vulnerable to XML attacks. Replace vulnerable imports with the equivalent defusedxml package, or make sure defusedxml.defuse_stdlib() is called.
 BANDIT_PY_B408             |     1 |     0 |    -1 | Using xml.dom.minidom to parse untrusted XML data is known to be vulnerable to XML attacks. Replace vulnerable imports with the equivalent defusedxml package, or make sure defusedxml.defuse_stdlib() is called.
 BANDIT_PY_B506             |     2 |     2 |       | Unsafe usage of the yaml.load function from the PyYAML package.
 BANDIT_PY_B602             |     5 |     4 |       | Subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell.
 BANDIT_PY_B603             |     1 |     0 |    -2 | Subprocess call - check for execution of untrusted input.
 BANDIT_PY_B604             |     2 |     6 |       | Method calls for the presence of a keyword parameter shell equalling true.
 COV_PY_RISKY_CRYPTO_PYTHON |     4 |     1 |       | A violation of user-specified RISKY_CRYPTO policy was detected.
----------------------------+-------+-------+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 TOTAL                      |       |    70 |    -8 |

Since this is all purely ignores, I see no reason to add the ruff-equivalent checks to CI yet. When we get to addressing real issues, those checks can be added then.

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[holmanb]

nit: could we please add some context about these suppressions to inline comments? A comment on the preceding line describing which Bandit code this suppresses and (optionally) why would go a long ways.

also, just a minor nitpick but I think that intermixing the URIs with the codes in the message makes the message a bit dense to parse - a list of links at the end with [N] refs would be easier to read

chore: Handle all level 1 TiCS security violations

Ignore these bandit violations as we're not dealing with untrusted
input. Violations ignored in this commit are:
* B314: xml_bad_ElementTree
  B318: xml_bad_mindom
https://bandit.readthedocs.io/en/latest/blacklists/blacklist_calls.html#b313-b320-xml
* B405: import_xml_etree
https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b405-import-xml-etree
* B406: import_xml_sax
https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b406-import-xml-sax
* B408: import_xml_minidom
https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b408-import-xml-minidom
* B603: subprocess_without_shell_equals_true
https://bandit.readthedocs.io/en/latest/plugins/b603_subprocess_without_shell_equals_true.html#b603-subprocess-without-shell-equals-tru

[holmanb]

+1, B603 is a pretty silly check - see PyCQA/bandit#333

[TheRealFalcon]

Yep...there's also a separate check for shell=True. So no shell gives you one error while having shell gives you another.

[holmanb]

So no shell gives you one error while having shell gives you another.

/facepalm

[holmanb]

This one is also to suppress B603, right?

Also I'm a little surprised that subp isn't used here.

[TheRealFalcon]

@holmanb , I updated all of the # nosecs to include the violation number we're ignoring. Do you think that's enough? it gives us a breadcrumb to look up what is being ignored. For all of the ones here, they're all about the possibility of insecurely handling untrusted input.

Also, I removed the URLs entirely from the commit message. They can be easily googleable.

Let me know if these changes make sense to you.

[holmanb]

One nit, but feel free to merge once fixed!

[holmanb]

is this supposed to have a nosec in the comment?