Rule S320 should be removed

[pythonweb2]

This comment seems to have been overlooked (at least was not responded to).

If lxml is fixed, then from what I can understand of rule S320, it should be removed as well.

[MichaReiser]

Do you have a reference that lxml is fixed. Checking defusedxml's documentation it seems that lxml is still vulnerable to entity expansions which is what S320 warns about. https://pypi.org/project/defusedxml/

[MichaReiser]

I also found PyCQA/bandit#767 but it's very unclear to me what the recommendation is...

[pythonweb2]

There is this section in the FAQ page:

Some of the most famous excessive content expansion attacks use XML entity references. Luckily, entity expansion is mostly useless for the data commonly sent through web services and can simply be disabled, which rules out several types of denial of service attacks at once. This also involves an attack that reads local files from the server, as XML entities can be defined to expand into the content of external resources. Consequently, version 1.2 of the SOAP standard explicitly disallows entity references in the XML stream.

To disable entity expansion, use an XML parser that is configured with the option resolve_entities=False. Then, after (or while) parsing the document, use root.iter(etree.Entity) to recursively search for entity references. If it contains any, reject the entire input document with a suitable error response. In lxml 3.x, you can also use the new DTD introspection API to apply your own restrictions on input documents. Since version 5.x, lxml disables the expansion of external entities (XXE) by default. If you really want to allow loading external files into XML documents using this functionality, you have to explicitly set resolve_entities=True.

[pythonweb2]

The current lxml version is 5.0, which seems to be safe by default against entity expansion (since it disables it).

So, idk if ruff is able to check the version of lxml, or perhaps check to see if there are any places where resolve_entities=True is being used? That seems like that would be a more useful thing to check for.

[MichaReiser]

The current lxml version is 5.0, which seems to be safe by default against entity expansion (since it disables it).

Yeah, it seems but I would like some more confirmation before deprecating a rule.

So, idk if ruff is able to check the version of lxml, or perhaps check to see if there are any places where resolve_entities=True is being used? That seems like that would be a more useful thing to check for.

Checking the version is beyond what Ruff can do today but checking for resolve_entities is a possibility for a new rule

[pawel-slowik]

The current lxml version is 5.0, which seems to be safe by default against entity expansion (since it disables it).

This is not entirely true. The 5.0 version only disables the expansion of external entities by default:

https://lxml.de/FAQ.html#how-do-i-use-lxml-safely-as-a-web-service-endpoint

Since version 5.x, lxml disables the expansion of external entities (XXE) by default.

https://lxml.de/5.0/changes-5.0.0.html

LP#1742885: lxml no longer expands external entities (XXE) by default to prevent the security risk of loading arbitrary files and URLs.

The default value for the resolve_entities argument has changed from True to 'internal', not to False: lxml/lxml@b38cebf#diff-d96803514b65b282182a7d8d4ab1c076c8e7726761aecf5bb8b5eb8355d14324R1581

And it is still 'internal' as of 5.3.0: https://github.com/lxml/lxml/blob/lxml-5.3.0/src/lxml/parser.pxi#L1600

The change protects from a file inclusion attack, but not from quadratic blowup.

[JeremyVriens]

Bandit removed both S410 and S320 from their blacklist since Bandit 1.8.1 (source: https://github.com/PyCQA/bandit/releases/tag/1.8.1).
Since S410 has already been removed from Ruff (source: #10154), my proposal is to also remove S320.

[ngnpope]

Checking defusedxml's documentation it seems that lxml is still vulnerable to entity expansions which is what S320 warns about

The documentation was changed on a pre-release version. Alas, it seems that defusedxml is now largely unmaintained.

• https://pypi.org/project/defusedxml/0.7.1/#defusedxml-lxml
• https://pypi.org/project/defusedxml/0.8.0rc2/#defusedxml-lxml

[MichaReiser]

I put up a PR to deprecate the rule (#16680). We should remove it in Ruff 0.11 (or 0.12)