From b9ce1aefc09f72f53a3ecc1940fe396a679433be Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Tue, 30 Mar 2021 09:31:38 +0200 Subject: [PATCH 01/16] Java: Convert unsafe URL opening sinks to CSV format --- java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql | 17 ++--------------- .../semmle/code/java/dataflow/ExternalFlow.qll | 9 ++++++++- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql b/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql index 306bf27ab9c0..d1bce930054a 100644 --- a/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql +++ b/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql @@ -13,6 +13,7 @@ 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() { @@ -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" } 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) { sinkNode(sink, "open-url") } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { exists(UrlConstructorCall u | diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 4bb79e84ead9..ece2c7cff749 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -184,7 +184,14 @@ 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" + ] +} private predicate summaryModelCsv(string row) { row = From 9e2832a82d7e3f51c7967c8aafee2d48c01b8b2c Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Tue, 30 Mar 2021 16:24:32 +0200 Subject: [PATCH 02/16] Java: Convert zipslip sinks to CSV format --- java/ql/src/Security/CWE/CWE-022/ZipSlip.ql | 31 ++----------------- .../code/java/dataflow/ExternalFlow.qll | 16 +++++++++- 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql b/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql index 7d74f8b79ac4..80de02d70679 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) { sinkNode(sink, "create-file") } override predicate isAdditionalTaintStep(Node n1, Node n2) { filePathStep(n1, n2) or fileTaintStep(n1, n2) diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index ece2c7cff749..2aa0b1d14e47 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -189,7 +189,21 @@ private predicate sinkModelCsv(string row) { [ // Open URL "java.net;URL;false;openConnection;;;Argument[-1];open-url", - "java.net;URL;false;openStream;;;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" ] } From f329c3fdab6064ffd82b2a3a54de826395c08c9e Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Wed, 31 Mar 2021 10:09:58 +0200 Subject: [PATCH 03/16] Java: Convert insecure bean validation sink to CSV format --- .../CWE/CWE-094/InsecureBeanValidation.ql | 21 ++----------------- .../code/java/dataflow/ExternalFlow.qll | 4 +++- 2 files changed, 5 insertions(+), 20 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql index 6b8ab0851329..e4ee42008a17 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,7 @@ 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) { sinkNode(sink, "bean-validation") } } from BeanValidationConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 2aa0b1d14e47..337f26d82c63 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -203,7 +203,9 @@ private predicate sinkModelCsv(string row) { "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" + "java.nio.file;Files;false;createTempFile;;;Argument[0];create-file", + // Bean validation + "javax.validation;ConstraintValidatorContext;true;buildConstraintViolationWithTemplate;;;Argument[0];bean-validation" ] } From 0b7a6671dd75b8cd1ab54cac2b7d0cbd8604dd2a Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Wed, 31 Mar 2021 10:16:14 +0200 Subject: [PATCH 04/16] Java: Convert header splitting sinks to CSV format --- .../code/java/dataflow/ExternalFlow.qll | 1 + .../code/java/security/ResponseSplitting.qll | 43 +++++++------------ 2 files changed, 17 insertions(+), 27 deletions(-) diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 337f26d82c63..f728c0f9fcaf 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -76,6 +76,7 @@ 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 predicate sourceModelCsv(string 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..040174d50b50 100644 --- a/java/ql/src/semmle/code/java/security/ResponseSplitting.qll +++ b/java/ql/src/semmle/code/java/security/ResponseSplitting.qll @@ -5,41 +5,30 @@ 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 { } +class HeaderSplittingSink extends DataFlow::Node { + HeaderSplittingSink() { sinkNode(this, "header-splitting") } +} + +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;header-splitting", + "javax.servlet.http;HttpServletResponse;false;setHeader;;;Argument;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 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) - ) - } -} - /** A default source that introduces data considered safe to use by a header splitting source. */ private class DefaultSafeHeaderSplittingSource extends SafeHeaderSplittingSource { DefaultSafeHeaderSplittingSource() { From 17fd758df1ec1c600ef8aef4454485b6c2bf5123 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Wed, 31 Mar 2021 10:31:21 +0200 Subject: [PATCH 05/16] Java: Convert XSS sinks to CSV format --- .../code/java/dataflow/ExternalFlow.qll | 1 + java/ql/src/semmle/code/java/security/XSS.qll | 28 ++++++++----------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index f728c0f9fcaf..402bb7fd7476 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -77,6 +77,7 @@ private module Frameworks { 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 predicate sourceModelCsv(string row) { diff --git a/java/ql/src/semmle/code/java/security/XSS.qll b/java/ql/src/semmle/code/java/security/XSS.qll index f51bc0bbdf56..9791eed203b8 100644 --- a/java/ql/src/semmle/code/java/security/XSS.qll +++ b/java/ql/src/semmle/code/java/security/XSS.qll @@ -29,33 +29,29 @@ class XssAdditionalTaintStep extends Unit { abstract predicate step(DataFlow::Node node1, DataFlow::Node node2); } +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 From e544faed6de23be9821771087b22c7ffb87a5005 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Thu, 1 Apr 2021 09:11:47 +0200 Subject: [PATCH 06/16] Java: Convert unsafe hostname verification sinks to CSV format --- .../Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 10 ++-------- java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll | 5 ++++- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 9c060565f284..058a1f9e1695 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) { sinkNode(sink, "set-hostname") } override predicate isBarrier(DataFlow::Node barrier) { // ignore nodes that are in functions that intentionally disable hostname verification diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 402bb7fd7476..ba329d99f21b 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -207,7 +207,10 @@ private predicate sinkModelCsv(string row) { "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" + "javax.validation;ConstraintValidatorContext;true;buildConstraintViolationWithTemplate;;;Argument[0];bean-validation", + // Set hostname + "javax.net.ssl;HttpsURLConnection;true;setDefaultHostnameVerifier;;;Argument[0];set-hostname", + "javax.net.ssl;HttpsURLConnection;true;setHostnameVerifier;;;Argument[0];set-hostname" ] } From 3e53484bb3b942c58f2bf1b90301a9779ebc34aa Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Thu, 1 Apr 2021 09:21:49 +0200 Subject: [PATCH 07/16] Java: Convert Google HTTP client API parseAs sink to CSV format --- .../code/java/dataflow/ExternalFlow.qll | 1 + .../frameworks/google/GoogleHttpClientApi.qll | 22 +++++++------------ 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index ba329d99f21b..154ec98f2ad8 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -76,6 +76,7 @@ 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.frameworks.google.GoogleHttpClientApi private import semmle.code.java.security.ResponseSplitting private import semmle.code.java.security.XSS } diff --git a/java/ql/src/semmle/code/java/frameworks/google/GoogleHttpClientApi.qll b/java/ql/src/semmle/code/java/frameworks/google/GoogleHttpClientApi.qll index ccc446892f12..07e30711b449 100644 --- a/java/ql/src/semmle/code/java/frameworks/google/GoogleHttpClientApi.qll +++ b/java/ql/src/semmle/code/java/frameworks/google/GoogleHttpClientApi.qll @@ -2,14 +2,7 @@ import java import semmle.code.java.Serializability import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.DataFlow5 - -/** The method `parseAs` in `com.google.api.client.http.HttpResponse`. */ -private class ParseAsMethod extends Method { - ParseAsMethod() { - this.getDeclaringType().hasQualifiedName("com.google.api.client.http", "HttpResponse") and - this.hasName("parseAs") - } -} +private import semmle.code.java.dataflow.ExternalFlow private class TypeLiteralToParseAsFlowConfiguration extends DataFlow5::Configuration { TypeLiteralToParseAsFlowConfiguration() { @@ -18,16 +11,17 @@ private class TypeLiteralToParseAsFlowConfiguration extends DataFlow5::Configura override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof TypeLiteral } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getAnArgument() = sink.asExpr() and - ma.getMethod() instanceof ParseAsMethod - ) - } + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "google-parse-as") } TypeLiteral getSourceWithFlowToParseAs() { hasFlow(DataFlow::exprNode(result), _) } } +private class ParseAsSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = ["com.google.api.client.http;HttpResponse;false;parseAs;;;Argument;google-parse-as"] + } +} + /** A field that is deserialized by `HttpResponse.parseAs`. */ class HttpResponseParseAsDeserializableField extends DeserializableField { HttpResponseParseAsDeserializableField() { From 87d42b02c0c49ff717c941d6ea5290b6ca3c40df Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Wed, 31 Mar 2021 10:21:12 +0200 Subject: [PATCH 08/16] Java: Convert other sinks --- .../CWE/CWE-209/StackTraceExposure.ql | 11 +- .../Security/CWE/CWE-036/OpenStream.ql | 15 +- .../Security/CWE/CWE-074/XsltInjectionLib.qll | 41 +++-- .../Security/CWE/CWE-094/JexlInjectionLib.qll | 139 +++++++-------- .../Security/CWE/CWE-094/MvelInjectionLib.qll | 56 +++--- .../CWE/CWE-326/InsufficientKeySize.ql | 23 +-- .../Security/CWE/CWE-327/SslLib.qll | 20 ++- .../Security/CWE/CWE-346/UnvalidatedCors.ql | 29 ++- .../Security/CWE/CWE-522/InsecureLdapAuth.ql | 26 +-- .../Security/CWE/CWE-643/XPathInjection.ql | 23 +-- .../Security/CWE/CWE-652/XQueryInjection.ql | 7 +- .../CWE/CWE-652/XQueryInjectionLib.qll | 12 ++ .../Security/CWE/CWE-917/OgnlInjectionLib.qll | 26 ++- .../Security/CWE/CWE-918/RequestForgery.ql | 5 +- .../Security/CWE/CWE-918/RequestForgery.qll | 168 ++++-------------- .../code/java/dataflow/ExternalFlow.qll | 23 ++- .../java/security/UnsafeDeserialization.qll | 32 ++-- 17 files changed, 302 insertions(+), 354 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql b/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql index c9c1e2917c0e..1f49d5cb06ac 100644 --- a/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql +++ b/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql @@ -16,6 +16,7 @@ import java import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.TaintTracking import semmle.code.java.security.XSS +private import semmle.code.java.dataflow.ExternalFlow /** * One of the `printStackTrace()` overloads on `Throwable`. @@ -37,10 +38,12 @@ class ServletWriterSourceToPrintStackTraceMethodFlowConfig extends TaintTracking override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof ServletWriterSource } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - sink.asExpr() = ma.getAnArgument() and ma.getMethod() instanceof PrintStackTraceMethod - ) + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "print-stack-trace") } +} + +private class PrintStackTraceSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = ["java.lang;Throwable;true;printStackTrace;;;Argument;print-stack-trace"] } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-036/OpenStream.ql b/java/ql/src/experimental/Security/CWE/CWE-036/OpenStream.ql index 871d6bb4737c..6f75b3fc8ae5 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-036/OpenStream.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-036/OpenStream.ql @@ -15,6 +15,7 @@ import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.ExternalFlow import DataFlow::PathGraph +private import semmle.code.java.dataflow.ExternalFlow class URLConstructor extends ClassInstanceExpr { URLConstructor() { this.getConstructor().getDeclaringType() instanceof TypeUrl } @@ -39,13 +40,7 @@ class RemoteURLToOpenStreamFlowConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess m | - sink.asExpr() = m.getQualifier() and m.getMethod() instanceof URLOpenStreamMethod - ) - or - sinkNode(sink, "url-open-stream") - } + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "url-open-stream") } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { exists(URLConstructor u | @@ -55,6 +50,12 @@ class RemoteURLToOpenStreamFlowConfig extends TaintTracking::Configuration { } } +private class URLOpenStreamSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = ["java.net;URL;false;openStream;;;Argument[-1];url-open-stream"] + } +} + from DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess call where sink.getNode().asExpr() = call.getQualifier() and diff --git a/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjectionLib.qll index 4ba0eb6d0b11..bc0c8352a20f 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjectionLib.qll @@ -2,6 +2,7 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.XmlParsers import DataFlow +private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration for unvalidated user input that is used in XSLT transformation. @@ -103,15 +104,20 @@ class TypeXsltPackage extends Class { /** A data flow sink for unvalidated user input that is used in XSLT transformation. */ class XsltInjectionSink extends DataFlow::ExprNode { - XsltInjectionSink() { - exists(MethodAccess ma, Method m | m = ma.getMethod() and ma.getQualifier() = this.getExpr() | - ma instanceof TransformerTransform or - m instanceof XsltTransformerTransformMethod or - m instanceof Xslt30TransformerTransformMethod or - m instanceof Xslt30TransformerApplyTemplatesMethod or - m instanceof Xslt30TransformerCallFunctionMethod or - m instanceof Xslt30TransformerCallTemplateMethod - ) + XsltInjectionSink() { sinkNode(this, "xslt") } +} + +private class XsltInjectionSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "net.sf.saxon.s9api;XsltTransformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;applyTemplates;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;callFunction;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;callTemplate;;;Argument[-1];xslt", + "javax.xml.transform;Transformer;false;transform;;;Argument[-1];xslt" + ] } } @@ -186,16 +192,21 @@ private class TransformerFactoryWithSecureProcessingFeatureFlowConfig extends Da ) } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - sink.asExpr() = ma.getQualifier() and - ma.getMethod().getDeclaringType() instanceof TransformerFactory - ) - } + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "xslt-transformer") } override int fieldFlowBranchLimit() { result = 0 } } +private class TransformerFactorySinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.xml.transform;TransformerFactory;false;;;;Argument[-1];xslt-transformer", + "javax.xml.transform.sax;SAXTransformerFactory;false;;;;Argument[-1];xslt-transformer" + ] + } +} + /** A `ParserConfig` specific to `TransformerFactory`. */ private class TransformerFactoryFeatureConfig extends ParserConfig { TransformerFactoryFeatureConfig() { diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/JexlInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-094/JexlInjectionLib.qll index 561d7e46ae90..8fcde750e54c 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/JexlInjectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-094/JexlInjectionLib.qll @@ -1,6 +1,7 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking +private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration for unsafe user input @@ -21,7 +22,7 @@ class JexlInjectionConfig extends TaintTracking::Configuration { } /** - * A sink for Expresssion Language injection vulnerabilities via Jexl, + * A sink for Expression Language injection vulnerabilities via Jexl, * i.e. method calls that run evaluation of a JEXL expression. * * Creating a `Callable` from a tainted JEXL expression or script is considered as a sink @@ -30,18 +31,41 @@ class JexlInjectionConfig extends TaintTracking::Configuration { * maybe stored in an object field and then reached by a different flow. */ private class JexlEvaluationSink extends DataFlow::ExprNode { - JexlEvaluationSink() { - exists(MethodAccess ma, Method m, Expr taintFrom | - ma.getMethod() = m and taintFrom = this.asExpr() - | - m instanceof DirectJexlEvaluationMethod and ma.getQualifier() = taintFrom - or - m instanceof CreateJexlCallableMethod and ma.getQualifier() = taintFrom - or - m instanceof JexlEngineGetSetPropertyMethod and - taintFrom.getType() instanceof TypeString and - ma.getAnArgument() = taintFrom - ) + JexlEvaluationSink() { sinkNode(this, "jexl") } +} + +private class JexlEvaluationSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + // Direct JEXL evaluation + "org.apache.commons.jexl2;Expression;false;evaluate;;;Argument[-1];jexl", + "org.apache.commons.jexl3;JexlExpression;false;evaluate;;;Argument[-1];jexl", + "org.apache.commons.jexl2;Script;false;execute;;;Argument[-1];jexl", + "org.apache.commons.jexl3;JexlScript;false;execute;;;Argument[-1];jexl", + "org.apache.commons.jexl2;JxltEngine$Expression;false;evaluate;;;Argument[-1];jexl", + "org.apache.commons.jexl3;JxltEngine$Expression;false;evaluate;;;Argument[-1];jexl", + "org.apache.commons.jexl2;JxltEngine$Expression;false;prepare;;;Argument[-1];jexl", + "org.apache.commons.jexl3;JxltEngine$Expression;false;prepare;;;Argument[-1];jexl", + "org.apache.commons.jexl2;JxltEngine$Template;false;evaluate;;;Argument[-1];jexl", + "org.apache.commons.jexl3;JxltEngine$Template;false;evaluate;;;Argument[-1];jexl", + "org.apache.commons.jexl2;UnifiedJEXL$Expression;false;evaluate;;;Argument[-1];jexl", + "org.apache.commons.jexl3;UnifiedJEXL$Expression;false;evaluate;;;Argument[-1];jexl", + "org.apache.commons.jexl2;UnifiedJEXL$Expression;false;prepare;;;Argument[-1];jexl", + "org.apache.commons.jexl3;UnifiedJEXL$Expression;false;prepare;;;Argument[-1];jexl", + "org.apache.commons.jexl2;UnifiedJEXL$Template;false;evaluate;;;Argument[-1];jexl", + "org.apache.commons.jexl3;UnifiedJEXL$Template;false;evaluate;;;Argument[-1];jexl", + // JEXL callable + "org.apache.commons.jexl2;Expression;false;callable;;;Argument[-1];jexl", + "org.apache.commons.jexl3;JexlExpression;false;callable;;;Argument[-1];jexl", + "org.apache.commons.jexl2;Script;false;callable;;;Argument[-1];jexl", + "org.apache.commons.jexl3;JexlScript;false;callable;;;Argument[-1];jexl", + // Methods in the `JexlEngine` class that gets or sets a property with a JEXL expression. + "org.apache.commons.jexl2;JexlEngine;false;getProperty;;;Argument[1..2];jexl", + "org.apache.commons.jexl3;JexlEngine;false;getProperty;;;Argument[1..2];jexl", + "org.apache.commons.jexl2;JexlEngine;false;setProperty;;;Argument[1];jexl", + "org.apache.commons.jexl3;JexlEngine;false;setProperty;;;Argument[1];jexl" + ] } } @@ -98,22 +122,36 @@ private class SandboxedJexlFlowConfig extends DataFlow2::Configuration { override predicate isSource(DataFlow::Node node) { node instanceof SandboxedJexlSource } - override predicate isSink(DataFlow::Node node) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - ( - m instanceof CreateJexlScriptMethod or - m instanceof CreateJexlExpressionMethod or - m instanceof CreateJexlTemplateMethod - ) and - ma.getQualifier() = node.asExpr() - ) - } + override predicate isSink(DataFlow::Node node) { sinkNode(node, "sandboxed-jexl") } override predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) { createsJexlEngine(fromNode, toNode) } } +private class SandboxedJexlEvaluationSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + // CreateJexlScriptMethod + "org.apache.commons.jexl2;JexlEngine;false;createScript;;;Argument[-1];sandboxed-jexl", + "org.apache.commons.jexl3;JexlEngine;false;createScript;;;Argument[-1];sandboxed-jexl", + // CreateJexlExpressionMethod + "org.apache.commons.jexl2;UnifiedJEXL;false;parse;;;Argument[-1];sandboxed-jexl", + "org.apache.commons.jexl3;UnifiedJEXL;false;parse;;;Argument[-1];sandboxed-jexl", + "org.apache.commons.jexl2;JxltEngine;false;createExpression;;;Argument[-1];sandboxed-jexl", + "org.apache.commons.jexl3;JxltEngine;false;createExpression;;;Argument[-1];sandboxed-jexl", + "org.apache.commons.jexl2;JexlEngine;false;createExpression;;;Argument[-1];sandboxed-jexl", + "org.apache.commons.jexl3;JexlEngine;false;createExpression;;;Argument[-1];sandboxed-jexl", + // CreateJexlTemplateMethod + "org.apache.commons.jexl2;JxltEngine;false;createTemplate;;;Argument[-1];sandboxed-jexl", + "org.apache.commons.jexl3;JxltEngine;false;createTemplate;;;Argument[-1];sandboxed-jexl", + "org.apache.commons.jexl2;UnifiedJEXL;false;createTemplate;;;Argument[-1];sandboxed-jexl", + "org.apache.commons.jexl3;UnifiedJEXL;false;createTemplate;;;Argument[-1];sandboxed-jexl" + ] + } +} + /** * Defines a data flow source for JEXL engines configured with a sandbox. */ @@ -164,35 +202,6 @@ private predicate returnsDataFromBean(DataFlow::Node fromNode, DataFlow::Node to ) } -/** - * A methods in the `JexlEngine` class that gets or sets a property with a JEXL expression. - */ -private class JexlEngineGetSetPropertyMethod extends Method { - JexlEngineGetSetPropertyMethod() { - getDeclaringType() instanceof JexlEngine and - hasName(["getProperty", "setProperty"]) - } -} - -/** - * A method that triggers direct evaluation of JEXL expressions. - */ -private class DirectJexlEvaluationMethod extends Method { - DirectJexlEvaluationMethod() { - getDeclaringType() instanceof JexlExpression and hasName("evaluate") - or - getDeclaringType() instanceof JexlScript and hasName("execute") - or - getDeclaringType() instanceof JxltEngineExpression and hasName(["evaluate", "prepare"]) - or - getDeclaringType() instanceof JxltEngineTemplate and hasName("evaluate") - or - getDeclaringType() instanceof UnifiedJexlExpression and hasName(["evaluate", "prepare"]) - or - getDeclaringType() instanceof UnifiedJexlTemplate and hasName("evaluate") - } -} - /** * A method that creates a JEXL script. */ @@ -200,16 +209,6 @@ private class CreateJexlScriptMethod extends Method { CreateJexlScriptMethod() { getDeclaringType() instanceof JexlEngine and hasName("createScript") } } -/** - * A method that creates a `Callable` for a JEXL expression or script. - */ -private class CreateJexlCallableMethod extends Method { - CreateJexlCallableMethod() { - (getDeclaringType() instanceof JexlExpression or getDeclaringType() instanceof JexlScript) and - hasName("callable") - } -} - /** * A method that creates a JEXL template. */ @@ -267,22 +266,6 @@ private class JexlUberspect extends Interface { } } -private class JxltEngineExpression extends NestedType { - JxltEngineExpression() { getEnclosingType() instanceof JxltEngine and hasName("Expression") } -} - -private class JxltEngineTemplate extends NestedType { - JxltEngineTemplate() { getEnclosingType() instanceof JxltEngine and hasName("Template") } -} - -private class UnifiedJexlExpression extends NestedType { - UnifiedJexlExpression() { getEnclosingType() instanceof UnifiedJexl and hasName("Expression") } -} - -private class UnifiedJexlTemplate extends NestedType { - UnifiedJexlTemplate() { getEnclosingType() instanceof UnifiedJexl and hasName("Template") } -} - private class Reader extends RefType { Reader() { hasQualifiedName("java.io", "Reader") } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll index a6cf891330f0..dec268a2a4bc 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll @@ -1,6 +1,7 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking +private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration for unsafe user input @@ -30,36 +31,31 @@ class MvelInjectionConfig extends TaintTracking::Configuration { * i.e. methods that run evaluation of a MVEL expression. */ class MvelEvaluationSink extends DataFlow::ExprNode { - MvelEvaluationSink() { - exists(StaticMethodAccess ma, Method m | m = ma.getMethod() | - ( - m instanceof MvelEvalMethod or - m instanceof TemplateRuntimeEvaluationMethod - ) and - ma.getArgument(0) = asExpr() - ) - or - exists(MethodAccess ma, Method m | m = ma.getMethod() | - m instanceof MvelScriptEngineEvaluationMethod and - ma.getArgument(0) = asExpr() - ) - or - exists(MethodAccess ma, Method m | m = ma.getMethod() | - ( - m instanceof ExecutableStatementEvaluationMethod or - m instanceof CompiledExpressionEvaluationMethod or - m instanceof CompiledAccExpressionEvaluationMethod or - m instanceof AccessorEvaluationMethod or - m instanceof CompiledScriptEvaluationMethod or - m instanceof MvelCompiledScriptEvaluationMethod - ) and - ma.getQualifier() = asExpr() - ) - or - exists(StaticMethodAccess ma, Method m | m = ma.getMethod() | - m instanceof MvelRuntimeEvaluationMethod and - ma.getArgument(1) = asExpr() - ) + MvelEvaluationSink() { sinkNode(this, "mvel") } +} + +private class MvelEvaluationSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "org.mvel2.jsr223;MvelScriptEngine;false;evaluate;;;Argument[0];mvel", + "org.mvel2.jsr223;MvelScriptEngine;false;eval;;;Argument[0];mvel", + "org.mvel2.compiler;ExecutableStatement;false;getValue;;;Argument[-1];mvel", + "org.mvel2.compiler;CompiledExpression;false;getDirectValue;;;Argument[-1];mvel", + "org.mvel2.compiler;CompiledAccExpression;false;getValue;;;Argument[-1];mvel", + "org.mvel2.compiler;Accessor;false;getValue;;;Argument[-1];mvel", + "javax.script;CompiledScript;false;eval;;;Argument[-1];mvel", + "org.mvel2.jsr223;MvelCompiledScript;false;eval;;;Argument[-1];mvel", + "org.mvel2;MVEL;false;eval;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeExpression;;;Argument[0];mvel", + "org.mvel2;MVEL;false;evalToBoolean;;;Argument[0];mvel", + "org.mvel2;MVEL;false;evalToString;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeAllExpression;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeSetExpression;;;Argument[0];mvel", + "org.mvel2.templates;TemplateRuntime;false;eval;;;Argument[0];mvel", + "org.mvel2.templates;TemplateRuntime;false;execute;;;Argument[0];mvel", + "org.mvel2;MVELRuntime;false;execute;;;Argument[1];mvel" + ] } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-326/InsufficientKeySize.ql b/java/ql/src/experimental/Security/CWE/CWE-326/InsufficientKeySize.ql index 155d05abfae3..ca86f42c561a 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-326/InsufficientKeySize.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-326/InsufficientKeySize.ql @@ -12,6 +12,7 @@ import java import semmle.code.java.security.Encryption import semmle.code.java.dataflow.TaintTracking +private import semmle.code.java.dataflow.ExternalFlow /** The Java class `java.security.spec.ECGenParameterSpec`. */ class ECGenParameterSpec extends RefType { @@ -55,11 +56,12 @@ class KeyGeneratorInitConfiguration extends TaintTracking::Configuration { exists(JavaxCryptoKeyGenerator jcg | jcg = source.asExpr()) } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod() instanceof KeyGeneratorInitMethod and - sink.asExpr() = ma.getQualifier() - ) + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "keygen") } +} + +private class KeyGeneratorInitSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = ["javax.crypto;KeyGenerator;false;init;;;Argument[-1];keygen"] } } @@ -71,11 +73,12 @@ class KeyPairGeneratorInitConfiguration extends TaintTracking::Configuration { exists(JavaSecurityKeyPairGenerator jkg | jkg = source.asExpr()) } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod() instanceof KeyPairGeneratorInitMethod and - sink.asExpr() = ma.getQualifier() - ) + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "keypairgen") } +} + +private class KeyPairGeneratorInitSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = ["java.security;KeyPairGenerator;false;initialize;;;Argument[-1];keypairgen"] } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll b/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll index bfa2530b07e7..3845953f0aab 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll @@ -3,6 +3,7 @@ import semmle.code.java.security.Encryption import semmle.code.java.dataflow.TaintTracking import DataFlow import PathGraph +private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration for unsafe SSL and TLS versions. @@ -12,11 +13,20 @@ class UnsafeTlsVersionConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof UnsafeTlsVersion } - override predicate isSink(DataFlow::Node sink) { - sink instanceof SslContextGetInstanceSink or - sink instanceof CreateSslParametersSink or - sink instanceof SslParametersSetProtocolsSink or - sink instanceof SetEnabledProtocolsSink + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "ssl") } +} + +private class UnsafeTlsVersionSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.net.ssl;SSLContext;false;getInstance;;;Argument[0];ssl", + "javax.net.ssl;SSLParameters;false;SSLParameters;;;Argument[1];ssl", + "javax.net.ssl;SSLParameters;false;setProtocols;;;Argument[0];ssl", + "javax.net.ssl;SSLSocket;false;setEnabledProtocols;;;Argument[0];ssl", + "javax.net.ssl;SSLServerSocket;false;setEnabledProtocols;;;Argument[0];ssl", + "javax.net.ssl;SSLEngine;false;setEnabledProtocols;;;Argument[0];ssl" + ] } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.ql b/java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.ql index c5a6c36d6a61..2fe6569318fc 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.ql @@ -15,6 +15,7 @@ import semmle.code.java.frameworks.Servlets import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.TaintTracking2 import DataFlow::PathGraph +private import semmle.code.java.dataflow.ExternalFlow /** * Holds if `header` sets `Access-Control-Allow-Credentials` to `true`. This ensures fair chances of exploitability. @@ -29,33 +30,29 @@ private predicate setsAllowCredentials(MethodAccess header) { header.getArgument(1).(CompileTimeConstantExpr).getStringValue().toLowerCase() = "true" } -private class CorsProbableCheckAccess extends MethodAccess { - CorsProbableCheckAccess() { - getMethod().hasName("contains") and - getMethod().getDeclaringType().getASourceSupertype*() instanceof CollectionType - or - getMethod().hasName("containsKey") and - getMethod().getDeclaringType().getASourceSupertype*() instanceof MapType - or - getMethod().hasName("equals") and - getQualifier().getType() instanceof TypeString - } -} - private Expr getAccessControlAllowOriginHeaderName() { result.(CompileTimeConstantExpr).getStringValue().toLowerCase() = "access-control-allow-origin" } /** - * This taintflow2 configuration checks if there is a flow from source node towards CorsProbableCheckAccess methods. + * This taintflow2 configuration checks if there is a flow from source node towards probably CORS checking methods. */ class CorsSourceReachesCheckConfig extends TaintTracking2::Configuration { CorsSourceReachesCheckConfig() { this = "CorsOriginConfig" } override predicate isSource(DataFlow::Node source) { any(CorsOriginConfig c).hasFlow(source, _) } - override predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(CorsProbableCheckAccess check).getAnArgument() + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "cors") } +} + +private class CorsProbableCheckAccessSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "java.util;Collection;true;contains;;;Argument;cors", + "java.util;Map;true;containsKey;;;Argument;cors", + "java.lang;String;true;equals;;;Argument;cors" + ] } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql index 4ce2b8b7134c..3c445934ad33 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -16,6 +16,7 @@ import semmle.code.java.frameworks.Jndi import semmle.code.java.frameworks.Networking import semmle.code.java.dataflow.TaintTracking import DataFlow::PathGraph +private import semmle.code.java.dataflow.ExternalFlow /** * Insecure (non-SSL, non-private) LDAP URL string literal. @@ -145,12 +146,7 @@ class InsecureUrlFlowConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl } /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { - exists(ConstructorCall cc | - cc.getConstructedType().getASupertype*() instanceof TypeDirContext and - sink.asExpr() = cc.getArgument(0) - ) - } + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "ldap") } /** Method call of `env.put()`. */ override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { @@ -176,11 +172,12 @@ class BasicAuthFlowConfig extends DataFlow::Configuration { } /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { - exists(ConstructorCall cc | - cc.getConstructedType().getASupertype*() instanceof TypeDirContext and - sink.asExpr() = cc.getArgument(0) - ) + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "ldap") } +} + +private class LdapSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = ["javax.naming.directory;DirContext;true;.ctor;;;Argument[0];ldap"] } } @@ -198,12 +195,7 @@ class SSLFlowConfig extends DataFlow::Configuration { } /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { - exists(ConstructorCall cc | - cc.getConstructedType().getASupertype*() instanceof TypeDirContext and - sink.asExpr() = cc.getArgument(0) - ) - } + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "ldap") } } from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureUrlFlowConfig config diff --git a/java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.ql index e5a29df46d59..598f42b0c703 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.ql @@ -15,6 +15,7 @@ import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking import semmle.code.java.security.XmlParsers import DataFlow::PathGraph +private import semmle.code.java.dataflow.ExternalFlow class XPathInjectionConfiguration extends TaintTracking::Configuration { XPathInjectionConfiguration() { this = "XPathInjection" } @@ -25,16 +26,18 @@ class XPathInjectionConfiguration extends TaintTracking::Configuration { } class XPathInjectionSink extends DataFlow::ExprNode { - XPathInjectionSink() { - exists(Method m, MethodAccess ma | ma.getMethod() = m | - m.getDeclaringType().hasQualifiedName("javax.xml.xpath", "XPath") and - (m.hasName("evaluate") or m.hasName("compile")) and - ma.getArgument(0) = this.getExpr() - or - m.getDeclaringType().hasQualifiedName("org.dom4j", "Node") and - (m.hasName("selectNodes") or m.hasName("selectSingleNode")) and - ma.getArgument(0) = this.getExpr() - ) + XPathInjectionSink() { sinkNode(this, "xpath") } +} + +private class XPathInjectionSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.xml.xpath;XPath;false;compile;;;Argument[0];xpath", + "javax.xml.xpath;XPath;false;evaluate;;;Argument[0];xpath", + "org.dom4j;Node;false;selectNodes;;;Argument[0];xpath", + "org.dom4j;Node;false;selectSingleNode;;;Argument[0];xpath" + ] } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjection.ql index 0bb85272f085..3b8e43c5d7d2 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjection.ql @@ -14,6 +14,7 @@ import java import semmle.code.java.dataflow.FlowSources import XQueryInjectionLib import DataFlow::PathGraph +private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration tracing flow from remote sources, through an XQuery parser, to its eventual execution. @@ -23,11 +24,7 @@ class XQueryInjectionConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(XQueryPreparedExecuteCall xpec).getPreparedExpression() or - sink.asExpr() = any(XQueryExecuteCall xec).getExecuteQueryArgument() or - sink.asExpr() = any(XQueryExecuteCommandCall xecc).getExecuteCommandArgument() - } + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "xquery") } /** * Holds if taint from the input `pred` to a `prepareExpression` call flows to the returned prepared expression `succ`. diff --git a/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjectionLib.qll index 2a4019f2c9a9..13452e4e55dc 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjectionLib.qll @@ -1,4 +1,5 @@ import java +private import semmle.code.java.dataflow.ExternalFlow /** A call to `XQConnection.prepareExpression`. */ class XQueryParserCall extends MethodAccess { @@ -66,3 +67,14 @@ class XQueryExecuteCommandCall extends MethodAccess { /** Return this execute command argument. */ Expr getExecuteCommandArgument() { result = this.getArgument(0) } } + +private class XQuerySinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.xml.xquery;XQPreparedExpression;true;executeQuery;;;Argument[-1];xquery", + "javax.xml.xquery;XQExpression;true;executeQuery;;;Argument[0];xquery", + "javax.xml.xquery;XQExpression;true;executeCommand;;;Argument[0];xquery" + ] + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll index 569e18a29c38..002e06aaafdc 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll @@ -2,6 +2,7 @@ import java import semmle.code.java.dataflow.FlowSources import DataFlow import DataFlow::PathGraph +private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration for unvalidated user input that is used in OGNL EL evaluation. @@ -82,12 +83,25 @@ predicate ognlInjectionSinkMethod(Method m, int index) { /** A data flow sink for unvalidated user input that is used in OGNL EL evaluation. */ class OgnlInjectionSink extends DataFlow::ExprNode { - OgnlInjectionSink() { - exists(MethodAccess ma, Method m, int index | - ma.getMethod() = m and - (ma.getArgument(index) = this.getExpr() or ma.getQualifier() = this.getExpr()) and - ognlInjectionSinkMethod(m, index) - ) + OgnlInjectionSink() { sinkNode(this, "ognl") } +} + +private class OgnlInjectionSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "org.apache.commons.ognl;Ognl;false;setValue;;;Argument[-1..0];ognl", + "org.apache.commons.ognl;Ognl;false;getValue;;;Argument[-1..0];ognl", + "ognl;Ognl;false;setValue;;;Argument[-1..0];ognl", + "ognl;Ognl;false;getValue;;;Argument[-1..0];ognl", + "org.apache.commons.ognl;Node;true;setValue;;;Argument[-1..0];ognl", + "org.apache.commons.ognl;Node;true;getValue;;;Argument[-1..0];ognl", + "ognl;Node;true;setValue;;;Argument[-1..0];ognl", + "ognl;Node;true;getValue;;;Argument[-1..0];ognl", + "com.opensymphony.xwork2.ognl;OgnlUtil;false;setValue;;;Argument[-1..0];ognl", + "com.opensymphony.xwork2.ognl;OgnlUtil;false;getValue;;;Argument[-1..0];ognl", + "com.opensymphony.xwork2.ognl;OgnlUtil;false;callMethod;;;Argument[-1..0];ognl" + ] } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.ql b/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.ql index c3bf787881fa..6b0333da4709 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.ql @@ -14,13 +14,16 @@ import java import semmle.code.java.dataflow.FlowSources import RequestForgery import DataFlow::PathGraph +private import semmle.code.java.dataflow.ExternalFlow class RequestForgeryConfiguration extends TaintTracking::Configuration { RequestForgeryConfiguration() { this = "Server Side Request Forgery" } override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgerySink } + override predicate isSink(DataFlow::Node sink) { + sink instanceof RequestForgerySink or sinkNode(sink, "request-forgery-sink") + } override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { requestForgeryStep(pred, succ) diff --git a/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.qll b/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.qll index 3fc52ddca766..fa784baa7f23 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.qll @@ -5,6 +5,7 @@ import semmle.code.java.frameworks.spring.Spring import semmle.code.java.frameworks.JaxWS import semmle.code.java.frameworks.javase.Http import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.dataflow.ExternalFlow predicate requestForgeryStep(DataFlow::Node pred, DataFlow::Node succ) { // propagate to a URI when its host is assigned to @@ -38,119 +39,42 @@ predicate requestForgeryStep(DataFlow::Node pred, DataFlow::Node succ) { ) } -/** A data flow sink for request forgery vulnerabilities. */ -abstract class RequestForgerySink extends DataFlow::Node { } - -/** - * An argument to an url `openConnection` or `openStream` call - * taken as a sink for request forgery vulnerabilities. - */ -private class UrlOpen extends RequestForgerySink { - UrlOpen() { - exists(MethodAccess ma | - ma.getMethod() instanceof UrlOpenConnectionMethod or - ma.getMethod() instanceof UrlOpenStreamMethod - | - this.asExpr() = ma.getQualifier() - ) - } -} - -/** - * An argument to an Apache `setURI` call taken as a - * sink for request forgery vulnerabilities. - */ -private class ApacheSetUri extends RequestForgerySink { - ApacheSetUri() { - exists(MethodAccess ma | - ma.getReceiverType() instanceof ApacheHttpRequest and - ma.getMethod().hasName("setURI") - | - this.asExpr() = ma.getArgument(0) - ) - } -} - -/** - * An argument to any Apache Request Instantiation call taken as a - * sink for request forgery vulnerabilities. - */ -private class ApacheHttpRequestInstantiation extends RequestForgerySink { - ApacheHttpRequestInstantiation() { - exists(ClassInstanceExpr c | c.getConstructedType() instanceof ApacheHttpRequest | - this.asExpr() = c.getArgument(0) - ) - } -} - -/** - * An argument to a Apache RequestBuilder method call taken as a - * sink for request forgery vulnerabilities. - */ -private class ApacheHttpRequestBuilderArgument extends RequestForgerySink { - ApacheHttpRequestBuilderArgument() { - exists(MethodAccess ma | - ma.getReceiverType() instanceof TypeApacheHttpRequestBuilder and - ma.getMethod().hasName(["setURI", "get", "post", "put", "optons", "head", "delete"]) - | - this.asExpr() = ma.getArgument(0) - ) - } -} - -/** - * An argument to any Java.net.http.request Instantiation call taken as a - * sink for request forgery vulnerabilities. - */ -private class HttpRequestNewBuilder extends RequestForgerySink { - HttpRequestNewBuilder() { - exists(MethodAccess call | - call.getCallee().hasName("newBuilder") and - call.getMethod().getDeclaringType().getName() = "HttpRequest" - | - this.asExpr() = call.getArgument(0) - ) - } -} - -/** - * An argument to an Http Builder `uri` call taken as a - * sink for request forgery vulnerabilities. - */ -private class HttpBuilderUriArgument extends RequestForgerySink { - HttpBuilderUriArgument() { - exists(MethodAccess ma | ma.getMethod() instanceof HttpBuilderUri | - this.asExpr() = ma.getArgument(0) - ) - } -} - -/** - * An argument to a Spring Rest Template method call taken as a - * sink for request forgery vulnerabilities. - */ -private class SpringRestTemplateArgument extends RequestForgerySink { - SpringRestTemplateArgument() { - exists(MethodAccess ma | - this.asExpr() = ma.getMethod().(SpringRestTemplateUrlMethods).getUrlArgument(ma) - ) +private class RequestForgerySinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "java.net;URL;false;openConnection;;;Argument[-1];request-forgery-sink", + "java.net;URL;false;openStream;;;Argument[-1];request-forgery-sink", + "org.apache.http.client.methods;HttpRequestBase;true;setURI;;;Argument[0];request-forgery-sink", + "org.apache.http.message;BasicHttpRequest;true;setURI;;;Argument[0];request-forgery-sink", + "org.apache.http.client.methods;HttpRequestBase;true;.ctor;;;Argument[0];request-forgery-sink", + "org.apache.http.message;BasicHttpRequest;true;.ctor;;;Argument[0];request-forgery-sink", + "org.apache.http.client.methods;RequestBuilder;false;setURI;;;Argument[0];request-forgery-sink", + "org.apache.http.client.methods;RequestBuilder;false;get;;;Argument[0];request-forgery-sink", + "org.apache.http.client.methods;RequestBuilder;false;post;;;Argument[0];request-forgery-sink", + "org.apache.http.client.methods;RequestBuilder;false;put;;;Argument[0];request-forgery-sink", + "org.apache.http.client.methods;RequestBuilder;false;options;;;Argument[0];request-forgery-sink", + "org.apache.http.client.methods;RequestBuilder;false;head;;;Argument[0];request-forgery-sink", + "org.apache.http.client.methods;RequestBuilder;false;delete;;;Argument[0];request-forgery-sink", + "java.net.http;HttpRequest$Builder;false;uri;;;Argument[0];request-forgery-sink", + "org.springframework.web.client;RestTemplate;false;patchForObject;;;Argument[0];request-forgery-sink", + "org.springframework.web.client;RestTemplate;false;getForObject;;;Argument[0];request-forgery-sink", + "org.springframework.web.client;RestTemplate;false;getForEntity;;;Argument[0];request-forgery-sink", + "org.springframework.web.client;RestTemplate;false;execute;;;Argument[0];request-forgery-sink", + "org.springframework.web.client;RestTemplate;false;exchange;;;Argument[0];request-forgery-sink", + "org.springframework.web.client;RestTemplate;false;put;;;Argument[0];request-forgery-sink", + "org.springframework.web.client;RestTemplate;false;postForObject;;;Argument[0];request-forgery-sink", + "org.springframework.web.client;RestTemplate;false;postForLocation;;;Argument[0];request-forgery-sink", + "org.springframework.web.client;RestTemplate;false;postForEntity;;;Argument[0];request-forgery-sink", + "org.springframework.web.client;RestTemplate;false;doExecute;;;Argument[0];request-forgery-sink", + "javax.ws.rs.client;Client;false;target;;;Argument[0];request-forgery-sink", + "java.net.http;HttpRequest;false;newBuilder;;;Argument[0];request-forgery-sink" // todo: this might be stricter than before. Previously the package name was not checked. + ] } } -/** - * An argument to `javax.ws.rs.Client`s `target` method call taken as a - * sink for request forgery vulnerabilities. - */ -private class JaxRsClientTarget extends RequestForgerySink { - JaxRsClientTarget() { - exists(MethodAccess ma | - ma.getMethod().getDeclaringType() instanceof JaxRsClient and - ma.getMethod().hasName("target") - | - this.asExpr() = ma.getArgument(0) - ) - } -} +/** A data flow sink for request forgery vulnerabilities. */ +abstract class RequestForgerySink extends DataFlow::Node { } /** * An argument to `org.springframework.http.RequestEntity`s constructor call @@ -166,27 +90,3 @@ private class RequestEntityUriArg extends RequestForgerySink { ) } } - -/** - * A class representing all Spring Rest Template methods - * which take an URL as an argument. - */ -private class SpringRestTemplateUrlMethods extends Method { - SpringRestTemplateUrlMethods() { - this.getDeclaringType() instanceof SpringRestTemplate and - this.hasName([ - "doExecute", "postForEntity", "postForLocation", "postForObject", "put", "exchange", - "execute", "getForEntity", "getForObject", "patchForObject" - ]) - } - - /** - * Gets the argument which corresponds to a URL argument - * passed as a `java.net.URL` object or as a string or the like - */ - Argument getUrlArgument(MethodAccess ma) { - // doExecute(URI url, HttpMethod method, RequestCallback requestCallback, - // ResponseExtractor responseExtractor) - result = ma.getArgument(0) - } -} diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 154ec98f2ad8..3cda09c14889 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -472,7 +472,7 @@ module CsvValidation { not type.regexpMatch("[a-zA-Z0-9_\\$]+") and msg = "Dubious type \"" + type + "\" in " + pred + " model." or - not name.regexpMatch("[a-zA-Z0-9_]*") and + not (name.regexpMatch("[a-zA-Z0-9_]*") or name = ".ctor") and msg = "Dubious name \"" + name + "\" in " + pred + " model." or not signature.regexpMatch("|\\([a-zA-Z0-9_\\.\\$<>,\\[\\]]*\\)") and @@ -562,23 +562,34 @@ private string paramsString(Callable c) { } private Element interpretElement0( - string namespace, string type, boolean subtypes, string name, string signature + string namespace, string type, boolean subtypes, string name, string signature, string ext ) { elementSpec(namespace, type, subtypes, name, signature, _) and exists(RefType t | t = interpretType(namespace, type, subtypes) | exists(Member m | result = m and m.getDeclaringType() = t and - m.hasName(name) + ( + m.hasName(name) + or + name = ".ctor" and m.hasName(t.getName()) + ) | signature = "" or m.(Callable).getSignature() = any(string nameprefix) + signature or paramsString(m) = signature - ) + ) and + ext = "" or result = t and name = "" and - signature = "" + signature = "" and + ext = "Annotated" + or + result = t.getAMember() and + name = "" and + signature = "" and + ext = "" ) } @@ -586,7 +597,7 @@ private Element interpretElement( string namespace, string type, boolean subtypes, string name, string signature, string ext ) { elementSpec(namespace, type, subtypes, name, signature, ext) and - exists(Element e | e = interpretElement0(namespace, type, subtypes, name, signature) | + exists(Element e | e = interpretElement0(namespace, type, subtypes, name, signature, ext) | ext = "" and result = e or ext = "Annotated" and result.(Annotatable).getAnAnnotation().getType() = e diff --git a/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll b/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll index ab809f07d6d1..d84ee67bca5c 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll @@ -3,6 +3,7 @@ import semmle.code.java.frameworks.XStream import semmle.code.java.frameworks.SnakeYaml import semmle.code.java.frameworks.FastJson import semmle.code.java.frameworks.apache.Lang +private import semmle.code.java.dataflow.ExternalFlow class ObjectInputStreamReadObjectMethod extends Method { ObjectInputStreamReadObjectMethod() { @@ -26,11 +27,16 @@ class SafeXStream extends DataFlow2::Configuration { src.asExpr() } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - sink.asExpr() = ma.getQualifier() and - ma.getMethod() instanceof XStreamReadObjectMethod - ) + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "safe-xstream") } +} + +private class SafeXStreamSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "com.thoughtworks.xstream;XStream;false;unmarshal;;;Argument[-1];safe-xstream", + "com.thoughtworks.xstream;XStream;false;fromXML;;;Argument[-1];safe-xstream" + ] } } @@ -42,11 +48,17 @@ class SafeKryo extends DataFlow2::Configuration { src.asExpr() } - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - sink.asExpr() = ma.getQualifier() and - ma.getMethod() instanceof KryoReadObjectMethod - ) + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "safe-kryo") } +} + +private class SafeKryoSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "com.esotericsoftware.kryo;Kryo;false;readObjectOrNull;;;Argument[-1];safe-kryo", + "com.esotericsoftware.kryo;Kryo;false;readObject;;;Argument[-1];safe-kryo", + "com.esotericsoftware.kryo;Kryo;false;readClassAndObject;;;Argument[-1];safe-kryo" + ] } } From 351f35d9bce68e79b921511ba91f9f17172de7c5 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Fri, 9 Apr 2021 13:13:49 +0200 Subject: [PATCH 09/16] Revert "Java: Convert other sinks" This reverts commit 87d42b02c0c49ff717c941d6ea5290b6ca3c40df. --- .../CWE/CWE-209/StackTraceExposure.ql | 11 +- .../Security/CWE/CWE-036/OpenStream.ql | 15 +- .../Security/CWE/CWE-074/XsltInjectionLib.qll | 41 ++--- .../Security/CWE/CWE-094/JexlInjectionLib.qll | 139 ++++++++------- .../Security/CWE/CWE-094/MvelInjectionLib.qll | 56 +++--- .../CWE/CWE-326/InsufficientKeySize.ql | 23 ++- .../Security/CWE/CWE-327/SslLib.qll | 20 +-- .../Security/CWE/CWE-346/UnvalidatedCors.ql | 29 +-- .../Security/CWE/CWE-522/InsecureLdapAuth.ql | 26 ++- .../Security/CWE/CWE-643/XPathInjection.ql | 23 ++- .../Security/CWE/CWE-652/XQueryInjection.ql | 7 +- .../CWE/CWE-652/XQueryInjectionLib.qll | 12 -- .../Security/CWE/CWE-917/OgnlInjectionLib.qll | 26 +-- .../Security/CWE/CWE-918/RequestForgery.ql | 5 +- .../Security/CWE/CWE-918/RequestForgery.qll | 168 ++++++++++++++---- .../code/java/dataflow/ExternalFlow.qll | 23 +-- .../java/security/UnsafeDeserialization.qll | 32 ++-- 17 files changed, 354 insertions(+), 302 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql b/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql index 1f49d5cb06ac..c9c1e2917c0e 100644 --- a/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql +++ b/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql @@ -16,7 +16,6 @@ import java import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.TaintTracking import semmle.code.java.security.XSS -private import semmle.code.java.dataflow.ExternalFlow /** * One of the `printStackTrace()` overloads on `Throwable`. @@ -38,12 +37,10 @@ class ServletWriterSourceToPrintStackTraceMethodFlowConfig extends TaintTracking override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof ServletWriterSource } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "print-stack-trace") } -} - -private class PrintStackTraceSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = ["java.lang;Throwable;true;printStackTrace;;;Argument;print-stack-trace"] + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + sink.asExpr() = ma.getAnArgument() and ma.getMethod() instanceof PrintStackTraceMethod + ) } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-036/OpenStream.ql b/java/ql/src/experimental/Security/CWE/CWE-036/OpenStream.ql index 6f75b3fc8ae5..871d6bb4737c 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-036/OpenStream.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-036/OpenStream.ql @@ -15,7 +15,6 @@ import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.ExternalFlow import DataFlow::PathGraph -private import semmle.code.java.dataflow.ExternalFlow class URLConstructor extends ClassInstanceExpr { URLConstructor() { this.getConstructor().getDeclaringType() instanceof TypeUrl } @@ -40,7 +39,13 @@ class RemoteURLToOpenStreamFlowConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "url-open-stream") } + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess m | + sink.asExpr() = m.getQualifier() and m.getMethod() instanceof URLOpenStreamMethod + ) + or + sinkNode(sink, "url-open-stream") + } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { exists(URLConstructor u | @@ -50,12 +55,6 @@ class RemoteURLToOpenStreamFlowConfig extends TaintTracking::Configuration { } } -private class URLOpenStreamSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = ["java.net;URL;false;openStream;;;Argument[-1];url-open-stream"] - } -} - from DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess call where sink.getNode().asExpr() = call.getQualifier() and diff --git a/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjectionLib.qll index bc0c8352a20f..4ba0eb6d0b11 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjectionLib.qll @@ -2,7 +2,6 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.XmlParsers import DataFlow -private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration for unvalidated user input that is used in XSLT transformation. @@ -104,20 +103,15 @@ class TypeXsltPackage extends Class { /** A data flow sink for unvalidated user input that is used in XSLT transformation. */ class XsltInjectionSink extends DataFlow::ExprNode { - XsltInjectionSink() { sinkNode(this, "xslt") } -} - -private class XsltInjectionSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "net.sf.saxon.s9api;XsltTransformer;false;transform;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;transform;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;applyTemplates;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;callFunction;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;callTemplate;;;Argument[-1];xslt", - "javax.xml.transform;Transformer;false;transform;;;Argument[-1];xslt" - ] + XsltInjectionSink() { + exists(MethodAccess ma, Method m | m = ma.getMethod() and ma.getQualifier() = this.getExpr() | + ma instanceof TransformerTransform or + m instanceof XsltTransformerTransformMethod or + m instanceof Xslt30TransformerTransformMethod or + m instanceof Xslt30TransformerApplyTemplatesMethod or + m instanceof Xslt30TransformerCallFunctionMethod or + m instanceof Xslt30TransformerCallTemplateMethod + ) } } @@ -192,21 +186,16 @@ private class TransformerFactoryWithSecureProcessingFeatureFlowConfig extends Da ) } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "xslt-transformer") } + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + sink.asExpr() = ma.getQualifier() and + ma.getMethod().getDeclaringType() instanceof TransformerFactory + ) + } override int fieldFlowBranchLimit() { result = 0 } } -private class TransformerFactorySinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "javax.xml.transform;TransformerFactory;false;;;;Argument[-1];xslt-transformer", - "javax.xml.transform.sax;SAXTransformerFactory;false;;;;Argument[-1];xslt-transformer" - ] - } -} - /** A `ParserConfig` specific to `TransformerFactory`. */ private class TransformerFactoryFeatureConfig extends ParserConfig { TransformerFactoryFeatureConfig() { diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/JexlInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-094/JexlInjectionLib.qll index 8fcde750e54c..561d7e46ae90 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/JexlInjectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-094/JexlInjectionLib.qll @@ -1,7 +1,6 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking -private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration for unsafe user input @@ -22,7 +21,7 @@ class JexlInjectionConfig extends TaintTracking::Configuration { } /** - * A sink for Expression Language injection vulnerabilities via Jexl, + * A sink for Expresssion Language injection vulnerabilities via Jexl, * i.e. method calls that run evaluation of a JEXL expression. * * Creating a `Callable` from a tainted JEXL expression or script is considered as a sink @@ -31,41 +30,18 @@ class JexlInjectionConfig extends TaintTracking::Configuration { * maybe stored in an object field and then reached by a different flow. */ private class JexlEvaluationSink extends DataFlow::ExprNode { - JexlEvaluationSink() { sinkNode(this, "jexl") } -} - -private class JexlEvaluationSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - // Direct JEXL evaluation - "org.apache.commons.jexl2;Expression;false;evaluate;;;Argument[-1];jexl", - "org.apache.commons.jexl3;JexlExpression;false;evaluate;;;Argument[-1];jexl", - "org.apache.commons.jexl2;Script;false;execute;;;Argument[-1];jexl", - "org.apache.commons.jexl3;JexlScript;false;execute;;;Argument[-1];jexl", - "org.apache.commons.jexl2;JxltEngine$Expression;false;evaluate;;;Argument[-1];jexl", - "org.apache.commons.jexl3;JxltEngine$Expression;false;evaluate;;;Argument[-1];jexl", - "org.apache.commons.jexl2;JxltEngine$Expression;false;prepare;;;Argument[-1];jexl", - "org.apache.commons.jexl3;JxltEngine$Expression;false;prepare;;;Argument[-1];jexl", - "org.apache.commons.jexl2;JxltEngine$Template;false;evaluate;;;Argument[-1];jexl", - "org.apache.commons.jexl3;JxltEngine$Template;false;evaluate;;;Argument[-1];jexl", - "org.apache.commons.jexl2;UnifiedJEXL$Expression;false;evaluate;;;Argument[-1];jexl", - "org.apache.commons.jexl3;UnifiedJEXL$Expression;false;evaluate;;;Argument[-1];jexl", - "org.apache.commons.jexl2;UnifiedJEXL$Expression;false;prepare;;;Argument[-1];jexl", - "org.apache.commons.jexl3;UnifiedJEXL$Expression;false;prepare;;;Argument[-1];jexl", - "org.apache.commons.jexl2;UnifiedJEXL$Template;false;evaluate;;;Argument[-1];jexl", - "org.apache.commons.jexl3;UnifiedJEXL$Template;false;evaluate;;;Argument[-1];jexl", - // JEXL callable - "org.apache.commons.jexl2;Expression;false;callable;;;Argument[-1];jexl", - "org.apache.commons.jexl3;JexlExpression;false;callable;;;Argument[-1];jexl", - "org.apache.commons.jexl2;Script;false;callable;;;Argument[-1];jexl", - "org.apache.commons.jexl3;JexlScript;false;callable;;;Argument[-1];jexl", - // Methods in the `JexlEngine` class that gets or sets a property with a JEXL expression. - "org.apache.commons.jexl2;JexlEngine;false;getProperty;;;Argument[1..2];jexl", - "org.apache.commons.jexl3;JexlEngine;false;getProperty;;;Argument[1..2];jexl", - "org.apache.commons.jexl2;JexlEngine;false;setProperty;;;Argument[1];jexl", - "org.apache.commons.jexl3;JexlEngine;false;setProperty;;;Argument[1];jexl" - ] + JexlEvaluationSink() { + exists(MethodAccess ma, Method m, Expr taintFrom | + ma.getMethod() = m and taintFrom = this.asExpr() + | + m instanceof DirectJexlEvaluationMethod and ma.getQualifier() = taintFrom + or + m instanceof CreateJexlCallableMethod and ma.getQualifier() = taintFrom + or + m instanceof JexlEngineGetSetPropertyMethod and + taintFrom.getType() instanceof TypeString and + ma.getAnArgument() = taintFrom + ) } } @@ -122,36 +98,22 @@ private class SandboxedJexlFlowConfig extends DataFlow2::Configuration { override predicate isSource(DataFlow::Node node) { node instanceof SandboxedJexlSource } - override predicate isSink(DataFlow::Node node) { sinkNode(node, "sandboxed-jexl") } + override predicate isSink(DataFlow::Node node) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + ( + m instanceof CreateJexlScriptMethod or + m instanceof CreateJexlExpressionMethod or + m instanceof CreateJexlTemplateMethod + ) and + ma.getQualifier() = node.asExpr() + ) + } override predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) { createsJexlEngine(fromNode, toNode) } } -private class SandboxedJexlEvaluationSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - // CreateJexlScriptMethod - "org.apache.commons.jexl2;JexlEngine;false;createScript;;;Argument[-1];sandboxed-jexl", - "org.apache.commons.jexl3;JexlEngine;false;createScript;;;Argument[-1];sandboxed-jexl", - // CreateJexlExpressionMethod - "org.apache.commons.jexl2;UnifiedJEXL;false;parse;;;Argument[-1];sandboxed-jexl", - "org.apache.commons.jexl3;UnifiedJEXL;false;parse;;;Argument[-1];sandboxed-jexl", - "org.apache.commons.jexl2;JxltEngine;false;createExpression;;;Argument[-1];sandboxed-jexl", - "org.apache.commons.jexl3;JxltEngine;false;createExpression;;;Argument[-1];sandboxed-jexl", - "org.apache.commons.jexl2;JexlEngine;false;createExpression;;;Argument[-1];sandboxed-jexl", - "org.apache.commons.jexl3;JexlEngine;false;createExpression;;;Argument[-1];sandboxed-jexl", - // CreateJexlTemplateMethod - "org.apache.commons.jexl2;JxltEngine;false;createTemplate;;;Argument[-1];sandboxed-jexl", - "org.apache.commons.jexl3;JxltEngine;false;createTemplate;;;Argument[-1];sandboxed-jexl", - "org.apache.commons.jexl2;UnifiedJEXL;false;createTemplate;;;Argument[-1];sandboxed-jexl", - "org.apache.commons.jexl3;UnifiedJEXL;false;createTemplate;;;Argument[-1];sandboxed-jexl" - ] - } -} - /** * Defines a data flow source for JEXL engines configured with a sandbox. */ @@ -202,6 +164,35 @@ private predicate returnsDataFromBean(DataFlow::Node fromNode, DataFlow::Node to ) } +/** + * A methods in the `JexlEngine` class that gets or sets a property with a JEXL expression. + */ +private class JexlEngineGetSetPropertyMethod extends Method { + JexlEngineGetSetPropertyMethod() { + getDeclaringType() instanceof JexlEngine and + hasName(["getProperty", "setProperty"]) + } +} + +/** + * A method that triggers direct evaluation of JEXL expressions. + */ +private class DirectJexlEvaluationMethod extends Method { + DirectJexlEvaluationMethod() { + getDeclaringType() instanceof JexlExpression and hasName("evaluate") + or + getDeclaringType() instanceof JexlScript and hasName("execute") + or + getDeclaringType() instanceof JxltEngineExpression and hasName(["evaluate", "prepare"]) + or + getDeclaringType() instanceof JxltEngineTemplate and hasName("evaluate") + or + getDeclaringType() instanceof UnifiedJexlExpression and hasName(["evaluate", "prepare"]) + or + getDeclaringType() instanceof UnifiedJexlTemplate and hasName("evaluate") + } +} + /** * A method that creates a JEXL script. */ @@ -209,6 +200,16 @@ private class CreateJexlScriptMethod extends Method { CreateJexlScriptMethod() { getDeclaringType() instanceof JexlEngine and hasName("createScript") } } +/** + * A method that creates a `Callable` for a JEXL expression or script. + */ +private class CreateJexlCallableMethod extends Method { + CreateJexlCallableMethod() { + (getDeclaringType() instanceof JexlExpression or getDeclaringType() instanceof JexlScript) and + hasName("callable") + } +} + /** * A method that creates a JEXL template. */ @@ -266,6 +267,22 @@ private class JexlUberspect extends Interface { } } +private class JxltEngineExpression extends NestedType { + JxltEngineExpression() { getEnclosingType() instanceof JxltEngine and hasName("Expression") } +} + +private class JxltEngineTemplate extends NestedType { + JxltEngineTemplate() { getEnclosingType() instanceof JxltEngine and hasName("Template") } +} + +private class UnifiedJexlExpression extends NestedType { + UnifiedJexlExpression() { getEnclosingType() instanceof UnifiedJexl and hasName("Expression") } +} + +private class UnifiedJexlTemplate extends NestedType { + UnifiedJexlTemplate() { getEnclosingType() instanceof UnifiedJexl and hasName("Template") } +} + private class Reader extends RefType { Reader() { hasQualifiedName("java.io", "Reader") } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll index dec268a2a4bc..a6cf891330f0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll @@ -1,7 +1,6 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking -private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration for unsafe user input @@ -31,31 +30,36 @@ class MvelInjectionConfig extends TaintTracking::Configuration { * i.e. methods that run evaluation of a MVEL expression. */ class MvelEvaluationSink extends DataFlow::ExprNode { - MvelEvaluationSink() { sinkNode(this, "mvel") } -} - -private class MvelEvaluationSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "org.mvel2.jsr223;MvelScriptEngine;false;evaluate;;;Argument[0];mvel", - "org.mvel2.jsr223;MvelScriptEngine;false;eval;;;Argument[0];mvel", - "org.mvel2.compiler;ExecutableStatement;false;getValue;;;Argument[-1];mvel", - "org.mvel2.compiler;CompiledExpression;false;getDirectValue;;;Argument[-1];mvel", - "org.mvel2.compiler;CompiledAccExpression;false;getValue;;;Argument[-1];mvel", - "org.mvel2.compiler;Accessor;false;getValue;;;Argument[-1];mvel", - "javax.script;CompiledScript;false;eval;;;Argument[-1];mvel", - "org.mvel2.jsr223;MvelCompiledScript;false;eval;;;Argument[-1];mvel", - "org.mvel2;MVEL;false;eval;;;Argument[0];mvel", - "org.mvel2;MVEL;false;executeExpression;;;Argument[0];mvel", - "org.mvel2;MVEL;false;evalToBoolean;;;Argument[0];mvel", - "org.mvel2;MVEL;false;evalToString;;;Argument[0];mvel", - "org.mvel2;MVEL;false;executeAllExpression;;;Argument[0];mvel", - "org.mvel2;MVEL;false;executeSetExpression;;;Argument[0];mvel", - "org.mvel2.templates;TemplateRuntime;false;eval;;;Argument[0];mvel", - "org.mvel2.templates;TemplateRuntime;false;execute;;;Argument[0];mvel", - "org.mvel2;MVELRuntime;false;execute;;;Argument[1];mvel" - ] + MvelEvaluationSink() { + exists(StaticMethodAccess ma, Method m | m = ma.getMethod() | + ( + m instanceof MvelEvalMethod or + m instanceof TemplateRuntimeEvaluationMethod + ) and + ma.getArgument(0) = asExpr() + ) + or + exists(MethodAccess ma, Method m | m = ma.getMethod() | + m instanceof MvelScriptEngineEvaluationMethod and + ma.getArgument(0) = asExpr() + ) + or + exists(MethodAccess ma, Method m | m = ma.getMethod() | + ( + m instanceof ExecutableStatementEvaluationMethod or + m instanceof CompiledExpressionEvaluationMethod or + m instanceof CompiledAccExpressionEvaluationMethod or + m instanceof AccessorEvaluationMethod or + m instanceof CompiledScriptEvaluationMethod or + m instanceof MvelCompiledScriptEvaluationMethod + ) and + ma.getQualifier() = asExpr() + ) + or + exists(StaticMethodAccess ma, Method m | m = ma.getMethod() | + m instanceof MvelRuntimeEvaluationMethod and + ma.getArgument(1) = asExpr() + ) } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-326/InsufficientKeySize.ql b/java/ql/src/experimental/Security/CWE/CWE-326/InsufficientKeySize.ql index ca86f42c561a..155d05abfae3 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-326/InsufficientKeySize.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-326/InsufficientKeySize.ql @@ -12,7 +12,6 @@ import java import semmle.code.java.security.Encryption import semmle.code.java.dataflow.TaintTracking -private import semmle.code.java.dataflow.ExternalFlow /** The Java class `java.security.spec.ECGenParameterSpec`. */ class ECGenParameterSpec extends RefType { @@ -56,12 +55,11 @@ class KeyGeneratorInitConfiguration extends TaintTracking::Configuration { exists(JavaxCryptoKeyGenerator jcg | jcg = source.asExpr()) } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "keygen") } -} - -private class KeyGeneratorInitSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = ["javax.crypto;KeyGenerator;false;init;;;Argument[-1];keygen"] + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + ma.getMethod() instanceof KeyGeneratorInitMethod and + sink.asExpr() = ma.getQualifier() + ) } } @@ -73,12 +71,11 @@ class KeyPairGeneratorInitConfiguration extends TaintTracking::Configuration { exists(JavaSecurityKeyPairGenerator jkg | jkg = source.asExpr()) } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "keypairgen") } -} - -private class KeyPairGeneratorInitSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = ["java.security;KeyPairGenerator;false;initialize;;;Argument[-1];keypairgen"] + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + ma.getMethod() instanceof KeyPairGeneratorInitMethod and + sink.asExpr() = ma.getQualifier() + ) } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll b/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll index 3845953f0aab..bfa2530b07e7 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-327/SslLib.qll @@ -3,7 +3,6 @@ import semmle.code.java.security.Encryption import semmle.code.java.dataflow.TaintTracking import DataFlow import PathGraph -private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration for unsafe SSL and TLS versions. @@ -13,20 +12,11 @@ class UnsafeTlsVersionConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof UnsafeTlsVersion } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "ssl") } -} - -private class UnsafeTlsVersionSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "javax.net.ssl;SSLContext;false;getInstance;;;Argument[0];ssl", - "javax.net.ssl;SSLParameters;false;SSLParameters;;;Argument[1];ssl", - "javax.net.ssl;SSLParameters;false;setProtocols;;;Argument[0];ssl", - "javax.net.ssl;SSLSocket;false;setEnabledProtocols;;;Argument[0];ssl", - "javax.net.ssl;SSLServerSocket;false;setEnabledProtocols;;;Argument[0];ssl", - "javax.net.ssl;SSLEngine;false;setEnabledProtocols;;;Argument[0];ssl" - ] + override predicate isSink(DataFlow::Node sink) { + sink instanceof SslContextGetInstanceSink or + sink instanceof CreateSslParametersSink or + sink instanceof SslParametersSetProtocolsSink or + sink instanceof SetEnabledProtocolsSink } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.ql b/java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.ql index 2fe6569318fc..c5a6c36d6a61 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-346/UnvalidatedCors.ql @@ -15,7 +15,6 @@ import semmle.code.java.frameworks.Servlets import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.TaintTracking2 import DataFlow::PathGraph -private import semmle.code.java.dataflow.ExternalFlow /** * Holds if `header` sets `Access-Control-Allow-Credentials` to `true`. This ensures fair chances of exploitability. @@ -30,29 +29,33 @@ private predicate setsAllowCredentials(MethodAccess header) { header.getArgument(1).(CompileTimeConstantExpr).getStringValue().toLowerCase() = "true" } +private class CorsProbableCheckAccess extends MethodAccess { + CorsProbableCheckAccess() { + getMethod().hasName("contains") and + getMethod().getDeclaringType().getASourceSupertype*() instanceof CollectionType + or + getMethod().hasName("containsKey") and + getMethod().getDeclaringType().getASourceSupertype*() instanceof MapType + or + getMethod().hasName("equals") and + getQualifier().getType() instanceof TypeString + } +} + private Expr getAccessControlAllowOriginHeaderName() { result.(CompileTimeConstantExpr).getStringValue().toLowerCase() = "access-control-allow-origin" } /** - * This taintflow2 configuration checks if there is a flow from source node towards probably CORS checking methods. + * This taintflow2 configuration checks if there is a flow from source node towards CorsProbableCheckAccess methods. */ class CorsSourceReachesCheckConfig extends TaintTracking2::Configuration { CorsSourceReachesCheckConfig() { this = "CorsOriginConfig" } override predicate isSource(DataFlow::Node source) { any(CorsOriginConfig c).hasFlow(source, _) } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "cors") } -} - -private class CorsProbableCheckAccessSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "java.util;Collection;true;contains;;;Argument;cors", - "java.util;Map;true;containsKey;;;Argument;cors", - "java.lang;String;true;equals;;;Argument;cors" - ] + override predicate isSink(DataFlow::Node sink) { + sink.asExpr() = any(CorsProbableCheckAccess check).getAnArgument() } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql b/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql index 3c445934ad33..4ce2b8b7134c 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-522/InsecureLdapAuth.ql @@ -16,7 +16,6 @@ import semmle.code.java.frameworks.Jndi import semmle.code.java.frameworks.Networking import semmle.code.java.dataflow.TaintTracking import DataFlow::PathGraph -private import semmle.code.java.dataflow.ExternalFlow /** * Insecure (non-SSL, non-private) LDAP URL string literal. @@ -146,7 +145,12 @@ class InsecureUrlFlowConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof InsecureLdapUrl } /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "ldap") } + override predicate isSink(DataFlow::Node sink) { + exists(ConstructorCall cc | + cc.getConstructedType().getASupertype*() instanceof TypeDirContext and + sink.asExpr() = cc.getArgument(0) + ) + } /** Method call of `env.put()`. */ override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { @@ -172,12 +176,11 @@ class BasicAuthFlowConfig extends DataFlow::Configuration { } /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "ldap") } -} - -private class LdapSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = ["javax.naming.directory;DirContext;true;.ctor;;;Argument[0];ldap"] + override predicate isSink(DataFlow::Node sink) { + exists(ConstructorCall cc | + cc.getConstructedType().getASupertype*() instanceof TypeDirContext and + sink.asExpr() = cc.getArgument(0) + ) } } @@ -195,7 +198,12 @@ class SSLFlowConfig extends DataFlow::Configuration { } /** Sink of directory context creation. */ - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "ldap") } + override predicate isSink(DataFlow::Node sink) { + exists(ConstructorCall cc | + cc.getConstructedType().getASupertype*() instanceof TypeDirContext and + sink.asExpr() = cc.getArgument(0) + ) + } } from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureUrlFlowConfig config diff --git a/java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.ql index 598f42b0c703..e5a29df46d59 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-643/XPathInjection.ql @@ -15,7 +15,6 @@ import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking import semmle.code.java.security.XmlParsers import DataFlow::PathGraph -private import semmle.code.java.dataflow.ExternalFlow class XPathInjectionConfiguration extends TaintTracking::Configuration { XPathInjectionConfiguration() { this = "XPathInjection" } @@ -26,18 +25,16 @@ class XPathInjectionConfiguration extends TaintTracking::Configuration { } class XPathInjectionSink extends DataFlow::ExprNode { - XPathInjectionSink() { sinkNode(this, "xpath") } -} - -private class XPathInjectionSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "javax.xml.xpath;XPath;false;compile;;;Argument[0];xpath", - "javax.xml.xpath;XPath;false;evaluate;;;Argument[0];xpath", - "org.dom4j;Node;false;selectNodes;;;Argument[0];xpath", - "org.dom4j;Node;false;selectSingleNode;;;Argument[0];xpath" - ] + XPathInjectionSink() { + exists(Method m, MethodAccess ma | ma.getMethod() = m | + m.getDeclaringType().hasQualifiedName("javax.xml.xpath", "XPath") and + (m.hasName("evaluate") or m.hasName("compile")) and + ma.getArgument(0) = this.getExpr() + or + m.getDeclaringType().hasQualifiedName("org.dom4j", "Node") and + (m.hasName("selectNodes") or m.hasName("selectSingleNode")) and + ma.getArgument(0) = this.getExpr() + ) } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjection.ql b/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjection.ql index 3b8e43c5d7d2..0bb85272f085 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjection.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjection.ql @@ -14,7 +14,6 @@ import java import semmle.code.java.dataflow.FlowSources import XQueryInjectionLib import DataFlow::PathGraph -private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration tracing flow from remote sources, through an XQuery parser, to its eventual execution. @@ -24,7 +23,11 @@ class XQueryInjectionConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "xquery") } + override predicate isSink(DataFlow::Node sink) { + sink.asExpr() = any(XQueryPreparedExecuteCall xpec).getPreparedExpression() or + sink.asExpr() = any(XQueryExecuteCall xec).getExecuteQueryArgument() or + sink.asExpr() = any(XQueryExecuteCommandCall xecc).getExecuteCommandArgument() + } /** * Holds if taint from the input `pred` to a `prepareExpression` call flows to the returned prepared expression `succ`. diff --git a/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjectionLib.qll index 13452e4e55dc..2a4019f2c9a9 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-652/XQueryInjectionLib.qll @@ -1,5 +1,4 @@ import java -private import semmle.code.java.dataflow.ExternalFlow /** A call to `XQConnection.prepareExpression`. */ class XQueryParserCall extends MethodAccess { @@ -67,14 +66,3 @@ class XQueryExecuteCommandCall extends MethodAccess { /** Return this execute command argument. */ Expr getExecuteCommandArgument() { result = this.getArgument(0) } } - -private class XQuerySinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "javax.xml.xquery;XQPreparedExpression;true;executeQuery;;;Argument[-1];xquery", - "javax.xml.xquery;XQExpression;true;executeQuery;;;Argument[0];xquery", - "javax.xml.xquery;XQExpression;true;executeCommand;;;Argument[0];xquery" - ] - } -} diff --git a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll index 002e06aaafdc..569e18a29c38 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll @@ -2,7 +2,6 @@ import java import semmle.code.java.dataflow.FlowSources import DataFlow import DataFlow::PathGraph -private import semmle.code.java.dataflow.ExternalFlow /** * A taint-tracking configuration for unvalidated user input that is used in OGNL EL evaluation. @@ -83,25 +82,12 @@ predicate ognlInjectionSinkMethod(Method m, int index) { /** A data flow sink for unvalidated user input that is used in OGNL EL evaluation. */ class OgnlInjectionSink extends DataFlow::ExprNode { - OgnlInjectionSink() { sinkNode(this, "ognl") } -} - -private class OgnlInjectionSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "org.apache.commons.ognl;Ognl;false;setValue;;;Argument[-1..0];ognl", - "org.apache.commons.ognl;Ognl;false;getValue;;;Argument[-1..0];ognl", - "ognl;Ognl;false;setValue;;;Argument[-1..0];ognl", - "ognl;Ognl;false;getValue;;;Argument[-1..0];ognl", - "org.apache.commons.ognl;Node;true;setValue;;;Argument[-1..0];ognl", - "org.apache.commons.ognl;Node;true;getValue;;;Argument[-1..0];ognl", - "ognl;Node;true;setValue;;;Argument[-1..0];ognl", - "ognl;Node;true;getValue;;;Argument[-1..0];ognl", - "com.opensymphony.xwork2.ognl;OgnlUtil;false;setValue;;;Argument[-1..0];ognl", - "com.opensymphony.xwork2.ognl;OgnlUtil;false;getValue;;;Argument[-1..0];ognl", - "com.opensymphony.xwork2.ognl;OgnlUtil;false;callMethod;;;Argument[-1..0];ognl" - ] + OgnlInjectionSink() { + exists(MethodAccess ma, Method m, int index | + ma.getMethod() = m and + (ma.getArgument(index) = this.getExpr() or ma.getQualifier() = this.getExpr()) and + ognlInjectionSinkMethod(m, index) + ) } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.ql b/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.ql index 6b0333da4709..c3bf787881fa 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.ql @@ -14,16 +14,13 @@ import java import semmle.code.java.dataflow.FlowSources import RequestForgery import DataFlow::PathGraph -private import semmle.code.java.dataflow.ExternalFlow class RequestForgeryConfiguration extends TaintTracking::Configuration { RequestForgeryConfiguration() { this = "Server Side Request Forgery" } override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { - sink instanceof RequestForgerySink or sinkNode(sink, "request-forgery-sink") - } + override predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgerySink } override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { requestForgeryStep(pred, succ) diff --git a/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.qll b/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.qll index fa784baa7f23..3fc52ddca766 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-918/RequestForgery.qll @@ -5,7 +5,6 @@ import semmle.code.java.frameworks.spring.Spring import semmle.code.java.frameworks.JaxWS import semmle.code.java.frameworks.javase.Http import semmle.code.java.dataflow.DataFlow -private import semmle.code.java.dataflow.ExternalFlow predicate requestForgeryStep(DataFlow::Node pred, DataFlow::Node succ) { // propagate to a URI when its host is assigned to @@ -39,42 +38,119 @@ predicate requestForgeryStep(DataFlow::Node pred, DataFlow::Node succ) { ) } -private class RequestForgerySinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "java.net;URL;false;openConnection;;;Argument[-1];request-forgery-sink", - "java.net;URL;false;openStream;;;Argument[-1];request-forgery-sink", - "org.apache.http.client.methods;HttpRequestBase;true;setURI;;;Argument[0];request-forgery-sink", - "org.apache.http.message;BasicHttpRequest;true;setURI;;;Argument[0];request-forgery-sink", - "org.apache.http.client.methods;HttpRequestBase;true;.ctor;;;Argument[0];request-forgery-sink", - "org.apache.http.message;BasicHttpRequest;true;.ctor;;;Argument[0];request-forgery-sink", - "org.apache.http.client.methods;RequestBuilder;false;setURI;;;Argument[0];request-forgery-sink", - "org.apache.http.client.methods;RequestBuilder;false;get;;;Argument[0];request-forgery-sink", - "org.apache.http.client.methods;RequestBuilder;false;post;;;Argument[0];request-forgery-sink", - "org.apache.http.client.methods;RequestBuilder;false;put;;;Argument[0];request-forgery-sink", - "org.apache.http.client.methods;RequestBuilder;false;options;;;Argument[0];request-forgery-sink", - "org.apache.http.client.methods;RequestBuilder;false;head;;;Argument[0];request-forgery-sink", - "org.apache.http.client.methods;RequestBuilder;false;delete;;;Argument[0];request-forgery-sink", - "java.net.http;HttpRequest$Builder;false;uri;;;Argument[0];request-forgery-sink", - "org.springframework.web.client;RestTemplate;false;patchForObject;;;Argument[0];request-forgery-sink", - "org.springframework.web.client;RestTemplate;false;getForObject;;;Argument[0];request-forgery-sink", - "org.springframework.web.client;RestTemplate;false;getForEntity;;;Argument[0];request-forgery-sink", - "org.springframework.web.client;RestTemplate;false;execute;;;Argument[0];request-forgery-sink", - "org.springframework.web.client;RestTemplate;false;exchange;;;Argument[0];request-forgery-sink", - "org.springframework.web.client;RestTemplate;false;put;;;Argument[0];request-forgery-sink", - "org.springframework.web.client;RestTemplate;false;postForObject;;;Argument[0];request-forgery-sink", - "org.springframework.web.client;RestTemplate;false;postForLocation;;;Argument[0];request-forgery-sink", - "org.springframework.web.client;RestTemplate;false;postForEntity;;;Argument[0];request-forgery-sink", - "org.springframework.web.client;RestTemplate;false;doExecute;;;Argument[0];request-forgery-sink", - "javax.ws.rs.client;Client;false;target;;;Argument[0];request-forgery-sink", - "java.net.http;HttpRequest;false;newBuilder;;;Argument[0];request-forgery-sink" // todo: this might be stricter than before. Previously the package name was not checked. - ] +/** A data flow sink for request forgery vulnerabilities. */ +abstract class RequestForgerySink extends DataFlow::Node { } + +/** + * An argument to an url `openConnection` or `openStream` call + * taken as a sink for request forgery vulnerabilities. + */ +private class UrlOpen extends RequestForgerySink { + UrlOpen() { + exists(MethodAccess ma | + ma.getMethod() instanceof UrlOpenConnectionMethod or + ma.getMethod() instanceof UrlOpenStreamMethod + | + this.asExpr() = ma.getQualifier() + ) } } -/** A data flow sink for request forgery vulnerabilities. */ -abstract class RequestForgerySink extends DataFlow::Node { } +/** + * An argument to an Apache `setURI` call taken as a + * sink for request forgery vulnerabilities. + */ +private class ApacheSetUri extends RequestForgerySink { + ApacheSetUri() { + exists(MethodAccess ma | + ma.getReceiverType() instanceof ApacheHttpRequest and + ma.getMethod().hasName("setURI") + | + this.asExpr() = ma.getArgument(0) + ) + } +} + +/** + * An argument to any Apache Request Instantiation call taken as a + * sink for request forgery vulnerabilities. + */ +private class ApacheHttpRequestInstantiation extends RequestForgerySink { + ApacheHttpRequestInstantiation() { + exists(ClassInstanceExpr c | c.getConstructedType() instanceof ApacheHttpRequest | + this.asExpr() = c.getArgument(0) + ) + } +} + +/** + * An argument to a Apache RequestBuilder method call taken as a + * sink for request forgery vulnerabilities. + */ +private class ApacheHttpRequestBuilderArgument extends RequestForgerySink { + ApacheHttpRequestBuilderArgument() { + exists(MethodAccess ma | + ma.getReceiverType() instanceof TypeApacheHttpRequestBuilder and + ma.getMethod().hasName(["setURI", "get", "post", "put", "optons", "head", "delete"]) + | + this.asExpr() = ma.getArgument(0) + ) + } +} + +/** + * An argument to any Java.net.http.request Instantiation call taken as a + * sink for request forgery vulnerabilities. + */ +private class HttpRequestNewBuilder extends RequestForgerySink { + HttpRequestNewBuilder() { + exists(MethodAccess call | + call.getCallee().hasName("newBuilder") and + call.getMethod().getDeclaringType().getName() = "HttpRequest" + | + this.asExpr() = call.getArgument(0) + ) + } +} + +/** + * An argument to an Http Builder `uri` call taken as a + * sink for request forgery vulnerabilities. + */ +private class HttpBuilderUriArgument extends RequestForgerySink { + HttpBuilderUriArgument() { + exists(MethodAccess ma | ma.getMethod() instanceof HttpBuilderUri | + this.asExpr() = ma.getArgument(0) + ) + } +} + +/** + * An argument to a Spring Rest Template method call taken as a + * sink for request forgery vulnerabilities. + */ +private class SpringRestTemplateArgument extends RequestForgerySink { + SpringRestTemplateArgument() { + exists(MethodAccess ma | + this.asExpr() = ma.getMethod().(SpringRestTemplateUrlMethods).getUrlArgument(ma) + ) + } +} + +/** + * An argument to `javax.ws.rs.Client`s `target` method call taken as a + * sink for request forgery vulnerabilities. + */ +private class JaxRsClientTarget extends RequestForgerySink { + JaxRsClientTarget() { + exists(MethodAccess ma | + ma.getMethod().getDeclaringType() instanceof JaxRsClient and + ma.getMethod().hasName("target") + | + this.asExpr() = ma.getArgument(0) + ) + } +} /** * An argument to `org.springframework.http.RequestEntity`s constructor call @@ -90,3 +166,27 @@ private class RequestEntityUriArg extends RequestForgerySink { ) } } + +/** + * A class representing all Spring Rest Template methods + * which take an URL as an argument. + */ +private class SpringRestTemplateUrlMethods extends Method { + SpringRestTemplateUrlMethods() { + this.getDeclaringType() instanceof SpringRestTemplate and + this.hasName([ + "doExecute", "postForEntity", "postForLocation", "postForObject", "put", "exchange", + "execute", "getForEntity", "getForObject", "patchForObject" + ]) + } + + /** + * Gets the argument which corresponds to a URL argument + * passed as a `java.net.URL` object or as a string or the like + */ + Argument getUrlArgument(MethodAccess ma) { + // doExecute(URI url, HttpMethod method, RequestCallback requestCallback, + // ResponseExtractor responseExtractor) + result = ma.getArgument(0) + } +} diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 3cda09c14889..154ec98f2ad8 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -472,7 +472,7 @@ module CsvValidation { not type.regexpMatch("[a-zA-Z0-9_\\$]+") and msg = "Dubious type \"" + type + "\" in " + pred + " model." or - not (name.regexpMatch("[a-zA-Z0-9_]*") or name = ".ctor") and + not name.regexpMatch("[a-zA-Z0-9_]*") and msg = "Dubious name \"" + name + "\" in " + pred + " model." or not signature.regexpMatch("|\\([a-zA-Z0-9_\\.\\$<>,\\[\\]]*\\)") and @@ -562,34 +562,23 @@ private string paramsString(Callable c) { } private Element interpretElement0( - string namespace, string type, boolean subtypes, string name, string signature, string ext + string namespace, string type, boolean subtypes, string name, string signature ) { elementSpec(namespace, type, subtypes, name, signature, _) and exists(RefType t | t = interpretType(namespace, type, subtypes) | exists(Member m | result = m and m.getDeclaringType() = t and - ( - m.hasName(name) - or - name = ".ctor" and m.hasName(t.getName()) - ) + m.hasName(name) | signature = "" or m.(Callable).getSignature() = any(string nameprefix) + signature or paramsString(m) = signature - ) and - ext = "" + ) or result = t and name = "" and - signature = "" and - ext = "Annotated" - or - result = t.getAMember() and - name = "" and - signature = "" and - ext = "" + signature = "" ) } @@ -597,7 +586,7 @@ private Element interpretElement( string namespace, string type, boolean subtypes, string name, string signature, string ext ) { elementSpec(namespace, type, subtypes, name, signature, ext) and - exists(Element e | e = interpretElement0(namespace, type, subtypes, name, signature, ext) | + exists(Element e | e = interpretElement0(namespace, type, subtypes, name, signature) | ext = "" and result = e or ext = "Annotated" and result.(Annotatable).getAnAnnotation().getType() = e diff --git a/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll b/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll index d84ee67bca5c..ab809f07d6d1 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeDeserialization.qll @@ -3,7 +3,6 @@ import semmle.code.java.frameworks.XStream import semmle.code.java.frameworks.SnakeYaml import semmle.code.java.frameworks.FastJson import semmle.code.java.frameworks.apache.Lang -private import semmle.code.java.dataflow.ExternalFlow class ObjectInputStreamReadObjectMethod extends Method { ObjectInputStreamReadObjectMethod() { @@ -27,16 +26,11 @@ class SafeXStream extends DataFlow2::Configuration { src.asExpr() } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "safe-xstream") } -} - -private class SafeXStreamSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "com.thoughtworks.xstream;XStream;false;unmarshal;;;Argument[-1];safe-xstream", - "com.thoughtworks.xstream;XStream;false;fromXML;;;Argument[-1];safe-xstream" - ] + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + sink.asExpr() = ma.getQualifier() and + ma.getMethod() instanceof XStreamReadObjectMethod + ) } } @@ -48,17 +42,11 @@ class SafeKryo extends DataFlow2::Configuration { src.asExpr() } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "safe-kryo") } -} - -private class SafeKryoSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "com.esotericsoftware.kryo;Kryo;false;readObjectOrNull;;;Argument[-1];safe-kryo", - "com.esotericsoftware.kryo;Kryo;false;readObject;;;Argument[-1];safe-kryo", - "com.esotericsoftware.kryo;Kryo;false;readClassAndObject;;;Argument[-1];safe-kryo" - ] + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + sink.asExpr() = ma.getQualifier() and + ma.getMethod() instanceof KryoReadObjectMethod + ) } } From 180904e9f6425db8e37d25abb2a780b78316bd6f Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Thu, 22 Apr 2021 11:14:51 +0200 Subject: [PATCH 10/16] Revert "Java: Convert Google HTTP client API parseAs sink to CSV format" This reverts commit 3e53484bb3b942c58f2bf1b90301a9779ebc34aa. --- .../code/java/dataflow/ExternalFlow.qll | 1 - .../frameworks/google/GoogleHttpClientApi.qll | 22 ++++++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 154ec98f2ad8..ba329d99f21b 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -76,7 +76,6 @@ 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.frameworks.google.GoogleHttpClientApi private import semmle.code.java.security.ResponseSplitting private import semmle.code.java.security.XSS } diff --git a/java/ql/src/semmle/code/java/frameworks/google/GoogleHttpClientApi.qll b/java/ql/src/semmle/code/java/frameworks/google/GoogleHttpClientApi.qll index 07e30711b449..ccc446892f12 100644 --- a/java/ql/src/semmle/code/java/frameworks/google/GoogleHttpClientApi.qll +++ b/java/ql/src/semmle/code/java/frameworks/google/GoogleHttpClientApi.qll @@ -2,7 +2,14 @@ import java import semmle.code.java.Serializability import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.DataFlow5 -private import semmle.code.java.dataflow.ExternalFlow + +/** The method `parseAs` in `com.google.api.client.http.HttpResponse`. */ +private class ParseAsMethod extends Method { + ParseAsMethod() { + this.getDeclaringType().hasQualifiedName("com.google.api.client.http", "HttpResponse") and + this.hasName("parseAs") + } +} private class TypeLiteralToParseAsFlowConfiguration extends DataFlow5::Configuration { TypeLiteralToParseAsFlowConfiguration() { @@ -11,17 +18,16 @@ private class TypeLiteralToParseAsFlowConfiguration extends DataFlow5::Configura override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof TypeLiteral } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "google-parse-as") } + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + ma.getAnArgument() = sink.asExpr() and + ma.getMethod() instanceof ParseAsMethod + ) + } TypeLiteral getSourceWithFlowToParseAs() { hasFlow(DataFlow::exprNode(result), _) } } -private class ParseAsSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = ["com.google.api.client.http;HttpResponse;false;parseAs;;;Argument;google-parse-as"] - } -} - /** A field that is deserialized by `HttpResponse.parseAs`. */ class HttpResponseParseAsDeserializableField extends DeserializableField { HttpResponseParseAsDeserializableField() { From 9b1c54e81b66d65e35f4cca6729e08cb337ed3c3 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Thu, 22 Apr 2021 11:17:25 +0200 Subject: [PATCH 11/16] Add argument indices to HTTP header splitting sinks --- java/ql/src/semmle/code/java/security/ResponseSplitting.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/ResponseSplitting.qll b/java/ql/src/semmle/code/java/security/ResponseSplitting.qll index 040174d50b50..abc1d2723c99 100644 --- a/java/ql/src/semmle/code/java/security/ResponseSplitting.qll +++ b/java/ql/src/semmle/code/java/security/ResponseSplitting.qll @@ -17,8 +17,8 @@ private class HeaderSplittingSinkModel extends SinkModelCsv { row = [ "javax.servlet.http;HttpServletResponse;false;addCookie;;;Argument[0];header-splitting", - "javax.servlet.http;HttpServletResponse;false;addHeader;;;Argument;header-splitting", - "javax.servlet.http;HttpServletResponse;false;setHeader;;;Argument;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" ] } From 6c78a247f2e4516a2e94963c3c2c2f3bb6ae390d Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Thu, 22 Apr 2021 11:20:39 +0200 Subject: [PATCH 12/16] Revert erroneous refactoring in header splitting sink base class --- java/ql/src/semmle/code/java/security/ResponseSplitting.qll | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/ResponseSplitting.qll b/java/ql/src/semmle/code/java/security/ResponseSplitting.qll index abc1d2723c99..70af02efc6dc 100644 --- a/java/ql/src/semmle/code/java/security/ResponseSplitting.qll +++ b/java/ql/src/semmle/code/java/security/ResponseSplitting.qll @@ -8,8 +8,10 @@ import semmle.code.java.frameworks.JaxWS private import semmle.code.java.dataflow.ExternalFlow /** A sink that is vulnerable to an HTTP header splitting attack. */ -class HeaderSplittingSink extends DataFlow::Node { - HeaderSplittingSink() { sinkNode(this, "header-splitting") } +abstract class HeaderSplittingSink extends DataFlow::Node { } + +private class DefaultHeaderSplittingSink extends HeaderSplittingSink { + DefaultHeaderSplittingSink() { sinkNode(this, "header-splitting") } } private class HeaderSplittingSinkModel extends SinkModelCsv { From 1caa5c47800ca3d3db58b4fda63b6e4b2b450bb5 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Thu, 22 Apr 2021 11:22:18 +0200 Subject: [PATCH 13/16] Adjust hostname verifier sink identifier name --- .../ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 2 +- java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql index 058a1f9e1695..d1fe8aee0750 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -50,7 +50,7 @@ class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration { source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof TrustAllHostnameVerifier } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "set-hostname") } + override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "set-hostname-verifier") } override predicate isBarrier(DataFlow::Node barrier) { // ignore nodes that are in functions that intentionally disable hostname verification diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index ba329d99f21b..b699f0922f39 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -209,8 +209,8 @@ private predicate sinkModelCsv(string row) { // Bean validation "javax.validation;ConstraintValidatorContext;true;buildConstraintViolationWithTemplate;;;Argument[0];bean-validation", // Set hostname - "javax.net.ssl;HttpsURLConnection;true;setDefaultHostnameVerifier;;;Argument[0];set-hostname", - "javax.net.ssl;HttpsURLConnection;true;setHostnameVerifier;;;Argument[0];set-hostname" + "javax.net.ssl;HttpsURLConnection;true;setDefaultHostnameVerifier;;;Argument[0];set-hostname-verifier", + "javax.net.ssl;HttpsURLConnection;true;setHostnameVerifier;;;Argument[0];set-hostname-verifier" ] } From 7134eb9079e7798db2c497a441912574ec15e477 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Thu, 22 Apr 2021 11:37:41 +0200 Subject: [PATCH 14/16] Improve documentation of csv sink models --- java/ql/src/Security/CWE/CWE-022/ZipSlip.ql | 9 ++++++++- .../src/Security/CWE/CWE-094/InsecureBeanValidation.ql | 10 +++++++++- .../Security/CWE/CWE-297/UnsafeHostnameVerification.ql | 9 ++++++++- java/ql/src/semmle/code/java/security/XSS.qll | 1 + 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql b/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql index 80de02d70679..6342a444a646 100644 --- a/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql +++ b/java/ql/src/Security/CWE/CWE-022/ZipSlip.ql @@ -124,7 +124,7 @@ class ZipSlipConfiguration extends TaintTracking::Configuration { source.asExpr().(MethodAccess).getMethod() instanceof ArchiveEntryNameMethod } - override predicate isSink(Node sink) { sinkNode(sink, "create-file") } + override predicate isSink(Node sink) { sink instanceof FileCreationSink } override predicate isAdditionalTaintStep(Node n1, Node n2) { filePathStep(n1, n2) or fileTaintStep(n1, n2) @@ -146,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 e4ee42008a17..aef404aabd13 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -60,7 +60,15 @@ class BeanValidationConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "bean-validation") } + 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 d1fe8aee0750..0bf7a164826a 100644 --- a/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql +++ b/java/ql/src/Security/CWE/CWE-297/UnsafeHostnameVerification.ql @@ -50,7 +50,7 @@ class TrustAllHostnameVerifierConfiguration extends DataFlow::Configuration { source.asExpr().(ClassInstanceExpr).getConstructedType() instanceof TrustAllHostnameVerifier } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "set-hostname-verifier") } + 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 @@ -78,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/semmle/code/java/security/XSS.qll b/java/ql/src/semmle/code/java/security/XSS.qll index 9791eed203b8..486e8053953c 100644 --- a/java/ql/src/semmle/code/java/security/XSS.qll +++ b/java/ql/src/semmle/code/java/security/XSS.qll @@ -29,6 +29,7 @@ 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 = From e08b629cb57ee7086e4df12b519a5ca2ff50a1b8 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Tue, 27 Apr 2021 10:32:41 +0200 Subject: [PATCH 15/16] Add documentation for URL opening sinks --- java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql b/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql index d1bce930054a..a68998b32118 100644 --- a/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql +++ b/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql @@ -36,7 +36,7 @@ class HTTPStringToURLOpenMethodFlowConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof HTTPString } - override predicate isSink(DataFlow::Node sink) { sinkNode(sink, "open-url") } + override predicate isSink(DataFlow::Node sink) { sink instanceof URLOpenSink } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { exists(UrlConstructorCall u | @@ -50,6 +50,13 @@ class HTTPStringToURLOpenMethodFlowConfig extends TaintTracking::Configuration { } } +/** + * 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 From 5b79094f34b9833e4706206bc8719c8459921558 Mon Sep 17 00:00:00 2001 From: Tamas Vajk Date: Tue, 27 Apr 2021 14:59:52 +0200 Subject: [PATCH 16/16] Fix naming in HTTPS URL check --- java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql b/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql index a68998b32118..77980f033f0b 100644 --- a/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql +++ b/java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql @@ -15,8 +15,8 @@ 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 | ( @@ -31,12 +31,12 @@ class HTTPString extends StringLiteral { } } -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) { sink instanceof URLOpenSink } + override predicate isSink(DataFlow::Node sink) { sink instanceof UrlOpenSink } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { exists(UrlConstructorCall u | @@ -53,14 +53,14 @@ class HTTPStringToURLOpenMethodFlowConfig extends TaintTracking::Configuration { /** * 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") } +private class UrlOpenSink extends DataFlow::Node { + UrlOpenSink() { sinkNode(this, "open-url") } } -from DataFlow::PathNode source, DataFlow::PathNode sink, MethodAccess m, HTTPString s +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"