Skip to content

Commit 8022530

Browse files
atorralbaatorralba
authored
Merge pull request #5983 from atorralba/atorralba/promote-insecure-basic-auth
Java: Promote Insecure Basic Authentication query from experimental
2 parents d3caa80 + d3cf697 commit 8022530

File tree

18 files changed

+509
-534
lines changed

18 files changed

+509
-534
lines changed

‎java/change-notes/2021-06-01-insecure-basic-auth-query.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The query "Insecure basic authentication" (`java/insecure-basic-auth`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3976).

‎java/ql/lib/semmle/code/java/frameworks/Networking.qll

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,11 +52,20 @@ class UriCreation extends Call {
5252
/**
5353
* Gets the host argument of the newly created URI. In the case where the
5454
* host is specified separately, this is only the host. In the case where the
55-
* uri is parsed from an input string, such as in
55+
* URI is parsed from an input string, such as in
5656
* `URI("http://foo.com/mypath")`, this is the entire argument passed in,
5757
* that is `"http://foo.com/mypath"`.
5858
*/
59-
Expr getHostArg() { none() }
59+
abstract Expr getHostArg();
60+
61+
/**
62+
* Gets the scheme argument of the newly created URI. In the case where the
63+
* scheme is specified separately, this is only the scheme. In the case where the
64+
* URI is parsed from an input string, such as in
65+
* `URI("http://foo.com/mypath")`, this is the entire argument passed in,
66+
* that is `"http://foo.com/mypath"`.
67+
*/
68+
abstract Expr getSchemeArg();
6069
}
6170

6271
/** A `java.net.URI` constructor call. */
@@ -74,13 +83,17 @@ class UriConstructorCall extends ClassInstanceExpr, UriCreation {
7483
// String fragment)
7584
result = this.getArgument(2) and this.getNumArgument() = 7
7685
}
86+
87+
override Expr getSchemeArg() { result = this.getArgument(0) }
7788
}
7889

7990
/** A call to `java.net.URI::create`. */
8091
class UriCreate extends UriCreation {
8192
UriCreate() { this.getCallee().hasName("create") }
8293

8394
override Expr getHostArg() { result = this.getArgument(0) }
95+
96+
override Expr getSchemeArg() { result = this.getArgument(0) }
8497
}
8598

8699
/** A `java.net.URL` constructor call. */
@@ -105,7 +118,7 @@ class UrlConstructorCall extends ClassInstanceExpr {
105118
}
106119

