Hi @porcupineyhairs, thanks for both submissions 👍

As @p0 highlighted on slack, when running Python tests that interacts with third-party libraries, we use stubbed versions. We try to keep these as minimal as possible, so it is clear that our queries doesn't depend on any specific implementation detail.

An example is the modeling of the flask library here. In the tests that use flask, use need to specify that the stubbed libraries should be imported using an options file (example for flask).

So if you could make stubs for lxml that would be great 👍 It would probably require adding the file python/ql/test/query-tests/Security/lib/lxml/etree.py, and filling out the minimal number of classes/functions that you need. How much is required depends on how much information that our points-to analysis is required to be able to figure out. I'ts very important to reproduce the behavior of the external library -- it's not good if our tests pass because we made a simple library stub that our points-to analysis can handle, but the real library is so complex that our points-to analysis can't handle it. Please keep all function signatures intact, but leave the bodies empty (if possible).

It seems like the only interaction with the Value API currently is

ClassValue etree() { result = Value::named("lxml.etree") }

But that won't work, since lxml.etree is a module

$ python -c 'from lxml import etree; print(type(etree))'
<class 'module'>

P.S. Just to clear up confusion debstubber is only used for Go

Unless you want me to review your code in its current state, I'll postpone reviewing it until you have done the library stubbing. Does that sound good with you?

Could you also please move all the contributed files to the experimental folder? (including tests and new library modules)

Oh yeah, both queries also seem to be missing .qhelp files right now. Would be great if you could add them 😊 They should highlight why this is a problem (so users are convinced to fix it), what the problem would usually look like, how to fix the problem, and some references to back up all of these claims. Specific details for writing these files can be found in https://github.com/github/codeql/blob/master/docs/query-help-style-guide.md

@RasmusWL I have stubbed the dependencies and added the qhelp file.

@RasmusWL I have included changes you mentioned in the other PR. I have also rebased this to the current master.

Please note that while this is similar to the XPath injection PR, both of them tackle different issues and different impact. In the case of this one, XSLT injection, the attacker can obtain code injection quite easily. While in the XPath injection's case, this was not possible.

As for "one valid CVE", I don't have any out the top of my head. Googling does give you a few results But this is a quite common vulnerability. Most of the other scanners such as Fortify support this. Hence, I think CodeQL should support this too.

@porcupineyhairs similar to #3522 for consideration of bounty award it is a requirement that the query finds a real world result. If you can map this query to any CVE or an existing project result, that would be helpful in further assessing the bounty award eligibility. We were unable to uncover a qualifying result set via LGTM. Thanks!

As an update, the same evaluation as #3522 applies to this PR re: bounty award.

@anticomputer @RasmusWL I am unable to find any cve's for this. Since the codebase is being rewritten, it makes little sense to make the changes @RasmusWL requested and get this ready to merge today. Hence, I am closing this one for now.

I would probably have merged this anyway, and fixed up with getASuccessor myself. Since the branch has been deleted, I can't reopen 😞

Turned out that git fetch upstream 'refs/pull/3521/head' && git checkout FETCH_HEAD worked like a charm, so we're back in business 😛

This can't detect the case mentioned here. Adding a comment here to keep track of this limitation.