Skip to content

Commit d9af087

Browse files
committed
Python: Reduce log injection false positives
Add `SimpleTypeSanitizer` in a new `semmle.python.security.Sanitizers`, the Python counterpart of the class the Java and C# queries already use. A routed parameter annotated with a simple type such as `int` or `uuid.UUID` is validated by the web framework before the request handler runs, so it cannot contain a line break and cannot be used to forge a log entry. The annotation is only trusted on routed parameters, since Python does not enforce annotations at run time. `Annotated[T, ...]`, `Optional[T]` and `T | None` are unwrapped. Recognize more ways of neutralizing a log message: `str.translate`, `str.encode("unicode_escape")`, a `re.sub` or `re.compile(...).sub` whose pattern matches control characters, and escaping conversions such as `repr` and `json.dumps`. Recognize a `logging.Formatter` subclass that strips control characters from the records it renders. Such a formatter sanitizes when the record is written rather than where it is created, so no sanitizing call appears between the source and the logging call. Callees are resolved through module-level definitions, which local flow does not reach from a nested scope.
1 parent d1fed84 commit d9af087

9 files changed

Lines changed: 455 additions & 0 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `py/log-injection` query no longer reports a routed parameter whose type annotation a
5+
web framework validates to be a simple type, such as `int` or `uuid.UUID`, since such a
6+
value cannot contain a line break. The query also recognizes more ways of neutralizing a
7+
log message, including `str.translate`, `re.sub` with a pattern matching control
8+
characters, escaping conversions such as `repr` and `json.dumps`, and a
9+
`logging.Formatter` subclass that strips control characters from every record it renders.
10+
This reduces false positives in applications that use typed path parameters or sanitize
11+
when the record is written rather than where it is created.
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
category: feature
3+
---
4+
* Added a `SimpleTypeSanitizer` class to the new `semmle.python.security.Sanitizers` module. It
5+
identifies values of a type that cannot carry an injection payload, such as numbers, UUIDs,
6+
dates, and enum members, either as the result of a conversion or as a routed parameter whose
7+
type annotation a web framework validates. This is the Python counterpart of the simple-type
8+
sanitizers used by the Java and C# queries.
Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
/**
2+
* Provides sanitizers that are applicable to several security queries.
3+
*/
4+
5+
private import python
6+
private import semmle.python.ApiGraphs
7+
private import semmle.python.Concepts
8+
private import semmle.python.dataflow.new.DataFlow
9+
10+
/** Gets a reference to `name` from the `typing` or `typing_extensions` module. */
11+
private API::Node typingRef(string name) {
12+
result = API::moduleImport(["typing", "typing_extensions"]).getMember(name)
13+
}
14+
15+
/**
16+
* Gets a reference to a type whose instances cannot carry an injection payload.
17+
*
18+
* These are types with a closed, machine-generated string representation, such as
19+
* numbers, UUIDs, dates and enum members. A value of such a type can never contain
20+
* a line break, a quote, or a control character, and so cannot be used to forge a
21+
* log entry, break out of a query string, and so on.
22+
*/
23+
private API::Node simpleTypeRef() {
24+
result = API::builtin(["int", "float", "bool", "complex"])
25+
or
26+
result = API::moduleImport("uuid").getMember("UUID")
27+
or
28+
result = API::moduleImport("datetime").getMember(["date", "datetime", "time", "timedelta"])
29+
or
30+
result = API::moduleImport("decimal").getMember("Decimal")
31+
or
32+
result = API::moduleImport("ipaddress").getMember(["IPv4Address", "IPv6Address"])
33+
or
34+
// Pydantic aliases that validate to the corresponding stdlib type.
35+
// See https://docs.pydantic.dev/latest/api/types/
36+
result =
37+
API::moduleImport("pydantic")
38+
.getMember([
39+
"UUID1", "UUID3", "UUID4", "UUID5", "StrictBool", "StrictInt", "StrictFloat",
40+
"PositiveInt", "NegativeInt", "NonNegativeInt", "NonPositiveInt", "PositiveFloat",
41+
"NegativeFloat", "NonNegativeFloat", "NonPositiveFloat"
42+
])
43+
or
44+
// The members of an enum are fixed when the class is defined, so an attacker can
45+
// at most select between values that already occur in the source code.
46+
result =
47+
API::moduleImport("enum")
48+
.getMember(["Enum", "IntEnum", "StrEnum", "Flag", "IntFlag"])
49+
.getASubclass+()
50+
}
51+
52+
/**
53+
* Gets the type denoted by the parameter annotation `annotation`, looking through the
54+
* `Annotated[T, ...]`, `Optional[T]` and `T | None` wrappers that web frameworks accept.
55+
*/
56+
private Expr unwrapTypeAnnotation(Expr annotation) {
57+
annotation = any(Parameter p).getAnnotation() and
58+
result = annotation
59+
or
60+
exists(Expr inner | inner = unwrapTypeAnnotation(annotation) |
61+
// `Annotated[T, ...]`, where only the first element denotes the type.
62+
inner.(Subscript).getObject() =
63+
typingRef("Annotated").getAValueReachableFromSource().asExpr() and
64+
result = inner.(Subscript).getIndex().(Tuple).getElt(0)
65+
or
66+
// `Optional[T]`
67+
inner.(Subscript).getObject() =
68+
typingRef("Optional").getAValueReachableFromSource().asExpr() and
69+
result = inner.(Subscript).getIndex()
70+
or
71+
// `T | None`
72+
exists(BinaryExpr union | union = inner and union.getOp() instanceof BitOr |
73+
result = union.getLeft() and union.getRight() instanceof None
74+
or
75+
result = union.getRight() and union.getLeft() instanceof None
76+
)
77+
)
78+
}
79+
80+
/**
81+
* A node whose value is of a simple type unlikely to carry taint, such as a number,
82+
* a `uuid.UUID`, a `datetime.datetime`, or an enum member.
83+
*
84+
* This is the Python counterpart of the `SimpleTypeSanitizer` classes used by the
85+
* Java and C# queries.
86+
*
87+
* Unlike in a statically typed language, a Python type annotation is not enforced at
88+
* run time, so an annotation is not trusted on its own. The annotation of a routed
89+
* parameter is trusted, however, since a web framework validates an incoming request
90+
* against it -- and rejects the request outright -- before the request handler body
91+
* runs.
92+
*/
93+
class SimpleTypeSanitizer extends DataFlow::Node {
94+
SimpleTypeSanitizer() {
95+
// A routed parameter whose annotation the web framework validates.
96+
exists(Parameter p |
97+
p = this.(DataFlow::ParameterNode).getParameter() and
98+
p = any(Http::Server::RequestHandler handler).getARoutedParameter() and
99+
unwrapTypeAnnotation(p.getAnnotation()) =
100+
simpleTypeRef().getAValueReachableFromSource().asExpr()
101+
)
102+
or
103+
// The result of converting a value to a simple type.
104+
this = simpleTypeRef().getACall()
105+
or
106+
this = API::builtin(["len", "hash", "id", "ord"]).getACall()
107+
}
108+
}

