Skip to content

Commit ff58abb

Browse files
luchua-bcluchua-bc
committed
Revamp the sink code
1 parent 81de1b1 commit ff58abb

3 files changed

Lines changed: 160 additions & 135 deletions

File tree

‎java/ql/src/experimental/Security/CWE/CWE-522/InsecureBasicAuth.ql‎

Lines changed: 115 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -32,32 +32,38 @@ class ApacheHttpRequest extends RefType {
3232
class URLConstructor extends ClassInstanceExpr {
3333
URLConstructor() { this.getConstructor().getDeclaringType() instanceof TypeUrl }
3434

35-
// URLs constructed with the string constructor `URL(String spec)`
36-
Expr specArg() {
35+
predicate hasHttpStringArg() {
3736
this.getConstructor().getParameter(0).getType() instanceof TypeString and
3837
(
38+
// URLs constructed with the string constructor `URL(String spec)`
3939
this.getConstructor().getNumberOfParameters() = 1 and
40-
result = this.getArgument(0) // First argument contains the whole spec.
41-
)
42-
}
43-
44-
// URLs constructed with any of the three string constructors below:
45-
// `URL(String protocol, String host, int port, String file)`,
46-
// `URL(String protocol, String host, int port, String file, URLStreamHandler handler)`,
47-
// `URL(String protocol, String host, String file)`
48-
Expr protocolArg() {
49-
this.getConstructor().getParameter(0).getType() instanceof TypeString and
50-
(
40+
this.getArgument(0) instanceof HttpString // First argument contains the whole spec.
41+
or
42+
// URLs constructed with any of the three string constructors below:
43+
// `URL(String protocol, String host, int port, String file)`,
44+
// `URL(String protocol, String host, int port, String file, URLStreamHandler handler)`,
45+
// `URL(String protocol, String host, String file)`
5146
this.getConstructor().getNumberOfParameters() > 1 and
52-
result = this.getArgument(0) // First argument contains the protocol part.
47+
concatHttpString(getArgument(0), this.getArgument(1)) // First argument contains the protocol part and the second argument contains the host part.
5348
)
5449
}
50+
}
5551

56-
Expr hostArg() {
57-
this.getConstructor().getParameter(0).getType() instanceof TypeString and
52+
/**
53+
* Class of Java URI constructor.
54+
*/
55+
class URIConstructor extends ClassInstanceExpr {
56+
URIConstructor() { this.getConstructor().getDeclaringType().hasQualifiedName("java.net", "URI") }
57+
58+
predicate hasHttpStringArg() {
5859
(
59-
this.getConstructor().getNumberOfParameters() > 1 and
60-
result = this.getArgument(1) // Second argument contains the host part.
60+
this.getNumArgument() = 1 // `URI(String str)`
61+
or
62+
this.getNumArgument() = 4 and
63+
concatHttpString(this.getArgument(0), this.getArgument(1)) // `URI(String scheme, String host, String path, String fragment)`
64+
or
65+
this.getNumArgument() = 7 and
66+
concatHttpString(this.getArgument(0), this.getArgument(2)) // `URI(String scheme, String userInfo, String host, int port, String path, String query, String fragment)`
6167
)
6268
}
6369
}
@@ -66,7 +72,7 @@ class URLConstructor extends ClassInstanceExpr {
6672
* Gets a regular expression for matching private hosts.
6773
*/
6874
private string getPrivateHostRegex() {
69-
result = "localhost(/.*)?" or
75+
result = "(?i)localhost(/.*)?" or
7076
result = "127\\.0\\.0\\.1(/.*)?" or // IPv4 patterns
7177
result = "10(\\.[0-9]+){3}(/.*)?" or
7278
result = "172\\.16(\\.[0-9]+){2}(/.*)?" or
@@ -78,13 +84,11 @@ private string getPrivateHostRegex() {
7884
/**
7985
* String of HTTP URLs not in private domains.
8086
*/
81-
class HttpString extends StringLiteral {
82-
HttpString() {
87+
class HttpStringLiteral extends StringLiteral {
88+
HttpStringLiteral() {
8389
// Match URLs with the HTTP protocol and without private IP addresses to reduce false positives.
8490
exists(string s | this.getRepresentedString() = s |
85-
s.regexpMatch("(?i)http")
86-
or
87-
s.regexpMatch("(?i)http://.*") and
91+
s.regexpMatch("(?i)http://[a-zA-Z0-9].*") and
8892
not s.substring(7, s.length()).regexpMatch(getPrivateHostRegex())
8993
)
9094
}
@@ -117,20 +121,22 @@ predicate concatHttpString(Expr protocol, Expr host) {
117121
}
118122

119123
/**
120-
* String concatenated with `HttpString`.
124+
* String concatenated with `HttpStringLiteral`.
121125
*/
122-
predicate builtFromHttpStringConcat(Expr expr) {
123-
expr instanceof HttpString
124-
or
125-
expr.(VarAccess).getVariable().getAnAssignedValue() instanceof HttpString
126-
or
127-
concatHttpString(expr.(AddExpr).getLeftOperand(), expr.(AddExpr).getRightOperand())
128-
or
129-
concatHttpString(expr.(AddExpr).getLeftOperand().(AddExpr).getLeftOperand(),
130-
expr.(AddExpr).getLeftOperand().(AddExpr).getRightOperand())
131-
or
132-
concatHttpString(expr.(AddExpr).getLeftOperand(),
133-
expr.(AddExpr).getRightOperand().(AddExpr).getLeftOperand())
126+
class HttpString extends Expr {
127+
HttpString() {
128+
this instanceof HttpStringLiteral
129+
or
130+
this.(VarAccess).getVariable().getAnAssignedValue() instanceof HttpStringLiteral
131+
or
132+
concatHttpString(this.(AddExpr).getLeftOperand(), this.(AddExpr).getRightOperand())
133+
or
134+
concatHttpString(this.(AddExpr).getLeftOperand().(AddExpr).getLeftOperand(),
135+
this.(AddExpr).getLeftOperand().(AddExpr).getRightOperand())
136+
or
137+
concatHttpString(this.(AddExpr).getLeftOperand(),
138+
this.(AddExpr).getRightOperand().(AddExpr).getLeftOperand()) // First two elements of a string concatenated from an arbitrary number of elements.
139+
}
134140
}
135141

136142
/**
@@ -161,10 +167,76 @@ class HttpURLOpenMethod extends Method {
161167
}
162168
}
163169

170+
/** Constructor of `ApacheHttpRequest` */
171+
predicate apacheHttpRequest(DataFlow::Node node1, DataFlow::Node node2) {
172+
exists(ConstructorCall cc |
173+
cc.getConstructedType() instanceof ApacheHttpRequest and
174+
node2.asExpr() = cc and
175+
cc.getAnArgument() = node1.asExpr()
176+
)
177+
}
178+
179+
/** Constructors of `URI` */
180+
predicate createURI(DataFlow::Node node1, DataFlow::Node node2) {
181+
exists(URIConstructor cc |
182+
node2.asExpr() = cc and
183+
cc.getArgument(0) = node1.asExpr() and
184+
cc.hasHttpStringArg()
185+
)
186+
or
187+
exists(
188+
StaticMethodAccess ma // URI.create
189+
|
190+
ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URI") and
191+
ma.getMethod().hasName("create") and
192+
node1.asExpr() = ma.getArgument(0) and
193+
node2.asExpr() = ma
194+
)
195+
}
196+
197+
/** Constructors of `URL` */
198+
predicate createURL(DataFlow::Node node1, DataFlow::Node node2) {
199+
exists(URLConstructor cc |
200+
node2.asExpr() = cc and
201+
cc.getArgument(0) = node1.asExpr() and
202+
cc.hasHttpStringArg()
203+
)
204+
}
205+
206+
/** Method call of `HttpURLOpenMethod` */
207+
predicate urlOpen(DataFlow::Node node1, DataFlow::Node node2) {
208+
exists(MethodAccess ma |
209+
ma.getMethod() instanceof HttpURLOpenMethod and
210+
node1.asExpr() = ma.getQualifier() and
211+
ma = node2.asExpr()
212+
)
213+
}
214+
215+
/** Constructor of `BasicRequestLine` */
216+
predicate basicRequestLine(DataFlow::Node node1, DataFlow::Node node2) {
217+
exists(ConstructorCall mcc |
218+
mcc.getConstructedType().hasQualifiedName("org.apache.http.message", "BasicRequestLine") and
219+
mcc.getArgument(1) = node1.asExpr() and // `BasicRequestLine(String method, String uri, ProtocolVersion version)
220+
node2.asExpr() = mcc
221+
)
222+
}
223+
164224
class BasicAuthFlowConfig extends TaintTracking::Configuration {
165225
BasicAuthFlowConfig() { this = "InsecureBasicAuth::BasicAuthFlowConfig" }
166226

167-
override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof HttpString }
227+
override predicate isSource(DataFlow::Node src) {
228+
src.asExpr() instanceof HttpString
229+
or
230+
exists(URLConstructor uc |
231+
uc.hasHttpStringArg() and
232+
src.asExpr() = uc.getArgument(0)
233+
)
234+
or
235+
exists(URIConstructor uc |
236+
uc.hasHttpStringArg() and
237+
src.asExpr() = uc.getArgument(0)
238+
)
239+
}
168240

169241
override predicate isSink(DataFlow::Node sink) {
170242
exists(MethodAccess ma |
@@ -180,81 +252,11 @@ class BasicAuthFlowConfig extends TaintTracking::Configuration {
180252
}
181253

182254
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
183-
exists(ConstructorCall cc |
184-
cc.getConstructedType() instanceof ApacheHttpRequest and
185-
node2.asExpr() = cc and
186-
(
187-
cc.getAnArgument() = node1.asExpr() and
188-
builtFromHttpStringConcat(cc.getAnArgument())
189-
or
190-
exists(ConstructorCall mcc |
191-
(
192-
mcc.getConstructedType().hasQualifiedName("org.apache.http.message", "BasicRequestLine") and
193-
mcc.getArgument(1) = node1.asExpr() and // `BasicRequestLine(String method, String uri, ProtocolVersion version)
194-
builtFromHttpStringConcat(mcc.getArgument(1))
195-
or
196-
mcc.getConstructedType().hasQualifiedName("java.net", "URI") and
197-
(
198-
mcc.getNumArgument() = 1 and
199-
mcc.getArgument(0) = node1.asExpr() and
200-
builtFromHttpStringConcat(mcc.getArgument(0)) // `URI(String str)`
201-
or
202-
mcc.getNumArgument() = 4 and
203-
mcc.getArgument(0) = node1.asExpr() and
204-
concatHttpString(mcc.getArgument(0), mcc.getArgument(1)) // `URI(String scheme, String host, String path, String fragment)`
205-
or
206-
mcc.getNumArgument() = 7 and
207-
mcc.getArgument(0) = node1.asExpr() and
208-
concatHttpString(mcc.getArgument(2), mcc.getArgument(1)) // `URI(String scheme, String userInfo, String host, int port, String path, String query, String fragment)`
209-
)
210-
) and
211-
(
212-
cc.getAnArgument() = mcc
213-
or
214-
exists(VarAccess va |
215-
cc.getAnArgument() = va and va.getVariable().getAnAssignedValue() = mcc
216-
)
217-
)
218-
)
219-
or
220-
exists(StaticMethodAccess ma |
221-
ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URI") and
222-
ma.getMethod().hasName("create") and
223-
builtFromHttpStringConcat(ma.getArgument(0)) and
224-
node1.asExpr() = ma.getArgument(0) and
225-
(
226-
cc.getArgument(0) = ma
227-
or
228-
exists(VarAccess va |
229-
cc.getArgument(0) = va and va.getVariable().getAnAssignedValue() = ma
230-
)
231-
)
232-
)
233-
)
234-
)
235-
or
236-
exists(MethodAccess ma |
237-
ma.getMethod() instanceof HttpURLOpenMethod and
238-
ma = node2.asExpr() and
239-
(
240-
node1.asExpr() = ma.getQualifier().(URLConstructor).getArgument(0) and
241-
(
242-
builtFromHttpStringConcat(ma.getQualifier().(URLConstructor).specArg()) or
243-
concatHttpString(ma.getQualifier().(URLConstructor).protocolArg(),
244-
ma.getQualifier().(URLConstructor).hostArg())
245-
)
246-
or
247-
exists(URLConstructor uc, VarAccess va |
248-
node1.asExpr() = uc.getAnArgument() and
249-
uc = va.getVariable().getAnAssignedValue() and
250-
ma.getQualifier() = va and
251-
(
252-
builtFromHttpStringConcat(uc.specArg()) or
253-
concatHttpString(uc.protocolArg(), uc.hostArg())
254-
)
255-
)
256-
)
257-
)
255+
apacheHttpRequest(node1, node2) or
256+
createURI(node1, node2) or
257+
basicRequestLine(node1, node2) or
258+
createURL(node1, node2) or
259+
urlOpen(node1, node2)
258260
}
259261
}
260262

0 commit comments

Comments
 (0)