JS: Taint analysis for win paths #7968

kaeluka · 2022-02-11T08:56:39Z

This contributes support for tracking win32 paths in tainted path queries.

The main difference is, that you can now extend query labels by extending the Platform class (as in TaintedWindowsPaths.ql) and path will then be tracked using 8 labels, instead of four.

By default, there's the four labels as before:

normalized-relative-posix-path
normalized-absolute-posix-path
raw-relative-posix-path
raw-absolute-posix-path

When extending the Platform class by a win32 variant, you'll additionally get:

normalized-relative-win32-path
normalized-absolute-win32-path
raw-relative-win32-path
raw-absolute-win32-path.

I also refactored the isAdditionalTaintStep predicate that would have — IMHO — become too large after this change by pulling out much of the logic into an abstract class and extensions thereof.

I ran an experiment, and looked at it with @esbena looking over my shoulder. It's not affecting the results of pre-existing queries. The results it finds for the new queries lgtm (I've looked at a subset of results from various projects).

javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

kaeluka · 2022-03-04T09:16:38Z

@erik-krogh has asked me to split the first commit into 3:

with the refactoring into a module
adding the platform class that creates new paths
adding the logic

javascript/ql/lib/change-notes/2022-03-04-tainted-windows-paths.md

javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

erik-krogh · 2022-03-10T14:14:29Z

javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