python/ql/lib/semmle/python/security/dataflow/LogInjectionCustomizations.qll

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,13 @@
55
*/
66

77
private import python
8+
private import semmle.python.ApiGraphs
89
private import semmle.python.dataflow.new.DataFlow
910
private import semmle.python.Concepts
1011
private import semmle.python.dataflow.new.RemoteFlowSources
1112
private import semmle.python.dataflow.new.BarrierGuards
1213
private import semmle.python.frameworks.data.ModelsAsData
14+
private import semmle.python.security.Sanitizers
1315

1416
/**
1517
* Provides default sources, sinks and sanitizers for detecting
@@ -81,6 +83,16 @@ module LogInjection {
8183
SinkFromModel() { ModelOutput::sinkNode(this, "log-injection") }
8284
}
8385

86+
/**
87+
* A value of a simple type, such as an `int` or a `uuid.UUID`, considered as a
88+
* sanitizer.
89+
*
90+
* Such a value has a machine-generated string representation that can never
91+
* contain a line break, so it cannot be used to forge a log entry. The Java and
92+
* C# log injection queries use the same sanitizer.
93+
*/
94+
class SimpleTypeAsSanitizer extends Sanitizer instanceof SimpleTypeSanitizer { }
95+
8496
/**
8597
* A comparison with a constant, considered as a sanitizer-guard.
8698
*/
@@ -107,6 +119,160 @@ module LogInjection {
107119
}
108120
}
109121

