Skip to content

Commit 12bd709

Browse files
authored
Merge pull request #21341 from owen-mc/rb/accept-mad-sanitizers
Ruby: Accept MaD sanitizers for queries with MaD sinks and convert some existing sanitizers
2 parents 480ae61 + 1d6b8c5 commit 12bd709

24 files changed

+209
-138
lines changed
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* We now track taint flow through `Shellwords.escape` and `Shellwords.shellescape` for all queries except command injection, for which they are sanitizers.

ruby/ql/lib/codeql/ruby/Concepts.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ private import codeql.ruby.CFG
99
private import codeql.ruby.DataFlow
1010
private import codeql.ruby.dataflow.internal.DataFlowImplSpecific
1111
private import codeql.ruby.Frameworks
12+
private import codeql.ruby.frameworks.data.internal.ApiGraphModels
1213
private import codeql.ruby.dataflow.RemoteFlowSources
1314
private import codeql.ruby.ApiGraphs
1415
private import codeql.ruby.Regexp as RE
@@ -95,6 +96,10 @@ module SqlSanitization {
9596
abstract class Range extends DataFlow::Node { }
9697
}
9798

99+
private class ExternalSqlInjectionSanitizer extends SqlSanitization::Range {
100+
ExternalSqlInjectionSanitizer() { ModelOutput::barrierNode(this, "sql-injection") }
101+
}
102+
98103
/**
99104
* A data-flow node that executes a regular expression.
100105
*
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/ruby-all
4+
extensible: summaryModel
5+
data:
6+
- ['Mysql2::Client!', 'Method[escape]', 'Argument[0]', 'ReturnValue', 'taint']
7+
- addsTo:
8+
pack: codeql/ruby-all
9+
extensible: barrierModel
10+
data:
11+
- ['Mysql2::Client!', 'Method[escape].ReturnValue', 'sql-injection']

ruby/ql/lib/codeql/ruby/frameworks/Mysql2.qll

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -48,26 +48,4 @@ module Mysql2 {
4848

4949
override DataFlow::Node getSql() { result = query }
5050
}
51-
52-
/**
53-
* A call to `Mysql2::Client.escape`, considered as a sanitizer for SQL statements.
54-
*/
55-
private class Mysql2EscapeSanitization extends SqlSanitization::Range {
56-
Mysql2EscapeSanitization() {
57-
this = API::getTopLevelMember("Mysql2").getMember("Client").getAMethodCall("escape")
58-
}
59-
}
60-
61-
/**
62-
* Flow summary for `Mysql2::Client.escape()`.
63-
*/
64-
private class EscapeSummary extends SummarizedCallable::Range {
65-
EscapeSummary() { this = "Mysql2::Client.escape()" }
66-
67-
override MethodCall getACall() { result = any(Mysql2EscapeSanitization c).asExpr().getExpr() }
68-
69-
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
70-
input = "Argument[0]" and output = "ReturnValue" and preservesValue = false
71-
}
72-
}
7351
}
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/ruby-all
4+
extensible: summaryModel
5+
data:
6+
- ['SQLite3::Database!', 'Method[quote]', 'Argument[0]', 'ReturnValue', 'taint']
7+
- addsTo:
8+
pack: codeql/ruby-all
9+
extensible: barrierModel
10+
data:
11+
- ['SQLite3::Database!', 'Method[quote].ReturnValue', 'sql-injection']

ruby/ql/lib/codeql/ruby/frameworks/Sqlite3.qll

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -76,26 +76,4 @@ module Sqlite3 {
7676

7777
override DataFlow::Node getSql() { result = this.getArgument(0) }
7878
}
79-
80-
/**
81-
* A call to `SQLite3::Database.quote`, considered as a sanitizer for SQL statements.
82-
*/
83-
private class SQLite3QuoteSanitization extends SqlSanitization {
84-
SQLite3QuoteSanitization() {
85-
this = API::getTopLevelMember("SQLite3").getMember("Database").getAMethodCall("quote")
86-
}
87-
}
88-
89-
/**
90-
* Flow summary for `SQLite3::Database.quote()`.
91-
*/
92-
private class QuoteSummary extends SummarizedCallable::Range {
93-
QuoteSummary() { this = "SQLite3::Database.quote()" }
94-
95-
override MethodCall getACall() { result = any(SQLite3QuoteSanitization c).asExpr().getExpr() }
96-
97-
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
98-
input = "Argument[0]" and output = "ReturnValue" and preservesValue = false
99-
}
100-
}
10179
}
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/ruby-all
4+
extensible: summaryModel
5+
data:
6+
- ['Shellwords!', 'Method[escape,shellescape]', 'Argument[0]', 'ReturnValue', 'taint']
7+
8+
- addsTo:
9+
pack: codeql/ruby-all
10+
extensible: barrierModel
11+
data:
12+
- ['Shellwords!', 'Method[escape,shellescape].ReturnValue', 'command-injection']

ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,4 +118,8 @@ module CodeInjection {
118118
private class ExternalCodeInjectionSink extends Sink {
119119
ExternalCodeInjectionSink() { ModelOutput::sinkNode(this, "code-injection") }
120120
}
121+
122+
private class ExternalCodeInjectionSanitizer extends Sanitizer {
123+
ExternalCodeInjectionSanitizer() { ModelOutput::barrierNode(this, "code-injection") }
124+
}
121125
}

ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,18 +43,20 @@ module CommandInjection {
4343
}
4444

4545
/**
46-
* A call to `Shellwords.escape` or `Shellwords.shellescape` sanitizes its input.
46+
* A call to `String#shellescape` sanitizes its input.
4747
*/
4848
class ShellwordsEscapeAsSanitizer extends Sanitizer {
4949
ShellwordsEscapeAsSanitizer() {
50-
this = API::getTopLevelMember("Shellwords").getAMethodCall(["escape", "shellescape"])
51-
or
52-
// The method is also added as `String#shellescape`.
50+
// The `Shellwords.shellescape` method is also added as `String#shellescape`.
5351
this.(DataFlow::CallNode).getMethodName() = "shellescape"
5452
}
5553
}
5654

5755
private class ExternalCommandInjectionSink extends Sink {
5856
ExternalCommandInjectionSink() { ModelOutput::sinkNode(this, "command-injection") }
5957
}
58+
59+
private class ExternalCommandInjectionSanitizer extends Sanitizer {
60+
ExternalCommandInjectionSanitizer() { ModelOutput::barrierNode(this, "command-injection") }
61+
}
6062
}

ruby/ql/lib/codeql/ruby/security/LogInjectionQuery.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,10 @@ class HtmlEscapingAsSanitizer extends Sanitizer {
6767
HtmlEscapingAsSanitizer() { this = any(HtmlEscaping esc).getOutput() }
6868
}
6969

70+
private class ExternalLogInjectionSanitizer extends Sanitizer {
71+
ExternalLogInjectionSanitizer() { ModelOutput::barrierNode(this, "log-injection") }
72+
}
73+
7074
private module LogInjectionConfig implements DataFlow::ConfigSig {
7175
predicate isSource(DataFlow::Node source) { source instanceof Source }
7276

0 commit comments

Comments
 (0)