From f5dc0337edda85b24f9ffeec9c589648233a8e8a Mon Sep 17 00:00:00 2001 From: "lcartey@github.com" Date: Fri, 15 May 2020 16:41:50 +0100 Subject: [PATCH] Java: Improve modelling of Spring request methods - Recognise @Mapping as well as @RequestMapping. - Identify tainted/not tainted parameters of RequestMapping methods. --- .../semmle/code/java/frameworks/SpringWeb.qll | 20 +---- .../frameworks/spring/SpringController.qll | 82 ++++++++++++++++++- .../code/java/frameworks/spring/SpringWeb.qll | 15 ++++ 3 files changed, 98 insertions(+), 19 deletions(-) create mode 100644 java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll diff --git a/java/ql/src/semmle/code/java/frameworks/SpringWeb.qll b/java/ql/src/semmle/code/java/frameworks/SpringWeb.qll index 8f3fafb5540..d3e195b93be 100644 --- a/java/ql/src/semmle/code/java/frameworks/SpringWeb.qll +++ b/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 \ No newline at end of file diff --git a/java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll b/java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll index 0db24786c35..6f530b2f576 100644 --- a/java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll +++ b/java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll @@ -1,4 +1,5 @@ import java +import SpringWeb /** * An annotation type that identifies Spring components. @@ -58,6 +59,19 @@ class SpringInitBinderMethod extends SpringControllerMethod { } } +/** + * An `AnnotationType` which is used to indicate a `RequestMapping`. + */ +class SpringRequestMappingAnnotationType extends AnnotationType { + SpringRequestMappingAnnotationType() { + // `@RequestMapping` used directly as an annotation. + hasQualifiedName("org.springframework.web.bind.annotation", "RequestMapping") + or + // `@RequestMapping` can be used as a meta-annotation on other annotation types, e.g. GetMapping, PostMapping etc. + getAnAnnotation().getType() instanceof SpringRequestMappingAnnotationType + } +} + /** * A method on a Spring controller that is executed in response to a web request. */ @@ -68,9 +82,75 @@ class SpringRequestMappingMethod extends SpringControllerMethod { // not declared with @Inherited. exists(Method superMethod | this.overrides*(superMethod) and - superMethod.hasAnnotation("org.springframework.web.bind.annotation", "RequestMapping") + superMethod.getAnAnnotation().getType() instanceof SpringRequestMappingAnnotationType ) } + + /** Gets a request mapping parameter. */ + SpringRequestMappingParameter getARequestParameter() { + result = getAParameter() + } +} + +/** 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") + ) + } +} + +class SpringRequestMappingParameter extends Parameter { + SpringRequestMappingParameter() { getCallable() instanceof SpringRequestMappingMethod } + + /** Holds if the parameter should not be consider a direct source of taint. */ + predicate isNotDirectlyTaintedInput() { + getType().(RefType).getAnAncestor() instanceof SpringWebRequest or + getType().(RefType).getAnAncestor() instanceof SpringNativeWebRequest or + getType().(RefType).getAnAncestor().hasQualifiedName("javax.servlet", "ServletRequest") or + getType().(RefType).getAnAncestor().hasQualifiedName("javax.servlet", "ServletResponse") or + getType().(RefType).getAnAncestor().hasQualifiedName("javax.servlet.http", "HttpSession") or + getType().(RefType).getAnAncestor().hasQualifiedName("javax.servlet.http", "PushBuilder") or + getType().(RefType).getAnAncestor().hasQualifiedName("java.security", "Principal") or + getType().(RefType).getAnAncestor().hasQualifiedName("org.springframework.http", "HttpMethod") or + getType().(RefType).getAnAncestor().hasQualifiedName("java.util", "Locale") or + getType().(RefType).getAnAncestor().hasQualifiedName("java.util", "TimeZone") or + 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 + // 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 + this instanceof SpringModel + } + + predicate isTaintedInput() { + // 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") or + this.getAnAnnotation().getType().hasQualifiedName("org.springframework.web.bind.annotation", "RequestAttribute") or + this.getAnAnnotation().getType().hasQualifiedName("org.springframework.web.bind.annotation", "SessionAttribute") 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() + } } /** diff --git a/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll b/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll new file mode 100644 index 00000000000..22addc852a4 --- /dev/null +++ b/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll @@ -0,0 +1,15 @@ +import java + +/** An interface for web requests in the Spring framework. */ +class SpringWebRequest extends Class { + SpringWebRequest() { + 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") + } +} \ No newline at end of file