Repository navigation
Python: promote log injection #7735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
20d5454
8b5114d
bf1145e
9d41666
c03f89d
7511b33
26befeb
ecea392
119a7e4
c587084
bec8c0d
448e078
f21ac04
bd14ade
62598c0
3a995ec
62d4bb5
b59ab7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- modernize the sanitizer, but do not make it less specific
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,35 @@ | ||
| /** | ||
| * Provides a taint-tracking configuration for tracking untrusted user input used in log entries. | ||
| * | ||
| * Note, for performance reasons: only import this file if | ||
| * `LogInjection::Configuration` is needed, otherwise | ||
| * `LogInjectionCustomizations` should be imported instead. | ||
| */ | ||
|
|
||
| import python | ||
| import semmle.python.Concepts | ||
| import semmle.python.Concepts | ||
| import semmle.python.dataflow.new.DataFlow | ||
| import semmle.python.dataflow.new.TaintTracking | ||
| import semmle.python.dataflow.new.RemoteFlowSources | ||
|
|
||
| /** | ||
| * A taint-tracking configuration for tracking untrusted user input used in log entries. | ||
| * Provides a taint-tracking configuration for tracking untrusted user input used in log entries. | ||
| */ | ||
| class LogInjectionFlowConfig extends TaintTracking::Configuration { | ||
| LogInjectionFlowConfig() { this = "LogInjectionFlowConfig" } | ||
| module LogInjection { | ||
| import LogInjectionCustomizations::LogInjection | ||
|
|
||
| /** | ||
| * A taint-tracking configuration for tracking untrusted user input used in log entries. | ||
| */ | ||
| class Configuration extends TaintTracking::Configuration { | ||
| Configuration() { this = "LogInjection" } | ||
|
|
||
| override predicate isSource(DataFlow::Node source) { source instanceof Source } | ||
|
|
||
| override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } | ||
| override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } | ||
|
|
||
| override predicate isSink(DataFlow::Node sink) { sink = any(Logging write).getAnInput() } | ||
| override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer } | ||
|
|
||
| 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"] | ||
| ) | ||
| override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { | ||
| guard instanceof SanitizerGuard | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /** | ||
| * Provides default sources, sinks and sanitizers for detecting | ||
| * "command injection" | ||
| * vulnerabilities, as well as extension points for adding your own. | ||
| */ | ||
|
|
||
| private import python | ||
| private import semmle.python.dataflow.new.DataFlow | ||
| private import semmle.python.Concepts | ||
| private import semmle.python.dataflow.new.RemoteFlowSources | ||
| private import semmle.python.dataflow.new.BarrierGuards | ||
|
|
||
| /** | ||
| * Provides default sources, sinks and sanitizers for detecting | ||
| * "command injection" | ||
| * vulnerabilities, as well as extension points for adding your own. | ||
| */ | ||
| module LogInjection { | ||
| /** | ||
| * A data flow source for "command injection" vulnerabilities. | ||
| */ | ||
| abstract class Source extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A data flow sink for "command injection" vulnerabilities. | ||
| */ | ||
| abstract class Sink extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A sanitizer for "command injection" vulnerabilities. | ||
| */ | ||
| abstract class Sanitizer extends DataFlow::Node { } | ||
|
|
||
| /** | ||
| * A sanitizer guard for "command injection" vulnerabilities. | ||
| */ | ||
| abstract class SanitizerGuard extends DataFlow::BarrierGuard { } | ||
|
|
||
| /** | ||
| * A source of remote user input, considered as a flow source. | ||
| */ | ||
| class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { } | ||
|
|
||
| /** | ||
| * A logging operation, considered as a flow sink. | ||
| */ | ||
| class LoggingAsSink extends Sink { | ||
| LoggingAsSink() { this = any(Logging write).getAnInput() } | ||
| } | ||
|
|
||
| /** | ||
| * A comparison with a constant string, considered as a sanitizer-guard. | ||
| */ | ||
| class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { } | ||
|
|
||
| /** | ||
| * A call to replace line breaks functions as a sanitizer. | ||
|
yoff marked this conversation as resolved.
Resolved
|
||
| */ | ||
| class ReplaceLineBreaksSanitizer extends Sanitizer, DataFlow::CallCfgNode { | ||
| ReplaceLineBreaksSanitizer() { | ||
| this.getFunction().(DataFlow::AttrRead).getAttributeName() = "replace" and | ||
| this.getArg(0).asExpr().(StrConst).getText() in ["\r\n", "\n"] | ||
| } | ||
| } | ||
|
Comment on lines
+59
to
+72
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sanitizer check seems a bit weak. What if it only did
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have had the same concerns from the beginning.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be great to have a small comment in the code to explain this dilemma, and explain why the solution implemented was deemed ok. |
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.