From 17d7ba80496ac5c4abb29e44d0a4a8c106407c90 Mon Sep 17 00:00:00 2001 From: Porcuiney Hairs Date: Tue, 2 Feb 2021 23:58:32 +0530 Subject: [PATCH 1/5] Add Log Injection Vulnerability --- .../Security/CWE/CWE-117/LogInjection.qhelp | 49 ++++++++++++++ .../Security/CWE/CWE-117/LogInjection.ql | 67 +++++++++++++++++++ .../Security/CWE/CWE-117/LogInjectionBad.java | 24 +++++++ .../CWE/CWE-117/LogInjectionGood.java | 25 +++++++ .../Security/CWE/CWE-532/SensitiveInfoLog.ql | 31 ++------- .../experimental/semmle/code/java/Logging.qll | 30 +++++++++ 6 files changed, 199 insertions(+), 27 deletions(-) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-117/LogInjectionBad.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-117/LogInjectionGood.java create mode 100644 java/ql/src/experimental/semmle/code/java/Logging.qll diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp new file mode 100644 index 000000000000..4ad7b2d5380d --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp @@ -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 creating the appearance of multiple + log entries. This can include unescaped new-line characters, or HTML or other markup.

+ + + +

+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 split into two separate lines, where the first line will be User:'Guest' and the second one will be User:'Admin'. +

+ + +

