Skip to content

Python: Add support for detecting XSLT Injection (#3521 revived) - #3801

Merged
tausbn merged 11 commits into
github:masterfrom
RasmusWL:python-3521-revived
Jun 26, 2020
Merged

Python: Add support for detecting XSLT Injection (#3521 revived)#3801
tausbn merged 11 commits into
github:masterfrom
RasmusWL:python-3521-revived

Conversation

@RasmusWL

Copy link
Copy Markdown
Member

This PR is bringing #3521 back from the dead (an external contribution).

I did some minor readjusting of the code, but nothing major.

@RasmusWL
RasmusWL requested a review from a team as a code owner June 25, 2020 10:10
yoff
yoff previously approved these changes Jun 26, 2020

@yoff yoff left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In ParseXSLTArgument, the flow from tree.parse to tree could be generalised into a taint flow of its own. However, I would consider that out of scope for this PR.

Comment thread python/ql/src/experimental/CWE-091/Xslt.qhelp Outdated
Comment thread python/ql/src/experimental/semmle/python/security/injection/XSLT.qll Outdated
Comment thread python/ql/src/experimental/semmle/python/security/injection/XSLT.qll Outdated
tausbn
tausbn previously approved these changes Jun 26, 2020

@tausbn tausbn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One minor documentation comment, otherwise LGTM.

@@ -0,0 +1,35 @@
/**
* @name Xslt query built from user-controlled sources

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know discussing how to write acronyms is all the rage, but perhaps this should be XSLT for consistency?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In the name, totally agree! (fix implemented)

I think the discussion would decide whether it should be called XSLT.ql or Xslt.ql

@RasmusWL
RasmusWL dismissed stale reviews from tausbn and yoff via 08384e3 June 26, 2020 10:06
@RasmusWL
RasmusWL requested review from tausbn and yoff June 26, 2020 10:10
@tausbn
tausbn merged commit e5d23b2 into github:master Jun 26, 2020
@RasmusWL
RasmusWL deleted the python-3521-revived branch June 26, 2020 11:24
@ghost

ghost commented Jun 26, 2020

Copy link
Copy Markdown

@RasmusWL Thanks a lot for updating this and getting this merged into the code base. I just noticed that the ql used in #3801 #3521 #3522 won't detect the following case. I think this is a limitation in the current taint tracking library but I may be wrong. I am noting this down here to keep track of this.

from lxml.etree import *
res = XPath('sink')

or

from lxml.etree import *
f = StringIO('<foo><bar></bar></foo>')
tree = parse(f)
r = tree.xpath("/tag[@id='%s']" % untrusted_value)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 participants