Merge pull request #1 from aschackmull/java/spring-3653

Java: Review changes for #3653

Author: lcartey

java/ql/src/semmle/code/java/dataflow/FlowSources.qll

@@ -217,18 +217,7 @@ private class RemoteTaintedMethod extends Method {
     this instanceof HttpServletRequestGetRequestURIMethod or
     this instanceof HttpServletRequestGetRequestURLMethod or
     this instanceof HttpServletRequestGetRemoteUserMethod or
-    exists(SpringWebRequest swr |
-      this = swr.getAMethod() |
-      this.hasName("getDescription") or
-      this.hasName("getHeader") or
-      this.hasName("getHeaderNames") or
-      this.hasName("getHeaderValues") or
-      this.hasName("getParameter") or
-      this.hasName("getParameterMap") or
-      this.hasName("getParameterNames") or
-      this.hasName("getParameterValues")
-      // TODO consider getRemoteUser
-    ) or
+    this instanceof SpringWebRequestGetMethod or
     this instanceof SpringRestTemplateResponseEntityMethod or
     this instanceof ServletRequestGetBodyMethod or
     this instanceof CookieGetValueMethod or
@@ -247,6 +236,22 @@ private class RemoteTaintedMethod extends Method {
   }
 }
 
+private class SpringWebRequestGetMethod extends Method {
+  SpringWebRequestGetMethod() {
+    exists(SpringWebRequest swr | this = swr.getAMethod() |
+      this.hasName("getDescription") or
+      this.hasName("getHeader") or
+      this.hasName("getHeaderNames") or
+      this.hasName("getHeaderValues") or
+      this.hasName("getParameter") or
+      this.hasName("getParameterMap") or
+      this.hasName("getParameterNames") or
+      this.hasName("getParameterValues")
+      // TODO consider getRemoteUser
+    )
+  }
+}
+
 private class EnvTaintedMethod extends Method {
   EnvTaintedMethod() {
     this instanceof MethodSystemGetenv or

java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

@@ -377,34 +377,19 @@ private predicate taintPreservingQualifierToMethod(Method m) {
   or
   m = any(ProtobufMessageLite p).getAGetterMethod()
   or
-  m instanceof MapMethod and
-  (
-    m.getName().regexpMatch("get|entrySet|keySet|values")
-  )
-  or
-  m.getDeclaringType().getSourceDeclaration().getASourceSupertype*().hasQualifiedName("java.util", "List") and
-  (
-    m.getName().regexpMatch("get|toArray|subList|spliterator|set|iterator|listIterator") or
-    (m.getName().regexpMatch("remove") and not m.getReturnType() instanceof BooleanType)
-  )
+  m instanceof GetterMethod and m.getDeclaringType() instanceof SpringUntrustedDataType
   or
-  m instanceof StringReplaceMethod
+  m.getDeclaringType() instanceof SpringHttpEntity and
+  m.getName().regexpMatch("getBody|getHeaders")
   or
-  exists(SpringUntrustedDataType dt |
-    m.(GetterMethod) = dt.getAMethod()
-  )
-  or
-  exists(SpringHttpEntity sre |
-    m = sre.getAMethod() and
-    m.getName().regexpMatch("getBody|getHeaders")
-  )
-  or
-  exists(SpringHttpHeaders headers |
-    m = headers.getAMethod() |
+  exists(SpringHttpHeaders headers | m = headers.getAMethod() |
     m.getReturnType() instanceof TypeString
     or
-    m.getReturnType().(RefType).getSourceDeclaration().getASourceSupertype*().hasQualifiedName("java.util", "List") and
-    m.getReturnType().(ParameterizedType).getTypeArgument(0) instanceof TypeString
+    exists(ParameterizedType stringlist |
+      m.getReturnType().(RefType).getASupertype*() = stringlist and
+      stringlist.getSourceDeclaration().hasQualifiedName("java.util", "List") and
+      stringlist.getTypeArgument(0) instanceof TypeString
+    )
   )
 }
 

java/ql/src/semmle/code/java/frameworks/SpringWeb.qll

@@ -1,3 +1,3 @@
 import java
 import spring.SpringController
-import spring.SpringWeb
+import spring.SpringWeb

java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll

@@ -19,12 +19,10 @@ class SpringControllerAnnotation extends AnnotationType {
 /**
  * An annotation type that identifies Spring rest controllers.
  *
- * Rest controllers are the same as controllers, but imply the @ResponseBody annotation.
+ * Rest controllers are the same as controllers, but imply the `@ResponseBody` annotation.
  */
 class SpringRestControllerAnnotation extends SpringControllerAnnotation {
-  SpringRestControllerAnnotation() {
-    hasName("RestController")
-  }
+  SpringRestControllerAnnotation() { hasName("RestController") }
 }
 
 /**
@@ -80,7 +78,7 @@ class SpringInitBinderMethod extends SpringControllerMethod {
 }
 
 /**
- * An `AnnotationType` which is used to indicate a `RequestMapping`.
+ * An `AnnotationType` that is used to indicate a `RequestMapping`.
  */
 class SpringRequestMappingAnnotationType extends AnnotationType {
   SpringRequestMappingAnnotationType() {
@@ -93,7 +91,7 @@ class SpringRequestMappingAnnotationType extends AnnotationType {
 }
 
 /**
- * An `AnnotationType` which is used to indicate a `ResponseBody`.
+ * An `AnnotationType` that is used to indicate a `ResponseBody`.
  */
 class SpringResponseBodyAnnotationType extends AnnotationType {
   SpringResponseBodyAnnotationType() {
@@ -107,6 +105,7 @@ class SpringResponseBodyAnnotationType extends AnnotationType {
  */
 class SpringRequestMappingMethod extends SpringControllerMethod {
   Annotation requestMappingAnnotation;
+
   SpringRequestMappingMethod() {
     // Any method that declares the @RequestMapping annotation, or overrides a method that declares
     // the annotation. We have to do this explicit check because the @RequestMapping annotation is
@@ -119,21 +118,18 @@ class SpringRequestMappingMethod extends SpringControllerMethod {
   }
 
   /** Gets a request mapping parameter. */
-  SpringRequestMappingParameter getARequestParameter() {
-    result = getAParameter()
-  }
+  SpringRequestMappingParameter getARequestParameter() { result = getAParameter() }
 
   /** Gets the "produces" @RequestMapping annotation value, if present. */
   string getProduces() {
-    result = requestMappingAnnotation.getValue("produces").(CompileTimeConstantExpr).getStringValue()
+    result =
+      requestMappingAnnotation.getValue("produces").(CompileTimeConstantExpr).getStringValue()
   }
 
-  /** Holds if this is considered an @ResponseBody method. */
+  /** Holds if this is considered an `@ResponseBody` method. */
   predicate isResponseBody() {
-    getAnAnnotation().getType() instanceof SpringResponseBodyAnnotationType
-    or
-    getDeclaringType().getAnAnnotation().getType() instanceof SpringResponseBodyAnnotationType
-    or
+    getAnAnnotation().getType() instanceof SpringResponseBodyAnnotationType or
+    getDeclaringType().getAnAnnotation().getType() instanceof SpringResponseBodyAnnotationType or
     getDeclaringType() instanceof SpringRestController
   }
 }
@@ -156,12 +152,14 @@ class SpringServletInputAnnotation extends Annotation {
   }
 }
 
+/** An annotation of the type `org.springframework.web.bind.annotation.ModelAttribute`. */
 class SpringModelAttributeAnnotation extends Annotation {
   SpringModelAttributeAnnotation() {
     getType().hasQualifiedName("org.springframework.web.bind.annotation", "ModelAttribute")
   }
 }
 
+/** A parameter of a `SpringRequestMappingMethod`. */
 class SpringRequestMappingParameter extends Parameter {
   SpringRequestMappingParameter() { getCallable() instanceof SpringRequestMappingMethod }
 
@@ -180,29 +178,47 @@ class SpringRequestMappingParameter extends Parameter {
     getType().(RefType).getAnAncestor().hasQualifiedName("java.time", "ZoneId") or
     getType().(RefType).getAnAncestor().hasQualifiedName("java.io", "OutputStream") or
     getType().(RefType).getAnAncestor().hasQualifiedName("java.io", "Writer") or
-    getType().(RefType).getAnAncestor().hasQualifiedName("org.springframework.web.servlet.mvc.support", "RedirectAttributes") or
+    getType()
+        .(RefType)
+        .getAnAncestor()
+        .hasQualifiedName("org.springframework.web.servlet.mvc.support", "RedirectAttributes") or
     // Also covers BindingResult. Note, you can access the field value through this interface, which should be considered tainted
     getType().(RefType).getAnAncestor().hasQualifiedName("org.springframework.validation", "Errors") or
-    getType().(RefType).getAnAncestor().hasQualifiedName("org.springframework.web.bind.support", "SessionStatus") or
-    getType().(RefType).getAnAncestor().hasQualifiedName("org.springframework.web.util", "UriComponentsBuilder") or
-    getType().(RefType).getAnAncestor().hasQualifiedName("org.springframework.data.domain", "Pageable") or
+    getType()
+        .(RefType)
+        .getAnAncestor()
+        .hasQualifiedName("org.springframework.web.bind.support", "SessionStatus") or
+    getType()
+        .(RefType)
+        .getAnAncestor()
+        .hasQualifiedName("org.springframework.web.util", "UriComponentsBuilder") or
+    getType()
+        .(RefType)
+        .getAnAncestor()
+        .hasQualifiedName("org.springframework.data.domain", "Pageable") or
     this instanceof SpringModel
   }
 
-  predicate isExplicitlyTaintedInput() {
+  private predicate isExplicitlyTaintedInput() {
     // InputStream or Reader parameters allow access to the body of a request
     getType().(RefType).getAnAncestor().hasQualifiedName("java.io", "InputStream") or
     getType().(RefType).getAnAncestor().hasQualifiedName("java.io", "Reader") or
     // The SpringServletInputAnnotations allow access to the URI, request parameters, cookie values and the body of the request
     this.getAnAnnotation() instanceof SpringServletInputAnnotation or
     // HttpEntity is like @RequestBody, but with a wrapper including the headers
     // TODO model unwrapping aspects
-    getType().(RefType).getAnAncestor().hasQualifiedName("org.springframework.http", "HttpEntity<T>") or
-    this.getAnAnnotation().getType().hasQualifiedName("org.springframework.web.bind.annotation", "RequestAttribute") or
-    this.getAnAnnotation().getType().hasQualifiedName("org.springframework.web.bind.annotation", "SessionAttribute")
+    getType().(RefType).getASourceSupertype*() instanceof SpringHttpEntity or
+    this
+        .getAnAnnotation()
+        .getType()
+        .hasQualifiedName("org.springframework.web.bind.annotation", "RequestAttribute") or
+    this
+        .getAnAnnotation()
+        .getType()
+        .hasQualifiedName("org.springframework.web.bind.annotation", "SessionAttribute")
   }
 
-  predicate isImplicitRequestParam() {
+  private predicate isImplicitRequestParam() {
     // Any parameter which is not explicitly handled, is consider to be an `@RequestParam`, if
     // it is a simple bean property
     not isNotDirectlyTaintedInput() and
@@ -213,23 +229,24 @@ class SpringRequestMappingParameter extends Parameter {
     )
   }
 
-  predicate isImplicitModelAttribute() {
+  private predicate isImplicitModelAttribute() {
     // Any parameter which is not explicitly handled, is consider to be an `@ModelAttribute`, if
     // it is not an implicit request param
     not isNotDirectlyTaintedInput() and
     not isExplicitlyTaintedInput() and
     not isImplicitRequestParam()
   }
 
-  /** Holds if this is an explicit or implicit @ModelAttribute parameter */
+  /** Holds if this is an explicit or implicit `@ModelAttribute` parameter. */
   predicate isModelAttribute() {
     isImplicitModelAttribute() or
     getAnAnnotation() instanceof SpringModelAttributeAnnotation
   }
 
-  /** Holds if the input is tainted */
+  /** Holds if the input is tainted. */
   predicate isTaintedInput() {
-    isExplicitlyTaintedInput() or
+    isExplicitlyTaintedInput()
+    or
     // Any parameter which is not explicitly identified, is consider to be an `@RequestParam`, if
     // it is a simple bean property) or a @ModelAttribute if not
     not isNotDirectlyTaintedInput()
@@ -305,18 +322,18 @@ private RefType stripType(Type t) {
 }
 
 /**
- * A user data type which may be populated from a HTTP request.
+ * A user data type that may be populated from an HTTP request.
  *
- * This includes types directly referred to as either @ModelAttribute or @RequestBody parameters,
- * or types which are referred to by those types.
+ * This includes types directly referred to as either `@ModelAttribute` or `@RequestBody` parameters,
+ * or types that are referred to by those types.
  */
 class SpringUntrustedDataType extends RefType {
   SpringUntrustedDataType() {
     exists(SpringRequestMappingParameter p |
       p.isModelAttribute()
       or
       p.getAnAnnotation().(SpringServletInputAnnotation).getType().hasName("RequestBody")
-      |
+    |
       this.fromSource() and
       this = stripType(p.getType())
     )

java/ql/src/semmle/code/java/frameworks/spring/SpringHttp.qll

@@ -1,39 +1,40 @@
+/**
+ * Provides classes for working with Spring classes and interfaces from
+ * `org.springframework.http`.
+ */
+
 import java
 
+/** The class `org.springframework.http.HttpEntity` or an instantiation of it. */
 class SpringHttpEntity extends Class {
   SpringHttpEntity() {
-    getSourceDeclaration()
-        .getErasure()
-        .(RefType)
-        .hasQualifiedName("org.springframework.http", "HttpEntity")
+    this.getSourceDeclaration().hasQualifiedName("org.springframework.http", "HttpEntity")
   }
 }
 
+/** The class `org.springframework.http.RequestEntity` or an instantiation of it. */
 class SpringRequestEntity extends Class {
   SpringRequestEntity() {
-    getSourceDeclaration()
-        .getErasure()
-        .(RefType)
-        .hasQualifiedName("org.springframework.http", "RequestEntity")
+    this.getSourceDeclaration().hasQualifiedName("org.springframework.http", "RequestEntity")
   }
 }
 
+/** The class `org.springframework.http.ResponseEntity` or an instantiation of it. */
 class SpringResponseEntity extends Class {
   SpringResponseEntity() {
-    getSourceDeclaration()
-        .getErasure()
-        .(RefType)
-        .hasQualifiedName("org.springframework.http", "ResponseEntity")
+    this.getSourceDeclaration().hasQualifiedName("org.springframework.http", "ResponseEntity")
   }
 }
 
+/** The nested class `BodyBuilder` in `org.springframework.http.ResponseEntity`. */
 class SpringResponseEntityBodyBuilder extends Interface {
   SpringResponseEntityBodyBuilder() {
-    getSourceDeclaration().getEnclosingType() = any(SpringResponseEntity sre) and
-    hasName("BodyBuilder")
+    this.getSourceDeclaration().getEnclosingType() instanceof SpringResponseEntity and
+    this.hasName("BodyBuilder")
   }
 }
 
+/** The class `org.springframework.http.HttpHeaders`. */
 class SpringHttpHeaders extends Class {
-  SpringHttpHeaders() { hasQualifiedName("org.springframework.http", "HttpHeaders") }
-}
+  SpringHttpHeaders() { this.hasQualifiedName("org.springframework.http", "HttpHeaders") }
+}

java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll

@@ -1,15 +1,19 @@
+/**
+ * Provides classes for working with Spring web requests.
+ */
+
 import java
 
 /** An interface for web requests in the Spring framework. */
 class SpringWebRequest extends Class {
   SpringWebRequest() {
-    hasQualifiedName("org.springframework.web.context.request", "WebRequest")
+    this.hasQualifiedName("org.springframework.web.context.request", "WebRequest")
   }
 }
 
 /** An interface for web requests in the Spring framework. */
 class SpringNativeWebRequest extends Class {
   SpringNativeWebRequest() {
-    hasQualifiedName("org.springframework.web.context.request", "NativeWebRequest")
+    this.hasQualifiedName("org.springframework.web.context.request", "NativeWebRequest")
   }
-}
+}