[Java] CodeQL query to detect Log Injection #3882

dellalibera · 2020-07-02T22:39:17Z

Log Injection query is available in c# query, javascript (experimental) query but it is not available in java query.
I created a query to detect a log injection vulnerability in java code.

This query addresses the scenario where user controlled input is logged-as is.

Examples of scenarios detected:

    private final Logger log = LoggerFactory.getLogger(LogInjection.class);

    @GetMapping("/bad")
    public String bad(@RequestParam(value = "username", defaultValue = "name") String username) {
        log.warn("User:'{}'", username);
        return username;
    }

Any feedback is welcome.
I hope this will help.

intrigus-lgtm · 2020-07-02T22:47:09Z

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

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


I guess this is a left-over from another query?

Yes, you are right! I was inspired by a similar query (SensitiveInfoLog.ql). Thanks for pointing this out. I fixed the message.

Marcono1234

#3090 already added some logger types and #3600 will add more.
Maybe it would make sense to define logger types at one place (though in a later pull request?) to prevent code duplication.
For example what about:

abstract class LoggerType extends RefType {
  abstract Method getALoggingMethod();
}

Where getALoggingMethod() then returns the respective logging methods for that type.
What do the maintainers of this project think about this?

Marcono1234 · 2020-07-02T23:32:17Z

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

+<p>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'`.
+</p>
+<p> In the second case (`\good` endpoint), <code>replace</code> 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'`.


Does ` work for formatting text as code, or does <code>...</code> have to be used?
Either way, to be consistent, it would probably be good to use <code> for formatting.

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

Marcono1234 · 2020-07-02T23:34:43Z

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

+</p>
+<p> In the second case (`\good` endpoint), <code>replace</code> 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'`.


two different lines

"two separate lines" might be more correct

Thanks. I made the change.

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

Marcono1234 · 2020-07-02T23:42:02Z

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

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


splitted in two different lines

"two separate lines" might be more correct

Thanks. I made the change.

Marcono1234 · 2020-07-03T00:01:59Z

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

