Skip to content

[JAVA] Partial Path Traversal Vuln Query#9742

Merged
smowton merged 35 commits into
github:mainfrom
smehta23:feat/SM/java_partial_path_traversal_vulnerability
Aug 15, 2022
Merged

[JAVA] Partial Path Traversal Vuln Query#9742
smowton merged 35 commits into
github:mainfrom
smehta23:feat/SM/java_partial_path_traversal_vulnerability

Conversation

@smehta23
Copy link
Copy Markdown
Contributor

  • [JAVA] Partial Path Traversal Vuln Query
  • Finish Partial Path Traversal Query
  • Add additional tests from real world query run

@smowton
Copy link
Copy Markdown
Contributor

smowton commented Jun 29, 2022

Could you clarify if you're making a bounty application for this or not?

@JLLeitschuh
Copy link
Copy Markdown
Contributor

Hi! This is submitted by my intern. I'd like him to turn this into a bounty application, if possible 🙂

Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalBad.java Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalGood.java Outdated
smehta23 and others added 2 commits July 1, 2022 10:39
Co-authored-by: Jonathan Leitschuh <jonathan.leitschuh@gmail.com>
Co-authored-by: Jonathan Leitschuh <jonathan.leitschuh@gmail.com>
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp Outdated
smehta23 and others added 4 commits July 1, 2022 10:51
Co-authored-by: Jonathan Leitschuh <jonathan.leitschuh@gmail.com>
Co-authored-by: Jonathan Leitschuh <jonathan.leitschuh@gmail.com>
Co-authored-by: Jonathan Leitschuh <jonathan.leitschuh@gmail.com>
@smehta23 smehta23 marked this pull request as ready for review July 1, 2022 15:25
@smehta23 smehta23 requested a review from a team as a code owner July 1, 2022 15:25
"qhelp.dtd">
<qhelp>
<overview>
<p>User supplied file paths can often pose security risks if a program does not correctly handle them. In particular, if a user
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would change the position of "correctly" to become "does not handle them correctly"

<qhelp>
<overview>
<p>User supplied file paths can often pose security risks if a program does not correctly handle them. In particular, if a user
is meant to access files under a certain directory but does not enters a path under that directory, they can gain access to
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There's a typo here, "does not *enter"

<overview>
<p>User supplied file paths can often pose security risks if a program does not correctly handle them. In particular, if a user
is meant to access files under a certain directory but does not enters a path under that directory, they can gain access to
(and potentially modify/delete) unexpected, possibly sensitive resources. </p>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

unexpectedly

Copy link
Copy Markdown
Contributor Author

@smehta23 smehta23 Jul 12, 2022

Choose a reason for hiding this comment

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

Should it then be "...they can unexpectedly gain access to (and potentially modify/delete) possibly sensitive resources"?
(I've currently phrased it that way, but let me know if you'd prefer something different!)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can do both here, choice is yours :-)

Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp Outdated
smehta23 and others added 6 commits August 9, 2022 11:58
…e.qhelp

Co-authored-by: Chris Smowton <smowton@github.com>
…e.qhelp

Co-authored-by: Chris Smowton <smowton@github.com>
Co-authored-by: Chris Smowton <smowton@github.com>
Co-authored-by: Chris Smowton <smowton@github.com>
A <p> at the top isn't allowed, and for some reason the inclusion is required to be a valid qhelp file.
@smowton
Copy link
Copy Markdown
Contributor

smowton commented Aug 10, 2022

I've pushed two commits that proved necessary to performance-evaluate this PR:

  1. 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.
  2. Dataflow-path queries need import DataFlow::PathGraph in order to emit the edges and nodes relations that drive the flow paths shown in the code-scanning and vscode user interface. codeql database interpret-results will fail unless the expected relations are present.

@smowton
Copy link
Copy Markdown
Contributor

smowton commented Aug 11, 2022

@github/docs-content-codeql highlighting @JLLeitschuh's request above to expedite docs review on this PR due to the author shortly finishing their internship.

@smowton
Copy link
Copy Markdown
Contributor

smowton commented Aug 11, 2022

Pushed one further commit to autoformat ql to hopefully get tests passing.

@smowton
Copy link
Copy Markdown
Contributor

smowton commented Aug 11, 2022

Performance eval completed with benign results; just need docs review now.

@mchammer01
Copy link
Copy Markdown
Contributor

Covering for the docs first responder here 👋🏻 - I added this PR to our board for review by the Docs team. Thanks for your patience 🙇🏻‍♀️ 😅

@mchammer01 mchammer01 self-requested a review August 15, 2022 08:39
@mchammer01
Copy link
Copy Markdown
Contributor

Apologies, I am on my own for 2 weeks, and am very busy, but will review this now. Thank you for your patience 🙇🏻‍♀️

mchammer01
mchammer01 previously approved these changes Aug 15, 2022
Copy link
Copy Markdown
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in reviewing this 😢
This LGTM 💖
I've made a few comments to bring the content in line with our content model.
Let me know if you have any questions 😃

Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.ql Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.ql Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp Outdated
Comment thread java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp Outdated
Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
smowton
smowton previously approved these changes Aug 15, 2022
@smowton smowton merged commit 774e379 into github:main Aug 15, 2022
@smowton
Copy link
Copy Markdown
Contributor

smowton commented Aug 15, 2022

@JLLeitschuh @smehta23 was there ever a bounty application for this?

@JLLeitschuh
Copy link
Copy Markdown
Contributor

We will submit one as soon as we both get back from Vegas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Java ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants