From 76cecc170ef5242a0bf699ae108b3daea6a06f32 Mon Sep 17 00:00:00 2001 From: Shyam Mehta Date: Wed, 3 Aug 2022 14:30:17 -0400 Subject: [PATCH] Fix documentation --- .../java/security/PartialPathTraversal.qll | 23 +++++---- .../security/PartialPathTraversalQuery.qll | 9 +++- .../CWE/CWE-023/PartialPathTraversal.qhelp | 47 ++--------------- .../PartialPathTraversalFromRemote.qhelp | 11 ++++ .../PartialPathTraversalOverview.inc.qhelp | 10 ++++ .../PartialPathTraversalRemainder.inc.qhelp | 51 +++++++++++++++++++ 6 files changed, 97 insertions(+), 54 deletions(-) create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp diff --git a/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll b/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll index 0997bfb7f24..956efbeaeb9 100644 --- a/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll +++ b/java/ql/lib/semmle/code/java/security/PartialPathTraversal.qll @@ -1,51 +1,56 @@ +/** Provides classes to reason about partial path traversal vulnerabilities. */ + import java private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.environment.SystemProperty -class MethodStringStartsWith extends Method { +private class MethodStringStartsWith extends Method { MethodStringStartsWith() { this.getDeclaringType() instanceof TypeString and this.hasName("startsWith") } } -class MethodFileGetCanonicalPath extends Method { +private class MethodFileGetCanonicalPath extends Method { MethodFileGetCanonicalPath() { this.getDeclaringType() instanceof TypeFile and this.hasName("getCanonicalPath") } } -class MethodAccessFileGetCanonicalPath extends MethodAccess { +private class MethodAccessFileGetCanonicalPath extends MethodAccess { MethodAccessFileGetCanonicalPath() { this.getMethod() instanceof MethodFileGetCanonicalPath } } -abstract class FileSeparatorExpr extends Expr { } +private abstract class FileSeparatorExpr extends Expr { } -class SystemPropFileSeparatorExpr extends FileSeparatorExpr { +private class SystemPropFileSeparatorExpr extends FileSeparatorExpr { SystemPropFileSeparatorExpr() { this = getSystemProperty("file.separator") } } -class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral { +private class StringLiteralFileSeparatorExpr extends FileSeparatorExpr, StringLiteral { StringLiteralFileSeparatorExpr() { this.getValue().matches("%/") or this.getValue().matches("%\\") } } -class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral { +private class CharacterLiteralFileSeparatorExpr extends FileSeparatorExpr, CharacterLiteral { CharacterLiteralFileSeparatorExpr() { this.getValue() = "/" or this.getValue() = "\\" } } -class FileSeparatorAppend extends AddExpr { +private class FileSeparatorAppend extends AddExpr { FileSeparatorAppend() { this.getRightOperand() instanceof FileSeparatorExpr } } -predicate isSafe(Expr expr) { +private predicate isSafe(Expr expr) { DataFlow::localExprFlow(any(Expr e | e instanceof FileSeparatorAppend or e instanceof FileSeparatorExpr ), expr) } +/** + * A method access that returns a boolean that incorrectly guards against Partial Path Traversal. + */ class PartialPathTraversalMethodAccess extends MethodAccess { PartialPathTraversalMethodAccess() { this.getMethod() instanceof MethodStringStartsWith and diff --git a/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll b/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll index 3ca8aaa1074..70416546b96 100644 --- a/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll +++ b/java/ql/lib/semmle/code/java/security/PartialPathTraversalQuery.qll @@ -1,3 +1,5 @@ +/** Provides taint tracking configurations to be used in partial path traversal queries. */ + import java import semmle.code.java.security.PartialPathTraversal import semmle.code.java.dataflow.DataFlow @@ -5,8 +7,13 @@ import semmle.code.java.dataflow.ExternalFlow import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.FlowSources +/** + * A taint-tracking configuration for unsafe user input + * that is used to validate against path traversal, but is insufficient + * and remains vulnerable to Partial Path Traversal. + */ class PartialPathTraversalFromRemoteConfig extends TaintTracking::Configuration { - PartialPathTraversalFromRemoteConfig() { this = "PartialPathTraversalFromRemoteConfig" } + PartialPathTraversalFromRemoteConfig() { this = "PartialPathTraversalFromRemoteConfig" } override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource } diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp index 902d0fc682c..462e15d8ccc 100644 --- a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversal.qhelp @@ -3,51 +3,10 @@ "qhelp.dtd"> -

A common way to check that a user-supplied path SUBDIR falls inside a directory DIR -is to use getCanonicalPath() to remove any path-traversal elements and then check that DIR -is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

+ +

See also java/partial-path-traversal-from-remote, which is similar to this query but only flags instances with evidence of remote exploitability

 - -

If the user should only access items within a certain directory DIR, ensure that DIR is slash-terminated -before checking that DIR is a prefix of the user-provided path, SUBDIR. Note, Java's getCanonicalPath() -returns a non-slash-terminated path string, so a slash must be added to DIR if that method is used.

+ - - - -

- - -In this example, the if statement checks if parent.getCanonicalPath() -is a prefix of dir.getCanonicalPath(). However, parent.getCanonicalPath() is -not slash-terminated. So, the user that supplies dir may be allowed to access siblings of parent -and not just children of parent, which is a security issue. - -

- - - -

- -In this example, the if statement checks if parent.getCanonicalPath() + File.separator -is a prefix of dir.getCanonicalPath(). Because parent.getCanonicalPath().toPath() is -indeed slash-terminated, the user supplying dir can only access children of -parent, as desired. - -

- - - - - - -
  • -OWASP: -Partial Path Traversal. -CVE-2022-23457: - ESAPI Vulnerability Report -
    • - -         diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp new file mode 100644 index 00000000000..ef9802ae45d --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalFromRemote.qhelp @@ -0,0 +1,11 @@ + + + + + + + + + diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp new file mode 100644 index 00000000000..63f17afb3f6 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalOverview.inc.qhelp @@ -0,0 +1,10 @@ + + + +

    A common way to check that a user-supplied path SUBDIR falls inside a directory DIR +is to use getCanonicalPath() to remove any path-traversal elements and then check that DIR +is a prefix. However, if DIR is not slash-terminated, this can unexpectedly allow accessing siblings of DIR.

    + +     \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp new file mode 100644 index 00000000000..94562d332c6 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-023/PartialPathTraversalRemainder.inc.qhelp @@ -0,0 +1,51 @@ + + + + + +

    If the user should only access items within a certain directory DIR, ensure that DIR is slash-terminated +before checking that DIR is a prefix of the user-provided path, SUBDIR. Note, Java's getCanonicalPath() +returns a non-slash-terminated path string, so a slash must be added to DIR if that method is used.

    + +     + + +

    + + +In this example, the if statement checks if parent.getCanonicalPath() +is a prefix of dir.getCanonicalPath(). However, parent.getCanonicalPath() is +not slash-terminated. So, the user that supplies dir may be allowed to access siblings of parent +and not just children of parent, which is a security issue. + +

    + + + +

    + +In this example, the if statement checks if parent.getCanonicalPath() + File.separator +is a prefix of dir.getCanonicalPath(). Because parent.getCanonicalPath().toPath() is +indeed slash-terminated, the user supplying dir can only access children of +parent, as desired. + +

    + + + +     + + +
  • +OWASP: +Partial Path Traversal. +CVE-2022-23457: + ESAPI Vulnerability Report +
    • + +     + + +     \ No newline at end of file