Python: CWE-117 Log injection #6182

haby0 · 2021-06-29T12:53:00Z

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

RasmusWL · 2021-06-30T13:00:42Z

Hi @haby0, thanks for this contribution 👍 We'll take a look at this as soon, but we do have a few other things we're also working on at the moment 😊

haby0 · 2021-06-30T13:21:48Z

Hi @haby0, thanks for this contribution 👍 We'll take a look at this as soon, but we do have a few other things we're also working on at the moment 😊

Ok, thanks for your reply.

haby0 · 2021-09-15T03:15:19Z

@yoff The conflict has been resolved, please review, thank you.

yoff

You are not handling HTML logging and encoding, did you plan to? It might require a second configuration, seeing as the sinks dictate which sanitizers are required...

yoff · 2021-09-15T13:05:48Z

python/ql/src/experimental/semmle/python/frameworks/Log.qll

+   * See https://docs.python.org/3/library/logging.html#logger-objects
+   */
+  private class LogOutputMethods extends string {
+    LogOutputMethods() { this in ["info", "error", "warn", "warning", "debug", "critical"] }


We seem to be missing "debug", "log" , and "exception" from this list.

Pr updated.

yoff · 2021-09-15T13:14:56Z

python/ql/src/experimental/semmle/python/frameworks/Log.qll

+      this = API::moduleImport("logging").getMember(any(LogOutputMethods m)).getACall()
+    }
+
+    override DataFlow::Node getAnInput() { result = this.getArg(_) }


Note that getArg(_), perhaps confusingly, will not return all arguments. In particular, starred arguments are filtered out, as are keyword only arguments. This means that for info(msg, *args, **kwargs) you will see neither args, nor kwargs.

Note also, that if you add log, its first argument is level which should probably not be considered an input.

Pr updated.

yoff · 2021-09-15T13:41:16Z

python/ql/src/experimental/semmle/python/security/injection/LogInjection.qll

+  override predicate isSanitizer(DataFlow::Node node) {
+    exists(CallNode call |
+      node.asCfgNode() = call.getFunction().(AttrNode).getObject("replace") and
+      call.getArg(0).getNode().(StrConst).getText() in ["\r\n", "\n"]
+    )
+  }


This is slightly unsafe as it deems either replacement ("\r\n" or "\n") a sanitizer when really both are required.
I am not sure how to improve that easily, though. Perhaps we will turn it into a concept with a single implementation and improve it over time, but I am not sure you need to do that now.

Further, this seems overly specific, so I would expect some false positives from this. Consider adding at least StringConstCompare as a sanitizer option. (Either directly here in this predicate, or by a construction similar to here.)

Do you have a specific case here? Because the safety laboratory did not give me advice on the elimination of FP.

If the user does replace("\r\n", ""), then that will match your predicate and be considered a sanitizer. However, that would not do any replacements on a message having only "\n"'s in it.

The other comment is that if the message is compared against a known string, that should be considered a sanitizer (since that is much stronger than doing a replacement).

I think it would be better if you give me the actual case.

@yoff Excuse me again. Do you have examples in actual open source projects for me?

Hm, I had to do a bit of digging. My comment came from seeing code like this, although in that particular case we do want to alert (because the logged value is outside the known set). I think I have seen also that only values in a known set is logged, which would be safe, and that is the sanitizer I proposed. However, it does not seem to be very common, so we could also add it once we see the first FP...

haby0 · 2021-09-15T13:54:40Z

You are not handling HTML logging and encoding, did you plan to? It might require a second configuration, seeing as the sinks dictate which sanitizers are required...

Thanks for the review, about this suggestion, can you describe it again? I am not too understanding.

yoff · 2021-09-16T14:59:13Z

You are not handling HTML logging and encoding, did you plan to? It might require a second configuration, seeing as the sinks dictate which sanitizers are required...

Thanks for the review, about this suggestion, can you describe it again? I am not too understanding.

I meant that to find html-based log injection, you would need sinks to be logs displayed as HTML and sanitizers to be HTML-relevant escaping. On the whole that seems to be an entirely separate taint tracking configuration. I wonder if that is out of scope for now, but it is mentioned in the help file.

