Skip to content

Commit fc74a48

Browse files
hvitvedhvitved
committed
C#: More XPath injection sinks
1 parent 0d45700 commit fc74a48

File tree

5 files changed

+86
-33
lines changed

5 files changed

+86
-33
lines changed

‎change-notes/1.24/analysis-csharp.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ The following changes in version 1.24 affect C# analysis in all applications.
2121
| Potentially dangerous use of non-short-circuit logic (`cs/non-short-circuit`) | Fewer false positive results | Results have been removed when the expression contains an `out` parameter. |
2222
| Dereferenced variable may be null (`cs/dereferenced-value-may-be-null`) | More results | Results are reported from parameters with a default value of `null`. |
2323
| Useless assignment to local variable (`cs/useless-assignment-to-local`) | Fewer false positive results | Results have been removed when the value assigned is an (implicitly or explicitly) cast default-like value. For example, `var s = (string)null` and `string s = default`. |
24-
| XPath injection (`cs/xml/xpath-injection`) | More results | The query now recognizes calls to selection methods on `System.Xml.XPath.XPathNavigator` objects. |
24+
| XPath injection (`cs/xml/xpath-injection`) | More results | The query now recognizes calls to methods on `System.Xml.XPath.XPathNavigator` objects. |
2525

2626
## Removal of old queries
2727

‎csharp/ql/src/semmle/code/csharp/frameworks/system/xml/XPath.qll

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,5 +30,20 @@ module SystemXmlXPath {
3030
csharp::Method getASelectMethod() {
3131
result = this.getAMethod() and result.getName().matches("Select%")
3232
}
33+
34+
/** Gets the `Compile` method. */
35+
csharp::Method getCompileMethod() {
36+
result = this.getAMethod("Compile")
37+
}
38+
39+
/** Gets an `Evaluate` method. */
40+
csharp::Method getAnEvaluateMethod() {
41+
result = this.getAMethod("Evaluate")
42+
}
43+
44+
/** Gets a `Matches` method. */
45+
csharp::Method getAMatchesMethod() {
46+
result = this.getAMethod("Matches")
47+
}
3348
}
3449
}

‎csharp/ql/src/semmle/code/csharp/security/dataflow/XPathInjection.qll

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,17 @@ module XPathInjection {
6565
}
6666
}
6767

68-
/** The `xpath` argument to an `XPathNavigator.Select*(..)` call. */
68+
/** The `xpath` argument to an `XPathNavigator` call. */
6969
class XmlNavigatorSink extends Sink {
7070
XmlNavigatorSink() {
71-
this.getExpr() =
72-
any(SystemXmlXPath::XPathNavigator xmlNav)
73-
.getASelectMethod()
74-
.getACall()
75-
.getArgumentForName("xpath")
71+
exists(SystemXmlXPath::XPathNavigator xmlNav, Method m |
72+
this.getExpr() = m.getACall().getArgumentForName("xpath")
73+
|
74+
m = xmlNav.getASelectMethod() or
75+
m = xmlNav.getCompileMethod() or
76+
m = xmlNav.getAnEvaluateMethod() or
77+
m = xmlNav.getAMatchesMethod()
78+
)
7679
}
7780
}
7881

‎csharp/ql/test/query-tests/Security Features/CWE-643/XPathInjection.cs

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,13 @@ public void ProcessRequest(HttpContext ctx)
1212
string userName = ctx.Request.QueryString["userName"];
1313
string password = ctx.Request.QueryString["password"];
1414

15+
var s = "//users/user[login/text()='" + userName + "' and password/text() = '" + password + "']/home_dir/text()";
16+
1517
// BAD: User input used directly in an XPath expression
16-
XPathExpression.Compile("//users/user[login/text()='" + userName + "' and password/text() = '" + password + "']/home_dir/text()");
18+
XPathExpression.Compile(s);
1719
XmlNode xmlNode = null;
1820
// BAD: User input used directly in an XPath expression to SelectNodes
19-
xmlNode.SelectNodes("//users/user[login/text()='" + userName + "' and password/text() = '" + password + "']/home_dir/text()");
21+
xmlNode.SelectNodes(s);
2022

2123
// GOOD: Uses parameters to avoid including user input directly in XPath expression
2224
var expr = XPathExpression.Compile("//users/user[login/text()=$username]/home_dir/text()");
@@ -25,16 +27,34 @@ public void ProcessRequest(HttpContext ctx)
2527
var nav = doc.CreateNavigator();
2628

2729
// BAD
28-
nav.Select("//users/user[login/text()='" + userName + "' and password/text() = '" + password + "']/home_dir/text()");
29-
30-
// BAD
31-
nav.SelectSingleNode("//users/user[login/text()='" + userName + "' and password/text() = '" + password + "']/home_dir/text()");
30+
nav.Select(s);
3231

3332
// GOOD
3433
nav.Select(expr);
3534

35+
// BAD
36+
nav.SelectSingleNode(s);
37+
3638
// GOOD
3739
nav.SelectSingleNode(expr);
40+
41+
// BAD
42+
nav.Compile(s);
43+
44+
// GOOD
45+
nav.Compile("//users/user[login/text()=$username]/home_dir/text()");
46+
47+
// BAD
48+
nav.Evaluate(s);
49+
50+
// Good
51+
nav.Evaluate(expr);
52+
53+
// BAD
54+
nav.Matches(s);
55+
56+
// GOOD
57+
nav.Matches(expr);
3858
}
3959