+import semmle.code.java.dataflow.FlowSources
+
+module LogInjection {
+  string logLevel() {


JBoss Logging also has ...f and ...v variants.

Marcono1234 · 2020-07-03T00:06:59Z

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

+   * A class of popular logging utilities.
+   */
+  class LoggingType extends RefType {
+    LoggingType() {


Would probably be good to cover the following ones as well:

Log4j1: org.apache.log4j.Category

Log4j2: org.apache.logging.log4j.LogBuilder

SLF4J: org.slf4j.spi.LoggingEventBuilder

Apache Commons Logging: org.apache.commons.logging.Log

JBoss Logging: org.jboss.logging.BasicLogger

And with that many types it might be good to add comments here explaining which type belongs to which framework, since especially for Log4j1, Log4j2 and Apache Commons Logging it can become quite confusing.

Marcono1234 · 2020-07-03T00:11:07Z

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

+
+  class TypeSanitizer extends Sanitizer {
+    TypeSanitizer() {
+      this.getType() instanceof PrimitiveType or this.getType() instanceof BoxedType


A primitive type could be '\n' which is still problematic and not a sanitizier I assume

Marcono1234 · 2020-07-03T00:12:10Z

java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java

+    // /good?username=Guest'%0AUser:'Admin
+    @GetMapping("/good")
+    public String good(@RequestParam(value = "username", defaultValue = "name") String username) {
+        username = username.replace("\n", "");


Maybe it would be good if a sanitizer would detect this.
Though it might be difficult to reduce false positives without introducing false negatives at the same time.

Marcono1234 · 2020-07-03T00:12:54Z

java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java

+    public String good(@RequestParam(value = "username", defaultValue = "name") String username) {
+        username = username.replace("\n", "");
+        log.warn("User:'{}'", username);
+        return new String(username);


Why return a new string (same in line 27)? String is immutable so there is no need to create a copy

You are right. I was doing some experiments and I forgot to change the return type. I made the change. Thanks for all the other feedback. I really appreciated. I will do the other changes/suggestions as soon as I will have some time (unfortunately I am a little bit busy at the moment).

Co-authored-by: Marcono1234 <[email protected]>

Marcono1234 · 2020-07-03T21:58:21Z

java/ql/src/experimental/Security/CWE/CWE-117/examples/LogInjection.java

+    // /good?username=Guest'%0AUser:'Admin
+    @GetMapping("/good")
+    public String good(@RequestParam(value = "username", defaultValue = "name") String username) {
+        username = username.replace("\n", "");


Replacing \n will likely not be enough, you probably also have to replace \r

dellalibera · 2020-07-26T21:13:46Z

Hi @Marcono1234,
sorry for the long delay.
I realized that I need more experience in Java CodeQL, and at this moment I don't have so much "free" time to spend on it.
Also during the "free" time that I have, I prefer to continue focusing on CodeQL for JS.

This mean that I will not be able to continue to work on this query in the next days/weeks/months.
Let me know if it's better to close this pull requests, for me there is no problem.
In this way I could reopen it later (if I will decide to work on it again) or maybe someone else could work on this.

Thank you for all the feedback and sorry for the inconvenience.

Marcono1234 · 2020-07-27T08:10:39Z

No problem and no worries. Note that I am not a member of this project so they might have different requirements for the queries. My review comments were mostly improvement suggestions.
I think leaving the pull request open for now should be fine (though maybe the project members disagree?). In case you consider the query incomplete yourself, you can also convert the pull request to a draft to indicate that.

smowton

Looks like a fine port of https://github.com/github/codeql/blob/main/csharp/ql/src/Security%20Features/CWE-117/LogForging.ql -- since my review suggests factoring in an existing logger definition with much more coverage, I'll wait for you to respond (either using it or opting to leave that until after merging to experimental) before I start an evaluation for this query.

smowton · 2020-10-20T11:22:45Z

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

+</example>
+
+<references>
+<li>OWASP: <a href="https://www.owasp.org/index.php/Log_Injection">Log Injection</a>.</li>


Recommend linking straight to the new URL (https://owasp.org/www-community/attacks/Log_Injection) -- perhaps update other links in other qhelp files at the same time.

smowton · 2020-10-20T11:23:43Z

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

+    result = "trace" or
+    result = "info" or
+    result = "warn" or
+    result = "error" or
+    result = "fatal"


Suggested change

result = "trace" or

result = "info" or

result = "warn" or

result = "error" or

result = "fatal"

result in ["trace", "info", "warn", "error", "fatal"]

smowton · 2020-10-20T11:35:11Z

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

+  /**
+   * A class of popular logging utilities.
+   */
+  class LoggingType extends RefType {


Instead of defining this, suggest factoring out https://github.com/github/codeql/blob/main/java/ql/src/experimental/Security/CWE/CWE-532/SensitiveInfoLog.ql#L31 as well as the logging methods used there (though we may not need the restriction to only low-priority messages seen in that query)

I'd say it belongs at java/ql/src/semmle/code/java/dataflow/FlowSinks.qll, alongside the existing FlowSources.qll.

smowton · 2020-10-20T11:40:08Z

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

+   * An argument to a logging mechanism.
+   */
+  class LoggingSink extends Sink {
+    LoggingSink() { this.asExpr() = any(LoggingCall console).getAnArgument() }


Suggested change

LoggingSink() { this.asExpr() = any(LoggingCall console).getAnArgument() }

LoggingSink() { this.asExpr() = any(LoggingCall lc).getAnArgument() }

Since it's not necessarily a console

smowton · 2020-11-06T13:55:58Z

@dellalibera are you still working on this one?

dellalibera · 2020-11-06T17:24:13Z

@smowton I apologize for the late reply. At the moment I'm very busy. To answer your question: I will not have enough time now to work on this PR (and also on others). Feel free to close it or reuse the code as a starting point for another PR.

smowton · 2020-11-07T10:46:41Z

If you're likely to come back to it at some point I'm happy to leave open; if not then you can close it.

dellalibera · 2020-11-08T10:32:13Z

Ok great. If in the next few months I will not come back on it, I will let know so we can close it. But for now, for me it's ok to leave it open.

atorralba · 2021-11-04T07:49:25Z

Superseded by #5099.

Add CodeQL query to detect Log Injection in Java code

47579fd

dellalibera requested a review from a team as a code owner July 2, 2020 22:39

dellalibera mentioned this pull request Jul 2, 2020

[Java] CWE-117: CodeQL query to detect Log Injection github/securitylab#144

Open

1 task

intrigus-lgtm reviewed Jul 2, 2020

View reviewed changes

Change message

aa0dc9b

Marcono1234 reviewed Jul 3, 2020

View reviewed changes

aschackmull added the Java label Jul 3, 2020

dellalibera and others added 4 commits July 3, 2020 19:51

Update java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help

bde715b

Co-authored-by: Marcono1234 <[email protected]>

Update java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.help

e4a5154

Co-authored-by: Marcono1234 <[email protected]>

Update .qhelp

5ddb15f

Update example

e9c8434

Marcono1234 reviewed Jul 3, 2020

View reviewed changes

adityasharad changed the base branch from master to main August 14, 2020 18:33

smowton reviewed Oct 20, 2020

View reviewed changes

smowton self-assigned this Oct 20, 2020

porcupineyhairs mentioned this pull request Feb 4, 2021

Java : Add Log Injection Vulnerability #5099

Merged

atorralba closed this Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java] CodeQL query to detect Log Injection #3882

[Java] CodeQL query to detect Log Injection #3882

dellalibera commented Jul 2, 2020

intrigus-lgtm Jul 2, 2020

dellalibera Jul 2, 2020

Marcono1234 left a comment

Marcono1234 Jul 2, 2020

Marcono1234 Jul 2, 2020

dellalibera Jul 3, 2020

Marcono1234 Jul 2, 2020

dellalibera Jul 3, 2020

Marcono1234 Jul 3, 2020

Marcono1234 Jul 3, 2020

Marcono1234 Jul 3, 2020

Marcono1234 Jul 3, 2020 •

edited

Loading

Marcono1234 Jul 3, 2020

dellalibera Jul 3, 2020

Marcono1234 Jul 3, 2020

dellalibera commented Jul 26, 2020

Marcono1234 commented Jul 27, 2020

smowton left a comment

smowton Oct 20, 2020

smowton Oct 20, 2020

smowton Oct 20, 2020

smowton Oct 20, 2020

smowton commented Nov 6, 2020

dellalibera commented Nov 6, 2020

smowton commented Nov 7, 2020

dellalibera commented Nov 8, 2020

atorralba commented Nov 4, 2021

	LoggingSink() { this.asExpr() = any(LoggingCall console).getAnArgument() }
	LoggingSink() { this.asExpr() = any(LoggingCall lc).getAnArgument() }

[Java] CodeQL query to detect Log Injection #3882

[Java] CodeQL query to detect Log Injection #3882

Conversation

dellalibera commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marcono1234 left a 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

Marcono1234 Jul 3, 2020 • edited Loading

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

dellalibera commented Jul 26, 2020

Marcono1234 commented Jul 27, 2020

smowton left a 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

smowton commented Nov 6, 2020

dellalibera commented Nov 6, 2020

smowton commented Nov 7, 2020

dellalibera commented Nov 8, 2020

atorralba commented Nov 4, 2021

Marcono1234 Jul 3, 2020 •

edited

Loading