Merge pull request #3653 from lcartey/java/improve-spring-support

Java: Improve modelling of Spring requests, flow steps and XSS sinks

Author: aschackmull

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

@@ -16,7 +16,9 @@ import semmle.code.java.frameworks.android.XmlParsing
 import semmle.code.java.frameworks.android.WebView
 import semmle.code.java.frameworks.JaxWS
 import semmle.code.java.frameworks.android.Intent
-import semmle.code.java.frameworks.SpringWeb
+import semmle.code.java.frameworks.spring.SpringWeb
+import semmle.code.java.frameworks.spring.SpringController
+import semmle.code.java.frameworks.spring.SpringWebClient
 import semmle.code.java.frameworks.Guice
 import semmle.code.java.frameworks.struts.StrutsActions
 import semmle.code.java.frameworks.Thrift
@@ -118,7 +120,7 @@ private class SpringMultipartFileSource extends RemoteFlowSource {
 
 private class SpringServletInputParameterSource extends RemoteFlowSource {
   SpringServletInputParameterSource() {
-    this.asParameter().getAnAnnotation() instanceof SpringServletInputAnnotation
+    this.asParameter() = any(SpringRequestMappingParameter srmp | srmp.isTaintedInput())
   }
 
   override string getSourceType() { result = "Spring servlet input parameter" }
@@ -215,6 +217,8 @@ private class RemoteTaintedMethod extends Method {
     this instanceof HttpServletRequestGetRequestURIMethod or
     this instanceof HttpServletRequestGetRequestURLMethod or
     this instanceof HttpServletRequestGetRemoteUserMethod or
+    this instanceof SpringWebRequestGetMethod or
+    this instanceof SpringRestTemplateResponseEntityMethod or
     this instanceof ServletRequestGetBodyMethod or
     this instanceof CookieGetValueMethod or
     this instanceof CookieGetNameMethod or
@@ -232,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

@@ -8,6 +8,8 @@ private import semmle.code.java.security.Validation
 private import semmle.code.java.frameworks.android.Intent
 private import semmle.code.java.frameworks.Guice
 private import semmle.code.java.frameworks.Protobuf
+private import semmle.code.java.frameworks.spring.SpringController
+private import semmle.code.java.frameworks.spring.SpringHttp
 private import semmle.code.java.Maps
 private import semmle.code.java.dataflow.internal.ContainerFlow
 private import semmle.code.java.frameworks.jackson.JacksonSerializability
@@ -252,6 +254,22 @@ private predicate constructorStep(Expr tracked, ConstructorCall sink) {
     or
     // a custom InputStream that wraps a tainted data source is tainted
     inputStreamWrapper(sink.getConstructor(), argi)
+    or
+    // A SpringHttpEntity is a wrapper around a body and some headers
+    // Track flow through iff body is a String
+    exists(SpringHttpEntity she |
+      sink.getConstructor() = she.getAConstructor() and
+      argi = 0 and
+      tracked.getType() instanceof TypeString
+    )
+    or
+    // A SpringRequestEntity is a wrapper around a body and some headers
+    // Track flow through iff body is a String
+    exists(SpringResponseEntity sre |
+      sink.getConstructor() = sre.getAConstructor() and
+      argi = 0 and
+      tracked.getType() instanceof TypeString
+    )
   )
 }
 
@@ -358,6 +376,21 @@ private predicate taintPreservingQualifierToMethod(Method m) {
   m = any(GuiceProvider gp).getAnOverridingGetMethod()
   or
   m = any(ProtobufMessageLite p).getAGetterMethod()
+  or
+  m instanceof GetterMethod and m.getDeclaringType() instanceof SpringUntrustedDataType
+  or
+  m.getDeclaringType() instanceof SpringHttpEntity and
+  m.getName().regexpMatch("getBody|getHeaders")
+  or
+  exists(SpringHttpHeaders headers | m = headers.getAMethod() |
+    m.getReturnType() instanceof TypeString
+    or
+    exists(ParameterizedType stringlist |
+      m.getReturnType().(RefType).getASupertype*() = stringlist and
+      stringlist.getSourceDeclaration().hasQualifiedName("java.util", "List") and
+      stringlist.getTypeArgument(0) instanceof TypeString
+    )
+  )
 }
 
 private class StringReplaceMethod extends Method {
@@ -393,6 +426,22 @@ private predicate argToMethodStep(Expr tracked, MethodAccess sink) {
     tracked = ma.getAnArgument() and
     sink = ma
   )
+  or
+  exists(Method springResponseEntityOfOk |
+    sink.getMethod() = springResponseEntityOfOk and
+    springResponseEntityOfOk.getDeclaringType() instanceof SpringResponseEntity and
+    springResponseEntityOfOk.getName().regexpMatch("ok|of") and
+    tracked = sink.getArgument(0) and
+    tracked.getType() instanceof TypeString
+  )
+  or
+  exists(Method springResponseEntityBody |
+    sink.getMethod() = springResponseEntityBody and
+    springResponseEntityBody.getDeclaringType() instanceof SpringResponseEntityBodyBuilder and
+    springResponseEntityBody.getName().regexpMatch("body") and
+    tracked = sink.getArgument(0) and
+    tracked.getType() instanceof TypeString
+  )
 }
 
 /**

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

@@ -1,19 +1,3 @@
 import java
-
-/** A Spring framework annotation indicating remote user input from servlets. */
-class SpringServletInputAnnotation extends Annotation {
-  SpringServletInputAnnotation() {
-    exists(AnnotationType a |
-      a = this.getType() and
-      a.getPackage().getName() = "org.springframework.web.bind.annotation"
-    |
-      a.hasName("MatrixVariable") or
-      a.hasName("RequestParam") or
-      a.hasName("RequestHeader") or
-      a.hasName("CookieValue") or
-      a.hasName("RequestPart") or
-      a.hasName("PathVariable") or
-      a.hasName("RequestBody")
-    )
-  }
-}
+import spring.SpringController
+import spring.SpringWeb