From 47579fd5ae1c4a90a87d31c8aa591c420b00a185 Mon Sep 17 00:00:00 2001 From: ubuntu <43420907+dellalibera@users.noreply.github.com> Date: Fri, 3 Jul 2020 00:35:39 +0200 Subject: [PATCH 1/6] Add CodeQL query to detect Log Injection in Java code --- .../Security/CWE/CWE-117/LogInjection.help | 49 +++++++++++ .../Security/CWE/CWE-117/LogInjection.ql | 21 +++++ .../Security/CWE/CWE-117/LogInjection.qll | 84 +++++++++++++++++++ .../CWE/CWE-117/examples/LogInjection.java | 33 ++++++++ 4 files changed, 187 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help create mode 100644 java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qll create mode 100644 java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help new file mode 100644 index 000000000000..751deb05fffa --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help @@ -0,0 +1,49 @@ + + + + + +

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 for example +String replace(char oldChar, char newChar) 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 example, a username, provided by the user, is logged using `logger.warn` (from `org.slf4j.Logger`). +In the first case (`\bad` endpoint), the username is logged without any sanitization. +If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, +the log entry will be splitted in two different lines, where the first line will be `User:'Guest', while the second one will be `User:'Admin'`. +

+

In the second case (`\good` endpoint), replace is used to ensure no line endings are present in the user input. +If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, +the log entry will not be splitted in two different lines, resulting in a single line `User:'Guest'User:'Admin'`. +

+ + + + + +
  • OWASP: Log Injection.
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql new file mode 100644 index 000000000000..1b93990dabc4 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql @@ -0,0 +1,21 @@ +/** + * @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 java/log-injection + * @tags security + * external/cwe/cwe-117 + */ + +import java +import semmle.code.java.dataflow.FlowSources +import LogInjection::LogInjection +import DataFlow::PathGraph + +from LogInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Outputting $@ to log.", source.getNode(), + "sensitive information" diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qll b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qll new file mode 100644 index 000000000000..89675cf7e1f4 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qll @@ -0,0 +1,84 @@ +/** + * Provides a taint-tracking configuration for reasoning about untrusted user input used in log entries. + */ + +import java +import semmle.code.java.dataflow.FlowSources + +module LogInjection { + string logLevel() { + result = "trace" or + result = "info" or + result = "warn" or + result = "error" or + result = "fatal" + } + + /** + * 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 } + } + + /** + * A class of popular logging utilities. + */ + class LoggingType extends RefType { + LoggingType() { + this.hasQualifiedName("org.apache.logging.log4j", "Logger") or + this.hasQualifiedName("org.slf4j", "Logger") + } + } + + /** + * A method call to a logging mechanism. + */ + class LoggingCall extends MethodAccess { + LoggingCall() { + this.getMethod().getDeclaringType() instanceof LoggingType and + this.getMethod().hasName(logLevel()) + } + } + + /** + * An argument to a logging mechanism. + */ + class LoggingSink extends Sink { + LoggingSink() { this.asExpr() = any(LoggingCall console).getAnArgument() } + } + + class TypeSanitizer extends Sanitizer { + TypeSanitizer() { + this.getType() instanceof PrimitiveType or this.getType() instanceof BoxedType + } + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java b/java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java new file mode 100644 index 000000000000..7936f9c29656 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java @@ -0,0 +1,33 @@ +package com.example.restservice; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; + +@RestController +public class LogInjection { + + private final Logger log = LoggerFactory.getLogger(LogInjection.class); + + // /good?username=Guest'%0AUser:'Admin + @GetMapping("/good") + public String good(@RequestParam(value = "username", defaultValue = "name") String username) { + username = username.replace("\n", ""); + log.warn("User:'{}'", username); + return new String(username); + // User:'Guest'User:'Admin' + } + + // /bad?username=Guest'%0AUser:'Admin + @GetMapping("/bad") + public String bad(@RequestParam(value = "username", defaultValue = "name") String username) { + log.warn("User:'{}'", username); + return new String(username); + // User:'Guest' + // User:'Admin' + + } + +} From aa0dc9be01db87d16cd4a0c2cecb50cde524f8d0 Mon Sep 17 00:00:00 2001 From: ubuntu <43420907+dellalibera@users.noreply.github.com> Date: Fri, 3 Jul 2020 00:57:31 +0200 Subject: [PATCH 2/6] Change message --- java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql index 1b93990dabc4..22b7f26768f6 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql @@ -17,5 +17,5 @@ import DataFlow::PathGraph from LogInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink where cfg.hasFlowPath(source, sink) -select sink.getNode(), source, sink, "Outputting $@ to log.", source.getNode(), - "sensitive information" +select sink.getNode(), source, sink, "$@ flows to log entry.", source.getNode(), + "User-provided value" From bde715bebfa9e5f311524750915f972d2e5baf33 Mon Sep 17 00:00:00 2001 From: Alessio Della Libera <43420907+dellalibera@users.noreply.github.com> Date: Fri, 3 Jul 2020 19:51:30 +0200 Subject: [PATCH 3/6] Update java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help Co-authored-by: Marcono1234 --- .../src/experimental/Security/CWE/CWE-117/LogInjection.help | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help index 751deb05fffa..64758351b655 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help @@ -31,11 +31,11 @@ other forms of HTML injection.

    In the example, a username, provided by the user, is logged using `logger.warn` (from `org.slf4j.Logger`). -In the first case (`\bad` endpoint), the username is logged without any sanitization. +In the first case (`/bad` endpoint), the username is logged without any sanitization. If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, the log entry will be splitted in two different lines, where the first line will be `User:'Guest', while the second one will be `User:'Admin'`.

    -

    In the second case (`\good` endpoint), replace is used to ensure no line endings are present in the user input. +

    In the second case (`/good` endpoint), replace is used to ensure no line endings are present in the user input. If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, the log entry will not be splitted in two different lines, resulting in a single line `User:'Guest'User:'Admin'`.

    From e4a5154b3db286634c0cb906dd8b6de1188d5706 Mon Sep 17 00:00:00 2001 From: Alessio Della Libera <43420907+dellalibera@users.noreply.github.com> Date: Fri, 3 Jul 2020 19:52:20 +0200 Subject: [PATCH 4/6] Update java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help Co-authored-by: Marcono1234 --- java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help index 64758351b655..f43be0d05d58 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help @@ -33,7 +33,7 @@ other forms of HTML injection.

    In the example, a username, provided by the user, is logged using `logger.warn` (from `org.slf4j.Logger`). In the first case (`/bad` endpoint), the username is logged without any sanitization. If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, -the log entry will be splitted in two different lines, where the first line will be `User:'Guest', while the second one will be `User:'Admin'`. +the log entry will be splitted in two different lines, where the first line will be `User:'Guest'`, while the second one will be `User:'Admin'`.

    In the second case (`/good` endpoint), replace is used to ensure no line endings are present in the user input. If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, From 5ddb15fdd1e9debf1c24efa7a71c4cefbab2c65d Mon Sep 17 00:00:00 2001 From: ubuntu <43420907+dellalibera@users.noreply.github.com> Date: Fri, 3 Jul 2020 19:53:43 +0200 Subject: [PATCH 5/6] Update .qhelp --- .../src/experimental/Security/CWE/CWE-117/LogInjection.help | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help index f43be0d05d58..7c6f1fd4dc06 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help @@ -33,11 +33,11 @@ other forms of HTML injection.

    In the example, a username, provided by the user, is logged using `logger.warn` (from `org.slf4j.Logger`). In the first case (`/bad` endpoint), the username is logged without any sanitization. If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, -the log entry will be splitted in two different lines, where the first line will be `User:'Guest'`, while the second one will be `User:'Admin'`. +the log entry will be splitted in two separate lines, where the first line will be `User:'Guest'`, while the second one will be `User:'Admin'`.

    In the second case (`/good` endpoint), replace is used to ensure no line endings are present in the user input. If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, -the log entry will not be splitted in two different lines, resulting in a single line `User:'Guest'User:'Admin'`. +the log entry will not be splitted in two separate lines, resulting in a single line `User:'Guest'User:'Admin'`.

    From e9c84349a49fd9c468201585e93b92ac3e75e6c5 Mon Sep 17 00:00:00 2001 From: ubuntu <43420907+dellalibera@users.noreply.github.com> Date: Fri, 3 Jul 2020 19:56:14 +0200 Subject: [PATCH 6/6] Update example --- .../Security/CWE/CWE-117/examples/LogInjection.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java b/java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java index 7936f9c29656..09f02ce2a1f7 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java +++ b/java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java @@ -16,7 +16,7 @@ public class LogInjection { public String good(@RequestParam(value = "username", defaultValue = "name") String username) { username = username.replace("\n", ""); log.warn("User:'{}'", username); - return new String(username); + return username; // User:'Guest'User:'Admin' } @@ -24,7 +24,7 @@ public String good(@RequestParam(value = "username", defaultValue = "name") Stri @GetMapping("/bad") public String bad(@RequestParam(value = "username", defaultValue = "name") String username) { log.warn("User:'{}'", username); - return new String(username); + return username; // User:'Guest' // User:'Admin'