haby0 · 2021-09-17T05:50:15Z

You are not handling HTML logging and encoding, did you plan to? It might require a second configuration, seeing as the sinks dictate which sanitizers are required...

Thanks for the review, about this suggestion, can you describe it again? I am not too understanding.

I meant that to find html-based log injection, you would need sinks to be logs displayed as HTML and sanitizers to be HTML-relevant escaping. On the whole that seems to be an entirely separate taint tracking configuration. I wonder if that is out of scope for now, but it is mentioned in the help file.

Thank you for your reply. I am not going to add HTML injection due to log injection in this PR. I will create a new PR to complete this query.

yoff

I have tried to clarify my points.

yoff · 2021-09-22T10:22:02Z

python/ql/src/experimental/semmle/python/frameworks/Log.qll

+    override DataFlow::Node getAnInput() {
+      this.getFunction().(DataFlow::AttrRead).getAttributeName() != "log" and
+      result = this.getArg(0)
+      or
+      this.getFunction().(DataFlow::AttrRead).getAttributeName() = "log" and
+      result = this.getArg(1)
+    }


Suggested change

override DataFlow::Node getAnInput() {

this.getFunction().(DataFlow::AttrRead).getAttributeName() != "log" and

result = this.getArg(0)

or

this.getFunction().(DataFlow::AttrRead).getAttributeName() = "log" and

result = this.getArg(1)

}