-  /**
-   * A call that preserves taint without changing the flow label.
-   */
-  class PreservingPathCall extends DataFlow::CallNode {
-    DataFlow::Node input;
-    DataFlow::Node output;
+    /** A call to the path.join method. */
+    class JoinPathCall extends PathTransformationNode {
+      JoinPathCall() {
+        this = NodeJSLib::Path::moduleMember("join").getACall() and
+        input = this.getAnArgument() and
+        output = this
+      }

-    PreservingPathCall() {
-      this =
-        NodeJSLib::Path::moduleMember(["dirname", "toNamespacedPath", "parse", "format"]).getACall() and


I'm confused by what you've done with this class.

It seems to me that you've changed the behavior of the first case (pasted below) in this "refactoring" commit.

NodeJSLib::Path::moduleMember(["dirname", "toNamespacedPath", "parse", "format"]).getACall() and input = this.getAnArgument() and output = this

This class was used in the isPosixPathStep predicate to just forward a step while preserving the flow-labels.

It seems to me that you've changed it.

The "dirname" and "format" case is in the PreservingPathCall class, with the same behavior.
The "toNamespacedPath" case is in ToNamespacedPathCall, with the same behavior.

But the "parse" case is in a new class ParseCall with different behavior.
You've added some property checks based on ["root", "dir"]

I don't think that check works, but that's another matter. (CallNode::getALocalUse() can never be a PropRead).

Sorry this reply took so long, I got really bogged down with other work and then time off.

What I tried to do here was to track through parse more precisely (it gives you an object which contains the root, dir, extension, baseName of a path). In principle, we may add more precise tracking of those individual properties — but that's neither a refactoring, nor what this PR really needs to be adding.

As the PR big enough on its own, I've amend the refactoring PR to remove the class ParseCall altogether (https://github.com/github/codeql/compare/7193fd691a391fb2f27db708b7877c648f797589..8b817fe555a0fdee7caed60ba44be1fc11ac39b7) and moved parse back to its friends dirname and format.

Thank you 👍

…rder to be able to track windows paths

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

erik-krogh · 2022-03-30T07:51:50Z

javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

-        exists(RegExpSequence seq | seq.getNumChild() = 2 and literal.getRoot() = seq |
-          seq.getChild(0) instanceof RegExpCaret and
-          seq.getChild(1) = term
+    class ToNamespacedPathCall extends PathTransformationNode {


This should be part of PreservingPathCall (like it was before).

erik-krogh · 2022-03-30T08:06:30Z

javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

-      output = this.getCallback(1).getParameter(1)
+  module PathTransformations {
+    /** An abstract description of what kind of paths a call may return. */
+    abstract class PathTransformationNode extends DataFlow::CallNode {


Suggested change

abstract class PathTransformationNode extends DataFlow::CallNode {

abstract class PathTransformationCall extends DataFlow::CallNode {

?

erik-krogh · 2022-03-30T08:07:49Z

javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

+  predicate isPathStep(
+    DataFlow::Node src, DataFlow::Node dst, Path::PathLabel srclabel, Path::PathLabel dstlabel
+  ) {
+    exists(PathTransformationNode ptn | ptn.step(srclabel, src, dstlabel, dst))
+  }


I think you should just inline this predicate into the place it's used.
With your refactoring the predicate has become a predicate for taint-steps that originate from some call, which is not reflected in the name/qldoc.

erik-krogh · 2022-03-30T08:11:03Z

javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

+    bindingset[s]
+    predicate isDrivePath(string s) { s.regexpMatch("^[a-zA-Z]:\\\\*") }
+
+    /**


Suggested change

/**

/** DEPRECATED: Use `PathLabel`. */

abstract deprecated class PosixLabel extends PathLabel {

PosixLabel() { platform = platformPosix() }

}

/**

erik-krogh · 2022-03-30T08:11:47Z

javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

@@ -33,7 +36,38 @@ module TaintedPath {
   */
  abstract class BarrierGuardNode extends DataFlow::LabeledBarrierGuardNode { }

-  module Label {
+  /**


Suggested change

/**

/** DEPRECATED: Use `Path`. */

deprecated module Label = Path;

/**

erik-krogh · 2022-03-30T08:34:07Z

javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

@@ -80,499 +132,596 @@ module TaintedPath {
      predicate isAbsolute() { relativeness = "absolute" }

      /** Gets the path label with normalized flag set to true. */
-      PosixPath toNormalized() {
+      PathLabel toNormalized() {


I think all of these utility predicates should preserve the platform
(add this.getPlatform() = result.getPlatform() to toNormalized, toNonNormalized, toAbsolute, toRelative`).

I think your NormalizingPathCall doesn't preserve the platform due to you not preserving the platform in these utility predicates.
(The rest look OK).

erik-krogh · 2022-03-30T08:38:57Z

javascript/ql/src/Security/CWE-022/TaintedWindowsPath.ql

+import javascript
+import semmle.javascript.security.dataflow.TaintedPathQuery
+import semmle.javascript.security.dataflow.TaintedPathCustomizations
+import DataFlow::PathGraph
+import semmle.javascript.frameworks.NodeJSLib


Suggested change

import javascript

import semmle.javascript.security.dataflow.TaintedPathQuery

import semmle.javascript.security.dataflow.TaintedPathCustomizations

import DataFlow::PathGraph

import semmle.javascript.frameworks.NodeJSLib

import javascript

import semmle.javascript.security.dataflow.TaintedPathQuery

import DataFlow::PathGraph

Note to self: add this as an example to the ql/redundant-import ticket.

erik-krogh · 2022-03-30T08:40:03Z

javascript/ql/src/Security/CWE-022/TaintedWindowsPath.ql

+
+class Win32Platform extends Path::Platform {


Suggested change

class Win32Platform extends Path::Platform {

/** Materializes the Win32 platform, which enables the Win32 tainted-path steps */

class Win32Platform extends Path::Platform {

erik-krogh · 2022-03-30T08:50:51Z

javascript/ql/test/query-tests/Security/CWE-022/TaintedWindowsPath/TaintedWindowsPath.qlref

@@ -0,0 +1 @@
+Security/CWE-022/TaintedWindowsPath.ql


I would like a test of which taint-flows are only an issue on Windows.

I've written it for you here:

import javascript import semmle.javascript.security.dataflow.TaintedPathQuery // TODO: Move this to TaintedWindowsPathQuery.qll? class Win32Platform extends Path::Platform { Win32Platform() { this = Path::platformWin32() } override string getSep() { result = "\\" } } from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink where cfg.hasFlowPath(source, sink) and sink.getFlowLabel().(Path::PathLabel).getPlatform() = Path::platformWin32() and not sink.getFlowLabel().(Path::PathLabel).getPlatform() = Path::platformPosix() select source, sink

It detects that express.js:27 is only an issue on windows (as expected).
But it doesn't flag express.js:35. I haven't looked into why.

erik-krogh · 2022-03-30T08:52:14Z

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/express.js

+
+app.get("/some/path/2", function (req, res) {
+  const p = path.join('.', req.query.filename);
+  if (!p.startsWith('..')) {
+    req.files.foo.mv(p); // OK (but not on windows)
+    res.status(200);
+  } else {
+    res.status(400);
+  }
+});


Could you move this test to the Windows-path test folder?
With the test I suggested we can actually test that req.files.foo.mv(p) is only flagged by the windows query.

kaeluka · 2022-07-27T07:03:39Z

Talked this over with Calum; I'll drop this for the time being.

github-actions bot added the JS label Feb 11, 2022

kaeluka changed the title ~~JS: Taint analysis for win paths~~ JS: Taint analysis for win paths [DRAFT] Feb 11, 2022

erik-krogh reviewed Feb 11, 2022

View reviewed changes

javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll Outdated Show resolved Hide resolved

github-actions bot added the documentation label Mar 3, 2022

kaeluka changed the title ~~JS: Taint analysis for win paths [DRAFT]~~ JS: Taint analysis for win paths Mar 4, 2022

kaeluka marked this pull request as ready for review March 4, 2022 09:00

kaeluka requested a review from a team as a code owner March 4, 2022 09:00

erik-krogh reviewed Mar 8, 2022

View reviewed changes

erik-krogh reviewed Mar 10, 2022

View reviewed changes

kaeluka and others added 9 commits March 25, 2022 09:49

modularize tainted path customizations (refactoring)

dd45cfd

add the platform property to js tainted path query's flow labels in o…

2dd1523

…rder to be able to track windows paths

logic changes for tracking tainted windows paths

265cb47

Tainted windows path query and test

47a9ccc

tainted path logic for path.win32.join

7b702ff

documentation for js/path-injection-win32-enabled

890b907

only use input and output nodes where needed

9639bef

comment minor classes and predicates

8b817fe

add explicit this.

20323eb

Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>

erik-krogh reviewed Mar 30, 2022

View reviewed changes

erik-krogh mentioned this pull request Apr 25, 2022

Remove redundant imports #8796

Merged

kaeluka marked this pull request as draft May 9, 2022 12:42

kaeluka closed this Jul 27, 2022

Apr	MAY	Jun
	21
2024	2025	2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS: Taint analysis for win paths #7968

JS: Taint analysis for win paths #7968

kaeluka commented Feb 11, 2022 •

edited

Loading

kaeluka commented Mar 4, 2022

erik-krogh Mar 10, 2022

kaeluka Mar 25, 2022

erik-krogh Mar 30, 2022

erik-krogh Mar 30, 2022

erik-krogh Mar 30, 2022

erik-krogh Mar 30, 2022

erik-krogh Mar 30, 2022

erik-krogh Mar 30, 2022

erik-krogh Mar 30, 2022

erik-krogh Mar 30, 2022

erik-krogh Mar 30, 2022

erik-krogh Mar 30, 2022

erik-krogh Mar 30, 2022

kaeluka commented Jul 27, 2022

	abstract class PathTransformationNode extends DataFlow::CallNode {
	abstract class PathTransformationCall extends DataFlow::CallNode {

-    /**
+    /** DEPRECATED: Use `PathLabel`. */
+    abstract deprecated class PosixLabel extends PathLabel {
+      PosixLabel() { platform = platformPosix() }
+    }
+    /**

-  /**
+  /** DEPRECATED: Use `Path`. */
+  deprecated module Label = Path;
+  /**

		@@ -0,0 +1 @@
		Security/CWE-022/TaintedWindowsPath.ql

JS: Taint analysis for win paths #7968

JS: Taint analysis for win paths #7968

Conversation

kaeluka commented Feb 11, 2022 • edited Loading

kaeluka commented Mar 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaeluka commented Jul 27, 2022

kaeluka commented Feb 11, 2022 •

edited

Loading