[javascript] CodeQL query to detect Log Injection

javascript/ql/src/experimental/Security/CWE-117/LogInjection.help

@@ -0,0 +1,47 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+
+<p>If unsanitized user input is written to a log entry, a malicious user may be able to forge new log entries.</p>
+
+<p>Forgery can occur if a user provides some input with characters that are interpreted
+when the log output is displayed. If the log is displayed as a plain text file, then new
+line characters can be used by a malicious user. If the log is displayed as HTML, then
+arbitrary HTML may be included to spoof log entries.</p>
+</overview>
+
+<recommendation>
+<p>
+User input should be suitably sanitized before it is logged.
+</p>
+<p>
+If the log entries are plain text then line breaks should be removed from user input, using
+<code>String.prototype.replace</code> or similar. Care should also be taken that user input is clearly marked
+in log entries, and that a malicious user cannot cause confusion in other ways.
+</p>
+<p>
+For log entries that will be displayed in HTML, user input should be HTML encoded before being logged, to prevent forgery and
+other forms of HTML injection.
+</p>
+
+</recommendation>
+
+<example>
+<p>In the first example, a username, provided by the user, is logged using `console.info`. In
+the first case, it is logged without any sanitization. In the second case the username is used to build an error that is logged using `console.error`.
+If a malicious user provides `username=Guest%0a[INFO]+User:+Admin%0a` as a username parameter, 
+the log entry will be splitted in two different lines, where the second line will be `[INFO]+User:+Admin`.
+</p>
+<sample src="examples/logInjectionBad.js" />
+
+<p> In the second example, <code>String.prototype.replace</code> is used to ensure no line endings are present in the user input.</p>
+<sample src="examples/logInjectionGood.js" />
+</example>
+
+<references>
+<li>OWASP: <a href="https://www.owasp.org/index.php/Log_Injection">Log Injection</a>.</li>
+</references>
+</qhelp>

javascript/ql/src/experimental/Security/CWE-117/LogInjection.ql

@@ -0,0 +1,20 @@
+/**
+ * @name Log Injection
+ * @description Building log entries from user-controlled sources is vulnerable to
+ *              insertion of forged log entries by a malicious user.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id js/log-injection
+ * @tags security
+ *       external/cwe/cwe-117
+ */
+
+import javascript
+import DataFlow::PathGraph
+import LogInjection::LogInjection
+
+from LogInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "$@ flows to log entry.", source.getNode(),
+  "User-provided value"

javascript/ql/src/experimental/Security/CWE-117/LogInjection.qll

