Skip to content

Commit 6c18e1d

Browse files
hmachmac
authored
Merge pull request #8272 from hmac/hmac/tainted-format-string
2 parents aff76b7 + 5a6da82 commit 6c18e1d

16 files changed

+345
-10
lines changed

‎config/identical-files.json

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -525,5 +525,13 @@
525525
"ApiGraphModels": [
526526
"javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModels.qll",
527527
"ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModels.qll"
528+
],
529+
"TaintedFormatStringQuery Ruby/JS": [
530+
"javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringQuery.qll",
531+
"ruby/ql/lib/codeql/ruby/security/TaintedFormatStringQuery.qll"
532+
],
533+
"TaintedFormatStringCustomizations Ruby/JS": [
534+
"javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringCustomizations.qll",
535+
"ruby/ql/lib/codeql/ruby/security/TaintedFormatStringCustomizations.qll"
528536
]
529-
}
537+
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringCustomizations.qll

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,13 @@
33
* format injections, as well as extension points for adding your own.
44
*/
55

6-
import javascript
7-
import semmle.javascript.security.dataflow.DOM
8-
6+
/**
7+
* Provides default sources, sinks and sanitizers for reasoning about
8+
* format injections, as well as extension points for adding your own.
9+
*/
910
module TaintedFormatString {
11+
import TaintedFormatStringSpecific
12+
1013
/**
1114
* A data flow source for format injections.
1215
*/
@@ -23,9 +26,7 @@ module TaintedFormatString {
2326
abstract class Sanitizer extends DataFlow::Node { }
2427

2528
/** A source of remote user input, considered as a flow source for format injection. */
26-
class RemoteSource extends Source {
27-
RemoteSource() { this instanceof RemoteFlowSource }
28-
}
29+
class RemoteSource extends Source instanceof RemoteFlowSource { }
2930

3031
/**
3132
* A format argument to a printf-like function, considered as a flow sink for format injection.

‎javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringQuery.qll

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,7 @@
88
* `TaintedFormatStringCustomizations` should be imported instead.
99
*/
1010

11-
import javascript
12-
import semmle.javascript.security.dataflow.DOM
13-
import TaintedFormatStringCustomizations::TaintedFormatString
11+
private import TaintedFormatStringCustomizations::TaintedFormatString
1412

1513
/**
1614
* A taint-tracking configuration for format injections.

‎javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringSpecific.qll

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
/**
2+
* Provides JS-specific imports needed for `TaintedFormatStringQuery` and `TaintedFormatStringCustomizations`.
3+
*/
4+
5+
import javascript
6+
import semmle.javascript.security.dataflow.DOM

‎ruby/ql/lib/codeql/ruby/security/TaintedFormatStringCustomizations.qll

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* format injections, as well as extension points for adding your own.
4+
*/
5+
6+
/**
7+
* Provides default sources, sinks and sanitizers for reasoning about
8+
* format injections, as well as extension points for adding your own.
9+
*/
10+
module TaintedFormatString {
11+
import TaintedFormatStringSpecific
12+
13+
/**
14+
* A data flow source for format injections.
15+
*/
16+
abstract class Source extends DataFlow::Node { }
17+
18+
/**
19+
* A data flow sink for format injections.
20+
*/
21+
abstract class Sink extends DataFlow::Node { }
22+
23+
/**
24+
* A sanitizer for format injections.
25+
*/
26+
abstract class Sanitizer extends DataFlow::Node { }
27+
28+
/** A source of remote user input, considered as a flow source for format injection. */
29+
class RemoteSource extends Source instanceof RemoteFlowSource { }
30+
31+
/**
32+
* A format argument to a printf-like function, considered as a flow sink for format injection.
33+
*/
34+
class FormatSink extends Sink {
35+
FormatSink() {
36+
exists(PrintfStyleCall printf |
37+
this = printf.getFormatString() and
38+
// exclude trivial case where there are no arguments to interpolate
39+
exists(printf.getFormatArgument(_))
40+
)
41+
}
42+
}
43+
}

‎ruby/ql/lib/codeql/ruby/security/TaintedFormatStringQuery.qll

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about format
3+
* injections.
4+
*
5+
*
6+
* Note, for performance reasons: only import this file if
7+
* `TaintedFormatString::Configuration` is needed, otherwise
8+
* `TaintedFormatStringCustomizations` should be imported instead.
9+
*/
10+
11+
private import TaintedFormatStringCustomizations::TaintedFormatString
12+
13+
/**
14+
* A taint-tracking configuration for format injections.
15+
*/
16+
class Configuration extends TaintTracking::Configuration {
17+
Configuration() { this = "TaintedFormatString" }
18+
19+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
20+
21+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
22+
23+
override predicate isSanitizer(DataFlow::Node node) {
24+
super.isSanitizer(node) or
25+
node instanceof Sanitizer
26+
}
27+
}

‎ruby/ql/lib/codeql/ruby/security/TaintedFormatStringSpecific.qll

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
/**
2+
* Provides Ruby-specific imports and classes needed for `TaintedFormatStringQuery` and `TaintedFormatStringCustomizations`.
3+
*/
4+
5+
import ruby
6+
import codeql.ruby.DataFlow
7+
import codeql.ruby.dataflow.RemoteFlowSources
8+
import codeql.ruby.ApiGraphs
9+
import codeql.ruby.TaintTracking
10+
private import codeql.ruby.frameworks.Files::IO
11+
private import codeql.ruby.controlflow.CfgNodes
12+
13+
/**
14+
* A call to `printf` or `sprintf`.
15+
*/
16+
abstract class PrintfStyleCall extends DataFlow::CallNode {
17+
// We assume that most printf-like calls have the signature f(format_string, args...)
18+
/**
19+
* Gets the format string of this call.
20+
*/
21+
DataFlow::Node getFormatString() { result = this.getArgument(0) }
22+
23+
/**
24+
* Gets then `n`th formatted argument of this call.
25+
*/
26+
DataFlow::Node getFormatArgument(int n) { n >= 0 and result = this.getArgument(n + 1) }
27+
}
28+
29+
/**
30+
* A call to `Kernel.printf`.
31+
*/
32+
class KernelPrintfCall extends PrintfStyleCall {
33+
KernelPrintfCall() {
34+
this = API::getTopLevelMember("Kernel").getAMethodCall("printf")
35+
or
36+
this.asExpr().getExpr() instanceof UnknownMethodCall and
37+
this.getMethodName() = "printf"
38+
}
39+
40+
// Kernel#printf supports two signatures:
41+
// printf(io, string, ...)
42+
// printf(string, ...)
43+
override DataFlow::Node getFormatString() {
44+
// Because `printf` has two different signatures, we can't be sure which
45+
// argument is the format string, so we use a heuristic:
46+
// If the first argument has a string value, then we assume it is the format string.
47+
// Otherwise we treat both the first and second args as the format string.
48+
if this.getArgument(0).getExprNode().getConstantValue().isString(_)
49+
then result = this.getArgument(0)
50+
else result = this.getArgument([0, 1])
51+
}
52+
}
53+
54+
/**
55+
* A call to `Kernel.sprintf`.
56+
*/
57+
class KernelSprintfCall extends PrintfStyleCall {
58+
KernelSprintfCall() {
59+
this = API::getTopLevelMember("Kernel").getAMethodCall("sprintf")
60+
or
61+
this.asExpr().getExpr() instanceof UnknownMethodCall and
62+
this.getMethodName() = "sprintf"
63+
}
64+
}
65+
66+
/**
67+
* A call to `IO#printf`.
68+
*/
69+
class IOPrintfCall extends PrintfStyleCall {
70+
IOPrintfCall() { this.getReceiver() instanceof IOInstance and this.getMethodName() = "printf" }
71+
}

‎ruby/ql/src/change-notes/2022-03-10-rb-tainted-format-string.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `rb/http-tainted-format-string`. The query finds cases where data from remote user input is used in a string formatting method in a way that allows arbitrary format specifiers to be inserted.

‎ruby/ql/src/queries/security/cwe-134/TaintedFormatString.qhelp

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Methods like <code>Kernel.printf</code> accept a format string that is used to format
9+
the remaining arguments by providing inline format specifiers. If the format string
10+
contains unsanitized input from an untrusted source, then that string may contain
11+
unexpected format specifiers that cause garbled output or throw an exception.
12+
</p>
13+
</overview>
14+
15+
<recommendation>
16+
<p>
17+
Either sanitize the input before including it in the format string, or use a
18+
<code>%s</code> specifier in the format string, and pass the untrusted data as corresponding
19+
argument.
20+
</p>
21+
</recommendation>
22+
23+
<example>
24+
<p>
25+
The following program snippet logs information about an unauthorized access attempt. The
26+
log message includes the user name, and the user's IP address is passed as an additional
27+
argument to <code>Kernel.printf</code> to be appended to the message:
28+
</p>
29+
<sample src="examples/tainted_format_string_bad.rb"/>
30+
<p>
31+
However, if a malicious user provides a format specified such as <code>%s</code>
32+
as their user name, <code>Kernel.printf</code> will throw an exception as there
33+
are too few arguments to satisfy the format. This can result in denial of
34+
service or leaking of internal information to the attacker via a stack trace.
35+
</p>
36+
<p>
37+
Instead, the user name should be included using the <code>%s</code> specifier:
38+
</p>
39+
<sample src="examples/tainted_format_string_good.rb"/>
40+
41+
<p>
42+
Alternatively, string interpolation should be used exclusively:
43+
</p>
44+
<sample src="examples/tainted_format_string_interpolation.rb"/>
45+
</example>
46+
47+
<references>
48+
<li>Ruby documentation for <a href="https://docs.ruby-lang.org/en/3.1/Kernel.html#method-i-sprintf">format strings</a>.</li>
49+
</references>
50+
</qhelp>

‎ruby/ql/src/queries/security/cwe-134/TaintedFormatString.ql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* @name Use of externally-controlled format string
3+
* @description Using external input in format strings can lead to garbled output.
4+
* @kind path-problem
5+
* @problem.severity warning
6+
* @security-severity 7.3
7+
* @precision high
8+
* @id rb/tainted-format-string
9+
* @tags security
10+
* external/cwe/cwe-134
11+
*/
12+
13+
import ruby
14+
import codeql.ruby.DataFlow
15+
import codeql.ruby.security.TaintedFormatStringQuery
16+
import DataFlow::PathGraph
17+
18+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
19+
where cfg.hasFlowPath(source, sink)
20+
select sink.getNode(), source, sink, "$@ flows here and is used in a format string.",
21+
source.getNode(), "User-provided value"

0 commit comments

Comments
 (0)