[http-client-python][bug]: use of xml.etree.ElementTree causing CWE-611 report

[joekiller]

Describe the bug

As a user I want to use typespec generated clients that are free from CWE warnings.

Actual behavior

Client is getting CWE warnings due to insecure function use.

The clients are using xml.etree.ElementTree.fromstring which is considered insecure generating CWE-611 warnings.

The jinja templates literally are trying to ignore this problem by using # nosec tags.

• typespec/packages/http-client-python/generator/pygen/codegen/templates/serialization.py.jinja2

  Line 103 in f7fc86f

  return ET.fromstring(data_as_str) # nosec

• typespec/packages/http-client-python/generator/pygen/codegen/templates/serialization.py.jinja2

  Line 103 in f7fc86f

  return ET.fromstring(data_as_str) # nosec

Expected behavior

Clients would use defusedxml which is considered safe.

It would be best to use the safe package. At a minimum a statement regarding why this may be ignored would be helpful.

Reproduction

npm install -g @typespec/compiler
tsp --version
1.2.1
tsp init
... choose REST
... include python client emitter
// update tsp config.yaml to emit to emitter-output-dir: "{cwd}/clientpy"
tsp compile .
snyk code test

FireShot Capture 018 - PR Check - Snyk - [app.snyk.io].pdf

Details:

• Review README on branch: https://github.com/joekiller/typespec-pyclient-snyk-CWE-611/tree/trigger-cwe-warning and
• feat: add python client triggering CWE-611  joekiller/typespec-pyclient-snyk-CWE-611#2

Checklist

• Follow our Code of Conduct
• Check that there isn't already an issue that request the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion.
• The provided reproduction is a minimal reproducible example of the bug.

[iscai-msft]

thanks for opening this issue @joekiller let me look into it today. thanks!

[iscai-msft]

@joekiller I've been looking into the correct replacement library and diving more into the security issues associated with xml.etree.ElementTree. Can you reply with the code scanner that is raising the cwe error, and what the code scanner is recommending as a workaround?

[iscai-msft]

@joekiller I'm also seeing that these warnings are now outdated, the stdlib handling of xml has fixed these security warnings, see here

[joekiller]

@iscai-msft thanks for your diligence here. Let me answer some of these questions.

The crux of the vulnerability is that xml.etree.ElementTree will parse external entities in the file unless resolve_entities=False is set prior to using the parser. I interpret the vulnerability to be that malicious code could inject references that could be used against the client or in some way to extract information about the host.

Can you reply with the code scanner that is raising the cwe error,

The code scanner is SNYK SAST Code Scanning Tool | Code Security Analysis & Fixes

The CLI can be installed via npm install -g synk and then run via snyk code test in the directory of the generated client target.

The scanner can also be hooked up to a public repository which is how I got the comments on the PR I linked. Running the cli will result in the same results. You may have to have a free account which can be created via a github login.

and what the code scanner is recommending as a workaround?

The guidance included in the scanner github comment links to their XML external entity injection (XXE) / Python article and recommends that if xml.etree.ElementTree is continued to be used then, The xml.etree.ElementTree version requires explicitly setting resolve_entities=False in the XMLParser to prevent parsing of external entities.

these warnings are now outdated

vstinner's comment does indicate that the stable branch can go without the warnings per updated expat >= 2.6 however the PR he links to (python/cpython#135294) indicates that only Python 3.11-3.15 would meet the requirement while the generator setup.py indicates the client would be good for Python 3.9-3.12 so I think the resolution would either need a mitigation for the lower versions or a high bar of entry on the setup.py.

I'm going to try to apply the suggestion to my POC code to see if it eliminates the warning.

[joekiller]

@iscai-msft please see the following commit I made applying the suggested fix from the articles. It appears to resolve the warmings. These same changes could be applied to the template to make generated clients not get the CWE-611 warning that I've reported.

joekiller/typespec-pyclient-snyk-CWE-611@063be15

[iscai-msft]

I'm so sorry I missed the earlier messages, not sure how that happened. Thanks so much for your PR, I'm going to reach out to more people on the Python side to confirm. Thanks again!

[joekiller]

Hi @iscai-msft the original article was misleading in that the examples it showed were for lxml or defusedxml. The article showed import xml.etree.ElementTree as ET but it was really supposed to say import lxml.etree as etree. I'm updating the PR to use lxml for y'alls consideration.

Here are some better references:

https://www.appmarq.com/public/security,1021102,Avoid-parsing-XML-data-without-restriction-of-XML-External-Entity-Reference-XXE-Python
https://codeql.github.com/codeql-query-help/python/py-xxe/#example

[iscai-msft]

thanks again @joekiller, I'm continuing to follow up with python people about this, I don't think using a third-party library will work out, but I'm waiting to see the response. Thanks again for opening the pr and all of your effort into this!