@@ -0,0 +1,99 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about untrusted user input used in log entries.
+ */
+
+import javascript
+
+module LogInjection {
+  /**
+   * A data flow source for user input used in log entries.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for user input used in log entries.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for malicious user input used in log entries.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A taint-tracking configuration for untrusted user input used in log entries.
+   */
+  class LogInjectionConfiguration extends TaintTracking::Configuration {
+    LogInjectionConfiguration() { this = "LogInjection" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+  }
+
+  /**
+   * A source of remote user controlled input.
+   */
+  class RemoteSource extends Source {
+    RemoteSource() { this instanceof RemoteFlowSource }
+  }
+
+  /**
+   * An source node representing a logging mechanism.
+   */
+  class ConsoleSource extends DataFlow::SourceNode {
+    ConsoleSource() {
+      exists(DataFlow::SourceNode node |
+        node = this and this = DataFlow::moduleImport("console")
+        or
+        this = DataFlow::globalVarRef("console")
+      )
+    }
+  }
+
+  /**
+   * A call to a logging mechanism. For example, the call could be in the following forms:
+   * `console.log('hello')` or
+   *
+   * `let logger = console.log;`
+   * `logger('hello')` or
+   *
+   * `let logger = {info: console.log};`
+   * `logger.info('hello')`
+   */
+  class LoggingCall extends DataFlow::CallNode {
+    LoggingCall() {
+      exists(DataFlow::SourceNode node, string propName |
+        any(ConsoleSource console).getAPropertyRead() = node.getAPropertySource(propName) and
+        this = node.getAPropertyRead(propName).getACall()
+      )
+      or
+      this = any(LoggerCall call)
+    }
+  }
+
+  /**
+   * An argument to a logging mechanism.
+   */
+  class LoggingSink extends Sink {
+    LoggingSink() { this = any(LoggingCall console).getAnArgument() }
+  }
+
+  /**
+   * A call to `String.prototype.replace` that replaces `\n` is considered to sanitize the replaced string (reduce false positive).
+   */
+  class StringReplaceSanitizer extends Sanitizer {
+    StringReplaceSanitizer() {
+      exists(string s | this.(StringReplaceCall).replaces(s, "") and s.regexpMatch("\\n"))
+    }
+  }
+
+  /**
+   * A call to an HTML sanitizer is considered to sanitize the user input.
+   */
+  class HtmlSanitizer extends Sanitizer {
+    HtmlSanitizer() { this instanceof HtmlSanitizerCall }
+  }
+}

javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionBad.js

@@ -0,0 +1,68 @@
+const http = require('http');
+const hostname = '127.0.0.1';
+const port = 3000;
+const url = require('url');
+
+
+const check_username = (username) => {
+    if (username != 'name') throw `${username} is not valid`;
+    // do something
+}
+
+const my_logger = {
+    log: console.log
+}
+
+const another_logger = console.log
+
+// http://127.0.0.1:3000/data?username=Guest%0a[INFO]+User:+Admin%0a
+
+
+
+const server = http.createServer((req, res) => {
+    let q = url.parse(req.url, true);
+
+    let username = q.query.username;
+
+    // BAD: User input logged as-is
+    console.info(`[INFO] User: ${username}`);
+    // [INFO] User: Guest
+    // [INFO] User: Admin
+    //
+
+    // BAD: User input logged as-is
+    console.info(`[INFO] User: %s`, username);
+    // [INFO] User: Guest
+    // [INFO] User: Admin
+    //
+
+
+    // BAD: User input logged as-is
+    my_logger.log('[INFO] User:', username);
+    // [INFO] User: Guest
+    // [INFO] User: Admin
+    //
+
+    // BAD: User input logged as-is
+    another_logger('[INFO] User:', username);
+    // [INFO] User: Guest
+    // [INFO] User: Admin
+    //
+
+    try {
+        check_username(username)
+
+    } catch (error) {
+        // BAD: Error with user input logged as-is
+        console.error(`[ERROR] Error: "${error}"`);
+        // [ERROR] Error: "Guest
+        // [INFO] User: Admin
+        //  is not valid"
+
+    }
+
+})
+
+server.listen(port, hostname, () => {
+    console.log(`Server running at http://${hostname}:${port}/`);
+});

javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionGood.js

@@ -0,0 +1,51 @@
+const http = require('http');
+const hostname = '127.0.0.1';
+const port = 3000;
+const url = require('url');
+
+const check_username = (username) => {
+    if (username != 'name') throw `${username} is not valid`;
+    // do something
+}
+
+const logger = {
+    log: console.log
+}
+
+const another_logger = console.log
+
+// http://127.0.0.1:3000/data?username=Guest%0a[INFO]+User:+Admin%0a
+
+const server = http.createServer((req, res) => {
+    let q = url.parse(req.url, true);
+
+    // GOOD: remove `\n` line from user controlled input before logging
+    let username = q.query.username.replace(/\n/g, "");
+
+    console.info(`[INFO] User: ${username}`);
+    // [INFO] User: Guest[INFO] User: Admin
+
+    console.info(`[INFO] User: %s`, username);
+    // [INFO] User: Guest[INFO] User: Admin
+
+    logger.log('[INFO] User:', username);
+    // [INFO] User: Guest[INFO] User: Admin
+
+    another_logger('[INFO] User:', username);
+    // [INFO] User: Guest[INFO] User: Admin
+
+    try {
+        check_username(username)
+
+    } catch (error) {
+        console.error(`[ERROR] Error: "${error}"`);
+        // [ERROR] Error: "Guest[INFO] User: Admin is not valid"
+
+    }
+
+})
+
+server.listen(port, hostname, () => {
+    console.log(`Server running at http://${hostname}:${port}/`);
+});
+