107120
/** Gets the argument that corresponds to the protocol of the URL. */
108-
Expr protocolArg() {
121+
Expr getProtocolArg() {
109122
// In all cases except where the first parameter is a URL, the argument
110123
// containing the protocol is the first one, otherwise it is the second.
111124
if this.getConstructor().getParameterType(0) instanceof TypeUrl

‎java/ql/lib/semmle/code/java/security/HttpsUrls.qll

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/** Provides classes and predicates to reason about plaintext HTTP vulnerabilities. */
2+
3+
import java
4+
private import semmle.code.java.dataflow.DataFlow
5+
private import semmle.code.java.dataflow.ExternalFlow
6+
private import semmle.code.java.dataflow.TaintTracking
7+
private import semmle.code.java.frameworks.ApacheHttp
8+
private import semmle.code.java.frameworks.Networking
9+
10+
/**
11+
* String of HTTP URLs not in private domains.
12+
*/
13+
class HttpStringLiteral extends StringLiteral {
14+
HttpStringLiteral() {
15+
exists(string s | this.getRepresentedString() = s |
16+
s = "http"
17+
or
18+
s.matches("http://%") and
19+
not s.substring(7, s.length()) instanceof PrivateHostName and
20+
not TaintTracking::localExprTaint(any(StringLiteral p |
21+
p.getRepresentedString() instanceof PrivateHostName
22+
), this.getParent*())
23+
)
24+
}
25+
}
26+
27+
/**
28+
* A sink that represents a URL opening method call, such as a call to `java.net.URL.openConnection()`.
29+
*/
30+
abstract class UrlOpenSink extends DataFlow::Node { }
31+
32+
private class DefaultUrlOpenSink extends UrlOpenSink {
33+
DefaultUrlOpenSink() { sinkNode(this, "open-url") }
34+
}
35+
36+
/**
37+
* A unit class for adding additional taint steps.
38+
*
39+
* Extend this class to add additional taint steps that should apply
40+
* to configurations working with HTTP URLs.
41+
*/
42+
class HttpUrlsAdditionalTaintStep extends Unit {
43+
/**
44+
* Holds if the step from `node1` to `node2` should be considered a taint
45+
* step for taint tracking configurations working with HTTP URLs.
46+
*/
47+
abstract predicate step(DataFlow::Node n1, DataFlow::Node n2);
48+
}
49+
50+
private class DefaultHttpUrlAdditionalTaintStep extends HttpUrlsAdditionalTaintStep {
51+
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
52+
apacheHttpRequestStep(n1, n2) or
53+
createUriStep(n1, n2) or
54+
createUrlStep(n1, n2) or
55+
urlOpenStep(n1, n2)
56+
}
57+
}
58+
59+
/** Constructor of `ApacheHttpRequest` */
60+
private predicate apacheHttpRequestStep(DataFlow::Node node1, DataFlow::Node node2) {
61+
exists(ConstructorCall cc |
62+
cc.getConstructedType() instanceof ApacheHttpRequest and
63+
node2.asExpr() = cc and
64+
cc.getAnArgument() = node1.asExpr()
65+
)
66+
}
67+
68+
/** `URI` methods */
69+
private predicate createUriStep(DataFlow::Node node1, DataFlow::Node node2) {
70+
exists(UriConstructorCall cc |
71+
cc.getSchemeArg() = node1.asExpr() and
72+
node2.asExpr() = cc
73+
)
74+
}
75+
76+
/** Constructors of `URL` */
77+
private predicate createUrlStep(DataFlow::Node node1, DataFlow::Node node2) {
78+
exists(UrlConstructorCall cc |
79+
cc.getProtocolArg() = node1.asExpr() and
80+
node2.asExpr() = cc
81+
)
82+
}
83+
84+
/** Method call of `HttpURLOpenMethod` */
85+
private predicate urlOpenStep(DataFlow::Node node1, DataFlow::Node node2) {
86+
exists(MethodAccess ma |
87+
ma.getMethod() instanceof UrlOpenConnectionMethod and
88+
node1.asExpr() = ma.getQualifier() and
89+
ma = node2.asExpr()
90+
)
91+
}

‎java/ql/lib/semmle/code/java/security/HttpsUrlsQuery.qll

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/** Provides taint tracking configurations to be used in HTTPS URLs queries. */
2+
3+
import java
4+
import semmle.code.java.dataflow.TaintTracking
5+
import semmle.code.java.frameworks.Networking
6+
import semmle.code.java.security.HttpsUrls
7+
8+
/**
9+
* A taint tracking configuration for HTTP connections.
10+
*/
11+
class HttpStringToUrlOpenMethodFlowConfig extends TaintTracking::Configuration {
12+
HttpStringToUrlOpenMethodFlowConfig() { this = "HttpStringToUrlOpenMethodFlowConfig" }
13+
14+
override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof HttpStringLiteral }
15+
16+
override predicate isSink(DataFlow::Node sink) { sink instanceof UrlOpenSink }
17+
18+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
19+
any(HttpUrlsAdditionalTaintStep c).step(node1, node2)
20+
}
21+
22+
override predicate isSanitizer(DataFlow::Node node) {
23+
node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
24+
}
25+
}