4060
public bool IsReusable

‎csharp/ql/test/query-tests/Security Features/CWE-643/XPathInjection.expected

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,40 @@
11
edges
2-
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:16:33:16:136 | ... + ... |
3-
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:19:29:19:132 | ... + ... |
4-
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:28:20:28:123 | ... + ... |
5-
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:31:30:31:133 | ... + ... |
6-
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:16:33:16:136 | ... + ... |
7-
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:19:29:19:132 | ... + ... |
8-
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:28:20:28:123 | ... + ... |
9-
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:31:30:31:133 | ... + ... |
2+
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:18:33:18:33 | access to local variable s |
3+
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:21:29:21:29 | access to local variable s |
4+
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:30:20:30:20 | access to local variable s |
5+
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:36:30:36:30 | access to local variable s |
6+
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:42:21:42:21 | access to local variable s |
7+
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:48:22:48:22 | access to local variable s |
8+
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:54:21:54:21 | access to local variable s |
9+
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:18:33:18:33 | access to local variable s |
10+
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:21:29:21:29 | access to local variable s |
11+
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:30:20:30:20 | access to local variable s |
12+
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:36:30:36:30 | access to local variable s |
13+
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:42:21:42:21 | access to local variable s |
14+
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:48:22:48:22 | access to local variable s |
15+
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:54:21:54:21 | access to local variable s |
1016
nodes
1117
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
1218
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
13-
| XPathInjection.cs:16:33:16:136 | ... + ... | semmle.label | ... + ... |
14-
| XPathInjection.cs:19:29:19:132 | ... + ... | semmle.label | ... + ... |
15-
| XPathInjection.cs:28:20:28:123 | ... + ... | semmle.label | ... + ... |
16-
| XPathInjection.cs:31:30:31:133 | ... + ... | semmle.label | ... + ... |
19+
| XPathInjection.cs:18:33:18:33 | access to local variable s | semmle.label | access to local variable s |
20+
| XPathInjection.cs:21:29:21:29 | access to local variable s | semmle.label | access to local variable s |
21+
| XPathInjection.cs:30:20:30:20 | access to local variable s | semmle.label | access to local variable s |
22+
| XPathInjection.cs:36:30:36:30 | access to local variable s | semmle.label | access to local variable s |
23+
| XPathInjection.cs:42:21:42:21 | access to local variable s | semmle.label | access to local variable s |
24+
| XPathInjection.cs:48:22:48:22 | access to local variable s | semmle.label | access to local variable s |
25+
| XPathInjection.cs:54:21:54:21 | access to local variable s | semmle.label | access to local variable s |
1726
#select
18-
| XPathInjection.cs:16:33:16:136 | ... + ... | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:16:33:16:136 | ... + ... | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
19-
| XPathInjection.cs:16:33:16:136 | ... + ... | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:16:33:16:136 | ... + ... | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |
20-
| XPathInjection.cs:19:29:19:132 | ... + ... | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:19:29:19:132 | ... + ... | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
21-
| XPathInjection.cs:19:29:19:132 | ... + ... | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:19:29:19:132 | ... + ... | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |
22-
| XPathInjection.cs:28:20:28:123 | ... + ... | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:28:20:28:123 | ... + ... | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
23-
| XPathInjection.cs:28:20:28:123 | ... + ... | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:28:20:28:123 | ... + ... | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |
24-
| XPathInjection.cs:31:30:31:133 | ... + ... | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:31:30:31:133 | ... + ... | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
25-
| XPathInjection.cs:31:30:31:133 | ... + ... | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:31:30:31:133 | ... + ... | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |
27+
| XPathInjection.cs:18:33:18:33 | access to local variable s | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:18:33:18:33 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
28+
| XPathInjection.cs:18:33:18:33 | access to local variable s | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:18:33:18:33 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |
29+
| XPathInjection.cs:21:29:21:29 | access to local variable s | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:21:29:21:29 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
30+
| XPathInjection.cs:21:29:21:29 | access to local variable s | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:21:29:21:29 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |
31+
| XPathInjection.cs:30:20:30:20 | access to local variable s | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:30:20:30:20 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
32+
| XPathInjection.cs:30:20:30:20 | access to local variable s | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:30:20:30:20 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |
33+
| XPathInjection.cs:36:30:36:30 | access to local variable s | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:36:30:36:30 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
34+
| XPathInjection.cs:36:30:36:30 | access to local variable s | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:36:30:36:30 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |
35+
| XPathInjection.cs:42:21:42:21 | access to local variable s | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:42:21:42:21 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
36+
| XPathInjection.cs:42:21:42:21 | access to local variable s | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:42:21:42:21 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |
37+
| XPathInjection.cs:48:22:48:22 | access to local variable s | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:48:22:48:22 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
38+
| XPathInjection.cs:48:22:48:22 | access to local variable s | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:48:22:48:22 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |
39+
| XPathInjection.cs:54:21:54:21 | access to local variable s | XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:54:21:54:21 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:12:27:12:49 | access to property QueryString | User-provided value |
40+
| XPathInjection.cs:54:21:54:21 | access to local variable s | XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:54:21:54:21 | access to local variable s | $@ flows to here and is used in an XPath expression. | XPathInjection.cs:13:27:13:49 | access to property QueryString | User-provided value |

0 commit comments

Comments
 (0)