Skip to content

Commit 9eee16b

Browse files
lcarteylcartey
authored
Merge pull request github#3091 from hvitved/csharp/xpath-injection-more-sinks
C#: Teach XPath injection query about `XPathNavigator`
2 parents 2c7af72 + 2d90e7d commit 9eee16b

File tree

5 files changed

+108
-14
lines changed

5 files changed

+108
-14
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +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 methods on `System.Xml.XPath.XPathNavigator` objects. |
2425

2526
## Removal of old queries
2627

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,23 @@ module SystemXmlXPath {
2121
class XPathExpression extends Class {
2222
XPathExpression() { this.hasName("XPathExpression") }
2323
}
24+
25+
/** The `System.Xml.XPath.XPathNavigator` class. */
26+
class XPathNavigator extends Class {
27+
XPathNavigator() { this.hasName("XPathNavigator") }
28+
29+
/** Gets a method that selects nodes. */
30+
csharp::Method getASelectMethod() {
31+
result = this.getAMethod() and result.getName().matches("Select%")
32+
}
33+
34+
/** Gets the `Compile` method. */
35+
csharp::Method getCompileMethod() { result = this.getAMethod("Compile") }
36+
37+
/** Gets an `Evaluate` method. */
38+
csharp::Method getAnEvaluateMethod() { result = this.getAMethod("Evaluate") }
39+
40+
/** Gets a `Matches` method. */
41+
csharp::Method getAMatchesMethod() { result = this.getAMethod("Matches") }
42+
}
2443
}

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

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

68+
/** The `xpath` argument to an `XPathNavigator` call. */
69+
class XmlNavigatorSink extends Sink {
70+
XmlNavigatorSink() {
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+
)
79+
}
80+
}
81+
6882
private class SimpleTypeSanitizer extends Sanitizer, SimpleTypeSanitizedExpr { }
6983

7084
private class GuidSanitizer extends Sanitizer, GuidSanitizedExpr { }

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

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// semmle-extractor-options: ${testdir}/../../../resources/stubs/System.Web.cs /r:System.Collections.Specialized.dll ${testdir}/../../../resources/stubs/System.Data.cs /r:System.Private.Xml.dll /r:System.Xml.XPath.dll /r:System.Data.Common.dll
1+
// semmle-extractor-options: ${testdir}/../../../resources/stubs/System.Web.cs /r:System.Collections.Specialized.dll ${testdir}/../../../resources/stubs/System.Data.cs /r:System.Private.Xml.dll /r:System.Xml.XPath.dll /r:System.Data.Common.dll /r:System.Runtime.Extensions.dll
22

33
using System;
44
using System.Web;
@@ -12,14 +12,49 @@ 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
22-
XPathExpression.Compile("//users/user[login/text()=$username]/home_dir/text()");
24+
var expr = XPathExpression.Compile("//users/user[login/text()=$username]/home_dir/text()");
25+
26+
var doc = new XPathDocument("");
27+
var nav = doc.CreateNavigator();
28+
29+
// BAD
30+
nav.Select(s);
31+
32+
// GOOD
33+
nav.Select(expr);
34+
35+
// BAD
36+
nav.SelectSingleNode(s);
37+
38+
// GOOD
39+
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);
2358
}
2459

2560
public bool IsReusable

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

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +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:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:16:33:16:136 | ... + ... |
5-
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | XPathInjection.cs:19:29:19:132 | ... + ... |
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 |
616
nodes
717
| XPathInjection.cs:12:27:12:49 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
818
| XPathInjection.cs:13:27:13:49 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
9-
| XPathInjection.cs:16:33:16:136 | ... + ... | semmle.label | ... + ... |
10-
| XPathInjection.cs:19:29:19:132 | ... + ... | 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 |
1126
#select
12-
| 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 |
13-
| 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 |
14-
| 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 |
15-
| 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 |
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)