Skip to content

Commit 73886b1

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#8948 from geoffw0/xxe3
C++: Add support for SAXParser to the CWE-611 XXE query.
2 parents 806dacb + 034c4fa commit 73886b1

File tree

6 files changed

+118
-32
lines changed

6 files changed

+118
-32
lines changed

‎cpp/ql/src/Security/CWE/CWE-611/XXE.ql

Lines changed: 55 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import cpp
1616
import semmle.code.cpp.ir.dataflow.DataFlow
1717
import DataFlow::PathGraph
1818
import semmle.code.cpp.ir.IR
19+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
1920

2021
/**
2122
* A flow state representing a possible configuration of an XML object.
@@ -57,42 +58,51 @@ class XercesDOMParserClass extends Class {
5758
}
5859

5960
/**
60-
* Gets a valid flow state for `XercesDOMParser` flow.
61+
* The `SAXParser` class.
62+
*/
63+
class SAXParserClass extends Class {
64+
SAXParserClass() { this.hasName("SAXParser") }
65+
}
66+
67+
/**
68+
* Gets a valid flow state for `AbstractDOMParser` or `SAXParser` flow.
6169
*
62-
* These flow states take the form `XercesDOM-A-B`, where:
70+
* These flow states take the form `Xerces-A-B`, where:
6371
* - A is 1 if `setDisableDefaultEntityResolution` is `true`, 0 otherwise.
6472
* - B is 1 if `setCreateEntityReferenceNodes` is `true`, 0 otherwise.
6573
*/
66-
predicate encodeXercesDOMFlowState(
74+
predicate encodeXercesFlowState(
6775
string flowstate, int disabledDefaultEntityResolution, int createEntityReferenceNodes
6876
) {
69-
flowstate = "XercesDOM-0-0" and
77+
flowstate = "Xerces-0-0" and
7078
disabledDefaultEntityResolution = 0 and
7179
createEntityReferenceNodes = 0
7280
or
73-
flowstate = "XercesDOM-0-1" and
81+
flowstate = "Xerces-0-1" and
7482
disabledDefaultEntityResolution = 0 and
7583
createEntityReferenceNodes = 1
7684
or
77-
flowstate = "XercesDOM-1-0" and
85+
flowstate = "Xerces-1-0" and
7886
disabledDefaultEntityResolution = 1 and
7987
createEntityReferenceNodes = 0
8088
or
81-
flowstate = "XercesDOM-1-1" and
89+
flowstate = "Xerces-1-1" and
8290
disabledDefaultEntityResolution = 1 and
8391
createEntityReferenceNodes = 1
8492
}
8593

8694
/**
87-
* A flow state representing the configuration of a `XercesDOMParser` object.
95+
* A flow state representing the configuration of an `AbstractDOMParser` or
96+
* `SAXParser` object.
8897
*/
89-
class XercesDOMParserFlowState extends XXEFlowState {
90-
XercesDOMParserFlowState() { encodeXercesDOMFlowState(this, _, _) }
98+
class XercesFlowState extends XXEFlowState {
99+
XercesFlowState() { encodeXercesFlowState(this, _, _) }
91100
}
92101

93102
/**
94103
* A flow state transformer for a call to
95-
* `AbstractDOMParser.setDisableDefaultEntityResolution`. Transforms the flow
104+
* `AbstractDOMParser.setDisableDefaultEntityResolution` or
105+
* `SAXParser.setDisableDefaultEntityResolution`. Transforms the flow
96106
* state through the qualifier according to the setting in the parameter.
97107
*/
98108
class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {
@@ -101,7 +111,10 @@ class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {
101111
DisableDefaultEntityResolutionTranformer() {
102112
exists(Call call, Function f |
103113
call.getTarget() = f and
104-
f.getDeclaringType() instanceof AbstractDOMParserClass and
114+
(
115+
f.getDeclaringType() instanceof AbstractDOMParserClass or
116+
f.getDeclaringType() instanceof SAXParserClass
117+
) and
105118
f.hasName("setDisableDefaultEntityResolution") and
106119
this = call.getQualifier() and
107120
newValue = call.getArgument(0)
@@ -110,13 +123,13 @@ class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {
110123

111124
final override XXEFlowState transform(XXEFlowState flowstate) {
112125
exists(int createEntityReferenceNodes |
113-
encodeXercesDOMFlowState(flowstate, _, createEntityReferenceNodes) and
126+
encodeXercesFlowState(flowstate, _, createEntityReferenceNodes) and
114127
(
115-
newValue.getValue().toInt() = 1 and // true
116-
encodeXercesDOMFlowState(result, 1, createEntityReferenceNodes)
128+
globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true
129+
encodeXercesFlowState(result, 1, createEntityReferenceNodes)
117130
or
118-
not newValue.getValue().toInt() = 1 and // false or unknown
119-
encodeXercesDOMFlowState(result, 0, createEntityReferenceNodes)
131+
not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown
132+
encodeXercesFlowState(result, 0, createEntityReferenceNodes)
120133
)
121134
)
122135
}
@@ -142,27 +155,31 @@ class CreateEntityReferenceNodesTranformer extends XXEFlowStateTranformer {
142155

143156
final override XXEFlowState transform(XXEFlowState flowstate) {
144157
exists(int disabledDefaultEntityResolution |
145-
encodeXercesDOMFlowState(flowstate, disabledDefaultEntityResolution, _) and
158+
encodeXercesFlowState(flowstate, disabledDefaultEntityResolution, _) and
146159
(
147-
newValue.getValue().toInt() = 1 and // true
148-
encodeXercesDOMFlowState(result, disabledDefaultEntityResolution, 1)
160+
globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true
161+
encodeXercesFlowState(result, disabledDefaultEntityResolution, 1)
149162
or
150-
not newValue.getValue().toInt() = 1 and // false or unknown
151-
encodeXercesDOMFlowState(result, disabledDefaultEntityResolution, 0)
163+
not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown
164+
encodeXercesFlowState(result, disabledDefaultEntityResolution, 0)
152165
)
153166
)
154167
}
155168
}
156169

157170
/**
158-
* The `AbstractDOMParser.parse` method.
171+
* The `AbstractDOMParser.parse` or `SAXParser.parse` method.
159172
*/
160173
class ParseFunction extends Function {
161-
ParseFunction() { this.getClassAndName("parse") instanceof AbstractDOMParserClass }
174+
ParseFunction() {
175+
this.getClassAndName("parse") instanceof AbstractDOMParserClass or
176+
this.getClassAndName("parse") instanceof SAXParserClass
177+
}
162178
}
163179

164180
/**
165-
* The `createLSParser` function that returns a newly created `LSParser` object.
181+
* The `createLSParser` function that returns a newly created `DOMLSParser`
182+
* object.
166183
*/
167184
class CreateLSParser extends Function {
168185
CreateLSParser() {
@@ -184,14 +201,23 @@ class XXEConfiguration extends DataFlow::Configuration {
184201
call.getStaticCallTarget() = any(XercesDOMParserClass c).getAConstructor() and
185202
node.asInstruction().(WriteSideEffectInstruction).getDestinationAddress() =
186203
call.getThisArgument() and
187-
encodeXercesDOMFlowState(flowstate, 0, 1) // default configuration
204+
encodeXercesFlowState(flowstate, 0, 1) // default configuration
188205
)
189206
or
190207
// source is the result of a call to `createLSParser`.
191208
exists(Call call |
192209
call.getTarget() instanceof CreateLSParser and
193210
call = node.asExpr() and
194-
encodeXercesDOMFlowState(flowstate, 0, 1) // default configuration
211+
encodeXercesFlowState(flowstate, 0, 1) // default configuration
212+
)
213+
or
214+
// source is the write on `this` of a call to the `SAXParser`
215+
// constructor.
216+
exists(CallInstruction call |
217+
call.getStaticCallTarget() = any(SAXParserClass c).getAConstructor() and
218+
node.asInstruction().(WriteSideEffectInstruction).getDestinationAddress() =
219+
call.getThisArgument() and
220+
encodeXercesFlowState(flowstate, 0, 1) // default configuration
195221
)
196222
}
197223

@@ -201,8 +227,8 @@ class XXEConfiguration extends DataFlow::Configuration {
201227
call.getTarget() instanceof ParseFunction and
202228
call.getQualifier() = node.asConvertedExpr()
203229
) and
204-
flowstate instanceof XercesDOMParserFlowState and
205-
not encodeXercesDOMFlowState(flowstate, 1, 1) // safe configuration
230+
flowstate instanceof XercesFlowState and
231+
not encodeXercesFlowState(flowstate, 1, 1) // safe configuration
206232
}
207233

208234
override predicate isAdditionalFlowStep(

‎cpp/ql/src/change-notes/2022-04-28-external-entity-expansion.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The "XML external entity expansion" (`cpp/external-entity-expansion`) query has been extended to support a broader selection of XML libraries and interfaces.

‎cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
edges
2+
| tests2.cpp:20:17:20:31 | SAXParser output argument | tests2.cpp:22:2:22:2 | p |
3+
| tests2.cpp:33:17:33:31 | SAXParser output argument | tests2.cpp:37:2:37:2 | p |
24
| tests.cpp:33:23:33:43 | XercesDOMParser output argument | tests.cpp:35:2:35:2 | p |
35
| tests.cpp:46:23:46:43 | XercesDOMParser output argument | tests.cpp:49:2:49:2 | p |
46
| tests.cpp:53:19:53:19 | VariableAddress [post update] | tests.cpp:55:2:55:2 | p |
@@ -27,6 +29,10 @@ edges
2729
| tests.cpp:146:18:146:18 | q | tests.cpp:134:39:134:39 | p |
2830
| tests.cpp:150:19:150:32 | call to createLSParser | tests.cpp:152:2:152:2 | p |
2931
nodes
32+
| tests2.cpp:20:17:20:31 | SAXParser output argument | semmle.label | SAXParser output argument |
33+
| tests2.cpp:22:2:22:2 | p | semmle.label | p |
34+
| tests2.cpp:33:17:33:31 | SAXParser output argument | semmle.label | SAXParser output argument |
35+
| tests2.cpp:37:2:37:2 | p | semmle.label | p |
3036
| tests.cpp:33:23:33:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
3137
| tests.cpp:35:2:35:2 | p | semmle.label | p |
3238
| tests.cpp:46:23:46:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
@@ -66,6 +72,8 @@ nodes
6672
| tests.cpp:152:2:152:2 | p | semmle.label | p |
6773
subpaths
6874
#select
75+
| tests2.cpp:22:2:22:2 | p | tests2.cpp:20:17:20:31 | SAXParser output argument | tests2.cpp:22:2:22:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:20:17:20:31 | SAXParser output argument | XML parser |
76+
| tests2.cpp:37:2:37:2 | p | tests2.cpp:33:17:33:31 | SAXParser output argument | tests2.cpp:37:2:37:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:33:17:33:31 | SAXParser output argument | XML parser |
6977
| tests.cpp:35:2:35:2 | p | tests.cpp:33:23:33:43 | XercesDOMParser output argument | tests.cpp:35:2:35:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:33:23:33:43 | XercesDOMParser output argument | XML parser |
7078
| tests.cpp:49:2:49:2 | p | tests.cpp:46:23:46:43 | XercesDOMParser output argument | tests.cpp:49:2:49:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:46:23:46:43 | XercesDOMParser output argument | XML parser |
7179
| tests.cpp:57:2:57:2 | p | tests.cpp:53:23:53:43 | XercesDOMParser output argument | tests.cpp:57:2:57:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:53:23:53:43 | XercesDOMParser output argument | XML parser |

‎cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@
44

55
// ---
66

7-
class SecurityManager;
8-
class InputSource;
7+
8+
99

1010
class AbstractDOMParser {
1111
public:

‎cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,4 @@
1-
// library functions for rule CWE-611
1+
// library/common functions for rule CWE-611
22

3+
class SecurityManager;
4+
class InputSource;

‎cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// test cases for rule CWE-611
2+
3+
#include "tests.h"
4+
5+
// ---
6+
7+
class SAXParser
8+
{
9+
public:
10+
SAXParser();
11+
12+
void setDisableDefaultEntityResolution(bool); // default is false
13+
void setSecurityManager(SecurityManager *const manager);
14+
void parse(const InputSource &data);
15+
};
16+
17+
// ---
18+
19+
void test2_1(InputSource &data) {
20+
SAXParser *p = new SAXParser();
21+
22+
p->parse(data); // BAD (parser not correctly configured)
23+
}
24+
25+
void test2_2(InputSource &data) {
26+
SAXParser *p = new SAXParser();
27+
28+
p->setDisableDefaultEntityResolution(true);
29+
p->parse(data); // GOOD
30+
}
31+
32+
void test2_3(InputSource &data) {
33+
SAXParser *p = new SAXParser();
34+
bool v = false;
35+
36+
p->setDisableDefaultEntityResolution(v);
37+
p->parse(data); // BAD (parser not correctly configured)
38+
}
39+
40+
void test2_4(InputSource &data) {
41+
SAXParser *p = new SAXParser();
42+
bool v = true;
43+
44+
p->setDisableDefaultEntityResolution(v);
45+
p->parse(data); // GOOD
46+
}

0 commit comments

Comments
 (0)