Skip to content

Commit 6ab05cd

Browse files
LWSimpkinsLWSimpkins
authored
Merge pull request #249 from microsoft/fix-fps-on-sql-injection
PS: Fix FPs on `powershell/microsoft/public/sql-injection`
2 parents 56977c7 + cb89695 commit 6ab05cd

File tree

5 files changed

+62
-7
lines changed

5 files changed

+62
-7
lines changed

‎powershell/ql/lib/semmle/code/powershell/dataflow/internal/TaintTrackingPrivate.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
1818
bindingset[node]
1919
predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c) {
2020
node instanceof ArgumentNode and
21-
c.isAnyElement()
21+
c.isAnyPositional()
2222
}
2323

2424
cached

‎powershell/ql/lib/semmle/code/powershell/security/SqlInjectionCustomizations.qll‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,14 @@ module SqlInjection {
2222
* A data flow sink for SQL-injection vulnerabilities.
2323
*/
2424
abstract class Sink extends DataFlow::Node {
25+
/** Gets a description of this sink. */
2526
abstract string getSinkType();
27+
28+
/**
29+
* Holds if this sink should allow for an implicit read of `cs` when
30+
* reached.
31+
*/
32+
predicate allowImplicitRead(DataFlow::ContentSet cs) { none() }
2633
}
2734

2835
/**
@@ -32,20 +39,32 @@ module SqlInjection {
3239

3340
/** A source of user input, considered as a flow source for command injection. */
3441
class FlowSourceAsSource extends Source instanceof SourceNode {
35-
override string getSourceType() { result = "user-provided value" }
42+
override string getSourceType() { result = SourceNode.super.getSourceType() }
3643
}
3744

3845
class InvokeSqlCmdSink extends Sink {
3946
InvokeSqlCmdSink() {
4047
exists(DataFlow::CallNode call | call.matchesName("Invoke-Sqlcmd") |
4148
this = call.getNamedArgument("query")
4249
or
50+
this = call.getNamedArgument("inputfile")
51+
or
4352
not call.hasNamedArgument("query") and
53+
not call.hasNamedArgument("inputfile") and
4454
this = call.getArgument(0)
55+
or
56+
// TODO: Here we really should pick a splat argument, but we don't yet extract whether an
57+
// argument is a splat argument.
58+
this = unique( | | call.getAnArgument())
4559
)
4660
}
4761

4862
override string getSinkType() { result = "call to Invoke-Sqlcmd" }
63+
64+
override predicate allowImplicitRead(DataFlow::ContentSet cs) {
65+
cs.getAStoreContent().(DataFlow::Content::KnownKeyContent).getIndex().asString().toLowerCase() =
66+
["query", "inputfile"]
67+
}
4968
}
5069

5170
class ConnectionStringWriteSink extends Sink {

‎powershell/ql/lib/semmle/code/powershell/security/SqlInjectionQuery.qll‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ private module Config implements DataFlow::ConfigSig {
1818
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
1919

2020
predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
21+
22+
predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet cs) {
23+
node.(Sink).allowImplicitRead(cs)
24+
}
2125
}
2226

2327
/**

‎powershell/ql/test/query-tests/security/cwe-089/SqlInjection.expected‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,22 @@ edges
33
| test.ps1:1:14:1:45 | Call to read-host | test.ps1:9:72:9:77 | query | provenance | Src:MaD:0 |
44
| test.ps1:1:14:1:45 | Call to read-host | test.ps1:17:24:17:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | provenance | Src:MaD:0 |
55
| test.ps1:1:14:1:45 | Call to read-host | test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | provenance | Src:MaD:0 |
6+
| test.ps1:1:14:1:45 | Call to read-host | test.ps1:78:13:78:22 | userinput | provenance | Src:MaD:0 |
7+
| test.ps1:72:15:79:1 | ${...} [element Query] | test.ps1:81:15:81:25 | QueryConn2 | provenance | |
8+
| test.ps1:78:13:78:22 | userinput | test.ps1:72:15:79:1 | ${...} [element Query] | provenance | |
69
nodes
710
| test.ps1:1:14:1:45 | Call to read-host | semmle.label | Call to read-host |
811
| test.ps1:5:72:5:77 | query | semmle.label | query |
912
| test.ps1:9:72:9:77 | query | semmle.label | query |
1013
| test.ps1:17:24:17:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | semmle.label | SELECT * FROM MyTable WHERE MyColumn = '$userinput' |
1114
| test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | semmle.label | SELECT * FROM MyTable WHERE MyColumn = '$userinput' |
15+
| test.ps1:72:15:79:1 | ${...} [element Query] | semmle.label | ${...} [element Query] |
16+
| test.ps1:78:13:78:22 | userinput | semmle.label | userinput |
17+
| test.ps1:81:15:81:25 | QueryConn2 | semmle.label | QueryConn2 |
1218
subpaths
1319
#select
14-
| test.ps1:5:72:5:77 | query | test.ps1:1:14:1:45 | Call to read-host | test.ps1:5:72:5:77 | query | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | user-provided value |
15-
| test.ps1:9:72:9:77 | query | test.ps1:1:14:1:45 | Call to read-host | test.ps1:9:72:9:77 | query | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | user-provided value |
16-
| test.ps1:17:24:17:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | test.ps1:1:14:1:45 | Call to read-host | test.ps1:17:24:17:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | user-provided value |
17-
| test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | test.ps1:1:14:1:45 | Call to read-host | test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | user-provided value |
20+
| test.ps1:5:72:5:77 | query | test.ps1:1:14:1:45 | Call to read-host | test.ps1:5:72:5:77 | query | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |
21+
| test.ps1:9:72:9:77 | query | test.ps1:1:14:1:45 | Call to read-host | test.ps1:9:72:9:77 | query | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |
22+
| test.ps1:17:24:17:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | test.ps1:1:14:1:45 | Call to read-host | test.ps1:17:24:17:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |
23+
| test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | test.ps1:1:14:1:45 | Call to read-host | test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |
24+
| test.ps1:81:15:81:25 | QueryConn2 | test.ps1:1:14:1:45 | Call to read-host | test.ps1:81:15:81:25 | QueryConn2 | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |

‎powershell/ql/test/query-tests/security/cwe-089/test.ps1‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,4 +53,29 @@ $parameter = $command.Parameters.Add("?", [System.Data.OleDb.OleDbType]::VarChar
5353
$parameter.Value = $userinput # GOOD
5454
$reader = $command.ExecuteReader()
5555
$reader.Close()
56-
$connection.Close()
56+
$connection.Close()
57+
58+
$server = $Env:SERVER_INSTANCE
59+
Invoke-Sqlcmd -ServerInstance $server -Database "MyDatabase" -InputFile "Foo/Bar/query.sql" # GOOD
60+
61+
$QueryConn = @{
62+
Database = "MyDB"
63+
ServerInstance = $server
64+
Username = "MyUserName"
65+
Password = "MyPassword"
66+
ConnectionTimeout = 0
67+
Query = ""
68+
}
69+
70+
Invoke-Sqlcmd @QueryConn # GOOD
71+
72+
$QueryConn2 = @{
73+
Database = "MyDB"
74+
ServerInstance = "MyServer"
75+
Username = "MyUserName"
76+
Password = "MyPassword"
77+
ConnectionTimeout = 0
78+
Query = $userinput
79+
}
80+
81+
Invoke-Sqlcmd @QueryConn2 # BAD

0 commit comments

Comments
 (0)