From 6f7d78183fe79eae09a201ed1abdbe301635fb40 Mon Sep 17 00:00:00 2001 From: Stephan Brandauer Date: Wed, 6 Sep 2023 17:04:04 +0200 Subject: [PATCH] Java: add endpoints for parameters of overridden methods in automodel application mode --- ...utomodelApplicationModeCharacteristics.qll | 52 ++++++++++++++++--- ...tomodelApplicationModeExtractCandidates.ql | 10 ++-- ...lApplicationModeExtractCandidates.expected | 1 + .../Test.java | 6 +++ 4 files changed, 57 insertions(+), 12 deletions(-) diff --git a/java/ql/automodel/src/AutomodelApplicationModeCharacteristics.qll b/java/ql/automodel/src/AutomodelApplicationModeCharacteristics.qll index 3379ce665cf..5c745bcc2ce 100644 --- a/java/ql/automodel/src/AutomodelApplicationModeCharacteristics.qll +++ b/java/ql/automodel/src/AutomodelApplicationModeCharacteristics.qll @@ -36,12 +36,18 @@ newtype TApplicationModeEndpoint = not exists(int i | i < idx and call.getArgument(i).(Argument).isVararg()) ) } or - TMethodCall(Call call) { not call instanceof ConstructorCall } + TMethodCall(Call call) { not call instanceof ConstructorCall } or + TOverriddenParameter(Parameter p) { + not p.getCallable().callsConstructor(_) and + p.getCallable().(Method).overrides(_) + } /** * An endpoint is a node that is a candidate for modeling. */ abstract private class ApplicationModeEndpoint extends TApplicationModeEndpoint { + abstract Callable getCallable(); + abstract Call getCall(); abstract string getMaDInput(); @@ -74,6 +80,8 @@ class ExplicitArgument extends ApplicationModeEndpoint, TExplicitArgument { ExplicitArgument() { this = TExplicitArgument(call, arg) } + override Callable getCallable() { result = call.getCallee() } + override Call getCall() { result = call } private int getArgIndex() { this.asTop() = call.getArgument(result) } @@ -95,6 +103,8 @@ class InstanceArgument extends ApplicationModeEndpoint, TInstanceArgument { InstanceArgument() { this = TInstanceArgument(call, arg) } + override Callable getCallable() { result = call.getCallee() } + override Call getCall() { result = call } override string getMaDInput() { result = "Argument[this]" } @@ -124,13 +134,15 @@ class ImplicitVarargsArray extends ApplicationModeEndpoint, TImplicitVarargsArra ImplicitVarargsArray() { this = TImplicitVarargsArray(call, vararg, idx) } + override Callable getCallable() { result = call.getCallee() } + override Call getCall() { result = call } override string getMaDInput() { result = "Argument[" + idx + "]" } override string getMaDOutput() { none() } - override Top asTop() { result = this.getCall() } + override Top asTop() { result = call } override DataFlow::Node asNode() { result = vararg } @@ -145,6 +157,8 @@ class MethodCall extends ApplicationModeEndpoint, TMethodCall { MethodCall() { this = TMethodCall(call) } + override Callable getCallable() { result = call.getCallee() } + override Call getCall() { result = call } override string getMaDInput() { result = "Argument[this]" } @@ -158,6 +172,28 @@ class MethodCall extends ApplicationModeEndpoint, TMethodCall { override string toString() { result = call.toString() } } +class OverriddenParameter extends ApplicationModeEndpoint, TOverriddenParameter { + Parameter p; + + OverriddenParameter() { this = TOverriddenParameter(p) } + + override Callable getCallable() { result = p.getCallable() } + + override Call getCall() { none() } + + private int getArgIndex() { p.getCallable().getParameter(result) = p } + + override string getMaDInput() { none() } + + override string getMaDOutput() { result = "Parameter[" + this.getArgIndex() + "]" } + + override Top asTop() { result = p } + + override DataFlow::Node asNode() { result.(DataFlow::ParameterNode).asParameter() = p } + + override string toString() { result = p.toString() } +} + /** * A candidates implementation. * @@ -208,7 +244,8 @@ module ApplicationCandidatesImpl implements SharedCharacteristics::CandidateSig predicate isSource(Endpoint e, string kind, string provenance) { exists(string package, string type, string name, string signature, string ext, string output | sourceSpec(e, package, type, name, signature, ext, output) and - ExternalFlow::sourceModel(package, type, _, name, [signature, ""], ext, output, kind, provenance) + ExternalFlow::sourceModel(package, type, _, name, [signature, ""], ext, output, kind, + provenance) ) } @@ -230,7 +267,8 @@ module ApplicationCandidatesImpl implements SharedCharacteristics::CandidateSig } additional predicate sourceSpec( - Endpoint e, string package, string type, string name, string signature, string ext, string output + Endpoint e, string package, string type, string name, string signature, string ext, + string output ) { ApplicationModeGetCallable::getCallable(e).hasQualifiedName(package, type, name) and signature = ExternalFlow::paramsString(ApplicationModeGetCallable::getCallable(e)) and @@ -293,7 +331,7 @@ class ApplicationModeMetadataExtractor extends string { string input, string output, string isVarargsArray ) { exists(Callable callable | - e.getCall().getCallee() = callable and + e.getCallable() = callable and (if exists(e.getMaDInput()) then input = e.getMaDInput() else input = "") and (if exists(e.getMaDOutput()) then output = e.getMaDOutput() else output = "") and package = callable.getDeclaringType().getPackage().getName() and @@ -328,8 +366,8 @@ private class UnexploitableIsCharacteristic extends CharacteristicsImpl::NotASin override predicate appliesToEndpoint(Endpoint e) { not ApplicationCandidatesImpl::isSink(e, _, _) and - ApplicationModeGetCallable::getCallable(e).getName().matches("is%") and - ApplicationModeGetCallable::getCallable(e).getReturnType() instanceof BooleanType + e.getCallable().getName().matches("is%") and + e.getCallable().getReturnType() instanceof BooleanType } } diff --git a/java/ql/automodel/src/AutomodelApplicationModeExtractCandidates.ql b/java/ql/automodel/src/AutomodelApplicationModeExtractCandidates.ql index e27c22fe13f..60182884bb4 100644 --- a/java/ql/automodel/src/AutomodelApplicationModeExtractCandidates.ql +++ b/java/ql/automodel/src/AutomodelApplicationModeExtractCandidates.ql @@ -66,11 +66,11 @@ where endpoint = getSampleForSignature(9, package, type, subtypes, name, signature, input, output, isVarargsArray, extensibleType) and - // If a node is already a known sink for any of our existing ATM queries and is already modeled as a MaD sink, we - // don't include it as a candidate. Otherwise, we might include it as a candidate for query A, but the model will - // label it as a sink for one of the sink types of query B, for which it's already a known sink. This would result in - // overlap between our detected sinks and the pre-existing modeling. We assume that, if a sink has already been - // modeled in a MaD model, then it doesn't belong to any additional sink types, and we don't need to reexamine it. + // If a node is already modeled in MaD, we don't include it as a candidate. Otherwise, we might include it as a + // candidate for query A, but the model will label it as a sink for one of the sink types of query B, for which it's + // already a known sink. This would result in overlap between our detected sinks and the pre-existing modeling. We + // assume that, if a sink has already been modeled in a MaD model, then it doesn't belong to any additional sink + // types, and we don't need to reexamine it. ( not CharacteristicsImpl::isSink(endpoint, _, _) and alreadyAiModeled = "" or diff --git a/java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractCandidates.expected b/java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractCandidates.expected index 6ef3ee17b32..43796af788e 100644 --- a/java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractCandidates.expected +++ b/java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractCandidates.expected @@ -9,3 +9,4 @@ | Test.java:54:4:54:4 | o | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:52:3:57:3 | walk(...) | CallContext | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://walk:1:1:1:1 | walk | name | file://(Path,FileVisitOption[]):1:1:1:1 | (Path,FileVisitOption[]) | signature | file://Argument[1]:1:1:1:1 | Argument[1] | input | file://:1:1:1:1 | | output | file://true:1:1:1:1 | true | isVarargsArray | file://:1:1:1:1 | | alreadyAiModeled | file://sinkModel:1:1:1:1 | sinkModel | extensibleType | | Test.java:61:3:61:3 | c | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:61:3:61:20 | getInputStream(...) | CallContext | file://java.net:1:1:1:1 | java.net | package | file://URLConnection:1:1:1:1 | URLConnection | type | file://true:1:1:1:1 | true | subtypes | file://getInputStream:1:1:1:1 | getInputStream | name | file://():1:1:1:1 | () | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://:1:1:1:1 | | output | file://false:1:1:1:1 | false | isVarargsArray | file://:1:1:1:1 | | alreadyAiModeled | file://sinkModel:1:1:1:1 | sinkModel | extensibleType | | Test.java:61:3:61:20 | getInputStream(...) | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:61:3:61:20 | getInputStream(...) | CallContext | file://java.net:1:1:1:1 | java.net | package | file://URLConnection:1:1:1:1 | URLConnection | type | file://true:1:1:1:1 | true | subtypes | file://getInputStream:1:1:1:1 | getInputStream | name | file://():1:1:1:1 | () | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray | file://:1:1:1:1 | | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType | +| Test.java:66:24:66:31 | o | command-injection, path-injection, request-forgery, sql-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:66:24:66:31 | o | CallContext | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://OverrideTest:1:1:1:1 | OverrideTest | type | file://true:1:1:1:1 | true | subtypes | file://equals:1:1:1:1 | equals | name | file://(Object):1:1:1:1 | (Object) | signature | file://:1:1:1:1 | | input | file://Parameter[0]:1:1:1:1 | Parameter[0] | output | file://false:1:1:1:1 | false | isVarargsArray | file://:1:1:1:1 | | alreadyAiModeled | file://sourceModel:1:1:1:1 | sourceModel | extensibleType | diff --git a/java/ql/automodel/test/AutomodelApplicationModeExtraction/Test.java b/java/ql/automodel/test/AutomodelApplicationModeExtraction/Test.java index f72a512c26a..f257ef2f188 100644 --- a/java/ql/automodel/test/AutomodelApplicationModeExtraction/Test.java +++ b/java/ql/automodel/test/AutomodelApplicationModeExtraction/Test.java @@ -61,3 +61,9 @@ class Test { c.getInputStream(); // the call is a source example, c is a sink candidate } } + +class OverrideTest { + public boolean equals(Object o) { // o is a source candidate because it overrides an existing method + return false; + } +}