Refactored into InsecureBasicAuth.qll

Author: atorralba

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

@@ -14,225 +14,23 @@
  */
 
 import java
-import semmle.code.java.frameworks.Networking
-import semmle.code.java.frameworks.ApacheHttp
 import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.security.InsecureBasicAuth
 import DataFlow::PathGraph
 
-/**
- * Class of Java URL constructor.
- */
-class URLConstructor extends ClassInstanceExpr {
-  URLConstructor() { this.getConstructor().getDeclaringType() instanceof TypeUrl }
-
-  predicate hasHttpStringArg() {
-    this.getConstructor().getParameter(0).getType() instanceof TypeString and
-    (
-      // URLs constructed with any of the three string constructors below:
-      // `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().getNumberOfParameters() > 1 and
-      concatHttpString(getArgument(0), this.getArgument(1)) // First argument contains the protocol part and the second argument contains the host part.
-      or
-      // URLs constructed with the string constructor `URL(String spec)`
-      this.getConstructor().getNumberOfParameters() = 1 and
-      this.getArgument(0) instanceof HttpString // First argument contains the whole spec.
-    )
-  }
-}
-
-/**
- * Class of Java URI constructor.
- */
-class URIConstructor extends ClassInstanceExpr {
-  URIConstructor() { this.getConstructor().getDeclaringType() instanceof TypeUri }
-
-  predicate hasHttpStringArg() {
-    (
-      this.getNumArgument() = 1 and
-      this.getArgument(0) instanceof HttpString // `URI(String str)`
-      or
-      this.getNumArgument() = 4 and
-      concatHttpString(this.getArgument(0), this.getArgument(1)) // `URI(String scheme, String host, String path, String fragment)`
-      or
-      this.getNumArgument() = 5 and
-      concatHttpString(this.getArgument(0), this.getArgument(1)) // `URI(String scheme, String authority, String path, String query, String fragment)` without user-info in authority
-      or
-      this.getNumArgument() = 7 and
-      concatHttpString(this.getArgument(0), this.getArgument(2)) // `URI(String scheme, String userInfo, String host, int port, String path, String query, String fragment)`
-    )
-  }
-}
-
-/**
- * String of HTTP URLs not in private domains.
- */
-class HttpStringLiteral extends StringLiteral {
-  HttpStringLiteral() {
-    // Match URLs with the HTTP protocol and without private IP addresses to reduce false positives.
-    exists(string s | this.getRepresentedString() = s |
-      s.regexpMatch("(?i)http://[\\[a-zA-Z0-9].*") and
-      not s.substring(7, s.length()) instanceof PrivateHostName
-    )
-  }
-}
-
-/**
- * Checks both parts of protocol and host.
- */
-predicate concatHttpString(Expr protocol, Expr host) {
-  (
-    protocol.(CompileTimeConstantExpr).getStringValue().regexpMatch("(?i)http(://)?") or
-    protocol
-        .(VarAccess)
-        .getVariable()
-        .getAnAssignedValue()
-        .(CompileTimeConstantExpr)
-        .getStringValue()
-        .regexpMatch("(?i)http(://)?")
-  ) and
-  not exists(string hostString |
-    hostString = host.(CompileTimeConstantExpr).getStringValue() or
-    hostString =
-      host.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getStringValue()
-  |
-    hostString.length() = 0 or // Empty host is loopback address
-    hostString instanceof PrivateHostName
-  )
-}
-
-/** Gets the leftmost operand in a concatenated string */
-Expr getLeftmostConcatOperand(Expr expr) {
-  if expr instanceof AddExpr
-  then result = getLeftmostConcatOperand(expr.(AddExpr).getLeftOperand())
-  else result = expr
-}
-
-/**
- * String concatenated with `HttpStringLiteral`.
- */
-class HttpString extends Expr {
-  HttpString() {
-    this instanceof HttpStringLiteral
-    or
-    concatHttpString(this.(AddExpr).getLeftOperand(),
-      getLeftmostConcatOperand(this.(AddExpr).getRightOperand()))
-  }
-}
-
-/**
- * String pattern of basic authentication.
- */
-class BasicAuthString extends StringLiteral {
-  BasicAuthString() { exists(string s | this.getRepresentedString() = s | s.matches("Basic %")) }
-}
-
-/**
- * String concatenated with `BasicAuthString`.
- */
-predicate builtFromBasicAuthStringConcat(Expr expr) {
-  expr instanceof BasicAuthString
-  or
-  builtFromBasicAuthStringConcat(expr.(AddExpr).getLeftOperand())
-  or
-  exists(Expr other | builtFromBasicAuthStringConcat(other) |
-    exists(Variable var | var.getAnAssignedValue() = other and var.getAnAccess() = expr)
-  )
-}
-
-/** The `openConnection` method of Java URL. Not to include `openStream` since it won't be used in this query. */
-class HttpURLOpenMethod extends Method {
-  HttpURLOpenMethod() {
-    this.getDeclaringType() instanceof TypeUrl and
-    this.getName() = "openConnection"
-  }
-}
-
-/** Constructor of `ApacheHttpRequest` */
-predicate apacheHttpRequest(DataFlow::Node node1, DataFlow::Node node2) {
-  exists(ConstructorCall cc |
-    cc.getConstructedType() instanceof ApacheHttpRequest and
-    node2.asExpr() = cc and
-    cc.getAnArgument() = node1.asExpr()
-  )
-}
-
-/** `URI` methods */
-predicate createURI(DataFlow::Node node1, DataFlow::Node node2) {
-  exists(
-    URIConstructor cc // new URI
-  |
-    node2.asExpr() = cc and
-    cc.getArgument(0) = node1.asExpr()
-  )
-  or
-  exists(
-    StaticMethodAccess ma // URI.create
-  |
-    ma.getMethod().getDeclaringType() instanceof TypeUri and
-    ma.getMethod().hasName("create") and
-    node1.asExpr() = ma.getArgument(0) and
-    node2.asExpr() = ma
-  )
-}
-
-/** Constructors of `URL` */
-predicate createURL(DataFlow::Node node1, DataFlow::Node node2) {
-  exists(URLConstructor cc |
-    node2.asExpr() = cc and
-    cc.getArgument(0) = node1.asExpr()
-  )
-}
-
-/** Method call of `HttpURLOpenMethod` */
-predicate urlOpen(DataFlow::Node node1, DataFlow::Node node2) {
-  exists(MethodAccess ma |
-    ma.getMethod() instanceof HttpURLOpenMethod and
-    node1.asExpr() = ma.getQualifier() and
-    ma = node2.asExpr()
-  )
-}
-
 class BasicAuthFlowConfig extends TaintTracking::Configuration {
   BasicAuthFlowConfig() { this = "InsecureBasicAuth::BasicAuthFlowConfig" }
 
-  override predicate isSource(DataFlow::Node src) {
-    src.asExpr() instanceof HttpString
-    or
-    exists(URLConstructor uc |
-      uc.hasHttpStringArg() and
-      src.asExpr() = uc.getArgument(0)
-    )
-    or
-    exists(URIConstructor uc |
-      uc.hasHttpStringArg() and
-      src.asExpr() = uc.getArgument(0)
-    )
-  }
+  override predicate isSource(DataFlow::Node src) { src instanceof InsecureBasicAuthSource }
 
-  override predicate isSink(DataFlow::Node sink) {
-    exists(MethodAccess ma |
-      sink.asExpr() = ma.getQualifier() and
-      (
-        ma.getMethod().hasName("addHeader") or
-        ma.getMethod().hasName("setHeader") or
-        ma.getMethod().hasName("setRequestProperty")
-      ) and
-      ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "Authorization" and
-      builtFromBasicAuthStringConcat(ma.getArgument(1))
-    )
-  }
+  override predicate isSink(DataFlow::Node sink) { sink instanceof InsecureBasicAuthSink }
 
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
-    apacheHttpRequest(node1, node2) or
-    createURI(node1, node2) or
-    createURL(node1, node2) or
-    urlOpen(node1, node2)
+    any(InsecureBasicAuthAdditionalTaintStep c).step(node1, node2)
   }
 }
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, BasicAuthFlowConfig config
 where config.hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "Insecure basic authentication from $@.", source.getNode(),
-  "HTTP url"
+  "HTTP URL"