diff --git a/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql b/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql index 7d74f8b79ac4..6342a444a646 100644 --- a/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql +++ b/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql @@ -17,6 +17,7 @@ import semmle.code.java.dataflow.SSA import semmle.code.java.dataflow.TaintTracking import DataFlow import PathGraph +private import semmle.code.java.dataflow.ExternalFlow /** * A method that returns the name of an archive entry. @@ -33,34 +34,6 @@ class ArchiveEntryNameMethod extends Method { } } -/** - * An expression that will be treated as the destination of a write. - */ -class WrittenFileName extends Expr { - WrittenFileName() { - // Constructors that write to their first argument. - exists(ConstructorCall ctr | this = ctr.getArgument(0) | - exists(Class c | ctr.getConstructor() = c.getAConstructor() | - c.hasQualifiedName("java.io", "FileOutputStream") or - c.hasQualifiedName("java.io", "RandomAccessFile") or - c.hasQualifiedName("java.io", "FileWriter") - ) - ) - or - // Methods that write to their n'th argument - exists(MethodAccess call, int n | this = call.getArgument(n) | - call.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Files") and - ( - call.getMethod().getName().regexpMatch("new.*Reader|newOutputStream|create.*") and n = 0 - or - call.getMethod().hasName("copy") and n = 1 - or - call.getMethod().hasName("move") and n = 1 - ) - ) - } -} - /** * Holds if `n1` to `n2` is a dataflow step that converts between `String`, * `File`, and `Path`. @@ -151,7 +124,7 @@ class ZipSlipConfiguration extends TaintTracking::Configuration { source.asExpr().(MethodAccess).getMethod() instanceof ArchiveEntryNameMethod } - override predicate isSink(Node sink) { sink.asExpr() instanceof WrittenFileName } + override predicate isSink(Node sink) { sink instanceof FileCreationSink } override predicate isAdditionalTaintStep(Node n1, Node n2) { filePathStep(n1, n2) or fileTaintStep(n1, n2) @@ -173,6 +146,13 @@ class ZipSlipConfiguration extends TaintTracking::Configuration { } } +/** + * A sink that represents a file creation, such as a file write, copy or move operation. + */ +private class FileCreationSink extends DataFlow::Node { + FileCreationSink() { sinkNode(this, "create-file") } +} + from PathNode source, PathNode sink where any(ZipSlipConfiguration c).hasFlowPath(source, sink) select source.getNode(), source, sink, diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql index 6b8ab0851329..aef404aabd13 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -13,6 +13,7 @@ import java import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.FlowSources import DataFlow::PathGraph +private import semmle.code.java.dataflow.ExternalFlow /** * A message interpolator Type that perform Expression Language (EL) evaluations @@ -50,19 +51,6 @@ class SetMessageInterpolatorCall extends MethodAccess { predicate isSafe() { not this.getAnArgument().getType() instanceof ELMessageInterpolatorType } } -/** - * A method named `buildConstraintViolationWithTemplate` declared on a subtype - * of `javax.validation.ConstraintValidatorContext`. - */ -class BuildConstraintViolationWithTemplateMethod extends Method { - BuildConstraintViolationWithTemplateMethod() { - this.getDeclaringType() - .getASupertype*() - .hasQualifiedName("javax.validation", "ConstraintValidatorContext") and - this.hasName("buildConstraintViolationWithTemplate") - } -} - /** * Taint tracking BeanValidationConfiguration describing the flow of data from user input * to the argument of a method that builds constraint error messages. @@ -72,12 +60,15 @@ class BeanValidationConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod() instanceof BuildConstraintViolationWithTemplateMethod and - sink.asExpr() = ma.getArgument(0) - ) - } + override predicate isSink(DataFlow::Node sink) { sink instanceof BeanValidationSink } +} + +/** + * A bean validation sink, such as method `buildConstraintViolationWithTemplate` + * declared on a subtype of `javax.validation.ConstraintValidatorContext`. + */ +private class BeanValidationSink extends DataFlow::Node { + BeanValidationSink() { sinkNode(this, "bean-validation") } } from BeanValidationConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 9c060565f284..0bf7a164826a 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -15,6 +15,7 @@ import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.Encryption import DataFlow::PathGraph +private import semmle.code.java.dataflow.ExternalFlow /** * Holds if `m` always returns `true` ignoring any exceptional flow. @@ -49,14 +50,7 @@ class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration { source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof TrustAllHostnameVerifier } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma, Method m | - (m instanceof SetDefaultHostnameVerifierMethod or m instanceof SetHostnameVerifierMethod) and - ma.getMethod() = m - | - ma.getArgument(0) = sink.asExpr() - ) - } + override predicate isSink(DataFlow::Node sink) { sink instanceof HostnameVerifierSink } override predicate isBarrier(DataFlow::Node barrier) { // ignore nodes that are in functions that intentionally disable hostname verification @@ -84,6 +78,13 @@ class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration { } } +/** + * A sink that sets the `HostnameVerifier` on `HttpsURLConnection`. + */ +private class HostnameVerifierSink extends DataFlow::Node { + HostnameVerifierSink() { sinkNode(this, "set-hostname-verifier") } +} + bindingset[result] private string getAFlagName() { result diff --git a/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql b/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql index 306bf27ab9c0..77980f033f0b 100644 --- a/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql +++ b/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql @@ -13,9 +13,10 @@ import java import semmle.code.java.dataflow.TaintTracking import semmle.code.java.frameworks.Networking import DataFlow::PathGraph +private import semmle.code.java.dataflow.ExternalFlow -class HTTPString extends StringLiteral { - HTTPString() { +class HttpString extends StringLiteral { + HttpString() { // Avoid matching "https" here. exists(string s | this.getRepresentedString() = s | ( @@ -30,26 +31,12 @@ class HTTPString extends StringLiteral { } } -class URLOpenMethod extends Method { - URLOpenMethod() { - this.getDeclaringType().getQualifiedName() = "java.net.URL" and - ( - this.getName() = "openConnection" or - this.getName() = "openStream" - ) - } -} +class HttpStringToUrlOpenMethodFlowConfig extends TaintTracking::Configuration { + HttpStringToUrlOpenMethodFlowConfig() { this = "HttpsUrls::HttpStringToUrlOpenMethodFlowConfig" } -class HTTPStringToURLOpenMethodFlowConfig extends TaintTracking::Configuration { - HTTPStringToURLOpenMethodFlowConfig() { this = "HttpsUrls::HTTPStringToURLOpenMethodFlowConfig" } + override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof HttpString } - override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof HTTPString } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess m | - sink.asExpr() = m.getQualifier() and m.getMethod() instanceof URLOpenMethod - ) - } + override predicate isSink(DataFlow::Node sink) { sink instanceof UrlOpenSink } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { exists(UrlConstructorCall u | @@ -63,10 +50,17 @@ class HTTPStringToURLOpenMethodFlowConfig extends TaintTracking::Configuration { } } -from DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess m, HTTPString s +/** + * A sink that represents a URL opening method call, such as a call to `java.net.URL.openConnection()`. + */ +private class UrlOpenSink extends DataFlow::Node { + UrlOpenSink() { sinkNode(this, "open-url") } +} + +from DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess m, HttpString s where source.getNode().asExpr() = s and sink.getNode().asExpr() = m.getQualifier() and - any(HTTPStringToURLOpenMethodFlowConfig c).hasFlowPath(source, sink) + any(HttpStringToUrlOpenMethodFlowConfig c).hasFlowPath(source, sink) select m, source, sink, "URL may have been constructed with HTTP protocol, using $@.", s, "this source" diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 7c7c2b54dc5a..7073c57ff9c7 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -77,6 +77,8 @@ private module Frameworks { private import semmle.code.java.frameworks.ApacheHttp private import semmle.code.java.frameworks.apache.Lang private import semmle.code.java.frameworks.guava.Guava + private import semmle.code.java.security.ResponseSplitting + private import semmle.code.java.security.XSS private import semmle.code.java.security.LdapInjection } @@ -186,7 +188,33 @@ private predicate sourceModelCsv(string row) { ] } -private predicate sinkModelCsv(string row) { none() } +private predicate sinkModelCsv(string row) { + row = + [ + // Open URL + "java.net;URL;false;openConnection;;;Argument[-1];open-url", + "java.net;URL;false;openStream;;;Argument[-1];open-url", + // Create file + "java.io;FileOutputStream;false;FileOutputStream;;;Argument[0];create-file", + "java.io;RandomAccessFile;false;RandomAccessFile;;;Argument[0];create-file", + "java.io;FileWriter;false;FileWriter;;;Argument[0];create-file", + "java.nio.file;Files;false;move;;;Argument[1];create-file", + "java.nio.file;Files;false;copy;;;Argument[1];create-file", + "java.nio.file;Files;false;newOutputStream;;;Argument[0];create-file", + "java.nio.file;Files;false;newBufferedReader;;;Argument[0];create-file", + "java.nio.file;Files;false;createDirectory;;;Argument[0];create-file", + "java.nio.file;Files;false;createFile;;;Argument[0];create-file", + "java.nio.file;Files;false;createLink;;;Argument[0];create-file", + "java.nio.file;Files;false;createSymbolicLink;;;Argument[0];create-file", + "java.nio.file;Files;false;createTempDirectory;;;Argument[0];create-file", + "java.nio.file;Files;false;createTempFile;;;Argument[0];create-file", + // Bean validation + "javax.validation;ConstraintValidatorContext;true;buildConstraintViolationWithTemplate;;;Argument[0];bean-validation", + // Set hostname + "javax.net.ssl;HttpsURLConnection;true;setDefaultHostnameVerifier;;;Argument[0];set-hostname-verifier", + "javax.net.ssl;HttpsURLConnection;true;setHostnameVerifier;;;Argument[0];set-hostname-verifier" + ] +} private predicate summaryModelCsv(string row) { row = diff --git a/java/ql/src/semmle/code/java/security/ResponseSplitting.qll b/java/ql/src/semmle/code/java/security/ResponseSplitting.qll index d09e6567b152..70af02efc6dc 100644 --- a/java/ql/src/semmle/code/java/security/ResponseSplitting.qll +++ b/java/ql/src/semmle/code/java/security/ResponseSplitting.qll @@ -5,41 +5,32 @@ import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.FlowSources import semmle.code.java.frameworks.Servlets import semmle.code.java.frameworks.JaxWS +private import semmle.code.java.dataflow.ExternalFlow /** A sink that is vulnerable to an HTTP header splitting attack. */ abstract class HeaderSplittingSink extends DataFlow::Node { } -/** A source that introduces data considered safe to use by a header splitting source. */ -abstract class SafeHeaderSplittingSource extends DataFlow::Node { - SafeHeaderSplittingSource() { this instanceof RemoteFlowSource } +private class DefaultHeaderSplittingSink extends HeaderSplittingSink { + DefaultHeaderSplittingSink() { sinkNode(this, "header-splitting") } } -/** A sink that identifies a Java Servlet or JaxWs method that is vulnerable to an HTTP header splitting attack. */ -private class ServletHeaderSplittingSink extends HeaderSplittingSink { - ServletHeaderSplittingSink() { - exists(ResponseAddCookieMethod m, MethodAccess ma | - ma.getMethod() = m and - this.asExpr() = ma.getArgument(0) - ) - or - exists(ResponseAddHeaderMethod m, MethodAccess ma | - ma.getMethod() = m and - this.asExpr() = ma.getAnArgument() - ) - or - exists(ResponseSetHeaderMethod m, MethodAccess ma | - ma.getMethod() = m and - this.asExpr() = ma.getAnArgument() - ) - or - exists(JaxRsResponseBuilder builder, Method m | - m = builder.getAMethod() and m.getName() = "header" - | - this.asExpr() = m.getAReference().getArgument(1) - ) +private class HeaderSplittingSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.servlet.http;HttpServletResponse;false;addCookie;;;Argument[0];header-splitting", + "javax.servlet.http;HttpServletResponse;false;addHeader;;;Argument[0..1];header-splitting", + "javax.servlet.http;HttpServletResponse;false;setHeader;;;Argument[0..1];header-splitting", + "javax.ws.rs.core;ResponseBuilder;false;header;;;Argument[1];header-splitting" + ] } } +/** A source that introduces data considered safe to use by a header splitting source. */ +abstract class SafeHeaderSplittingSource extends DataFlow::Node { + SafeHeaderSplittingSource() { this instanceof RemoteFlowSource } +} + /** A default source that introduces data considered safe to use by a header splitting source. */ private class DefaultSafeHeaderSplittingSource extends SafeHeaderSplittingSource { DefaultSafeHeaderSplittingSource() { diff --git a/java/ql/src/semmle/code/java/security/XSS.qll b/java/ql/src/semmle/code/java/security/XSS.qll index f51bc0bbdf56..486e8053953c 100644 --- a/java/ql/src/semmle/code/java/security/XSS.qll +++ b/java/ql/src/semmle/code/java/security/XSS.qll @@ -29,33 +29,30 @@ class XssAdditionalTaintStep extends Unit { abstract predicate step(DataFlow::Node node1, DataFlow::Node node2); } +/** CSV sink models representing methods susceptible to XSS attacks. */ +private class DefaultXssSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.servlet.http;HttpServletResponse;false;sendError;(int,String);;Argument[1];xss", + "android.webkit;WebView;false;loadData;;;Argument[0];xss", + "android.webkit;WebView;false;loadUrl;;;Argument[0];xss", + "android.webkit;WebView;false;loadDataWithBaseURL;;;Argument[1];xss" + ] + } +} + /** A default sink representing methods susceptible to XSS attacks. */ private class DefaultXssSink extends XssSink { DefaultXssSink() { sinkNode(this, "xss") or - exists(HttpServletResponseSendErrorMethod m, MethodAccess ma | - ma.getMethod() = m and - this.asExpr() = ma.getArgument(1) - ) - or exists(ServletWriterSourceToWritingMethodFlowConfig writer, MethodAccess ma | ma.getMethod() instanceof WritingMethod and writer.hasFlowToExpr(ma.getQualifier()) and this.asExpr() = ma.getArgument(_) ) or - exists(Method m | - m.getDeclaringType() instanceof TypeWebView and - ( - m.getAReference().getArgument(0) = this.asExpr() and m.getName() = "loadData" - or - m.getAReference().getArgument(0) = this.asExpr() and m.getName() = "loadUrl" - or - m.getAReference().getArgument(1) = this.asExpr() and m.getName() = "loadDataWithBaseURL" - ) - ) - or exists(SpringRequestMappingMethod requestMappingMethod, ReturnStmt rs | requestMappingMethod = rs.getEnclosingCallable() and this.asExpr() = rs.getResult() and