codeql
codeql copied to clipboard
github
[JAVA] Partial Path Traversal Vuln Query
- [JAVA] Partial Path Traversal Vuln Query
- Finish Partial Path Traversal Query
- Add additional tests from real world query run
Hi! This is submitted by my intern. I'd like him to turn this into a bounty application, if possible 🙂
This is ready for review. I'd like to include it, it's result, and the bounty in my Black Hat and DEFCON talks. If this could be prioritized, that would be appreciated
This is not targeting experimental, so will need a thorough FP evaluation before merge.
Suggestion from security lab review: make two variants of this, one with high precision that checks for flow from a RemoteSource (i.e., exploitability) + make the existing one medium precision (might not be exploitable, but still worth fixing)
Let's go remote in the name of higher precision; local cases will be picked up by the medium-precision query.
Docs should be similar but note the difference between the two queries and the existence of the other one.
IIRC Zip Slip doesn't attempt to break itself into two different queries. I'm curious why you think it's important for this query. We're willing to do it, but we're wondering why the same wasn't done for Zip Slip.
Most likely, how common are zip-unpacking routines vs. path-concatenating routines? The more common in general it is, the more annoying someone will find it if they're working in a scenario where the input is trusted
I disagree with your perspective that if the tainted value isn't from a remote source it isn't still high precision. I think the impact is potentially lower if it's not from a remote source. But that doesn't lower it's precision, just it's impact.
Using "precision" to categorise like this is a consequence of how the different CodeQL query suites are organised. By default we run the high and very-high precision queries, and a non-default but easily opted-in suite can add the medium-precision queries. The term can make sense if you consider it not to mean "how precisely is this query identifying what it says it does," but rather "how precisely can it identify an exploitable security hole"? Without a remote-flow check quite a few results will be potentially broken but not exploitable by a malicious user.
From this, I'm implying that "how precisely can it identify an exploitable security hole" is actually "how precisely can it identify a remotely exploitable security hole".
It sound's like you're trying to communicate CVSS (which includes whether or not a remote (ie. network) attacker can exploit a given vulnerability). Why not just move CodeQL towards CVSS for severity scoring instead of these arbitrary numbers and quantities.
Could this get a re-review with the split changes having been made as requested.
Can we just have both queries qdoc reference the same qdoc file, or do you really truly want different files? Is there a way to write common paragraphs of text and only use those sub-sections. That way we aren't copy-pasting documentation between two qdoc files
Can we just have both queries qdoc reference the same qdoc file, or do you really truly want different files? Is there a way to write common paragraphs of text and only use those sub-sections. That way we aren't copy-pasting documentation between two qdoc files
If you mean query help files with "qdoc", then you should be able to use query help inclusion.
The query help fragment that is to be included should have the .inc.qhelp file extension.
As an example, you can look at these three query help files: https://github.com/github/codeql/blob/main/java/ql/src/Language%20Abuse/IterableOverview.inc.qhelp https://github.com/github/codeql/blob/main/java/ql/src/Language%20Abuse/WrappedIterator.qhelp https://github.com/github/codeql/blob/main/java/ql/src/Language%20Abuse/IterableIterator.qhelp
@smowton can you be more clear about what you'd like to see different in the documentation between each query?
Only to describe the difference between the queries (and cite the other one, with text like See also java/query-name, which is similar to this query but only flags instances with evidence of remote exploitability or the like) -- one shows evidence of remote exploitability, one flags a potentially-problematic pattern but doesn't care if it's exploitable or not.
As a meta note, this sort of stuff is why I would always recommend targeting the experimental query suite first. To get a query into experimental it just has to be plausibly useful, and can be rough round the edges. To get into the default query suite we need to be happy we're providing the false-positive / false-negative tradeoff our customers will be happy with.
As a meta note, this sort of stuff is why I would always recommend targeting the experimental query suite first.
I like to achieve the highest quality up-front to maximize the GitHub security lab BB payout
Any doc reviews? @smehta23 is only my intern for another 2 weeks so if we could finish this by then, that would be awesome
I've pushed two commits that proved necessary to performance-evaluate this PR:
- Inlined one of the qhelp includes, because the included doc is required to itself be a valid qhelp doc, but
<p>can't occur at top level, only inside an<overview>or similar top-level section. We can look at generalising the lightly-used qhelp-inclusion feature, but for now just duplicating that one paragraph is the quickest way to get this working. - Dataflow-path queries need
import DataFlow::PathGraphin order to emit theedgesandnodesrelations that drive the flow paths shown in the code-scanning and vscode user interface.codeql database interpret-resultswill fail unless the expected relations are present.
@github/docs-content-codeql highlighting @JLLeitschuh's request above to expedite docs review on this PR due to the author shortly finishing their internship.
Covering for the docs first responder here 👋🏻 - I added this PR to our board for review by the Docs team. Thanks for your patience 🙇🏻♀️ 😅
Apologies, I am on my own for 2 weeks, and am very busy, but will review this now. Thank you for your patience 🙇🏻♀️