diff --git a/java/ql/lib/ext/unsafeUrlForwardExperimentalMove.model.yml b/java/ql/lib/ext/unsafeUrlForwardExperimentalMove.model.yml index b48d891e692..27c1094d765 100644 --- a/java/ql/lib/ext/unsafeUrlForwardExperimentalMove.model.yml +++ b/java/ql/lib/ext/unsafeUrlForwardExperimentalMove.model.yml @@ -6,56 +6,18 @@ extensions: - ["jakarta.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual"] - ["javax.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual"] - # # ! below added by me when debugging CVEs: - # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "retrieve", "(String,String,String,ServletWebRequest,boolean)", "", "Parameter[3]", "remote", "manual"] - # - ["org.springframework.web.context.request", "ServletWebRequest", True, "getContextPath", "()", "", "ReturnValue", "remote", "manual"] - - addsTo: pack: codeql/java-all extensible: sinkModel data: - ["java.util.concurrent", "TimeUnit", True, "sleep", "", "", "Argument[0]", "thread-pause", "manual"] # ! this seems like a typo; doesn't look like it's used in the query at all - - ["org.springframework.core.io", "ClassPathResource", True, "getFilename", "", "", "Argument[this]", "get-resource", "manual"] # ! Note: `ClassPathResource` implements `Resource`, so it might make more sense to model some of these as `Resource` with subtype True. - - ["org.springframework.core.io", "ClassPathResource", True, "getPath", "", "", "Argument[this]", "get-resource", "manual"] - - ["org.springframework.core.io", "ClassPathResource", True, "getURL", "", "", "Argument[this]", "get-resource", "manual"] - - ["org.springframework.core.io", "ClassPathResource", True, "resolveURL", "", "", "Argument[this]", "get-resource", "manual"] - # # ! below added by me when debugging CVEs: - # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "retrieve", "", "", "Argument[0]", "get-resource", "manual"] # don't need - # # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "getFilePath", "", "", "Argument[0..3]", "get-resource", "manual"] # don't need - # # - ["org.springframework.cloud.config.server.resource", "ResourceRepository", True, "findOne", "", "", "Argument[0..3]", "get-resource", "manual"] # convert to summary - # # - ["org.springframework.core.io", "InputStreamSource", True, "getInputStream", "", "", "Argument[this]", "get-resource", "manual"] # convert to summary - # - ["org.springframework.util", "StreamUtils", True, "copyToString", "", "", "Argument[0]", "get-resource", "manual"] # * public class with good docs - # # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator", True, "getLocations", "(String,String,String)", "", "Argument[0..2]", "get-resource", "manual"] # convert to summary - # # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator$Locations", True, "getLocations", "()", "", "Argument[this]", "get-resource", "manual"] # convert to summary - # - ["org.springframework.core.io", "ResourceLoader", True, "getResource", "", "", "Argument[0]", "get-resource", "manual"] # * public interface with good docs, might be problematic for FPs based on fact that the ext contributor changed this to a taint step to avoid "exists" FPS (maybe there's another way to exclude those FPs though). - # - ["javax.servlet", "ServletContext", True, "getResource", "", "", "Argument[0]", "get-resource", "manual"] - # - ["javax.servlet", "ServletContext", True, "getResourceAsStream", "", "", "Argument[0]", "get-resource", "manual"] - # - ["javax.servlet", "ServletContext", True, "getResourcePaths", "", "", "Argument[0]", "get-resource", "manual"] - # - ["javax.servlet", "ServletContext", True, "getResource", "", "", "Argument[this]", "get-resource", "manual"] - # - ["javax.servlet", "ServletContext", True, "getResourceAsStream", "", "", "Argument[this]", "get-resource", "manual"] - # - ["javax.servlet", "ServletContext", True, "getResourcePaths", "", "", "Argument[this]", "get-resource", "manual"] - # # - ["org.apache.tomcat.util.http", "RequestUtil", True, "normalize", "", "", "Argument[0]", "get-resource", "manual"] - - addsTo: pack: codeql/java-all extensible: summaryModel data: - - ["io.undertow.server.handlers.resource", "Resource", True, "getFile", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] - - ["io.undertow.server.handlers.resource", "Resource", True, "getFilePath", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] - - ["io.undertow.server.handlers.resource", "Resource", True, "getPath", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! this as a taint step seems to contradict the fact that they did `ClassPathResource.getPath` as a sink for Spring... - - ["java.nio.file", "Path", True, "normalize", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! shouldn't this be a sanitizer instead??? Or no because WEB-INF ones don't care about normalization? + - ["java.nio.file", "Path", True, "normalize", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] - ["java.nio.file", "Path", True, "resolve", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! check if this and the below are already in the default models - ["java.nio.file", "Path", True, "resolve", "", "", "Argument[0]", "ReturnValue", "taint", "manual"] - ["java.nio.file", "Path", True, "toString", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] - ["java.nio.file", "Paths", True, "get", "", "", "Argument[0..1]", "ReturnValue", "taint", "manual"] - - - ["org.springframework.core.io", "ClassPathResource", False, "ClassPathResource", "", "", "Argument[0]", "Argument[this]", "taint", "manual"] - - ["org.springframework.core.io", "Resource", True, "createRelative", "", "", "Argument[0]", "ReturnValue", "taint", "manual"] - # # ! below added/modified by me when debugging CVEs: - - ["org.springframework.core.io", "ResourceLoader", True, "getResource", "", "", "Argument[0]", "ReturnValue", "taint", "manual"] - # - ["org.springframework.cloud.config.server.resource", "ResourceRepository", True, "findOne", "", "", "Argument[0..3]", "ReturnValue", "taint", "manual"] # * public interface, but might be too specific, no easily findable docs... - # - ["org.springframework.core.io", "InputStreamSource", True, "getInputStream", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # * public interface with good docs, Note: other `getInputStream`s are remote source and/or taint step, so this as taint step versus sink probably is more consistent - # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator", True, "getLocations", "(String,String,String)", "", "Argument[0..2]", "ReturnValue", "taint", "manual"] # * public interface with docs: https://www.javadoc.io/static/org.springframework.cloud/spring-cloud-config-server/2.1.0.RELEASE/org/springframework/cloud/config/server/environment/SearchPathLocator.html - # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator$Locations", True, "getLocations", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! is the `Locations` class package-private? Or does it inherit public from it's enclosing interface? - # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "getFilePath", "", "", "Argument[0..3]", "ReturnValue", "taint", "manual"] # don't need diff --git a/java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll b/java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll index 48b4431015e..dd3e17aa832 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeUrlForward.qll @@ -22,96 +22,7 @@ private class RequestDispatcherSink extends UnsafeUrlForwardSink { } } -/** The `getResource` method of `Class`. */ -class GetClassResourceMethod extends Method { - GetClassResourceMethod() { - this.getDeclaringType() instanceof TypeClass and - this.hasName("getResource") - } -} - -/** The `getResourceAsStream` method of `Class`. */ -class GetClassResourceAsStreamMethod extends Method { - GetClassResourceAsStreamMethod() { - this.getDeclaringType() instanceof TypeClass and - this.hasName("getResourceAsStream") - } -} - -/** The `getResource` method of `ClassLoader`. */ -class GetClassLoaderResourceMethod extends Method { - GetClassLoaderResourceMethod() { - this.getDeclaringType() instanceof ClassLoaderClass and - this.hasName("getResource") - } -} - -/** The `getResourceAsStream` method of `ClassLoader`. */ -class GetClassLoaderResourceAsStreamMethod extends Method { - GetClassLoaderResourceAsStreamMethod() { - this.getDeclaringType() instanceof ClassLoaderClass and - this.hasName("getResourceAsStream") - } -} - -/** The JBoss class `FileResourceManager`. */ -class FileResourceManager extends RefType { - FileResourceManager() { - this.hasQualifiedName("io.undertow.server.handlers.resource", "FileResourceManager") - } -} - -/** The JBoss method `getResource` of `FileResourceManager`. */ -class GetWildflyResourceMethod extends Method { - GetWildflyResourceMethod() { - this.getDeclaringType().getASupertype*() instanceof FileResourceManager and - this.hasName("getResource") - } -} - -/** The JBoss class `VirtualFile`. */ -class VirtualFile extends RefType { - VirtualFile() { this.hasQualifiedName("org.jboss.vfs", "VirtualFile") } -} - -/** The JBoss method `getChild` of `FileResourceManager`. */ -class GetVirtualFileChildMethod extends Method { - GetVirtualFileChildMethod() { - this.getDeclaringType().getASupertype*() instanceof VirtualFile and - this.hasName("getChild") - } -} - -/** An argument to `getResource()` or `getResourceAsStream()`. */ -private class GetResourceSink extends UnsafeUrlForwardSink { - GetResourceSink() { - sinkNode(this, "request-forgery") - or - sinkNode(this, "get-resource") - or - exists(MethodCall ma | - ( - ma.getMethod() instanceof GetServletResourceAsStreamMethod or - ma.getMethod() instanceof GetFacesResourceAsStreamMethod or - ma.getMethod() instanceof GetClassResourceAsStreamMethod or - ma.getMethod() instanceof GetClassLoaderResourceAsStreamMethod or - ma.getMethod() instanceof GetVirtualFileChildMethod - ) and - ma.getArgument(0) = this.asExpr() - ) - } -} - -/** A sink for methods that load Spring resources. */ -private class SpringResourceSink extends UnsafeUrlForwardSink { - SpringResourceSink() { - exists(MethodCall ma | - ma.getMethod() instanceof GetResourceUtilsMethod and - ma.getArgument(0) = this.asExpr() - ) - } -} - +// TODO: look into `StaplerResponse.forward`, etc., and think about re-adding the MaD "request-forgery" sinks as a result /** An argument to `new ModelAndView` or `ModelAndView.setViewName`. */ private class SpringModelAndViewSink extends UnsafeUrlForwardSink { SpringModelAndViewSink() { diff --git a/java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll b/java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll index 9ee3f2ab417..6cd419a5e13 100644 --- a/java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll +++ b/java/ql/lib/semmle/code/java/security/UnsafeUrlForwardQuery.qll @@ -1,3 +1,5 @@ +/** Provides configurations to be used in queries related to unsafe URL forwarding. */ + import java import semmle.code.java.security.UnsafeUrlForward import semmle.code.java.dataflow.FlowSources @@ -5,9 +7,13 @@ import semmle.code.java.dataflow.TaintTracking import semmle.code.java.Jsf import semmle.code.java.security.PathSanitizer +/** + * A taint-tracking configuration for untrusted user input in a URL forward or include. + */ module UnsafeUrlForwardFlowConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource and + // TODO: move below logic to class in UnsafeUrlForward.qll? not exists(MethodCall ma, Method m | ma.getMethod() = m | ( m instanceof HttpServletRequestGetRequestUriMethod or @@ -25,21 +31,11 @@ module UnsafeUrlForwardFlowConfig implements DataFlow::ConfigSig { node instanceof PathInjectionSanitizer } + // TODO: check if the below is still needed after removing path-injection related sinks. DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } - - predicate isAdditionalFlowStep(DataFlow::Node prev, DataFlow::Node succ) { - exists(MethodCall ma | - ( - ma.getMethod() instanceof GetServletResourceMethod or - ma.getMethod() instanceof GetFacesResourceMethod or - ma.getMethod() instanceof GetClassResourceMethod or - ma.getMethod() instanceof GetClassLoaderResourceMethod or - ma.getMethod() instanceof GetWildflyResourceMethod - ) and - ma.getArgument(0) = prev.asExpr() and - ma = succ.asExpr() - ) - } } +/** + * Taint-tracking flow for untrusted user input in a URL forward or include. + */ module UnsafeUrlForwardFlow = TaintTracking::Global; diff --git a/java/ql/test/query-tests/security/CWE-552/UnsafeLoadSpringResource.java b/java/ql/test/query-tests/security/CWE-552/UnsafeLoadSpringResource.java deleted file mode 100644 index 363d84cabe9..00000000000 --- a/java/ql/test/query-tests/security/CWE-552/UnsafeLoadSpringResource.java +++ /dev/null @@ -1,155 +0,0 @@ -package com.example; - -import java.io.File; -import java.io.FileReader; -import java.io.InputStreamReader; -import java.io.IOException; -import java.io.Reader; -import java.io.UnsupportedEncodingException; -import java.net.URLDecoder; -import java.nio.file.Files; - -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.core.io.ClassPathResource; -import org.springframework.core.io.Resource; -import org.springframework.core.io.ResourceLoader; -import org.springframework.util.ResourceUtils; -import org.springframework.util.StringUtils; -import org.springframework.web.bind.annotation.GetMapping; -import org.springframework.web.bind.annotation.RequestParam; -import org.springframework.web.bind.annotation.RestController; - -/** Sample class of Spring RestController */ -@RestController -public class UnsafeLoadSpringResource { - @GetMapping("/file1") - //BAD: Get resource from ClassPathResource without input validation - public String getFileContent1(@RequestParam(name="fileName") String fileName) { - // A request such as the following can disclose source code and application configuration - // fileName=/../../WEB-INF/views/page.jsp - // fileName=/com/example/package/SampleController.class - ClassPathResource clr = new ClassPathResource(fileName); - char[] buffer = new char[4096]; - StringBuilder out = new StringBuilder(); - try { - Reader in = new FileReader(clr.getFilename()); // $ hasUnsafeUrlForward (path-inj?) - for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) { - out.append(buffer, 0, numRead); - } - } catch (IOException ie) { - ie.printStackTrace(); - } - return out.toString(); - } - - @GetMapping("/file1a") - //GOOD: Get resource from ClassPathResource with input path validation - public String getFileContent1a(@RequestParam(name="fileName") String fileName) { - String result = null; - if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) { - ClassPathResource clr = new ClassPathResource(fileName); - char[] buffer = new char[4096]; - StringBuilder out = new StringBuilder(); - try { - Reader in = new InputStreamReader(clr.getInputStream(), "UTF-8"); - for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) { - out.append(buffer, 0, numRead); - } - } catch (IOException ie) { - ie.printStackTrace(); - } - result = out.toString(); - } - return result; - } - - @GetMapping("/file2") - //BAD: Get resource from ResourceUtils without input validation - public String getFileContent2(@RequestParam(name="fileName") String fileName) { - String content = null; - - try { - // A request such as the following can disclose source code and system configuration - // fileName=/etc/hosts - // fileName=file:/etc/hosts - // fileName=/opt/appdir/WEB-INF/views/page.jsp - File file = ResourceUtils.getFile(fileName); // $ hasUnsafeUrlForward (path-inj?) - //Read File Content - content = new String(Files.readAllBytes(file.toPath())); - } catch (IOException ie) { - ie.printStackTrace(); - } - return content; - } - - @GetMapping("/file2a") - //GOOD: Get resource from ResourceUtils with input path validation - public String getFileContent2a(@RequestParam(name="fileName") String fileName) { - String content = null; - - if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) { - try { - File file = ResourceUtils.getFile(fileName); - //Read File Content - content = new String(Files.readAllBytes(file.toPath())); - } catch (IOException ie) { - ie.printStackTrace(); - } - } - return content; - } - - @Autowired - ResourceLoader resourceLoader; - - @GetMapping("/file3") - //BAD: Get resource from ResourceLoader (same as application context) without input validation - // Note it is not detected without the generic `resource.getInputStream()` check - public String getFileContent3(@RequestParam(name="fileName") String fileName) { - String content = null; - - try { - // A request such as the following can disclose source code and system configuration - // fileName=/WEB-INF/views/page.jsp - // fileName=/WEB-INF/classes/com/example/package/SampleController.class - // fileName=file:/etc/hosts - Resource resource = resourceLoader.getResource(fileName); // $ hasUnsafeUrlForward (path-inj?) - - char[] buffer = new char[4096]; - StringBuilder out = new StringBuilder(); - - Reader in = new InputStreamReader(resource.getInputStream(), "UTF-8"); - for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) { - out.append(buffer, 0, numRead); - } - content = out.toString(); - } catch (IOException ie) { - ie.printStackTrace(); - } - return content; - } - - @GetMapping("/file3a") - //GOOD: Get resource from ResourceLoader (same as application context) with input path validation - public String getFileContent3a(@RequestParam(name="fileName") String fileName) { - String content = null; - - if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) { - try { - Resource resource = resourceLoader.getResource(fileName); - - char[] buffer = new char[4096]; - StringBuilder out = new StringBuilder(); - - Reader in = new InputStreamReader(resource.getInputStream(), "UTF-8"); - for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) { - out.append(buffer, 0, numRead); - } - content = out.toString(); - } catch (IOException ie) { - ie.printStackTrace(); - } - } - return content; - } -} diff --git a/java/ql/test/query-tests/security/CWE-552/UnsafeResourceGet.java b/java/ql/test/query-tests/security/CWE-552/UnsafeResourceGet.java deleted file mode 100644 index 053887984c6..00000000000 --- a/java/ql/test/query-tests/security/CWE-552/UnsafeResourceGet.java +++ /dev/null @@ -1,270 +0,0 @@ -package com.example; - -import java.io.InputStream; -import java.io.IOException; -import java.io.PrintWriter; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.net.URI; -import java.net.URL; -import java.net.URISyntaxException; - -import javax.servlet.http.HttpServlet; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; -import javax.servlet.ServletOutputStream; -import javax.servlet.ServletException; -import javax.servlet.ServletConfig; -import javax.servlet.ServletContext; - -import io.undertow.server.handlers.resource.FileResourceManager; -import io.undertow.server.handlers.resource.Resource; -import org.jboss.vfs.VFS; -import org.jboss.vfs.VirtualFile; - -public class UnsafeResourceGet extends HttpServlet { - private static final String BASE_PATH = "/pages"; - - @Override - // BAD: getResource constructed from `ServletContext` without input validation - protected void doGet(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - // A sample request /fake.jsp/../WEB-INF/web.xml can load the web.xml file - URL url = sc.getResource(requestUrl); - - InputStream in = url.openStream(); // $ hasUnsafeUrlForward (SSRF) - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - - // GOOD: getResource constructed from `ServletContext` with input validation - protected void doGetGood(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - Path path = Paths.get(requestUrl).normalize().toRealPath(); - if (path.startsWith(BASE_PATH)) { - URL url = sc.getResource(path.toString()); - - InputStream in = url.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - } - - // GOOD: getResource constructed from `ServletContext` with null check only - protected void doGetGood2(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - PrintWriter writer = response.getWriter(); - - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - // A sample request /fake.jsp/../WEB-INF/web.xml can load the web.xml file - URL url = sc.getResource(requestUrl); - if (url == null) { - writer.println("Requested source not found"); - } - } - - // GOOD: getResource constructed from `ServletContext` with `equals` check - protected void doGetGood3(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - ServletContext sc = request.getServletContext(); - - if (requestUrl.equals("/public/crossdomain.xml")) { - URL url = sc.getResource(requestUrl); - - InputStream in = url.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - } - - @Override - // BAD: getResourceAsStream constructed from `ServletContext` without input validation - protected void doPost(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - ServletOutputStream out = response.getOutputStream(); - - // A sample request /fake.jsp/../WEB-INF/web.xml can load the web.xml file - InputStream in = request.getServletContext().getResourceAsStream(requestPath); // $ hasUnsafeUrlForward (path-inj?) - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - - // GOOD: getResourceAsStream constructed from `ServletContext` with input validation - protected void doPostGood(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - ServletOutputStream out = response.getOutputStream(); - - if (!requestPath.contains("..") && requestPath.startsWith("/trusted")) { - InputStream in = request.getServletContext().getResourceAsStream(requestPath); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - } - - @Override - // BAD: getResource constructed from `Class` without input validation - protected void doHead(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - // A sample request /fake.jsp/../../../WEB-INF/web.xml can load the web.xml file - // Note the class is in two levels of subpackages and `Class.getResource` starts from its own directory - URL url = getClass().getResource(requestUrl); - - InputStream in = url.openStream(); // $ hasUnsafeUrlForward (SSRF) - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - - // GOOD: getResource constructed from `Class` with input validation - protected void doHeadGood(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - Path path = Paths.get(requestUrl).normalize().toRealPath(); - if (path.startsWith(BASE_PATH)) { - URL url = getClass().getResource(path.toString()); - - InputStream in = url.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - } - - @Override - // BAD: getResourceAsStream constructed from `ClassLoader` without input validation - protected void doPut(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - ServletOutputStream out = response.getOutputStream(); - - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - // A sample request /fake.jsp/../../../WEB-INF/web.xml can load the web.xml file - // Note the class is in two levels of subpackages and `ClassLoader.getResourceAsStream` starts from its own directory - InputStream in = getClass().getClassLoader().getResourceAsStream(requestPath); // $ hasUnsafeUrlForward (path-inj?) - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - - // GOOD: getResourceAsStream constructed from `ClassLoader` with input validation - protected void doPutGood(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - ServletOutputStream out = response.getOutputStream(); - - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - if (!requestPath.contains("..") && requestPath.startsWith("/trusted")) { - InputStream in = getClass().getClassLoader().getResourceAsStream(requestPath); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - } - - // BAD: getResource constructed from `ClassLoader` without input validation - protected void doPutBad(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestUrl = request.getParameter("requestURL"); - ServletOutputStream out = response.getOutputStream(); - - // A sample request /fake.jsp/../../../WEB-INF/web.xml can load the web.xml file - // Note the class is in two levels of subpackages and `ClassLoader.getResource` starts from its own directory - URL url = getClass().getClassLoader().getResource(requestUrl); - - InputStream in = url.openStream(); // $ hasUnsafeUrlForward (SSRF) - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); - } - } - - // BAD: getResource constructed using Undertow IO without input validation - protected void doPutBad2(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - - try { - FileResourceManager rm = new FileResourceManager(VFS.getChild(new URI("/usr/share")).getPhysicalFile()); - Resource rs = rm.getResource(requestPath); - - VirtualFile overlay = VFS.getChild(new URI("EAP_HOME/modules/")); - // Do file operations - overlay.getChild(rs.getPath()); // $ hasUnsafeUrlForward (path-inj?) - } catch (URISyntaxException ue) { - throw new IOException("Cannot parse the URI"); - } - } - - // GOOD: getResource constructed using Undertow IO with input validation - protected void doPutGood2(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - String requestPath = request.getParameter("requestPath"); - - try { - FileResourceManager rm = new FileResourceManager(VFS.getChild(new URI("/usr/share")).getPhysicalFile()); - Resource rs = rm.getResource(requestPath); - - VirtualFile overlay = VFS.getChild(new URI("EAP_HOME/modules/")); - String path = rs.getPath(); - if (path.startsWith("/trusted_path") && !path.contains("..")) { - // Do file operations - overlay.getChild(path); - } - } catch (URISyntaxException ue) { - throw new IOException("Cannot parse the URI"); - } - } -} diff --git a/java/ql/test/query-tests/security/CWE-552/UnsafeResourceGet2.java b/java/ql/test/query-tests/security/CWE-552/UnsafeResourceGet2.java deleted file mode 100644 index 0043bb06c67..00000000000 --- a/java/ql/test/query-tests/security/CWE-552/UnsafeResourceGet2.java +++ /dev/null @@ -1,58 +0,0 @@ -package com.example; - -import javax.faces.context.FacesContext; -import java.io.BufferedReader; -import java.io.InputStream; -import java.io.InputStreamReader; -import java.io.IOException; -import java.net.URL; -import java.util.Map; - -/** Sample class of JSF managed bean */ -public class UnsafeResourceGet2 { - // BAD: getResourceAsStream constructed from `ExternalContext` without input validation - public String parameterActionBad1() throws IOException { - FacesContext fc = FacesContext.getCurrentInstance(); - Map params = fc.getExternalContext().getRequestParameterMap(); - String loadUrl = params.get("loadUrl"); - - InputStreamReader isr = new InputStreamReader(fc.getExternalContext().getResourceAsStream(loadUrl)); // $ hasUnsafeUrlForward (path-inj?) - BufferedReader br = new BufferedReader(isr); - if(br.ready()) { - //Do Stuff - return "result"; - } - - return "home"; - } - - // BAD: getResource constructed from `ExternalContext` without input validation - public String parameterActionBad2() throws IOException { - FacesContext fc = FacesContext.getCurrentInstance(); - Map params = fc.getExternalContext().getRequestParameterMap(); - String loadUrl = params.get("loadUrl"); - - URL url = fc.getExternalContext().getResource(loadUrl); - - InputStream in = url.openStream(); // $ hasUnsafeUrlForward (SSRF) - //Do Stuff - return "result"; - } - - // GOOD: getResource constructed from `ExternalContext` with input validation - public String parameterActionGood1() throws IOException { - FacesContext fc = FacesContext.getCurrentInstance(); - Map params = fc.getExternalContext().getRequestParameterMap(); - String loadUrl = params.get("loadUrl"); - - if (loadUrl.equals("/public/crossdomain.xml")) { - URL url = fc.getExternalContext().getResource(loadUrl); - - InputStream in = url.openStream(); - //Do Stuff - return "result"; - } - - return "home"; - } -}