fix(userid): parse XML through defusedxml to prevent XXE - #629
Open
DearMoon50 wants to merge 2 commits into
Open
DearMoon50 wants to merge 2 commits into
DearMoon50 wants to merge 2 commits into
Conversation
panos/userid.py built XML objects via the unprotected stdlib xml.etree.ElementTree.fromstring, with no defusedxml/defuse_stdlib usage anywhere in the codebase. Route the two fromstring() call sites in UserId (initial uid-message template, and the registered-user command builder in get_user_tags) through defusedxml.ElementTree instead, so a malicious external entity/billion-laughs payload is rejected rather than resolved. Left the other 9 modules named in the report (base.py, firewall.py, network.py, panorama.py, policies.py, objects.py, ha.py, plugins.py, __init__.py) untouched: they only use ElementTree for outbound construction/serialization (Element/SubElement/tostring), which defusedxml.ElementTree does not provide -- a blind import swap there would break construction, not add any parsing protection, since none of them call fromstring/parse/XML on untrusted input. Added defusedxml as a runtime dependency in pyproject.toml and a regression test proving the parser now in use rejects XXE payloads. poetry.lock/requirements.txt are not regenerated here since our local poetry could not resolve this project's existing pyproject.toml (unrelated docutils/python-range conflict); please run `make sync-deps` on your end if the lockfile needs updating. Fixes part of PaloAltoNetworks#617 (Finding 1). Finding 2 (SHA-1 usage in _sha1_hash) is left for maintainers to weigh in on -- that hash format is used to match a specific PAN-OS device API field, so changing the algorithm risks breaking interop with real devices. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
docs/conf.py imports panos directly for autodoc, which now transitively imports defusedxml via panos.userid. RTD's build environment installs from docs/requirements.txt independently of pyproject.toml, so it didn't have defusedxml and the docs build failed on import. Also regenerated the root requirements.txt (already declared in pyproject.toml, just needed the pinned/hashed entry) via poetry export. Left poetry.lock alone -- regenerating it with the poetry version available here produced an ~800-line diff across unrelated dependencies (different resolver/poetry version than whatever the maintainers use), so a maintainer should regenerate that one with their own toolchain if needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
panos/userid.py built XML objects via the unprotected stdlib xml.etree.ElementTree.fromstring, with no defusedxml/defuse_stdlib usage anywhere in the codebase.
Route the two fromstring() call sites in UserId (initial uid-message template, and the registered-user command builder in get_user_tags) through defusedxml.ElementTree instead, so a malicious external entity/billion-laughs payload is rejected rather than resolved.
Left the other 9 modules named in the report (base.py, firewall.py, network.py, panorama.py, policies.py, objects.py, ha.py, plugins.py, init.py) untouched: they only use ElementTree for outbound construction/serialization (Element/SubElement/tostring), which defusedxml.ElementTree does not provide -- a blind import swap there would break construction, not add any parsing protection, since none of them call fromstring/parse/XML on untrusted input.
Added defusedxml as a runtime dependency in pyproject.toml and a regression test proving the parser now in use rejects XXE payloads. poetry.lock/requirements.txt are not regenerated here since our local poetry could not resolve this project's existing pyproject.toml (unrelated docutils/python-range conflict); please run
make sync-depson your end if the lockfile needs updating.Fixes part of #617 (Finding 1). Finding 2 (SHA-1 usage in _sha1_hash) is left for maintainers to weigh in on -- that hash format is used to match a specific PAN-OS device API field, so changing the algorithm risks breaking interop with real devices.
Description
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate)
Types of changes
Checklist