122+
/**
123+
* Gets the pattern of a call to `re.sub` or `re.subn`, whether the pattern is
124+
* passed directly or was compiled with `re.compile`.
125+
*/
126+
private string getARegexSubstitutionPattern(DataFlow::CallCfgNode call) {
127+
call = API::moduleImport("re").getMember(["sub", "subn"]).getACall() and
128+
result = call.getArg(0).asExpr().(StringLiteral).getText()
129+
or
130+
exists(API::CallNode compile |
131+
compile = API::moduleImport("re").getMember("compile").getACall() and
132+
call = compile.getReturn().getMember(["sub", "subn"]).getACall() and
133+
result = compile.getArg(0).asExpr().(StringLiteral).getText()
134+
)
135+
}
136+
137+
/**
138+
* Holds if `call` removes or escapes the characters that could be used to forge a
139+
* log entry.
140+
*
141+
* Like `ReplaceLineBreaksSanitizer`, this is deliberately generous: we do not
142+
* verify that every kind of line break is handled, only that the value has been
143+
* put through an operation whose purpose is to neutralize them.
144+
*/
145+
private predicate stripsControlCharacters(DataFlow::CallCfgNode call) {
146+
exists(string method | method = call.getFunction().(DataFlow::AttrRead).getAttributeName() |
147+
// `x.replace("\n", "")`
148+
method = "replace" and
149+
call.getArg(0).asExpr().(StringLiteral).getText() in ["\r\n", "\n", "\r"]
150+
or
151+
// `x.translate(table)`. The table is assumed to escape control characters, as
152+
// `str.translate` has no other common use on a log message.
153+
method = "translate"
154+
or
155+
// `x.encode("unicode_escape")`
156+
method = "encode" and
157+
call.getArg(0).asExpr().(StringLiteral).getText() = "unicode_escape"
158+
)
159+
or
160+
// A substitution whose pattern mentions a line break or a control character class.
161+
getARegexSubstitutionPattern(call)
162+
.matches(["%\\n%", "%\\r%", "%\\x0%", "%\\s%", "%cntrl%", "%\n%"])
163+
or
164+
// Conversions that render a control character inert by escaping it.
165+
call = API::builtin(["repr", "ascii"]).getACall()
166+
or
167+
call = API::moduleImport("json").getMember("dumps").getACall()
168+
or
169+
call = API::moduleImport("urllib.parse").getMember(["quote", "quote_plus"]).getACall()
170+
}
171+
172+
/**
173+
* A call that strips control characters from its input, considered as a sanitizer.
174+
*
175+
* This subsumes `ReplaceLineBreaksSanitizer`, which is retained because it is part
176+
* of the public API of this module.
177+
*/
178+
class StripControlCharactersSanitizer extends Sanitizer, DataFlow::CallCfgNode {
179+
StripControlCharactersSanitizer() { stripsControlCharacters(this) }
180+
}
181+
182+
/**
183+
* Gets a callable that `call` may invoke, where the callable is defined in the
184+
* module being analyzed.
185+
*
186+
* Local flow does not cross a scope boundary, so a module-level definition that is
187+
* referenced from inside a function or a method has to be resolved by name.
188+
*/
189+
private Scope getALocallyResolvedCallee(DataFlow::CallCfgNode call) {
190+
exists(DataFlow::LocalSourceNode callee | callee.flowsTo(call.getFunction()) |
191+
callee.asExpr() = result.(Function).getDefinition()
192+
or
193+
callee.asExpr().(ClassExpr).getInnerScope() = result
194+
)
195+
or
196+
exists(DataFlow::ModuleVariableNode var |
197+
var.getARead() = call.getFunction() and
198+
result.getName() = var.getVariable().getId() and
199+
result.getEnclosingScope() = var.getModule()
200+
)
201+
}
202+
203+
/** Gets a method of `cls` that renders a log record. */
204+
private Function getARecordRenderingMethod(Class cls) {
205+
result = cls.getAMethod() and
206+
result.getName() in ["format", "formatMessage"]
207+
}
208+
209+
/**
210+
* Gets a function called, directly or indirectly, from the record-rendering method
211+
* `f`.
212+
*
213+
* Only calls that resolve to a definition in the module being analyzed are
214+
* followed, which covers the common case of a formatter delegating to a helper
215+
* defined alongside it.
216+
*/
217+
private Function getATransitiveCallee(Function f) {
218+
f = getARecordRenderingMethod(_) and
219+
result = f
220+
or
221+
exists(DataFlow::CallCfgNode call |
222+
call.getScope() = getATransitiveCallee(f) and
223+
result = getALocallyResolvedCallee(call)
224+
)
225+
}
226+
227+
/**
228+
* Holds if `cls` is a `logging.Formatter` subclass that strips control characters
229+
* from the records it renders.
230+
*
231+
* Such a formatter sanitizes when the record is written rather than where it is
232+
* created, so no sanitizing call appears between the source and the logging call.
233+
*/
234+
private predicate isSanitizingFormatter(Class cls) {
235+
cls.getABase() =
236+
API::moduleImport("logging").getMember("Formatter").getAValueReachableFromSource().asExpr() and
237+
exists(DataFlow::CallCfgNode strip |
238+
stripsControlCharacters(strip) and
239+
strip.getScope() = getATransitiveCallee(getARecordRenderingMethod(cls))
240+
)
241+
}
242+
243+
/**
244+
* Holds if a `logging.Formatter` that strips control characters is installed on a
245+
* handler.
246+
*
247+
* This is a property of the application as a whole rather than of an individual
248+
* logging call. We do not determine which loggers the formatter ends up attached
249+
* to, as that would require resolving handler registration, which is typically
250+
* spread across module-level configuration code and often iterates over
251+
* `logging.getLogger().handlers`. Such a formatter is nearly always installed on
252+
* the root logger, where it applies to every record that propagates to it, so its
253+
* presence is treated as sanitizing all logging calls. The trade-off is a false
254+
* negative for an application that also logs through a deliberately unsanitized
255+
* handler.
256+
*/
257+
private predicate hasSanitizingFormatter() {
258+
exists(DataFlow::MethodCallNode setFormatter, DataFlow::CallCfgNode instantiation |
259+
setFormatter.getMethodName() = "setFormatter" and
260+
instantiation.flowsTo(setFormatter.getArg(0)) and
261+
isSanitizingFormatter(getALocallyResolvedCallee(instantiation))
262+
)
263+
}
264+
265+
/**
266+
* A logging operation in an application that installs a `logging.Formatter`
267+
* stripping control characters, considered as a sanitizer.
268+
*/
269+
class LoggingSanitizedByFormatter extends Sanitizer {
270+
LoggingSanitizedByFormatter() {
271+
hasSanitizingFormatter() and
272+
this instanceof LoggingAsSink
273+
}
274+
}
275+
110276
/**
111277
* A sanitizer defined via models-as-data with kind "log-injection".
112278
*/
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
#select
2+
edges
3+
nodes
4+
subpaths
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
query: Security/CWE-117/LogInjection.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
#!/usr/bin/env python
2+
# -*- coding: UTF-8 -*-
3+
"""
4+
@Desc :Log Injection prevented by a sanitizing `logging.Formatter`
5+
6+
A formatter that strips control characters sanitizes when the record is written,
7+
so the individual logging calls are not vulnerable.
8+
"""
9+
import logging
10+
11+
from flask import Flask
12+
from flask import request
13+
14+
app = Flask(__name__)
15+
16+
ESCAPE_TABLE = {ord("\n"): "\\n", ord("\r"): "\\r", ord("\t"): "\\t"}
17+
18+
19+
def escape_controls(text):
20+
"""Escape the control characters in a log message."""
21+
return text.translate(ESCAPE_TABLE)
22+
23+
24+
class EscapingFormatter(logging.Formatter):
25+
"""A formatter that escapes control characters in the rendered message."""
26+
27+
def format(self, record):
28+
record.msg = escape_controls(record.getMessage())
29+
record.args = None
30+
return super().format(record)
31+
32+
33+
logging.basicConfig(level=logging.INFO, handlers=[logging.StreamHandler()])
34+
35+
for handler in logging.getLogger().handlers:
36+
handler.setFormatter(EscapingFormatter())
37+
38+
logger = logging.getLogger("test")
39+
40+
41+
@app.route("/f-string")
42+
def f_string():
43+
name = request.args.get("name")
44+
logger.warning(f"User name: {name}") # Good
45+
return "ok"
46+
47+
48+
@app.route("/percent-args")
49+
def percent_args():
50+
name = request.args.get("name")
51+
logger.info("User name: %s", name) # Good
52+
return "ok"
53+
54+
55+
@app.route("/escaped-at-call-site")
56+
def escaped_at_call_site():
57+
name = request.args.get("name")
58+
logger.warning("User name: " + escape_controls(name)) # Good
59+
return "ok"

0 commit comments

Comments
 (0)