Enhance the query

Author: luchua-bc

java/ql/src/experimental/Security/CWE/CWE-522/UnsecureBasicAuth.java

@@ -1,15 +1,15 @@
 public class UnsecureBasicAuth {
   /**
-   *Test basic authentication with Apache HTTP request.
+   * Test basic authentication with Apache HTTP request.
    */	
   public void testApacheHttpRequest(String username, String password) {
   {
-    // BAD: Use HTTP with basic authentication
+    // BAD: basic authentication over HTTP
     String url = "http://www.example.com/rest/getuser.do?uid=abcdx";
   }
 
   {
-    // GOOD: Use HTTPS with basic authentication
+    // GOOD: basic authentication over HTTPS
     String url = "https://www.example.com/rest/getuser.do?uid=abcdx";
   }
 
@@ -29,12 +29,12 @@ public void testApacheHttpRequest(String username, String password) {
    */
   public void testHttpUrlConnection(String username, String password) {
   {
-    // BAD: Use HTTP with basic authentication
+    // BAD: basic authentication over HTTP
     String urlStr = "http://www.example.com/rest/getuser.do?uid=abcdx";
   }
 
   {
-    // GOOD: Use HTTPS with basic authentication
+    // GOOD: basic authentication over HTTPS
     String urlStr = "https://www.example.com/rest/getuser.do?uid=abcdx";
   }
 

java/ql/src/experimental/Security/CWE/CWE-522/UnsecureBasicAuth.qhelp

@@ -10,15 +10,20 @@
   </recommendation>
 
   <example>
-    <p>The following example shows two ways of using basic authentication. In the 'BAD' case,
-the credentials are transmitted over HTTP. In the 'GOOD' case, the credentials are transmitted over HTTPS.</p>
+    <p>The following example shows two ways of using basic authentication. In the 'BAD' case, the credentials are transmitted over HTTP. In the 'GOOD' case, the credentials are transmitted over HTTPS.</p>
     <sample src="UnsecureBasicAuth.java" />
   </example>
 
   <references>
     <li>
       <a href="https://cwe.mitre.org/data/definitions/522">CWE-522</a>
+    </li>
+    <li>
+      SonarSource rule:
       <a href="https://rules.sonarsource.com/java/tag/owasp/RSPEC-2647">Basic authentication should not be used</a>
+    </li>
+    <li>
+      Acunetix:
       <a href="https://www.acunetix.com/vulnerabilities/web/basic-authentication-over-http/">WEB VULNERABILITIES INDEX - Basic authentication over HTTP</a>
     </li>
   </references>

java/ql/src/experimental/Security/CWE/CWE-522/UnsecureBasicAuth.ql

@@ -1,6 +1,6 @@
 /**
  * @name Unsecure basic authentication
- * @description Basic authentication only obfuscates username/password in Base64 encoding, which can be easily recognized and reversed. Transmission of sensitive information not in HTTPS is vulnerable to packet sniffing.
+ * @description Basic authentication only obfuscates username/password in Base64 encoding, which can be easily recognized and reversed. Transmission of sensitive information not over HTTPS is vulnerable to packet sniffing.
  * @kind problem
  * @id java/unsecure-basic-auth
  * @tags security
@@ -14,23 +14,27 @@ import semmle.code.java.dataflow.TaintTracking
 import DataFlow::PathGraph
 
 /**
- * The Java class `org.apache.http.message.AbstractHttpMessage`. Popular subclasses include `HttpGet`, `HttpPost`, and `HttpPut`.
+ * The Java class `org.apache.http.client.methods.HttpRequestBase`. Popular subclasses include `HttpGet`, `HttpPost`, and `HttpPut`.
+ * And the Java class `org.apache.http.message.BasicHttpRequest`.
  */
 class ApacheHttpRequest extends RefType {
-  ApacheHttpRequest() { hasQualifiedName("org.apache.http.message", "AbstractHttpMessage") }
+  ApacheHttpRequest() {
+    hasQualifiedName("org.apache.http.client.methods", "HttpRequestBase") or
+    hasQualifiedName("org.apache.http.message", "BasicHttpRequest")
+  }
 }
 
 /**
- * Class of Java URL constructor
+ * Class of Java URL constructor.
  */
 class URLConstructor extends ClassInstanceExpr {
   URLConstructor() { this.getConstructor().getDeclaringType() instanceof TypeUrl }
 
   Expr stringArg() {
-    // Query only in URL's that were constructed by calling the single parameter string constructor.
-    this.getConstructor().getNumberOfParameters() = 1 and
+    // URLs constructed with any of the four string constructors below:
+    // URL(String spec), URL(String protocol, String host, int port, String file), URL(String protocol, String host, int port, String file, URLStreamHandler handler), URL(String protocol, String host, String file)
     this.getConstructor().getParameter(0).getType() instanceof TypeString and
-    result = this.getArgument(0)
+    result = this.getArgument(0) // First argument contains the protocol part.
   }
 }
 
@@ -42,12 +46,14 @@ class TypeHttpUrlConnection extends RefType {
 }
 
 /**
- * String of HTTP URLs not in private domains
+ * String of HTTP URLs not in private domains.
  */
 class HttpString extends StringLiteral {
   HttpString() {
     // Match URLs with the HTTP protocol and without private IP addresses to reduce false positives.
     exists(string s | this.getRepresentedString() = s |
+      s = "http"
+      or
       s.matches("http://%") and
       not s.matches("%/localhost%") and
       not s.matches("%/127.0.0.1%") and
@@ -59,21 +65,36 @@ class HttpString extends StringLiteral {
 }
 
 /**
- * String concatenated with `HttpString`
+ * String concatenated with `HttpString`.
  */
 predicate builtFromHttpStringConcat(Expr expr) {
   expr instanceof HttpString
   or
-  exists(AddExpr concatExpr | concatExpr = expr |
-    builtFromHttpStringConcat(concatExpr.getLeftOperand())
-  )
+  builtFromHttpStringConcat(expr.(AddExpr).getLeftOperand())
   or
-  exists(AddExpr concatExpr | concatExpr = expr |
-    exists(Expr arg | arg = concatExpr.getLeftOperand() | arg instanceof HttpString)
+  exists(Expr other | builtFromHttpStringConcat(other) |
+    exists(Variable var | var.getAnAssignedValue() = other and var.getAnAccess() = expr)
   )
   or
   exists(Expr other | builtFromHttpStringConcat(other) |
-    exists(Variable var | var.getAnAssignedValue() = other and var.getAnAccess() = expr)
+    exists(ConstructorCall cc |
+      (
+        cc.getConstructedType().hasQualifiedName("org.apache.http.message", "BasicRequestLine") and
+        cc.getArgument(1) = other // BasicRequestLine(String method, String uri, ProtocolVersion version)
+        or
+        cc.getConstructedType().hasQualifiedName("java.net", "URI") and
+        cc.getArgument(0) = other // URI(String str) or URI(String scheme, ...)
+      ) and
+      (
+        cc = expr
+        or
+        exists(Variable var, VariableAssign va |
+          var.getAnAccess() = expr and
+          va.getDestVar() = var and
+          va.getSource() = cc
+        )
+      )
+    )
   )
 }
 
@@ -100,11 +121,11 @@ class AddBasicAuthHeaderMethodAccess extends MethodAccess {
       )
     ) and
     exists(VarAccess request, VariableAssign va, ConstructorCall cc, Expr arg0 |
-      this.getQualifier() = request and //Check the method invocation with the pattern post.addHeader("Authorization", "Basic " + authStringEnc)
+      this.getQualifier() = request and // Constructor call like HttpPost post = new HttpPost("http://www.example.com/rest/endpoint.do"); and BasicHttpRequest post = new BasicHttpRequest("POST", uriStr);
       va.getDestVar() = request.getVariable() and
       va.getSource() = cc and
-      cc.getArgument(0) = arg0 and
-      builtFromHttpStringConcat(arg0) // Check url string
+      cc.getAnArgument() = arg0 and
+      builtFromHttpStringConcat(arg0)
     )
   }
 }
@@ -118,7 +139,7 @@ class HttpURLOpenMethod extends Method {
 }
 
 /**
- * Tracks the flow of data from parameter of URL constructor to the url instance
+ * Tracks the flow of data from parameter of URL constructor to the url instance.
  */
 class URLConstructorTaintStep extends TaintTracking::AdditionalTaintStep {
   override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
@@ -130,7 +151,7 @@ class URLConstructorTaintStep extends TaintTracking::AdditionalTaintStep {
 }
 
 /**
- * Tracks the flow of data from `openConnection` method to the connection object
+ * Tracks the flow of data from `openConnection` method to the connection object.
  */
 class OpenHttpURLTaintStep extends TaintTracking::AdditionalTaintStep {
   override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
@@ -160,7 +181,7 @@ class HttpStringToHttpURLOpenMethodFlowConfig extends TaintTracking::Configurati
 
   override predicate isSink(DataFlow::Node sink) {
     sink.asExpr().(VarAccess).getVariable().getType() instanceof TypeUrlConnection or
-    sink.asExpr().(VarAccess).getVariable().getType() instanceof TypeHttpUrlConnection // Somehow TypeHttpUrlConnection isn't instance of TypeUrlConnection
+    sink.asExpr().(VarAccess).getVariable().getType() instanceof TypeHttpUrlConnection // TypeHttpUrlConnection isn't instance of TypeUrlConnection
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
@@ -210,4 +231,4 @@ from MethodAccess ma
 where
   ma instanceof AddBasicAuthHeaderMethodAccess or
   ma instanceof SetBasicAuthPropertyMethodAccess
-select ma, "Unsafe basic authentication"
+select ma, "Insecure basic authentication"

java/ql/test/experimental/query-tests/security/CWE-522/UnsecureBasicAuth.expected

@@ -1,13 +1,16 @@
 edges
-| UnsecureBasicAuth.java:41:19:41:68 | "http://www.example.com/rest/getuser.do?uid=abcdx" : String | UnsecureBasicAuth.java:46:3:46:6 | conn |
-| UnsecureBasicAuth.java:41:19:41:68 | "http://www.example.com/rest/getuser.do?uid=abcdx" : String | UnsecureBasicAuth.java:47:3:47:6 | conn |
-| UnsecureBasicAuth.java:41:19:41:68 | "http://www.example.com/rest/getuser.do?uid=abcdx" : String | UnsecureBasicAuth.java:48:3:48:6 | conn |
+| UnsecureBasicAuth.java:94:19:94:68 | "http://www.example.com/rest/getuser.do?uid=abcdx" : String | UnsecureBasicAuth.java:99:3:99:6 | conn |
+| UnsecureBasicAuth.java:94:19:94:68 | "http://www.example.com/rest/getuser.do?uid=abcdx" : String | UnsecureBasicAuth.java:100:3:100:6 | conn |
+| UnsecureBasicAuth.java:94:19:94:68 | "http://www.example.com/rest/getuser.do?uid=abcdx" : String | UnsecureBasicAuth.java:101:3:101:6 | conn |
 nodes
-| UnsecureBasicAuth.java:41:19:41:68 | "http://www.example.com/rest/getuser.do?uid=abcdx" : String | semmle.label | "http://www.example.com/rest/getuser.do?uid=abcdx" : String |
-| UnsecureBasicAuth.java:46:3:46:6 | conn | semmle.label | conn |
-| UnsecureBasicAuth.java:47:3:47:6 | conn | semmle.label | conn |
-| UnsecureBasicAuth.java:48:3:48:6 | conn | semmle.label | conn |
+| UnsecureBasicAuth.java:94:19:94:68 | "http://www.example.com/rest/getuser.do?uid=abcdx" : String | semmle.label | "http://www.example.com/rest/getuser.do?uid=abcdx" : String |
+| UnsecureBasicAuth.java:99:3:99:6 | conn | semmle.label | conn |
+| UnsecureBasicAuth.java:100:3:100:6 | conn | semmle.label | conn |
+| UnsecureBasicAuth.java:101:3:101:6 | conn | semmle.label | conn |
 #select
-| UnsecureBasicAuth.java:24:3:24:59 | addHeader(...) | Unsafe basic authentication |
-| UnsecureBasicAuth.java:34:3:34:108 | setHeader(...) | Unsafe basic authentication |
-| UnsecureBasicAuth.java:48:3:48:63 | setRequestProperty(...) | Unsafe basic authentication |
+| UnsecureBasicAuth.java:28:3:28:59 | addHeader(...) | Insecure basic authentication |
+| UnsecureBasicAuth.java:38:3:38:108 | setHeader(...) | Insecure basic authentication |
+| UnsecureBasicAuth.java:54:3:54:59 | addHeader(...) | Insecure basic authentication |
+| UnsecureBasicAuth.java:70:3:70:59 | addHeader(...) | Insecure basic authentication |
+| UnsecureBasicAuth.java:87:3:87:59 | addHeader(...) | Insecure basic authentication |
+| UnsecureBasicAuth.java:101:3:101:63 | setRequestProperty(...) | Insecure basic authentication |

java/ql/test/experimental/query-tests/security/CWE-522/UnsecureBasicAuth.java

@@ -1,15 +1,19 @@
+import org.apache.http.RequestLine;
 import org.apache.http.client.methods.HttpRequestBase;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpPost;
+import org.apache.http.message.BasicHttpRequest;
+import org.apache.http.message.BasicRequestLine;
 
+import java.net.URI;
 import java.net.URL;
 import java.net.HttpURLConnection;
 import java.net.URLConnection;
 import java.util.Base64;
 
 public class UnsecureBasicAuth {
 	/**
-	 * Test basic authentication with Apache HTTP POST request.
+	 * Test basic authentication with Apache HTTP POST request using string constructor.
 	 */
 	public void testApacheHttpRequest(String username, String password) {
 		String host = "www.example.com";
@@ -34,6 +38,55 @@ public void testApacheHttpRequest2(String url) throws java.io.IOException {
 		get.setHeader("Authorization", "Basic " + new String(Base64.getEncoder().encode("admin:test".getBytes())));
 	}
 
+	/**
+	 * Test basic authentication with Apache HTTP POST request using URI constructor.
+	 */
+	public void testApacheHttpRequest3(String username, String password) {
+		String uriStr = "http://www.example.com/rest/getuser.do?uid=abcdx";
+		HttpRequestBase post = new HttpPost(new URI(uriStr));
+		post.setHeader("Accept", "application/json");
+		post.setHeader("Content-type", "application/json");
+		
+		String authString = username + ":" + password;
+		byte[] authEncBytes = Base64.getEncoder().encode(authString.getBytes());
+		String authStringEnc = new String(authEncBytes);
+
+		post.addHeader("Authorization", "Basic " + authStringEnc);
+	}
+
+	/**
+	 * Test basic authentication with Apache HTTP `BasicHttpRequest` using string constructor.
+	 */
+	public void testApacheHttpRequest4(String username, String password) {
+		String uriStr = "http://www.example.com/rest/getuser.do?uid=abcdx";
+		BasicHttpRequest post = new BasicHttpRequest("POST", uriStr);
+		post.setHeader("Accept", "application/json");
+		post.setHeader("Content-type", "application/json");
+		
+		String authString = username + ":" + password;
+		byte[] authEncBytes = Base64.getEncoder().encode(authString.getBytes());
+		String authStringEnc = new String(authEncBytes);
+
+		post.addHeader("Authorization", "Basic " + authStringEnc);
+	}
+
+	/**
+	 * Test basic authentication with Apache HTTP `BasicHttpRequest` using `RequestLine`.
+	 */
+	public void testApacheHttpRequest5(String username, String password) {
+		String uriStr = "http://www.example.com/rest/getuser.do?uid=abcdx";
+		RequestLine requestLine = new BasicRequestLine("POST", uriStr, null);
+		BasicHttpRequest post = new BasicHttpRequest(requestLine);
+		post.setHeader("Accept", "application/json");
+		post.setHeader("Content-type", "application/json");
+		
+		String authString = username + ":" + password;
+		byte[] authEncBytes = Base64.getEncoder().encode(authString.getBytes());
+		String authStringEnc = new String(authEncBytes);
+
+		post.addHeader("Authorization", "Basic " + authStringEnc);
+	}
+
 	/**
 	 * Test basic authentication with Java HTTP URL connection.
 	 */

java/ql/test/stubs/apache-http-4.4.13/org/apache/http/client/methods/HttpRequestBase.java

@@ -79,4 +79,12 @@ public void abort() {
     public boolean isAborted() {
         return false;
     }
+
+    // Add method from `org.apache.http.HttpMessage`
+    public void addHeader(String name, String value) {
+    }
+
+    // Add method from `org.apache.http.HttpMessage`
+    public void setHeader(String name, String value) {
+    }
 }

java/ql/test/stubs/apache-http-4.4.13/org/apache/http/message/BasicHttpRequest.java

@@ -61,11 +61,18 @@ public BasicHttpRequest(final RequestLine requestline) {
     }
 
     public ProtocolVersion getProtocolVersion() {
-        return null;
+    return null;
     }
 
     public RequestLine getRequestLine() {
         return null;
     }
 
+    // Add method from `org.apache.http.HttpMessage`
+    public void addHeader(String name, String value) {
+    }
+    
+    // Add method from `org.apache.http.HttpMessage`
+    public void setHeader(String name, String value) {
+    }
 }