‎java/ql/lib/semmle/code/java/security/InsecureBasicAuth.qll

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
/** Provides classes and predicates to reason about Insecure Basic Authentication vulnerabilities. */
2+
3+
import java
4+
import semmle.code.java.dataflow.DataFlow
5+
import semmle.code.java.dataflow.TaintTracking
6+
import semmle.code.java.security.HttpsUrls
7+
8+
/**
9+
* A source that represents HTTP URLs.
10+
* Extend this class to add your own Insecure Basic Authentication sources.
11+
*/
12+
abstract class InsecureBasicAuthSource extends DataFlow::Node { }
13+
14+
/** A default source representing HTTP strings, URLs or URIs. */
15+
private class DefaultInsecureBasicAuthSource extends InsecureBasicAuthSource {
16+
DefaultInsecureBasicAuthSource() { this.asExpr() instanceof HttpStringLiteral }
17+
}
18+
19+
/**
20+
* A sink that represents a method that sets Basic Authentication.
21+
* Extend this class to add your own Insecure Basic Authentication sinks.
22+
*/
23+
abstract class InsecureBasicAuthSink extends DataFlow::Node { }
24+
25+
/** A default sink representing methods that set an Authorization header. */
26+
private class DefaultInsecureBasicAuthSink extends InsecureBasicAuthSink {
27+
DefaultInsecureBasicAuthSink() {
28+
exists(MethodAccess ma |
29+
ma.getMethod().hasName("addHeader") or
30+
ma.getMethod().hasName("setHeader") or
31+
ma.getMethod().hasName("setRequestProperty")
32+
|
33+
this.asExpr() = ma.getQualifier() and
34+
ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "Authorization" and
35+
TaintTracking::localExprTaint(any(BasicAuthString b), ma.getArgument(1))
36+
)
37+
}
38+
}
39+
40+
/**
41+
* String pattern of basic authentication.
42+
*/
43+
private class BasicAuthString extends StringLiteral {
44+
BasicAuthString() { exists(string s | this.getRepresentedString() = s | s.matches("Basic %")) }
45+
}

‎java/ql/lib/semmle/code/java/security/InsecureBasicAuthQuery.qll

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/** Provides taint tracking configurations to be used in Insecure Basic Authentication queries. */
2+
3+
import java
4+
import semmle.code.java.security.HttpsUrls
5+
import semmle.code.java.security.InsecureBasicAuth
6+
import semmle.code.java.dataflow.TaintTracking
7+
8+
/**
9+
* A taint tracking configuration for the Basic authentication scheme
10+
* being used in HTTP connections.
11+
*/
12+
class BasicAuthFlowConfig extends TaintTracking::Configuration {
13+
BasicAuthFlowConfig() { this = "InsecureBasicAuth::BasicAuthFlowConfig" }
14+
15+
override predicate isSource(DataFlow::Node src) { src instanceof InsecureBasicAuthSource }
16+
17+
override predicate isSink(DataFlow::Node sink) { sink instanceof InsecureBasicAuthSink }
18+
19+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
20+
any(HttpUrlsAdditionalTaintStep c).step(node1, node2)
21+
}
22+
}

‎java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql

Lines changed: 2 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -11,54 +11,10 @@
1111
*/
1212

1313
import java
14-
import semmle.code.java.dataflow.TaintTracking
15-
import semmle.code.java.frameworks.Networking
14+
import semmle.code.java.security.HttpsUrlsQuery
1615
import DataFlow::PathGraph
17-
private import semmle.code.java.dataflow.ExternalFlow
1816

