Skip to content

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented May 12, 2022

Fixes some typos and increases the XXE query precision to @high.

The latter is justified by results on 207 LGTM projects: https://lgtm.com/query/5698038244523748150/

  • the first two projects with results use libxml2 and fail to follow the advice in https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
  • the third and fourth use the Xerces SAX2XMLReader (XMLReaderFactory::createXMLReader) interface and fails to set fgXercesDisableDefaultEntityResolution.
  • the fifth has examples of both Xerces SAX2XMLReader and createLSParser. In all cases results are failing to set fgXercesDisableDefaultEntityResolution, though note that for createLSParser the need to do this is inferred rather than directly stated by the OWASP doc.

There's also an all-LGTM run linked from the internal issue. 26 projects with results, I've checked at least one result from each project and found no surprises. pwsafe/pwsafe sets pSAX2Parser->setFeature(XMLUni::fgXercesLoadExternalDTD, false);, which is interesting, at a glance I'm a little surprised the OWASP guidelines don't require doing this. I've added this to my list of extensions to potentially look into.

It is of course possible there are other safety mechanisms that the OWASP doc doesn't describe and we are unaware of - I do not claim to be an expert in either library. Aside from that limitation I have confidence in this query.

@geoffw0 geoffw0 added the C++ label May 12, 2022
@geoffw0 geoffw0 requested a review from a team as a code owner May 12, 2022 20:14
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

I was about to suggest that we do a DCA run with the Code Scanning suite to ensure that we don't have IR re-evaluation in that suite, but then I remembered that this is a thing of the past now 🎉.

@MathiasVP MathiasVP merged commit cee7aed into github:main May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants