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

Rule S320 should be removed #13707

Open
pythonweb2 opened this issue Oct 10, 2024 · 9 comments
Open

Rule S320 should be removed #13707

pythonweb2 opened this issue Oct 10, 2024 · 9 comments
Labels
rule Implementing or modifying a lint rule
Milestone

Comments

@pythonweb2
Copy link

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
Copy link
Member

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
Copy link
Member

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

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Oct 14, 2024
@pythonweb2
Copy link
Author

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
Copy link
Author

pythonweb2 commented Oct 14, 2024

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
Copy link
Member

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
Copy link

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
Copy link

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.

@MichaReiser MichaReiser added this to the v0.10 milestone Feb 7, 2025
@ngnpope
Copy link
Contributor

ngnpope commented Feb 14, 2025

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.

@MichaReiser
Copy link
Member

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

MichaReiser added a commit that referenced this issue Mar 13, 2025
…6680)

## Summary
Deprecate `S320` because defusedxml has deprecated there `lxml` module
and `lxml` has been hardened since.

flake8-bandit has removed their implementation as well
(PyCQA/bandit#1212).

Addresses #13707


## Test Plan

I verified that selecting `S320` prints a warning and fails if the
preview mode is enabled.
MichaReiser added a commit that referenced this issue Mar 13, 2025
…6680)

## Summary
Deprecate `S320` because defusedxml has deprecated there `lxml` module
and `lxml` has been hardened since.

flake8-bandit has removed their implementation as well
(PyCQA/bandit#1212).

Addresses #13707


## Test Plan

I verified that selecting `S320` prints a warning and fails if the
preview mode is enabled.
MichaReiser added a commit that referenced this issue Mar 13, 2025
…6680)

## Summary
Deprecate `S320` because defusedxml has deprecated there `lxml` module
and `lxml` has been hardened since.

flake8-bandit has removed their implementation as well
(PyCQA/bandit#1212).

Addresses #13707


## Test Plan

I verified that selecting `S320` prints a warning and fails if the
preview mode is enabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

5 participants