From 10a6362357baa65a3eebb25cbe8040f34410303d Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Fri, 3 May 2019 15:46:41 +0200 Subject: [PATCH 1/3] Java: Introduce an abstract class RemoteFlowSource to ease customization. --- .../semmle/code/java/dataflow/FlowSources.qll | 134 +++++++++++++----- 1 file changed, 99 insertions(+), 35 deletions(-) diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll index ad0ad55e9fc..dd993fb32db 100644 --- a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll +++ b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll @@ -21,8 +21,54 @@ import semmle.code.java.frameworks.Guice import semmle.code.java.frameworks.struts.StrutsActions import semmle.code.java.frameworks.Thrift -/** Class for `tainted` user input. */ -abstract class UserInput extends DataFlow::Node { } +/** A data flow source of remote user input. */ +abstract class RemoteFlowSource extends DataFlow::Node { + /** Gets a string that describes the type of this remote flow source. */ + abstract string getSourceType(); +} + +private class RemoteTaintedMethodAccessSource extends RemoteFlowSource { + RemoteTaintedMethodAccessSource() { + this.asExpr().(MethodAccess).getMethod() instanceof RemoteTaintedMethod + } + + override string getSourceType() { result = "network data source" } +} + +private class RmiMethodParameterSource extends RemoteFlowSource { + RmiMethodParameterSource() { + exists(RemoteCallableMethod method | + method.getAParameter() = this.asParameter() and + ( + getType() instanceof PrimitiveType or + getType() instanceof TypeString + ) + ) + } + + override string getSourceType() { result = "RMI method parameter" } +} + +private class JaxWsMethodParameterSource extends RemoteFlowSource { + JaxWsMethodParameterSource() { + exists(JaxWsEndpoint endpoint | + endpoint.getARemoteMethod().getAParameter() = this.asParameter() + ) + } + + override string getSourceType() { result = "Jax WS method parameter" } +} + +private class JaxRsMethodParameterSource extends RemoteFlowSource { + JaxRsMethodParameterSource() { + exists(JaxRsResourceClass service | + service.getAnInjectableCallable().getAParameter() = this.asParameter() or + service.getAnInjectableField().getAnAccess() = this.asExpr() + ) + } + + override string getSourceType() { result = "Jax Rs method parameter" } +} private predicate variableStep(Expr tracked, VarAccess sink) { exists(VariableAssign def | @@ -31,32 +77,9 @@ private predicate variableStep(Expr tracked, VarAccess sink) { ) } -/** Input that may be controlled by a remote user. */ -class RemoteUserInput extends UserInput { - RemoteUserInput() { - this.asExpr().(MethodAccess).getMethod() instanceof RemoteTaintedMethod - or - // Parameters to RMI methods. - exists(RemoteCallableMethod method | - method.getAParameter() = this.asParameter() and - ( - getType() instanceof PrimitiveType or - getType() instanceof TypeString - ) - ) - or - // Parameters to Jax WS methods. - exists(JaxWsEndpoint endpoint | - endpoint.getARemoteMethod().getAParameter() = this.asParameter() - ) - or - // Parameters to Jax Rs methods. - exists(JaxRsResourceClass service | - service.getAnInjectableCallable().getAParameter() = this.asParameter() or - service.getAnInjectableField().getAnAccess() = this.asExpr() - ) - or - // Reverse DNS. Try not to trigger on `localhost`. +private class ReverseDnsSource extends RemoteFlowSource { + ReverseDnsSource() { + // Try not to trigger on `localhost`. exists(MethodAccess m | m = this.asExpr() | m.getMethod() instanceof ReverseDNSMethod and not exists(MethodAccess l | @@ -64,25 +87,66 @@ class RemoteUserInput extends UserInput { l.getMethod().getName() = "getLocalHost" ) ) - or - //MessageBodyReader + } + + override string getSourceType() { result = "reverse DNS lookup" } +} + +private class MessageBodyReaderParameterSource extends RemoteFlowSource { + MessageBodyReaderParameterSource() { exists(MessageBodyReaderRead m | m.getParameter(4) = this.asParameter() or m.getParameter(5) = this.asParameter() ) - or + } + + override string getSourceType() { result = "MessageBodyReader parameter" } +} + +private class SpringServletInputParameterSource extends RemoteFlowSource { + SpringServletInputParameterSource() { this.asParameter().getAnAnnotation() instanceof SpringServletInputAnnotation - or + } + + override string getSourceType() { result = "Spring servlet input parameter" } +} + +private class GuiceRequestParameterSource extends RemoteFlowSource { + GuiceRequestParameterSource() { exists(GuiceRequestParametersAnnotation a | a = this.asParameter().getAnAnnotation() or a = this.asExpr().(FieldRead).getField().getAnAnnotation() ) - or - exists(Struts2ActionSupportClass c | c.getASetterMethod().getField() = this.asExpr().(FieldRead).getField()) - or + } + + override string getSourceType() { result = "Guice request parameter" } +} + +private class Struts2ActionSupportClassFieldReadSource extends RemoteFlowSource { + Struts2ActionSupportClassFieldReadSource() { + exists(Struts2ActionSupportClass c | + c.getASetterMethod().getField() = this.asExpr().(FieldRead).getField() + ) + } + + override string getSourceType() { result = "Struts2 ActionSupport field" } +} + +private class ThriftIfaceParameterSource extends RemoteFlowSource { + ThriftIfaceParameterSource() { exists(ThriftIface i | i.getAnImplementingMethod().getAParameter() = this.asParameter()) } + override string getSourceType() { result = "Thrift Iface parameter" } +} + +/** Class for `tainted` user input. */ +abstract class UserInput extends DataFlow::Node { } + +/** Input that may be controlled by a remote user. */ +class RemoteUserInput extends UserInput { + RemoteUserInput() { this instanceof RemoteFlowSource } + /** * DEPRECATED: Use a configuration with a defined sink instead. * From f367427fb81cda52249407a5bbcea7201de0e317 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Mon, 6 May 2019 13:43:58 +0200 Subject: [PATCH 2/3] Java: Deprecate RemoteUserInput. --- java/ql/src/Security/CWE/CWE-022/TaintedPath.ql | 2 +- java/ql/src/Security/CWE/CWE-078/ExecCommon.qll | 2 +- java/ql/src/Security/CWE/CWE-079/XSS.ql | 2 +- java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll | 2 +- java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql | 2 +- java/ql/src/Security/CWE/CWE-113/ResponseSplitting.qll | 2 +- .../CWE/CWE-129/ImproperValidationOfArrayConstruction.ql | 2 +- .../CWE/CWE-129/ImproperValidationOfArrayIndex.ql | 2 +- .../CWE/CWE-134/ExternallyControlledFormatString.ql | 2 +- java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql | 2 +- java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.ql | 2 +- java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql | 2 +- java/ql/src/Security/CWE/CWE-611/XXE.ql | 2 +- java/ql/src/Security/CWE/CWE-681/NumericCastTainted.ql | 2 +- java/ql/src/semmle/code/java/dataflow/FlowSources.qll | 8 ++++++-- java/ql/test/library-tests/frameworks/guice/flow.ql | 2 +- 16 files changed, 21 insertions(+), 17 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql b/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql index 0aef239d264..31fb0c5035c 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql @@ -20,7 +20,7 @@ import DataFlow::PathGraph class TaintedPathConfig extends TaintTracking::Configuration { TaintedPathConfig() { this = "TaintedPathConfig" } - override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { exists(Expr e | e = sink.asExpr() | e = any(PathCreation p).getInput() and not guarded(e)) diff --git a/java/ql/src/Security/CWE/CWE-078/ExecCommon.qll b/java/ql/src/Security/CWE/CWE-078/ExecCommon.qll index 00c7fea3e02..4d6bf8c848a 100644 --- a/java/ql/src/Security/CWE/CWE-078/ExecCommon.qll +++ b/java/ql/src/Security/CWE/CWE-078/ExecCommon.qll @@ -6,7 +6,7 @@ private class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::C this = "ExecCommon::RemoteUserInputToArgumentToExecFlowConfig" } - override predicate isSource(DataFlow::Node src) { src instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec } diff --git a/java/ql/src/Security/CWE/CWE-079/XSS.ql b/java/ql/src/Security/CWE/CWE-079/XSS.ql index dc4c7f606bc..5b1bfb2ab16 100644 --- a/java/ql/src/Security/CWE/CWE-079/XSS.ql +++ b/java/ql/src/Security/CWE/CWE-079/XSS.ql @@ -18,7 +18,7 @@ import DataFlow2::PathGraph class XSSConfig extends TaintTracking::Configuration2 { XSSConfig() { this = "XSSConfig" } - override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink instanceof XssSink } diff --git a/java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll b/java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll index 09da2e6de2a..3d2e2b91434 100644 --- a/java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll +++ b/java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll @@ -49,7 +49,7 @@ class PersistenceQueryInjectionSink extends QueryInjectionSink { private class QueryInjectionFlowConfig extends TaintTracking::Configuration { QueryInjectionFlowConfig() { this = "SqlInjectionLib::QueryInjectionFlowConfig" } - override predicate isSource(DataFlow::Node src) { src instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink instanceof QueryInjectionSink } diff --git a/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql b/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql index c2a6e0363a0..ce5efe56d70 100644 --- a/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql +++ b/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql @@ -18,7 +18,7 @@ class ResponseSplittingConfig extends TaintTracking::Configuration { ResponseSplittingConfig() { this = "ResponseSplittingConfig" } override predicate isSource(DataFlow::Node source) { - source instanceof RemoteUserInput and + source instanceof RemoteFlowSource and not source instanceof WhitelistedSource } diff --git a/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.qll b/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.qll index f2bcf80d1cc..f17ac91fa97 100644 --- a/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.qll +++ b/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.qll @@ -30,7 +30,7 @@ class HeaderSplittingSink extends DataFlow::ExprNode { } } -class WhitelistedSource extends RemoteUserInput { +class WhitelistedSource extends DataFlow::ExprNode { WhitelistedSource() { this.asExpr().(MethodAccess).getMethod() instanceof HttpServletRequestGetHeaderMethod or this.asExpr().(MethodAccess).getMethod() instanceof CookieGetNameMethod diff --git a/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayConstruction.ql b/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayConstruction.ql index 3d9c6b0e483..b3d9b9f1884 100644 --- a/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayConstruction.ql +++ b/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayConstruction.ql @@ -17,7 +17,7 @@ import DataFlow::PathGraph class Conf extends TaintTracking::Configuration { Conf() { this = "RemoteUserInputTocanThrowOutOfBoundsDueToEmptyArrayConfig" } - override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { any(CheckableArrayAccess caa).canThrowOutOfBoundsDueToEmptyArray(sink.asExpr(), _) diff --git a/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayIndex.ql b/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayIndex.ql index 32571c7f540..9f0d9fa92a2 100644 --- a/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayIndex.ql +++ b/java/ql/src/Security/CWE/CWE-129/ImproperValidationOfArrayIndex.ql @@ -17,7 +17,7 @@ import DataFlow::PathGraph class Conf extends TaintTracking::Configuration { Conf() { this = "RemoteUserInputTocanThrowOutOfBoundsDueToEmptyArrayConfig" } - override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { any(CheckableArrayAccess caa).canThrowOutOfBounds(sink.asExpr()) diff --git a/java/ql/src/Security/CWE/CWE-134/ExternallyControlledFormatString.ql b/java/ql/src/Security/CWE/CWE-134/ExternallyControlledFormatString.ql index 4251e275e0d..7a9e3a2baab 100644 --- a/java/ql/src/Security/CWE/CWE-134/ExternallyControlledFormatString.ql +++ b/java/ql/src/Security/CWE/CWE-134/ExternallyControlledFormatString.ql @@ -17,7 +17,7 @@ import DataFlow::PathGraph class ExternallyControlledFormatStringConfig extends TaintTracking::Configuration { ExternallyControlledFormatStringConfig() { this = "ExternallyControlledFormatStringConfig" } - override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(StringFormat formatCall).getFormatArgument() diff --git a/java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql b/java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql index b607b0fcad5..5219d994e5e 100644 --- a/java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql +++ b/java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql @@ -32,7 +32,7 @@ predicate sink(ArithExpr exp, VarAccess tainted, string effect) { class RemoteUserInputConfig extends TaintTracking::Configuration { RemoteUserInputConfig() { this = "ArithmeticTainted.ql:RemoteUserInputConfig" } - override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink(_, sink.asExpr(), _) } diff --git a/java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.ql b/java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.ql index 9d857203538..18cf624375f 100644 --- a/java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.ql +++ b/java/ql/src/Security/CWE/CWE-502/UnsafeDeserialization.ql @@ -18,7 +18,7 @@ import DataFlow::PathGraph class UnsafeDeserializationConfig extends TaintTracking::Configuration { UnsafeDeserializationConfig() { this = "UnsafeDeserializationConfig" } - override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeDeserializationSink } } diff --git a/java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql b/java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql index b343ae1e8a8..0249051820e 100644 --- a/java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql +++ b/java/ql/src/Security/CWE/CWE-601/UrlRedirect.ql @@ -18,7 +18,7 @@ import DataFlow::PathGraph class UrlRedirectConfig extends TaintTracking::Configuration { UrlRedirectConfig() { this = "UrlRedirectConfig" } - override predicate isSource(DataFlow::Node source) { source instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink instanceof UrlRedirectSink } } diff --git a/java/ql/src/Security/CWE/CWE-611/XXE.ql b/java/ql/src/Security/CWE/CWE-611/XXE.ql index fba77e0640a..35764f88bc8 100644 --- a/java/ql/src/Security/CWE/CWE-611/XXE.ql +++ b/java/ql/src/Security/CWE/CWE-611/XXE.ql @@ -40,7 +40,7 @@ class UnsafeXxeSink extends DataFlow::ExprNode { class XxeConfig extends TaintTracking::Configuration { XxeConfig() { this = "XXE.ql::XxeConfig" } - override predicate isSource(DataFlow::Node src) { src instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeXxeSink } } diff --git a/java/ql/src/Security/CWE/CWE-681/NumericCastTainted.ql b/java/ql/src/Security/CWE/CWE-681/NumericCastTainted.ql index 840d1173070..1f54800a091 100644 --- a/java/ql/src/Security/CWE/CWE-681/NumericCastTainted.ql +++ b/java/ql/src/Security/CWE/CWE-681/NumericCastTainted.ql @@ -19,7 +19,7 @@ import DataFlow::PathGraph private class NumericCastFlowConfig extends TaintTracking::Configuration { NumericCastFlowConfig() { this = "NumericCastTainted::RemoteUserInputToNumericNarrowingCastExpr" } - override predicate isSource(DataFlow::Node src) { src instanceof RemoteUserInput } + override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink.asExpr() = any(NumericNarrowingCastExpr cast).getExpr() diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll index dd993fb32db..9059e9b01cc 100644 --- a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll +++ b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll @@ -143,8 +143,12 @@ private class ThriftIfaceParameterSource extends RemoteFlowSource { /** Class for `tainted` user input. */ abstract class UserInput extends DataFlow::Node { } -/** Input that may be controlled by a remote user. */ -class RemoteUserInput extends UserInput { +/** + * DEPRECATED: Use `RemoteFlowSource` instead. + * + * Input that may be controlled by a remote user. + */ +deprecated class RemoteUserInput extends UserInput { RemoteUserInput() { this instanceof RemoteFlowSource } /** diff --git a/java/ql/test/library-tests/frameworks/guice/flow.ql b/java/ql/test/library-tests/frameworks/guice/flow.ql index d447f92fbee..8afd4f440ad 100644 --- a/java/ql/test/library-tests/frameworks/guice/flow.ql +++ b/java/ql/test/library-tests/frameworks/guice/flow.ql @@ -6,7 +6,7 @@ class Conf extends TaintTracking::Configuration { Conf() { this = "conf" } override predicate isSource(DataFlow::Node src) { - src instanceof RemoteUserInput + src instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { From 66813a91efec43dbfb2da23b3c220f5d81f48938 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Thu, 9 May 2019 13:40:25 +0200 Subject: [PATCH 3/3] Java: Postpone deprecation to separate PR. --- java/ql/src/semmle/code/java/dataflow/FlowSources.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll index 9059e9b01cc..9aa5ea5c296 100644 --- a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll +++ b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll @@ -148,7 +148,7 @@ abstract class UserInput extends DataFlow::Node { } * * Input that may be controlled by a remote user. */ -deprecated class RemoteUserInput extends UserInput { +class RemoteUserInput extends UserInput { RemoteUserInput() { this instanceof RemoteFlowSource } /**