19-
class HttpString extends StringLiteral {
20-
HttpString() {
21-
// Avoid matching "https" here.
22-
exists(string s | this.getRepresentedString() = s |
23-
(
24-
// Either the literal "http", ...
25-
s = "http"
26-
or
27-
// ... or the beginning of a http URL.
28-
s.matches("http://%")
29-
) and
30-
not s.matches("%/localhost%")
31-
)
32-
}
33-
}
34-
35-
class HttpStringToUrlOpenMethodFlowConfig extends TaintTracking::Configuration {
36-
HttpStringToUrlOpenMethodFlowConfig() { this = "HttpsUrls::HttpStringToUrlOpenMethodFlowConfig" }
37-
38-
override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof HttpString }
39-
40-
override predicate isSink(DataFlow::Node sink) { sink instanceof UrlOpenSink }
41-
42-
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
43-
exists(UrlConstructorCall u |
44-
node1.asExpr() = u.protocolArg() and
45-
node2.asExpr() = u
46-
)
47-
}
48-
49-
override predicate isSanitizer(DataFlow::Node node) {
50-
node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
51-
}
52-
}
53-
54-
/**
55-
* A sink that represents a URL opening method call, such as a call to `java.net.URL.openConnection()`.
56-
*/
57-
private class UrlOpenSink extends DataFlow::Node {
58-
UrlOpenSink() { sinkNode(this, "open-url") }
59-
}
60-
61-
from DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess m, HttpString s
17+
from DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess m, HttpStringLiteral s
6218
where
6319
source.getNode().asExpr() = s and
6420
sink.getNode().asExpr() = m.getQualifier() and

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

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,19 @@
11
public class InsecureBasicAuth {
22
/**
33
* Test basic authentication with Apache HTTP request.
4-
*/
4+
*/
55
public void testApacheHttpRequest(String username, String password) {
6-
{
6+
77
// BAD: basic authentication over HTTP
88
String url = "http://www.example.com/rest/getuser.do?uid=abcdx";
9-
}
109

11-
{
1210
// GOOD: basic authentication over HTTPS
13-
String url = "https://www.example.com/rest/getuser.do?uid=abcdx";
14-
}
11+
url = "https://www.example.com/rest/getuser.do?uid=abcdx";
1512

1613
HttpPost post = new HttpPost(url);
1714
post.setHeader("Accept", "application/json");
1815
post.setHeader("Content-type", "application/json");
19-
16+
2017
String authString = username + ":" + password;
2118
byte[] authEncBytes = Base64.getEncoder().encode(authString.getBytes());
2219
String authStringEnc = new String(authEncBytes);
@@ -28,15 +25,12 @@ public void testApacheHttpRequest(String username, String password) {
2825
* Test basic authentication with Java HTTP URL connection.
2926
*/
3027
public void testHttpUrlConnection(String username, String password) {
31-
{
28+
3229
// BAD: basic authentication over HTTP
3330
String urlStr = "http://www.example.com/rest/getuser.do?uid=abcdx";
34-
}
3531

36-
{
3732
// GOOD: basic authentication over HTTPS
38-
String urlStr = "https://www.example.com/rest/getuser.do?uid=abcdx";
39-
}
33+
urlStr = "https://www.example.com/rest/getuser.do?uid=abcdx";
4034

4135
String authString = username + ":" + password;
4236
String encoding = Base64.getEncoder().encodeToString(authString.getBytes("UTF-8"));

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
<qhelp>
33

44
<overview>
5-
<p>Basic authentication only obfuscates username/password in Base64 encoding, which can be easily recognized and reversed, thus it must not be transmitted over the cleartext HTTP channel. Transmission of sensitive information not in HTTPS is vulnerable to packet sniffing.</p>
5+
<p>Basic authentication only obfuscates usernames and passwords in Base64 encoding, which can be easily recognized and reversed, thus it must not be transmitted over the cleartext HTTP channel. Transmitting sensitive information without using HTTPS makes the data vulnerable to packet sniffing.</p>
66
</overview>
77

88
<recommendation>
@@ -15,16 +15,13 @@
1515
</example>
1616

1717
<references>
18-
<li>
19-
<a href="https://cwe.mitre.org/data/definitions/522">CWE-522</a>
20-
</li>
2118
<li>
2219
SonarSource rule:
23-
<a href="https://rules.sonarsource.com/java/tag/owasp/RSPEC-2647">Basic authentication should not be used</a>
20+
<a href="https://rules.sonarsource.com/java/tag/owasp/RSPEC-2647">Basic authentication should not be used</a>.
2421
</li>
2522
<li>
2623
Acunetix:
27-
<a href="https://www.acunetix.com/vulnerabilities/web/basic-authentication-over-http/">WEB VULNERABILITIES INDEX - Basic authentication over HTTP</a>
24+
<a href="https://www.acunetix.com/vulnerabilities/web/basic-authentication-over-http/">WEB VULNERABILITIES INDEX - Basic authentication over HTTP</a>.
2825
</li>
2926
</references>
3027
</qhelp>

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Insecure basic authentication
3+
* @description Basic authentication only obfuscates username/password in
4+
* Base64 encoding, which can be easily recognized and reversed.
5+
* Transmitting sensitive information without using HTTPS makes
6+
* the data vulnerable to packet sniffing.
7+
* @kind path-problem
8+
* @problem.severity warning
9+
* @precision medium
10+
* @id java/insecure-basic-auth
11+
* @tags security
12+
* external/cwe/cwe-522
13+
* external/cwe/cwe-319
14+
*/
15+
16+
import java
17+
import semmle.code.java.security.InsecureBasicAuthQuery
18+
import DataFlow::PathGraph
19+
20+
from DataFlow::PathNode source, DataFlow::PathNode sink, BasicAuthFlowConfig config
21+
where config.hasFlowPath(source, sink)
22+
select sink.getNode(), source, sink, "Insecure basic authentication from $@.", source.getNode(),
23+
"HTTP URL"

0 commit comments

Comments
 (0)