Enhance the query

2026-04-30 11:15:13 +02:00 · 2020-07-27 18:50:47 +00:00
parent 01fb51829c
commit 3a23451395
8 changed files with 211 additions and 42 deletions
--- a/java/ql/src/experimental/Security/CWE/CWE-522/UnsecureBasicAuth.java
+++ b/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 class UnsecureBasicAuth {
   */
  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";
  }

--- a/java/ql/src/experimental/Security/CWE/CWE-522/UnsecureBasicAuth.qhelp
+++ b/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>
--- a/java/ql/src/experimental/Security/CWE/CWE-522/UnsecureBasicAuth.ql
+++ b/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,22 +65,37 @@ 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())
-  )
-  or
-  exists(AddExpr concatExpr | concatExpr = expr |
-    exists(Expr arg | arg = concatExpr.getLeftOperand() | arg instanceof HttpString)
-  )
+  builtFromHttpStringConcat(expr.(AddExpr).getLeftOperand())
  or
  exists(Expr other | builtFromHttpStringConcat(other) |
    exists(Variable var | var.getAnAssignedValue() = other and var.getAnAccess() = expr)
  )
+  or
+  exists(Expr other | builtFromHttpStringConcat(other) |
+    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"
--- a/java/ql/test/experimental/query-tests/security/CWE-522/UnsecureBasicAuth.expected
+++ b/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 |
--- a/java/ql/test/experimental/query-tests/security/CWE-522/UnsecureBasicAuth.java
+++ b/java/ql/test/experimental/query-tests/security/CWE-522/UnsecureBasicAuth.java
@@ -1,7 +1,11 @@
+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;
@@ -9,7 +13,7 @@ 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 class UnsecureBasicAuth {
 		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.
 	 */
--- a/java/ql/test/stubs/apache-http-4.4.13/org/apache/http/client/methods/HttpRequestBase.java
+++ b/java/ql/test/stubs/apache-http-4.4.13/org/apache/http/client/methods/HttpRequestBase.java
@@ -79,4 +79,12 @@ public abstract class HttpRequestBase extends AbstractHttpMessage {
    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) {
+    }
 }
--- a/java/ql/test/stubs/apache-http-4.4.13/org/apache/http/message/BasicHttpRequest.java
+++ b/java/ql/test/stubs/apache-http-4.4.13/org/apache/http/message/BasicHttpRequest.java
@@ -61,11 +61,18 @@ public class BasicHttpRequest extends AbstractHttpMessage implements HttpRequest
    }

    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) {
+    }
 }
--- a/java/ql/test/stubs/apache-http-4.4.13/org/apache/http/message/BasicRequestLine.java
+++ b/java/ql/test/stubs/apache-http-4.4.13/org/apache/http/message/BasicRequestLine.java
@@ -0,0 +1,72 @@
+/*
+ * $HeadURL: http://svn.apache.org/repos/asf/httpcomponents/httpcore/trunk/module-main/src/main/java/org/apache/http/message/BasicRequestLine.java $
+ * $Revision: 604625 $
+ * $Date: 2007-12-16 06:11:11 -0800 (Sun, 16 Dec 2007) $
+ *
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation.  For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.http.message;
+import org.apache.http.ProtocolVersion;
+import org.apache.http.RequestLine;
+
+/**
+ * The first line of an {@link org.apache.http.HttpRequest HttpRequest}.
+ * It contains the method, URI, and HTTP version of the request.
+ * For details, see RFC 2616.
+ *
+ * @author <a href="mailto:oleg at ural.ru">Oleg Kalnichevski</a>
+ *
+ *
+ * <!-- empty lines above to avoid 'svn diff' context problems -->
+ * @version $Revision: 604625 $
+ * 
+ * @since 4.0
+ *
+ * @deprecated Please use {@link java.net.URL#openConnection} instead.
+ *     Please visit <a href="http://android-developers.blogspot.com/2011/09/androids-http-clients.html">this webpage</a>
+ *     for further details.
+ */
+@Deprecated
+public class BasicRequestLine implements RequestLine, Cloneable {
+    public BasicRequestLine(final String method, final String uri, final ProtocolVersion version) {
+    }
+
+    public String getMethod() {
+    return null;
+    }
+
+    public ProtocolVersion getProtocolVersion() {
+        return null;
+    }
+
+    public String getUri() {
+        return null;
+    }
+
+    public String toString() {
+        return null;
+    }    
+}