diff --git a/javascript/ql/src/experimental/Security/CWE-117/LogInjection.help b/javascript/ql/src/experimental/Security/CWE-117/LogInjection.help new file mode 100644 index 000000000000..b125d3beadb3 --- /dev/null +++ b/javascript/ql/src/experimental/Security/CWE-117/LogInjection.help @@ -0,0 +1,47 @@ + + + + + +

If unsanitized user input is written to a log entry, a malicious user may be able to forge new log entries.

+ +

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.

+
+ + +

+User input should be suitably sanitized before it is logged. +

+

+If the log entries are plain text then line breaks should be removed from user input, using +String.prototype.replace 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. +

+

+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. +

+ +
+ + +

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`. +

+ + +

In the second example, String.prototype.replace is used to ensure no line endings are present in the user input.

+ +
+ + +
  • OWASP: Log Injection.
  • +
    +
    diff --git a/javascript/ql/src/experimental/Security/CWE-117/LogInjection.ql b/javascript/ql/src/experimental/Security/CWE-117/LogInjection.ql new file mode 100644 index 000000000000..0ccab98798c2 --- /dev/null +++ b/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" diff --git a/javascript/ql/src/experimental/Security/CWE-117/LogInjection.qll b/javascript/ql/src/experimental/Security/CWE-117/LogInjection.qll new file mode 100644 index 000000000000..0ac5ee4f3618 --- /dev/null +++ b/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 } + } +} diff --git a/javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionBad.js b/javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionBad.js new file mode 100644 index 000000000000..9d2c81ba0234 --- /dev/null +++ b/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}/`); +}); diff --git a/javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionGood.js b/javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionGood.js new file mode 100644 index 000000000000..6be10534c707 --- /dev/null +++ b/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}/`); +}); +