override DataFlow::Node getAnInput() {

this.getFunction().(DataFlow::AttrRead).getAttributeName() != "log" and

result in [this.getArg(_), this.getArgByName(_) ] // this includes the arg named "msg"

or

this.getFunction().(DataFlow::AttrRead).getAttributeName() = "log" and

result in [this.getArg(any(int i | i > 0)), this.getArgByName(any(string s | s != "level")]

}

I think my previous comment was a bit unclear, it should be something like the above.

If you have a signature like

warning(msg, *args, **kwargs)

you can call it with

warning("msg", "arg1", "arg2", kwarg1="kwarg1", kwargs="kwarg2")

and all of these arguments seem to end up in the log.

yoff · 2021-09-22T10:24:54Z

python/ql/src/experimental/semmle/python/security/injection/LogInjection.qll

+  override predicate isSanitizer(DataFlow::Node node) {
+    exists(CallNode call |
+      node.asCfgNode() = call.getFunction().(AttrNode).getObject("replace") and
+      call.getArg(0).getNode().(StrConst).getText() in ["\r\n", "\n"]
+    )
+  }


If the user does replace("\r\n", ""), then that will match your predicate and be considered a sanitizer. However, that would not do any replacements on a message having only "\n"'s in it.

yoff · 2021-09-22T10:25:39Z

python/ql/src/experimental/semmle/python/security/injection/LogInjection.qll

+  override predicate isSanitizer(DataFlow::Node node) {
+    exists(CallNode call |
+      node.asCfgNode() = call.getFunction().(AttrNode).getObject("replace") and
+      call.getArg(0).getNode().(StrConst).getText() in ["\r\n", "\n"]
+    )
+  }


The other comment is that if the message is compared against a known string, that should be considered a sanitizer (since that is much stronger than doing a replacement).

haby0 · 2021-09-22T10:34:13Z

@yoff Compared with the known string, is the known string "\r\n" or "\n"?

haby0 · 2021-09-30T09:33:27Z

Friendly ping.

yoff

I am still vary of the FP rate, but I do not have any evidence for it, so happy to approve for now.

haby0 · 2021-10-05T08:55:42Z

I am still vary of the FP rate, but I do not have any evidence for it, so happy to approve for now.

Thank you for your reply. If you have any suggestions, please let me know and I will improve the configuration query.

yoff · 2021-10-11T08:39:53Z

The language tests fail because we now have a mechanism for computing subpaths. You can merge from main and rerun tests. I will also make a suggestion, that I think will make them pass..

python/ql/test/experimental/query-tests/Security/CWE-117/LogInjection.expected

…jection.expected Co-authored-by: yoff <lerchedahl@gmail.com>

yoff · 2021-10-11T15:33:47Z

The final error is due to formatting, particularly ql/python/ql/src/experimental/semmle/python/frameworks/Log.qll and ql/python/ql/src/experimental/semmle/python/Frameworks.qll.

haby0 · 2021-10-12T01:43:41Z

The final error is due to formatting, particularly ql/python/ql/src/experimental/semmle/python/frameworks/Log.qll and ql/python/ql/src/experimental/semmle/python/Frameworks.qll.

@yoff Automatic formatting is done, please review whether it is passed, thank you.

yoff

LGTM

haby0 requested a review from a team as a code owner June 29, 2021 12:53

github-actions bot added documentation Python labels Jun 29, 2021

haby0 mentioned this pull request Jun 30, 2021

[Python]: CWE-117 Log Injection github/securitylab#388

Closed

1 task

RasmusWL assigned yoff Aug 23, 2021

Fix conflicting

c60eded

yoff requested changes Sep 15, 2021

View reviewed changes

Modify sinks

9916753

yoff requested changes Sep 22, 2021

View reviewed changes

Apply @yoff's suggestion

6c07a3e

haby0 requested a review from yoff September 23, 2021 09:05

yoff previously approved these changes Oct 4, 2021

View reviewed changes

yoff reviewed Oct 11, 2021

View reviewed changes

python/ql/test/experimental/query-tests/Security/CWE-117/LogInjection.expected Show resolved Hide resolved

Update python/ql/test/experimental/query-tests/Security/CWE-117/LogIn…

29ddc76

…jection.expected Co-authored-by: yoff <lerchedahl@gmail.com>

haby0 dismissed yoff’s stale review via 29ddc76 October 11, 2021 08:43

Auto Formatting

d52f95d

yoff approved these changes Oct 12, 2021

View reviewed changes

yoff merged commit 43f7eed into github:main Oct 12, 2021

haby0 deleted the python/LogInjection branch October 13, 2021 08:29

yoff mentioned this pull request Feb 1, 2022

Python: promote log injection #7735

Merged

Onyx2406 mentioned this pull request Jul 14, 2023

Mitigation of Log Injection Vulnerabilities in Sender.java apache/fineract#3308

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: CWE-117 Log injection #6182

Python: CWE-117 Log injection #6182

haby0 commented Jun 29, 2021

RasmusWL commented Jun 30, 2021

haby0 commented Jun 30, 2021

haby0 commented Sep 15, 2021

yoff left a comment

yoff Sep 15, 2021

haby0 Sep 17, 2021

yoff Sep 15, 2021

haby0 Sep 17, 2021

yoff Sep 15, 2021

haby0 Sep 17, 2021

yoff Sep 22, 2021

yoff Sep 22, 2021

haby0 Sep 22, 2021

haby0 Sep 24, 2021

yoff Oct 4, 2021

haby0 commented Sep 15, 2021

yoff commented Sep 16, 2021

haby0 commented Sep 17, 2021

yoff left a comment

yoff Sep 22, 2021

yoff Sep 22, 2021

yoff Sep 22, 2021

haby0 commented Sep 22, 2021

haby0 commented Sep 30, 2021

yoff left a comment

haby0 commented Oct 5, 2021

yoff commented Oct 11, 2021

yoff commented Oct 11, 2021

haby0 commented Oct 12, 2021

yoff left a comment

Python: CWE-117 Log injection #6182

Python: CWE-117 Log injection #6182

Conversation

haby0 commented Jun 29, 2021

RasmusWL commented Jun 30, 2021

haby0 commented Jun 30, 2021

haby0 commented Sep 15, 2021

yoff 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haby0 commented Sep 15, 2021

yoff commented Sep 16, 2021

haby0 commented Sep 17, 2021

yoff 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

haby0 commented Sep 22, 2021

haby0 commented Sep 30, 2021

yoff left a comment

Choose a reason for hiding this comment

haby0 commented Oct 5, 2021

yoff commented Oct 11, 2021

yoff commented Oct 11, 2021

haby0 commented Oct 12, 2021

yoff left a comment

Choose a reason for hiding this comment