Skip to content

Commit 4ab6f5c

Browse files
chanel-ychanel-y
authored
Merge pull request #290 from microsoft/require-string-ops-in-sql-injection
PS: Require string concat in SQL injection query
2 parents a130224 + bfb10a2 commit 4ab6f5c

File tree

5 files changed

+56
-19
lines changed

5 files changed

+56
-19
lines changed

‎powershell/ql/lib/semmle/code/powershell/controlflow/CfgNodes.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -965,7 +965,7 @@ module ExprNodes {
965965

966966
override ExpandableSubExpr getExpr() { result = e }
967967

968-
ExprCfgNode getSubExpr() { e.hasCfgChild(e.getExpr(), this, result) }
968+
StmtNodes::StmtBlockCfgNode getSubExpr() { e.hasCfgChild(e.getExpr(), this, result) }
969969
}
970970

971971
private class UsingExprChildMapping extends ExprChildMapping, UsingExpr {

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

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ private import TaintTrackingPublic
44
private import semmle.code.powershell.Cfg
55
private import semmle.code.powershell.dataflow.DataFlow
66
private import FlowSummaryImpl as FlowSummaryImpl
7+
private import PipelineReturns as PipelineReturns
78

89
/**
910
* Holds if `node` should be a sanitizer in all global taint flow configurations
@@ -21,6 +22,44 @@ predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c)
2122
c.isAnyPositional()
2223
}
2324

25+
private module ExpandableSubExprInput implements PipelineReturns::InputSig {
26+
additional predicate isSourceImpl(
27+
CfgNodes::ExprNodes::ExpandableSubExprCfgNode expandableSubExpr, CfgNodes::AstCfgNode source
28+
) {
29+
source = expandableSubExpr.getSubExpr()
30+
}
31+
32+
predicate isSource(CfgNodes::AstCfgNode source) { isSourceImpl(_, source) }
33+
}
34+
35+
/**
36+
* Holds if `nodeFrom` flows to `nodeTo` via string interpolation.
37+
*/
38+
predicate stringInterpolationTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
39+
// flow from $x to "Hello $x"
40+
exists(CfgNodes::ExprNodes::ExpandableStringExprCfgNode es |
41+
nodeFrom.asExpr() = es.getAnExpr() and
42+
nodeTo.asExpr() = es
43+
)
44+
or
45+
// Flow from $x to $($x)
46+
exists(
47+
CfgNodes::ExprNodes::ExpandableSubExprCfgNode es,
48+
CfgNodes::StmtNodes::StmtBlockCfgNode blockStmt
49+
|
50+
ExpandableSubExprInput::isSourceImpl(es, blockStmt) and
51+
nodeFrom.asExpr() = PipelineReturns::Make<ExpandableSubExprInput>::getAReturn(blockStmt) and
52+
nodeTo.asExpr() = es
53+
)
54+
}
55+
56+
predicate operationTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
57+
exists(CfgNodes::ExprNodes::OperationCfgNode op |
58+
op = nodeTo.asExpr() and
59+
op.getAnOperand() = nodeFrom.asExpr()
60+
)
61+
}
62+
2463
cached
2564
private module Cached {
2665
private import semmle.code.powershell.dataflow.internal.DataFlowImplCommon as DataFlowImplCommon
@@ -36,16 +75,10 @@ private module Cached {
3675
predicate defaultAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, string model) {
3776
(
3877
// Flow from an operand to an operation
39-
exists(CfgNodes::ExprNodes::OperationCfgNode op |
40-
op = nodeTo.asExpr() and
41-
op.getAnOperand() = nodeFrom.asExpr()
42-
)
78+
operationTaintStep(nodeFrom, nodeTo)
4379
or
4480
// Flow through string interpolation
45-
exists(CfgNodes::ExprNodes::ExpandableStringExprCfgNode es |
46-
nodeFrom.asExpr() = es.getAnExpr() and
47-
nodeTo.asExpr() = es
48-
)
81+
stringInterpolationTaintStep(nodeFrom, nodeTo)
4982
or
5083
// Although flow through collections is modeled precisely using stores/reads, we still
5184
// allow flow out of a _tainted_ collection. This is needed in order to support taint-

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
private import powershell
2-
private import TaintTrackingPrivate
2+
private import TaintTrackingPrivate as Private
33
private import semmle.code.powershell.Cfg
44
private import semmle.code.powershell.dataflow.DataFlow
55

@@ -19,4 +19,8 @@ predicate localExprTaint(CfgNodes::ExprCfgNode e1, CfgNodes::ExprCfgNode e2) {
1919
localTaintStep*(DataFlow::exprNode(e1), DataFlow::exprNode(e2))
2020
}
2121

22-
predicate localTaintStep = localTaintStepCached/2;
22+
predicate stringInterpolationTaintStep = Private::stringInterpolationTaintStep/2;
23+
24+
predicate operationTaintStep = Private::operationTaintStep/2;
25+
26+
predicate localTaintStep = Private::localTaintStepCached/2;

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,15 @@ edges
33
| test.ps1:1:1:1:10 | userinput | test.ps1:8:1:8:6 | query | provenance | |
44
| test.ps1:1:1:1:10 | userinput | test.ps1:17:24:17:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | provenance | |
55
| test.ps1:1:1:1:10 | userinput | test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | provenance | |
6-
| test.ps1:1:1:1:10 | userinput | test.ps1:78:13:78:22 | userinput | provenance | |
6+
| test.ps1:1:1:1:10 | userinput | test.ps1:78:13:78:59 | SELECT * FROM Customers WHERE id = $userinput | provenance | |
77
| test.ps1:1:1:1:10 | userinput | test.ps1:128:28:128:37 | userinput | provenance | |
88
| test.ps1:1:14:1:45 | Call to read-host | test.ps1:1:1:1:10 | userinput | provenance | Src:MaD:0 |
99
| test.ps1:4:1:4:6 | query | test.ps1:5:72:5:77 | query | provenance | |
1010
| test.ps1:8:1:8:6 | query | test.ps1:9:72:9:77 | query | provenance | |
1111
| test.ps1:72:1:72:11 | QueryConn2 [element Query] | test.ps1:81:15:81:25 | QueryConn2 | provenance | |
1212
| test.ps1:72:15:79:1 | ${...} [element Query] | test.ps1:72:1:72:11 | QueryConn2 [element Query] | provenance | |
13-
| test.ps1:78:13:78:22 | userinput | test.ps1:72:15:79:1 | ${...} [element Query] | provenance | |
14-
| test.ps1:121:9:121:56 | unvalidated | test.ps1:125:92:125:103 | unvalidated | provenance | |
13+
| test.ps1:78:13:78:59 | SELECT * FROM Customers WHERE id = $userinput | test.ps1:72:15:79:1 | ${...} [element Query] | provenance | |
14+
| test.ps1:121:9:121:56 | unvalidated | test.ps1:125:92:125:143 | SELECT * FROM Customers where id = $($unvalidated) | provenance | |
1515
| test.ps1:128:28:128:37 | userinput | test.ps1:121:9:121:56 | unvalidated | provenance | |
1616
nodes
1717
| test.ps1:1:1:1:10 | userinput | semmle.label | userinput |
@@ -24,10 +24,10 @@ nodes
2424
| test.ps1:28:24:28:76 | SELECT * FROM MyTable WHERE MyColumn = '$userinput' | semmle.label | SELECT * FROM MyTable WHERE MyColumn = '$userinput' |
2525
| test.ps1:72:1:72:11 | QueryConn2 [element Query] | semmle.label | QueryConn2 [element Query] |
2626
| test.ps1:72:15:79:1 | ${...} [element Query] | semmle.label | ${...} [element Query] |
27-
| test.ps1:78:13:78:22 | userinput | semmle.label | userinput |
27+
| test.ps1:78:13:78:59 | SELECT * FROM Customers WHERE id = $userinput | semmle.label | SELECT * FROM Customers WHERE id = $userinput |
2828
| test.ps1:81:15:81:25 | QueryConn2 | semmle.label | QueryConn2 |
2929
| test.ps1:121:9:121:56 | unvalidated | semmle.label | unvalidated |
30-
| test.ps1:125:92:125:103 | unvalidated | semmle.label | unvalidated |
30+
| test.ps1:125:92:125:143 | SELECT * FROM Customers where id = $($unvalidated) | semmle.label | SELECT * FROM Customers where id = $($unvalidated) |
3131
| test.ps1:128:28:128:37 | userinput | semmle.label | userinput |
3232
subpaths
3333
#select
@@ -36,4 +36,4 @@ subpaths
3636
| 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 |
3737
| 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 |
3838
| 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 |
39-
| test.ps1:125:92:125:103 | unvalidated | test.ps1:1:14:1:45 | Call to read-host | test.ps1:125:92:125:103 | unvalidated | This SQL query depends on a $@. | test.ps1:1:14:1:45 | Call to read-host | read from stdin |
39+
| test.ps1:125:92:125:143 | SELECT * FROM Customers where id = $($unvalidated) | test.ps1:1:14:1:45 | Call to read-host | test.ps1:125:92:125:143 | SELECT * FROM Customers where id = $($unvalidated) | 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: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ $QueryConn2 = @{
7575
Username = "MyUserName"
7676
Password = "MyPassword"
7777
ConnectionTimeout = 0
78-
Query = $userinput
78+
Query = "SELECT * FROM Customers WHERE id = $userinput"
7979
}
8080

8181
Invoke-Sqlcmd @QueryConn2 # BAD
@@ -122,7 +122,7 @@ function With-Validation() {
122122
)
123123

124124
Invoke-Sqlcmd -unknown $userinput -ServerInstance "MyServer" -Database "MyDatabase" -q $validated # GOOD
125-
Invoke-Sqlcmd -unknown $userinput -ServerInstance "MyServer" -Database "MyDatabase" -q $unvalidated # BAD
125+
Invoke-Sqlcmd -unknown $userinput -ServerInstance "MyServer" -Database "MyDatabase" -q "SELECT * FROM Customers where id = $($unvalidated)" # BAD
126126
}
127127

128128
With-Validation $userinput $userinput

0 commit comments

Comments
 (0)