Java: Promote XPath Injection query from experimental

[atorralba]

PR to promote the XPath injection query created in #2800

Changes

• Existing files were moved out of experimental
• The XPathl.qll file was created. The existing XPath injection sinks wee moved here, and were refactored to use the new CSV sink model.
  • New sinks were added: basically, more methods that evaluate XPath expressions from dom4j and the standard library.
• Added tests using InlineExpectationsTest and new stubs for dom4j and jaxen

Evaluation

After the changes, the query produces alerts from 31 projects. After review, all seem TPs, but:

• 14 are intentionally vulnerable for educational purposes
• 3 seem actual vulnerabilities
• The rest are either not actually exploitable (because e.g. admin rights are required or the accessed document is unimportant) or are forks of one of the above

[Marcono1234]

From dom4j there might be other interesting classes as well:

• Constructor DefaultXPath(String)
• AbstractNode methods createXPathFilter and createPattern, not sure if they are problematic as well.
• DocumentFactory methods
• DocumentHelper methods
• XPathPattern constructor
• ProxyDocumentFactory methods

Though maybe this is out of scope for this pull request. These are only suggestions; I am not a member of this project.

Also the .expected file is currently empty, but you are probably still working on it.

[joefarebrother]

@Marcono1234

Also the .expected file is currently empty, but you are probably still working on it.

The inline expectation test system requires the .expected file to be empty.

[atorralba]

From dom4j there might be other interesting classes as well:

• Constructor DefaultXPath(String)
• AbstractNode methods createXPathFilter and createPattern, not sure if they are problematic as well.
• DocumentFactory methods
• DocumentHelper methods
• XPathPattern constructor
• ProxyDocumentFactory methods

Though maybe this is out of scope for this pull request. These are only suggestions; I am not a member of this project.

Also the .expected file is currently empty, but you are probably still working on it.

These are great suggestions @Marcono1234, thank you! I refactored the XPath.qll file to use the new CSV-like sink model, and while I was at it I added these sinks. In some cases (like AbstractNode.createXPath or DefaultXPath()) supertypes were used to catch other potential implementations.

[atorralba]

@github/codeql-java I consider this ready for a final review.

[Marcono1234]

Thanks for considering my review comments!
There are only a few more minor things.

[felicitymay]

Many thanks for including a change note for this pull request ✨

I was unable to comment on the Qhelp file or associated .java file as they are simply renamed. For the Qhelp file suggestions, I ended up raising: #5837.

My only other real feedback is that it would be good to include an example of a good dom4J expression and not only a bad one. Otherwise the user-facing content looks good to me.

[felicitymay]

One question here - has the query been added to the query suite files for LGTM and Code scanning? I don't see any changes to .qls files in this PR.

[aschackmull]

Wasn't there a PR check for that at some point?

[aschackmull]

I've lost track of how query suite files are managed these days.

[felicitymay]

That rings a bell but I've also forgotten. Maybe it hasn't been reimplemented after we moved to Actions?

[shati-patel]

I believe that new queries are added to code scanning suites automatically (via https://github.com/github/codeql/blob/main/misc/suite-helpers/code-scanning-selectors.yml and similar "helpers")

[felicitymay]

Thanks Shati 😄 That's great to know.

[atorralba]

Force-pushed to rebase the changes suggested by in #5837.

[atorralba]

My only other real feedback is that it would be good to include an example of a good dom4J expression and not only a bad one. Otherwise the user-facing content looks good to me.

@felicitymay this was included in 7646855, thanks for the suggestion!

[felicitymay]

Many thanks for adding the extra example @atorralba ✨

Two final comments on the user-facing text, but otherwise this is a 👍🏻 from the docs side. I'll leave it to the CodeQL Java team to decide when the whole PR is ready to merge.

[aschackmull]

This is really minor and not something I think we actually ever talked about, but we tend to put the import DataFlow::PathGraph last in the sequence of imports. As I said, it's really not very important (putting import java first is important, though, but that's another matter entirely), but the reason I mention this is to highlight that the PathGraph import is a different kind of import than the others: the other imports reference file-modules, whereas PathGraph is a module in scope and this import statement merely opens it to the top-level scope. (There might even have been discussions about whether this ought to have been a different keyword than import, such as perhaps open.)
Also, for completeness of information, importing an in-scope module mostly affects naming, i.e. we can import DataFlow if we want to write Node rather than DataFlow::Node, but in this case there's a separate reason: The PathGraph module contains two predicates annotated with query and putting such predicates in the top-level scope makes them part of the query on the same level as the select statement (and those query predicates contain the graph that's required for a path-problem query).

[atorralba]

That's really interesting, thank you for taking the time to explain it with so much detail!

I moved the import down as you suggested (see b69be30) - now that I know the reason, it makes a lot of sense.

[aschackmull]

The ExternalFlow import needs to be bidirectional. I.e. add a private import semmle.code.java.security.XPath inside the private module Frameworks in ExternalFlow.qll. This is important to ensure that ExternalFlow.qll always sees the same set of csv rows to enable efficient caching. Otherwise, a lot of things will be recomputed based on which csv rows are transitively imported.

[atorralba]

Fixed in b69be30. Another "gotcha" to keep in mind, thanks!

[aschackmull]

I really want to make this a compilation error, but we'll need a little bit of additional language support for that.

[aschackmull]

This looks like the only change in the CWE-611 directory. Was this committed by mistake?

[atorralba]

I changed this because tests for CWE-611 started failing when I introduced a Jaxen import in a stub used both in CWE-611 and CWE-643 (here 7646855#diff-da791bf0774d1d6fe164bf5fa3cae3cec838ee5f8a7218737364701624c5716bR95)

[aschackmull]

Some inline comments, otherwise LGTM.

[aschackmull]

If this ought to be reported, the inline expectations test has an annotation for that:

Suggested change

	new XPathPattern(new PatternStub(user)); // Jaxen is not modeled yet
	new XPathPattern(new PatternStub(user)); // Jaxen is not modeled yet $ MISSING: hasXPathInjection

That way, the qltest error, if this does get modelled, will list this as "Fixed" rather than "Spurious new result".
(I think you can leave the "Jaxen is not modeled" comment before the $, but I'm actually not sure about that part - best look into the internals of inline expectations to check or simply test it)

[aschackmull]

Perhaps just move the regular comment to its own line in order to not mix it with the expectation syntax.

Suggested change

	new XPathPattern(new PatternStub(user)); // Jaxen is not modeled yet
	// Jaxen is not modeled yet
	new XPathPattern(new PatternStub(user)); // $ MISSING: hasXPathInjection

[atorralba]

Apparently, it needs to start with $ MISSING: to work properly:

codeql/java/ql/test/TestUtilities/InlineExpectationsTest.qll

Line 72 in ef0ea24

* `$ MISSING: tag=expected-value` // Missing result

codeql/java/ql/test/TestUtilities/InlineExpectationsTest.qll

Lines 162 to 165 in ef0ea24

	* RegEx pattern to match a comment containing one or more expected results. The comment must have
	* `$` as its first non-whitespace character. Any subsequent character
	* is treated as part of the expected results, except that the comment may contain a `//` sequence
	* to treat the remainder of the line as a regular (non-interpreted) comment.

I tested it that way and indeed the test result shows Fixed missing result instead of Unexpected result (assuming the line raises an alert).

Added in 2a50195. Another cool thing to know, thanks!