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

Fix security issue with XML parsing #5686

Merged
merged 5 commits into from
May 26, 2021

Conversation

mvafin
Copy link
Contributor

@mvafin mvafin commented May 19, 2021

Root cause analysis: xml.etree.ElementTree.parse is unsafe to use, xml.etree.ElementTree is unsafe to include.

Solution: Replace unsafe xml.etree.ElementTree.parse with safe defusedxml.ElementTree.parse. Use defusedxml.defuse_sdtlib() to patch xml.etree.ElementTree and use objects from patched library.

Ticket: 55711

Code:

  • Comments
  • Code style (PEP8)
  • Transformation generates reshape-able IR
  • Transformation preserves original framework node names

Validation:

  • Unit tests
  • Framework operation tests: N/A - No new enabled operations;
  • Transformation tests: N/A - No new implemented transformations;
  • Model Optimizer IR Reader check: N/A

Documentation:

  • Supported frameworks operations list: N/A - No new supported operations;
  • Supported public models list: N/A - Nothing to add;
  • User guide update: N/A - Nothing to update.

@mvafin mvafin force-pushed the fix/xml-issue branch 4 times, most recently from 4bb9bbe to e1a0ca8 Compare May 20, 2021 19:42
@mvafin mvafin marked this pull request as ready for review May 20, 2021 20:04
@mvafin mvafin requested review from a team, achetver, rkazants, evolosen and asomsiko and removed request for a team May 20, 2021 20:04
@rkazants
Copy link
Contributor

@mvafin, @asomsiko, why should we care of this if the MO tool is offline tool? We can break something in MO by changing this logic that is more risky.

@mvafin mvafin requested a review from rkazants May 21, 2021 14:32
@mvafin mvafin requested a review from rkazants May 25, 2021 11:50
@lazarevevgeny lazarevevgeny enabled auto-merge (squash) May 25, 2021 12:53
@lazarevevgeny lazarevevgeny disabled auto-merge May 25, 2021 13:05
@lazarevevgeny lazarevevgeny merged commit d8d9819 into openvinotoolkit:master May 26, 2021
apankratovantonp pushed a commit to apankratovantonp/openvino that referenced this pull request May 26, 2021
* Fix security issue with XML parsing

* Apply review feedback

* Rework defusing stdlib solution

* Apply review feedback
AlexeyLebedev1 pushed a commit to AlexeyLebedev1/openvino that referenced this pull request May 27, 2021
* Fix security issue with XML parsing

* Apply review feedback

* Rework defusing stdlib solution

* Apply review feedback
yekruglov pushed a commit to yekruglov/openvino that referenced this pull request Jun 7, 2021
* Fix security issue with XML parsing

* Apply review feedback

* Rework defusing stdlib solution

* Apply review feedback
rnugmanx pushed a commit to rnugmanx/openvino that referenced this pull request Aug 26, 2021
* Fix security issue with XML parsing

* Apply review feedback

* Rework defusing stdlib solution

* Apply review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants