Skip to content

Added defusedxml to parse untrusted XML data#3636

Merged
fmassa merged 5 commits intopytorch:masterfrom
mstfbl:mstfbl/add-defusedxml
Apr 8, 2021
Merged

Added defusedxml to parse untrusted XML data#3636
fmassa merged 5 commits intopytorch:masterfrom
mstfbl:mstfbl/add-defusedxml

Conversation

@mstfbl
Copy link
Copy Markdown
Contributor

@mstfbl mstfbl commented Apr 5, 2021

This PR fixes an issue pointed out by Bandit w.r.t. using the xml library to parse untrusted data, by using defusedxml instead.

Bandit output:

>> Issue: [B314:blacklist] Using xml.etree.ElementTree.parse to parse untrusted XML data is known to be vulnerable to XML attacks. Replace xml.etree.ElementTree.parse with its defusedxml equivalent function or make sure defusedxml.defuse_stdlib() is called
   Severity: Medium   Confidence: High
   Location: ./torchvision/datasets/voc.py:206
   More Info: https://bandit.readthedocs.io/en/latest/blacklists/blacklist_calls.html#b313-b320-xml-bad-elementtree

@mstfbl mstfbl mentioned this pull request Apr 5, 2021
Merged
@mstfbl mstfbl force-pushed the mstfbl/add-defusedxml branch from 84df7f4 to 5f75e06 Compare April 6, 2021 00:06
fmassa
fmassa reviewed Apr 7, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@fmassa fmassa left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

type check is failing, can you fix it so that we can merge the PR?

@fmassa
Copy link
Copy Markdown
Member

fmassa commented Apr 7, 2021

One possible solution to the type checking is to disable it for defusedxml, as was done for some other libraries like

vision/mypy.ini

Lines 63 to 65 in 9e5edbd

[mypy-av.*]
ignore_missing_imports = True

@mstfbl
Copy link
Copy Markdown
Contributor Author

mstfbl commented Apr 7, 2021

Thank you @fmassa for your reviews and for merging the previous PRs. I have added the mypy type check disable for the defusedxml package.

fmassa
fmassa approved these changes Apr 8, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@fmassa fmassa merged commit 7f4ae8c into pytorch:master Apr 8, 2021
facebook-github-bot pushed a commit that referenced this pull request Apr 13, 2021
@fmassa @NicolasHug @facebook-github-bot
[fbsync] Added defusedxml to parse untrusted XML data (#3636)
f30512c 
Summary:
* Added defusedxml to parse untrusted XML data

* Added typecheck disable for defusedxml

Reviewed By: NicolasHug

Differential Revision: D27706948

fbshipit-source-id: 4334745d939c83e763ea5508b6284275c5c7bc32

Co-authored-by: Nicolas Hug <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fmassa fmassa fmassa approved these changes

Assignees

No one assigned

Labels

cla signed module: ci

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mstfbl @fmassa @NicolasHug @facebook-github-bot