In the second case (/good endpoint), matches() is used to ensure the user input only has alphanumeric characters. + If a malicious user provides `Guest'%0AUser:'Admin` as a username parameter, + the log entry will not be split into two separate 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..440c39b77e8a --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql @@ -0,0 +1,67 @@ +/** + * @name Log Injection + * @description Building log entries from user-controlled data 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 DataFlow::PathGraph +import experimental.semmle.code.java.Logging +import semmle.code.java.dataflow.FlowSources + +/** + * A taint-tracking configuration for tracking untrusted user input used in log entries. + */ +private class LogInjectionConfiguration extends TaintTracking::Configuration { + LogInjectionConfiguration() { this = "Log Injection" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { + sink.asExpr() = any(LoggingCall c).getALogArgument() + } + + override predicate isSanitizer(DataFlow::Node node) { + node.getType() instanceof BoxedType or node.getType() instanceof PrimitiveType + } + + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { + guard instanceof StrCheckSanitizerGuard + } +} + +/** + * Models any regex or equality check as a sanitizer guard. + * Assumes any check on the taint to be a valid sanitizing check. + */ +private class StrCheckSanitizerGuard extends DataFlow::BarrierGuard { + StrCheckSanitizerGuard() { + exists(Method m | + m.getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and + m.hasName("matches") + or + m.getDeclaringType() instanceof TypeString and + m.hasName([ + "startsWith", "regionMatches", "matches", "equals", "equalsIgnoreCase", "endsWith", + "contentEquals", "contains" + ]) + | + m.getAReference() = this + ) + } + + override predicate checks(Expr e, boolean branch) { + e = this.(MethodAccess).getQualifier() and branch = true + } +} + +from LogInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "$@ flows to log entry.", source.getNode(), + "User-provided value" diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjectionBad.java b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjectionBad.java new file mode 100644 index 000000000000..620b98016317 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjectionBad.java @@ -0,0 +1,24 @@ +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); + + // /bad?username=Guest'%0AUser:'Admin + @GetMapping("/bad") + public String bad(@RequestParam(value = "username", defaultValue = "name") String username) { + log.warn("User:'{}'", username); + // The logging call above would result in multiple log entries as shown below: + // User:'Guest' + // User:'Admin' + return username; + } +} + diff --git a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjectionGood.java b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjectionGood.java new file mode 100644 index 000000000000..2ed683a27602 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjectionGood.java @@ -0,0 +1,25 @@ +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) { + // The regex check here, allows only alphanumeric characters to pass. + // Hence, does not result in log injection + if (username.matches("\w*")) { + log.warn("User:'{}'", username); + + return username; + } + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql b/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql index 15690b7c32bc..853bbb6bace5 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql @@ -10,6 +10,7 @@ import java import semmle.code.java.dataflow.TaintTracking import semmle.code.java.security.SensitiveActions +import experimental.semmle.code.java.Logging import DataFlow import PathGraph @@ -27,38 +28,14 @@ class CredentialExpr extends Expr { } } -/** Class of popular logging utilities * */ -class LoggerType extends RefType { - LoggerType() { - this.hasQualifiedName("org.apache.log4j", "Category") or //Log4J - this.hasQualifiedName("org.apache.logging.log4j", "Logger") or //Log4J 2 - this.hasQualifiedName("org.slf4j", "Logger") or //SLF4j and Gradle Logging - this.hasQualifiedName("org.jboss.logging", "BasicLogger") or //JBoss Logging - this.hasQualifiedName("org.jboss.logging", "Logger") or //JBoss Logging (`org.jboss.logging.Logger` in some implementations like JBoss Application Server 4.0.4 did not implement `BasicLogger`) - this.hasQualifiedName("org.apache.commons.logging", "Log") or //Apache Commons Logging - this.hasQualifiedName("org.scijava.log", "Logger") //SciJava Logging - } -} - -predicate isSensitiveLoggingSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod().getDeclaringType() instanceof LoggerType and - ( - ma.getMethod().hasName("debug") or - ma.getMethod().hasName("trace") or - ma.getMethod().hasName("debugf") or - ma.getMethod().hasName("debugv") - ) and //Check low priority log levels which are more likely to be real issues to reduce false positives - sink.asExpr() = ma.getAnArgument() - ) -} - class LoggerConfiguration extends DataFlow::Configuration { LoggerConfiguration() { this = "Logger Configuration" } override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof CredentialExpr } - override predicate isSink(DataFlow::Node sink) { isSensitiveLoggingSink(sink) } + override predicate isSink(DataFlow::Node sink) { + exists(LoggingCall c | sink.asExpr() = c.getALogArgument()) + } override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { TaintTracking::localTaintStep(node1, node2) diff --git a/java/ql/src/experimental/semmle/code/java/Logging.qll b/java/ql/src/experimental/semmle/code/java/Logging.qll new file mode 100644 index 000000000000..6e50dc163bcf --- /dev/null +++ b/java/ql/src/experimental/semmle/code/java/Logging.qll @@ -0,0 +1,30 @@ +/** + * Provides classes and predicates for working with loggers. + */ + +import java + +/** Models a call to a logging method. */ +class LoggingCall extends MethodAccess { + LoggingCall() { + exists(RefType t, Method m | + t.hasQualifiedName("org.apache.log4j", "Category") or // Log4j 1 + t.hasQualifiedName("org.apache.logging.log4j", ["Logger", "LogBuilder"]) or // Log4j 2 + t.hasQualifiedName("org.apache.commons.logging", "Log") or + // JBoss Logging (`org.jboss.logging.Logger` in some implementations like JBoss Application Server 4.0.4 did not implement `BasicLogger`) + t.hasQualifiedName("org.jboss.logging", ["BasicLogger", "Logger"]) or + t.hasQualifiedName("org.slf4j.spi", "LoggingEventBuilder") or + t.hasQualifiedName("org.slf4j", "Logger") or + t.hasQualifiedName("org.scijava.log", "Logger") or + t.hasQualifiedName("java.lang", "System$Logger") or + t.hasQualifiedName("java.util.logging","Logger") + | + m.getDeclaringType().(RefType).extendsOrImplements*(t) and + m.getReturnType() instanceof VoidType and + this = m.getAReference() + ) + } + + /** Returns an argument which would be logged by this call. */ + Argument getALogArgument() { result = this.getArgument(_) } +} From d0c82d3756e78678e981b301215d0f7120acb4ce Mon Sep 17 00:00:00 2001 From: Porcuiney Hairs Date: Wed, 3 Mar 2021 03:42:21 +0530 Subject: [PATCH 2/5] Add flogger and android logging support --- java/ql/src/experimental/semmle/code/java/Logging.qll | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/java/ql/src/experimental/semmle/code/java/Logging.qll b/java/ql/src/experimental/semmle/code/java/Logging.qll index 6e50dc163bcf..c99334cdedd6 100644 --- a/java/ql/src/experimental/semmle/code/java/Logging.qll +++ b/java/ql/src/experimental/semmle/code/java/Logging.qll @@ -9,17 +9,22 @@ class LoggingCall extends MethodAccess { LoggingCall() { exists(RefType t, Method m | t.hasQualifiedName("org.apache.log4j", "Category") or // Log4j 1 - t.hasQualifiedName("org.apache.logging.log4j", ["Logger", "LogBuilder"]) or // Log4j 2 + t.hasQualifiedName("org.apache.logging.log4j", ["Logger", "LogBuilder"]) or // Log4j 2 t.hasQualifiedName("org.apache.commons.logging", "Log") or // JBoss Logging (`org.jboss.logging.Logger` in some implementations like JBoss Application Server 4.0.4 did not implement `BasicLogger`) t.hasQualifiedName("org.jboss.logging", ["BasicLogger", "Logger"]) or t.hasQualifiedName("org.slf4j.spi", "LoggingEventBuilder") or t.hasQualifiedName("org.slf4j", "Logger") or t.hasQualifiedName("org.scijava.log", "Logger") or + t.hasQualifiedName("com.google.common.flogger", "LoggingApi") or t.hasQualifiedName("java.lang", "System$Logger") or - t.hasQualifiedName("java.util.logging","Logger") + t.hasQualifiedName("java.util.logging", "Logger") or + t.hasQualifiedName("android.util.Log", _) | - m.getDeclaringType().(RefType).extendsOrImplements*(t) and + ( + m.getDeclaringType().getASourceSupertype*() = t or + m.getDeclaringType().(RefType).extendsOrImplements*(t) + ) and m.getReturnType() instanceof VoidType and this = m.getAReference() ) From f27d2bdf6d3ebbb37bb006d249494b96cfafadf9 Mon Sep 17 00:00:00 2001 From: porcupineyhairs <61983466+porcupineyhairs@users.noreply.github.com> Date: Wed, 3 Mar 2021 12:36:01 +0530 Subject: [PATCH 3/5] Update java/ql/src/experimental/semmle/code/java/Logging.qll Co-authored-by: Marcono1234 --- java/ql/src/experimental/semmle/code/java/Logging.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/semmle/code/java/Logging.qll b/java/ql/src/experimental/semmle/code/java/Logging.qll index c99334cdedd6..7a70cd8149b4 100644 --- a/java/ql/src/experimental/semmle/code/java/Logging.qll +++ b/java/ql/src/experimental/semmle/code/java/Logging.qll @@ -19,7 +19,7 @@ class LoggingCall extends MethodAccess { t.hasQualifiedName("com.google.common.flogger", "LoggingApi") or t.hasQualifiedName("java.lang", "System$Logger") or t.hasQualifiedName("java.util.logging", "Logger") or - t.hasQualifiedName("android.util.Log", _) + t.hasQualifiedName("android.util", "Log") | ( m.getDeclaringType().getASourceSupertype*() = t or From 84c9137152a4533393bea86292debb3dad1b4fff Mon Sep 17 00:00:00 2001 From: Porcuiney Hairs Date: Wed, 3 Mar 2021 17:27:56 +0530 Subject: [PATCH 4/5] Include suggestions from review --- java/ql/src/experimental/semmle/code/java/Logging.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/experimental/semmle/code/java/Logging.qll b/java/ql/src/experimental/semmle/code/java/Logging.qll index 7a70cd8149b4..b3444972710e 100644 --- a/java/ql/src/experimental/semmle/code/java/Logging.qll +++ b/java/ql/src/experimental/semmle/code/java/Logging.qll @@ -23,7 +23,7 @@ class LoggingCall extends MethodAccess { | ( m.getDeclaringType().getASourceSupertype*() = t or - m.getDeclaringType().(RefType).extendsOrImplements*(t) + m.getDeclaringType().extendsOrImplements*(t) ) and m.getReturnType() instanceof VoidType and this = m.getAReference() From a88c3682ff429dddb09dfd826c9c5efb4bc2c593 Mon Sep 17 00:00:00 2001 From: Porcuiney Hairs Date: Thu, 18 Mar 2021 16:11:27 +0530 Subject: [PATCH 5/5] remove sanitiserGuards --- .../Security/CWE/CWE-117/LogInjection.ql | 29 ------------------- 1 file changed, 29 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 440c39b77e8a..7183c74b5bf7 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql @@ -30,35 +30,6 @@ private class LogInjectionConfiguration extends TaintTracking::Configuration { override predicate isSanitizer(DataFlow::Node node) { node.getType() instanceof BoxedType or node.getType() instanceof PrimitiveType } - - override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { - guard instanceof StrCheckSanitizerGuard - } -} - -/** - * Models any regex or equality check as a sanitizer guard. - * Assumes any check on the taint to be a valid sanitizing check. - */ -private class StrCheckSanitizerGuard extends DataFlow::BarrierGuard { - StrCheckSanitizerGuard() { - exists(Method m | - m.getDeclaringType().hasQualifiedName("java.util.regex", "Pattern") and - m.hasName("matches") - or - m.getDeclaringType() instanceof TypeString and - m.hasName([ - "startsWith", "regionMatches", "matches", "equals", "equalsIgnoreCase", "endsWith", - "contentEquals", "contains" - ]) - | - m.getAReference() = this - ) - } - - override predicate checks(Expr e, boolean branch) { - e = this.(MethodAccess).getQualifier() and branch = true - } } from LogInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink