From 5fc0c52261e9b58c6a2f458fe04f5d90acdf4630 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Mon, 8 Dec 2025 14:53:09 +0100 Subject: [PATCH 1/8] Java: Basic support for pass-through barrier models. --- .../code/java/dataflow/ExternalFlow.qll | 52 ++++++++++++++++++- .../internal/ExternalFlowExtensions.qll | 8 +++ .../dataflow/internal/FlowSummaryImpl.qll | 22 +++++++- .../dataflow/internal/FlowSummaryImpl.qll | 39 +++++++++++++- shared/mad/codeql/mad/ModelValidation.qll | 2 +- 5 files changed, 118 insertions(+), 5 deletions(-) diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index d1849df0f3ec..d5582760c8f6 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -174,6 +174,15 @@ predicate sinkModel( ) } +/** Holds if a barrier model exists for the given parameters. */ +predicate barrierModel( + string package, string type, boolean subtypes, string name, string signature, string ext, + string output, string kind, string provenance, QlBuiltins::ExtensionId madId +) { + Extensions::barrierModel(package, type, subtypes, name, signature, ext, output, kind, provenance, + madId) +} + /** Holds if a summary model exists for the given parameters. */ predicate summaryModel( string package, string type, boolean subtypes, string name, string signature, string ext, @@ -234,6 +243,7 @@ predicate interpretModelForTest(QlBuiltins::ExtensionId madId, string model) { "Summary: " + package + "; " + type + "; " + subtypes + "; " + name + "; " + signature + "; " + ext + "; " + input + "; " + output + "; " + kind + "; " + provenance ) + //TODO: possibly barrier models? } /** Holds if a neutral model exists for the given parameters. */ @@ -292,6 +302,7 @@ predicate modelCoverage(string package, int pkgs, string kind, string part, int summaryModel(subpkg, type, subtypes, name, signature, ext, input, output, kind, provenance, _) ) + // TODO: possibly barrier models? ) } @@ -303,7 +314,8 @@ module ModelValidation { summaryModel(_, _, _, _, _, _, path, _, _, _, _) or summaryModel(_, _, _, _, _, _, _, path, _, _, _) or sinkModel(_, _, _, _, _, _, path, _, _, _) or - sourceModel(_, _, _, _, _, _, path, _, _, _) + sourceModel(_, _, _, _, _, _, path, _, _, _) or + barrierModel(_, _, _, _, _, _, path, _, _, _) } private module MkAccessPath = AccessPathSyntax::AccessPath; @@ -338,6 +350,8 @@ module ModelValidation { exists(string pred, AccessPath output, AccessPathToken part | sourceModel(_, _, _, _, _, _, output, _, _, _) and pred = "source" or + barrierModel(_, _, _, _, _, _, output, _, _, _) and pred = "barrier" + or summaryModel(_, _, _, _, _, _, _, output, _, _, _) and pred = "summary" | ( @@ -355,7 +369,11 @@ module ModelValidation { private module KindValConfig implements SharedModelVal::KindValidationConfigSig { predicate summaryKind(string kind) { summaryModel(_, _, _, _, _, _, _, _, kind, _, _) } - predicate sinkKind(string kind) { sinkModel(_, _, _, _, _, _, _, kind, _, _) } + predicate sinkKind(string kind) { + sinkModel(_, _, _, _, _, _, _, kind, _, _) + or + barrierModel(_, _, _, _, _, _, _, kind, _, _) + } predicate sourceKind(string kind) { sourceModel(_, _, _, _, _, _, _, kind, _, _) } @@ -373,6 +391,8 @@ module ModelValidation { or sinkModel(package, type, _, name, signature, ext, _, _, provenance, _) and pred = "sink" or + barrierModel(package, type, _, name, signature, ext, _, _, provenance, _) and pred = "barrier" + or summaryModel(package, type, _, name, signature, ext, _, _, _, provenance, _) and pred = "summary" or @@ -418,6 +438,8 @@ private predicate elementSpec( or sinkModel(package, type, subtypes, name, signature, ext, _, _, _, _) or + barrierModel(package, type, subtypes, name, signature, ext, _, _, _, _) + or summaryModel(package, type, subtypes, name, signature, ext, _, _, _, _, _) or neutralModel(package, type, name, signature, _, _) and ext = "" and subtypes = true @@ -578,6 +600,26 @@ private module Cached { isSinkNode(n, kind, model) and n.asNode() = node ) } + + // private predicate barrierGuardChecks() { + // exists( + // SourceSinkInterpretationInput::InterpretNode n, ArgumentNode arg, GuardAcceptingValue gv + // | + // isBarrierGuardNode(n, gv, kind, model) and n.asNode() = arg + // ) + // } + /** + * Holds if `node` is specified as a barrier with the given kind in a MaD flow + * model. + */ + cached + predicate barrierNode(Node node, string kind, string model) { + exists(SourceSinkInterpretationInput::InterpretNode n | + isBarrierNode(n, kind, model) and n.asNode() = node + ) + // or + // BarrierGuard::getABarrierNode() = node + } } import Cached @@ -594,6 +636,12 @@ predicate sourceNode(Node node, string kind) { sourceNode(node, kind, _) } */ predicate sinkNode(Node node, string kind) { sinkNode(node, kind, _) } +/** + * Holds if `node` is specified as a barrier with the given kind in a MaD flow + * model. + */ +predicate barrierNode(Node node, string kind) { barrierNode(node, kind, _) } + // adapter class for converting Mad summaries to `SummarizedCallable`s private class SummarizedCallableAdapter extends SummarizedCallable { SummarizedCallableAdapter() { summaryElement(this, _, _, _, _, _, _) } diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll b/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll index 32b5d289e28c..8b3d91017f6b 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll @@ -20,6 +20,14 @@ extensible predicate sinkModel( string input, string kind, string provenance, QlBuiltins::ExtensionId madId ); +/** + * Holds if a barrier model exists for the given parameters. + */ +extensible predicate barrierModel( + string package, string type, boolean subtypes, string name, string signature, string ext, + string output, string kind, string provenance, QlBuiltins::ExtensionId madId +); + /** * Holds if a summary model exists for the given parameters. */ diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll index 8dc28ea2f601..b3e0dda2b3d2 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll @@ -158,7 +158,8 @@ private predicate relatedArgSpec(Callable c, string spec) { summaryModel(namespace, type, subtypes, name, signature, ext, spec, _, _, _, _) or summaryModel(namespace, type, subtypes, name, signature, ext, _, spec, _, _, _) or sourceModel(namespace, type, subtypes, name, signature, ext, spec, _, _, _) or - sinkModel(namespace, type, subtypes, name, signature, ext, spec, _, _, _) + sinkModel(namespace, type, subtypes, name, signature, ext, spec, _, _, _) or + barrierModel(namespace, type, subtypes, name, signature, ext, spec, _, _, _) | c = interpretElement(namespace, type, subtypes, name, signature, ext, _) ) @@ -259,6 +260,25 @@ module SourceSinkInterpretationInput implements ) } + predicate barrierElement( + Element e, string output, string kind, Public::Provenance provenance, string model + ) { + exists( + string namespace, string type, boolean subtypes, string name, string signature, string ext, + SourceOrSinkElement baseBarrier, string originalOutput, QlBuiltins::ExtensionId madId + | + barrierModel(namespace, type, subtypes, name, signature, ext, originalOutput, kind, provenance, + madId) and + model = "MaD:" + madId.toString() and + baseBarrier = interpretElement(namespace, type, subtypes, name, signature, ext, _) and + ( + e = baseBarrier and output = originalOutput + or + correspondingKotlinParameterDefaultsArgSpec(baseBarrier, e, originalOutput, output) + ) + ) + } + class SourceOrSinkElement = Element; private newtype TInterpretNode = diff --git a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll index 6cc9d6f88a45..206e846b88cb 100644 --- a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll @@ -2052,6 +2052,14 @@ module Make< Element n, string input, string kind, Provenance provenance, string model ); + /** + * Holds if an external barrier specification exists for `n` with output specification + * `output` and kind `kind`. + */ + predicate barrierElement( + Element n, string output, string kind, Provenance provenance, string model + ); + class SourceOrSinkElement extends Element; /** An entity used to interpret a source/sink specification. */ @@ -2105,7 +2113,8 @@ module Make< private predicate sourceSinkSpec(string spec) { sourceElement(_, spec, _, _, _) or - sinkElement(_, spec, _, _, _) + sinkElement(_, spec, _, _, _) or + barrierElement(_, spec, _, _, _) } private module AccessPath = AccessPathSyntax::AccessPath; @@ -2160,6 +2169,17 @@ module Make< ) } + private predicate barrierElementRef( + InterpretNode ref, SourceSinkAccessPath output, string kind, string model + ) { + exists(SourceOrSinkElement e | + barrierElement(e, output, kind, _, model) and + if outputNeedsReferenceExt(output.getToken(0)) + then e = ref.getCallTarget() + else e = ref.asElement() + ) + } + /** Holds if the first `n` tokens of `output` resolve to the given interpretation. */ private predicate interpretOutput( SourceSinkAccessPath output, int n, InterpretNode ref, InterpretNode node @@ -2280,6 +2300,23 @@ module Make< ) } + /** + * Holds if `node` is specified as a barrier with the given kind in a MaD flow + * model. + */ + predicate isBarrierNode(InterpretNode node, string kind, string model) { + exists(InterpretNode ref, SourceSinkAccessPath output | + barrierElementRef(ref, output, kind, model) and + interpretOutput(output, output.getNumToken(), ref, node) + ) + } + + // predicate isBarrierGuardNode(InterpretNode node, GuardAcceptingValue gv, string kind, string model) { + // exists(InterpretNode ref, SourceSinkAccessPath input | + // barrierGuardElementRef(ref, input, kind, model) and + // interpretInput(input, input.getNumToken(), ref, node) + // ) + // } final private class SourceOrSinkElementFinal = SourceOrSinkElement; signature predicate sourceOrSinkElementSig( diff --git a/shared/mad/codeql/mad/ModelValidation.qll b/shared/mad/codeql/mad/ModelValidation.qll index 5d4698bed1d4..9791355d03ae 100644 --- a/shared/mad/codeql/mad/ModelValidation.qll +++ b/shared/mad/codeql/mad/ModelValidation.qll @@ -173,7 +173,7 @@ module KindValidation { or exists(string kind, string msg | Config::sinkKind(kind) | not kind instanceof ValidSinkKind and - msg = "Invalid kind \"" + kind + "\" in sink model." and + msg = "Invalid kind \"" + kind + "\" in sink or barrier model." and // The part of this message that refers to outdated sink kinds can be deleted after June 1st, 2024. if kind instanceof OutdatedSinkKind then result = msg + " " + kind.(OutdatedSinkKind).outdatedMessage() From 7e9196e6aa83faa6fe773ba29ace51f90beeeeee Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 9 Dec 2025 12:35:27 +0100 Subject: [PATCH 2/8] Java: Add support for boolean MaD barrier guards. --- .../code/java/dataflow/ExternalFlow.qll | 79 ++++++++++++++++--- .../java/dataflow/internal/DataFlowUtil.qll | 29 +++++++ .../internal/ExternalFlowExtensions.qll | 8 ++ .../dataflow/internal/FlowSummaryImpl.qll | 27 ++++++- .../code/java/dataflow/internal/SsaImpl.qll | 31 ++++++++ .../dataflow/internal/FlowSummaryImpl.qll | 78 ++++++++++++++++-- 6 files changed, 233 insertions(+), 19 deletions(-) diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index d5582760c8f6..5aa6f48ee20e 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -91,6 +91,7 @@ module; import java private import semmle.code.java.dataflow.DataFlow::DataFlow +private import semmle.code.java.controlflow.Guards private import FlowSummary as FlowSummary private import internal.DataFlowPrivate private import internal.FlowSummaryImpl @@ -183,6 +184,15 @@ predicate barrierModel( madId) } +/** Holds if a barrier guard model exists for the given parameters. */ +predicate barrierGuardModel( + string package, string type, boolean subtypes, string name, string signature, string ext, + string input, string acceptingvalue, string kind, string provenance, QlBuiltins::ExtensionId madId +) { + Extensions::barrierGuardModel(package, type, subtypes, name, signature, ext, input, + acceptingvalue, kind, provenance, madId) +} + /** Holds if a summary model exists for the given parameters. */ predicate summaryModel( string package, string type, boolean subtypes, string name, string signature, string ext, @@ -315,7 +325,8 @@ module ModelValidation { summaryModel(_, _, _, _, _, _, _, path, _, _, _) or sinkModel(_, _, _, _, _, _, path, _, _, _) or sourceModel(_, _, _, _, _, _, path, _, _, _) or - barrierModel(_, _, _, _, _, _, path, _, _, _) + barrierModel(_, _, _, _, _, _, path, _, _, _) or + barrierGuardModel(_, _, _, _, _, _, path, _, _, _, _) } private module MkAccessPath = AccessPathSyntax::AccessPath; @@ -328,6 +339,8 @@ module ModelValidation { exists(string pred, AccessPath input, AccessPathToken part | sinkModel(_, _, _, _, _, _, input, _, _, _) and pred = "sink" or + barrierGuardModel(_, _, _, _, _, _, input, _, _, _, _) and pred = "barrier guard" + or summaryModel(_, _, _, _, _, _, input, _, _, _, _) and pred = "summary" | ( @@ -373,6 +386,8 @@ module ModelValidation { sinkModel(_, _, _, _, _, _, _, kind, _, _) or barrierModel(_, _, _, _, _, _, _, kind, _, _) + or + barrierGuardModel(_, _, _, _, _, _, _, _, kind, _, _) } predicate sourceKind(string kind) { sourceModel(_, _, _, _, _, _, _, kind, _, _) } @@ -393,6 +408,9 @@ module ModelValidation { or barrierModel(package, type, _, name, signature, ext, _, _, provenance, _) and pred = "barrier" or + barrierGuardModel(package, type, _, name, signature, ext, _, _, _, provenance, _) and + pred = "barrier guard" + or summaryModel(package, type, _, name, signature, ext, _, _, _, provenance, _) and pred = "summary" or @@ -418,6 +436,14 @@ module ModelValidation { invalidProvenance(provenance) and result = "Unrecognized provenance description \"" + provenance + "\" in " + pred + " model." ) + or + exists(string acceptingvalue | + barrierGuardModel(_, _, _, _, _, _, _, acceptingvalue, _, _, _) and + invalidAcceptingValue(acceptingvalue) and + result = + "Unrecognized accepting value description \"" + acceptingvalue + + "\" in barrier guard model." + ) } /** Holds if some row in a MaD flow model appears to contain typos. */ @@ -440,6 +466,8 @@ private predicate elementSpec( or barrierModel(package, type, subtypes, name, signature, ext, _, _, _, _) or + barrierGuardModel(package, type, subtypes, name, signature, ext, _, _, _, _, _) + or summaryModel(package, type, subtypes, name, signature, ext, _, _, _, _, _) or neutralModel(package, type, name, signature, _, _) and ext = "" and subtypes = true @@ -601,13 +629,45 @@ private module Cached { ) } - // private predicate barrierGuardChecks() { - // exists( - // SourceSinkInterpretationInput::InterpretNode n, ArgumentNode arg, GuardAcceptingValue gv - // | - // isBarrierGuardNode(n, gv, kind, model) and n.asNode() = arg - // ) + private newtype TKindModelPair = + TMkPair(string kind, string model) { isBarrierGuardNode(_, _, kind, model) } + + private boolean convertAcceptingValue(AcceptingValue av) { + av.isTrue() and result = true + or + av.isFalse() and result = false + } + + // private GuardValue convertAcceptingValue(AcceptingValue av) { + // av.isTrue() and result.asBooleanValue() = true + // or + // av.isFalse() and result.asBooleanValue() = false + // or + // av.isNoException() and result.getDualValue().isThrowsException() + // or + // av.isZero() and result.asIntValue() = 0 + // or + // av.isNotZero() and result.getDualValue().asIntValue() = 0 + // or + // av.isNull() and result.isNullValue() + // or + // av.isNotNull() and result.isNonNullValue() // } + // private predicate barrierGuardChecks(Guard g, Expr e, GuardValue gv, TKindModelPair kmp) { + private predicate barrierGuardChecks(Guard g, Expr e, boolean branch, TKindModelPair kmp) { + exists( + SourceSinkInterpretationInput::InterpretNode n, AcceptingValue acceptingvalue, string kind, + string model + | + isBarrierGuardNode(n, acceptingvalue, kind, model) and + n.asNode().asExpr() = e and + kmp = TMkPair(kind, model) and + g.(Call).getAnArgument() = e and + // gv = convertAcceptingValue(acceptingvalue) + branch = convertAcceptingValue(acceptingvalue) + ) + } + /** * Holds if `node` is specified as a barrier with the given kind in a MaD flow * model. @@ -617,8 +677,9 @@ private module Cached { exists(SourceSinkInterpretationInput::InterpretNode n | isBarrierNode(n, kind, model) and n.asNode() = node ) - // or - // BarrierGuard::getABarrierNode() = node + or + ParameterizedBarrierGuard::getABarrierNode(TMkPair(kind, + model)) = node } } diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll index 00e7d15ee8b8..4ace6d2a12b0 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll @@ -396,3 +396,32 @@ module BarrierGuard { SsaImpl::DataFlowIntegration::BarrierGuard::getABarrierNode() } } + +bindingset[this] +signature class ParamSig; + +module WithParam { + /** + * Holds if the guard `g` validates the expression `e` upon evaluating to `gv`. + * + * The expression `e` is expected to be a syntactic part of the guard `g`. + * For example, the guard `g` might be a call `isSafe(x)` and the expression `e` + * the argument `x`. + */ + // signature predicate guardChecksSig(Guard g, Expr e, GuardValue gv, P param); + signature predicate guardChecksSig(Guard g, Expr e, boolean branch, P param); +} + +/** + * Provides a set of barrier nodes for a guard that validates an expression. + * + * This is expected to be used in `isBarrier`/`isSanitizer` definitions + * in data flow and taint tracking. + */ +module ParameterizedBarrierGuard::guardChecksSig/4 guardChecks> { + /** Gets a node that is safely guarded by the given guard check. */ + Node getABarrierNode(P param) { + SsaFlow::asNode(result) = + SsaImpl::DataFlowIntegration::ParameterizedBarrierGuard::getABarrierNode(param) + } +} diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll b/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll index 8b3d91017f6b..5386d916e240 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/ExternalFlowExtensions.qll @@ -28,6 +28,14 @@ extensible predicate barrierModel( string output, string kind, string provenance, QlBuiltins::ExtensionId madId ); +/** + * Holds if a barrier guard model exists for the given parameters. + */ +extensible predicate barrierGuardModel( + string package, string type, boolean subtypes, string name, string signature, string ext, + string input, string acceptingvalue, string kind, string provenance, QlBuiltins::ExtensionId madId +); + /** * Holds if a summary model exists for the given parameters. */ diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll index b3e0dda2b3d2..19d9a33328b8 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll @@ -159,7 +159,8 @@ private predicate relatedArgSpec(Callable c, string spec) { summaryModel(namespace, type, subtypes, name, signature, ext, _, spec, _, _, _) or sourceModel(namespace, type, subtypes, name, signature, ext, spec, _, _, _) or sinkModel(namespace, type, subtypes, name, signature, ext, spec, _, _, _) or - barrierModel(namespace, type, subtypes, name, signature, ext, spec, _, _, _) + barrierModel(namespace, type, subtypes, name, signature, ext, spec, _, _, _) or + barrierGuardModel(namespace, type, subtypes, name, signature, ext, spec, _, _, _, _) | c = interpretElement(namespace, type, subtypes, name, signature, ext, _) ) @@ -267,8 +268,28 @@ module SourceSinkInterpretationInput implements string namespace, string type, boolean subtypes, string name, string signature, string ext, SourceOrSinkElement baseBarrier, string originalOutput, QlBuiltins::ExtensionId madId | - barrierModel(namespace, type, subtypes, name, signature, ext, originalOutput, kind, provenance, - madId) and + barrierModel(namespace, type, subtypes, name, signature, ext, originalOutput, kind, + provenance, madId) and + model = "MaD:" + madId.toString() and + baseBarrier = interpretElement(namespace, type, subtypes, name, signature, ext, _) and + ( + e = baseBarrier and output = originalOutput + or + correspondingKotlinParameterDefaultsArgSpec(baseBarrier, e, originalOutput, output) + ) + ) + } + + predicate barrierGuardElement( + Element e, string output, Public::AcceptingValue acceptingvalue, string kind, + Public::Provenance provenance, string model + ) { + exists( + string namespace, string type, boolean subtypes, string name, string signature, string ext, + SourceOrSinkElement baseBarrier, string originalOutput, QlBuiltins::ExtensionId madId + | + barrierGuardModel(namespace, type, subtypes, name, signature, ext, originalOutput, + acceptingvalue, kind, provenance, madId) and model = "MaD:" + madId.toString() and baseBarrier = interpretElement(namespace, type, subtypes, name, signature, ext, _) and ( diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll b/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll index 624f82fd341d..954af3dd3440 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll @@ -586,6 +586,37 @@ private module Cached { predicate getABarrierNode = getABarrierNodeImpl/0; } + + bindingset[this] + signature class ParamSig; + + cached // nothing is actually cached + module WithParam { + // signature predicate guardChecksSig(Guards::Guard g, Expr e, Guards::GuardValue gv, P param); + signature predicate guardChecksSig(Guards::Guard g, Expr e, boolean branch, P param); + } + cached // nothing is actually cached + module ParameterizedBarrierGuard::guardChecksSig/4 guardChecks> { + // private predicate guardChecksAdjTypes(Guards::Guards_v3::Guard g, Expr e, Guards::GuardValue gv, P param) { + // guardChecks(g, e, gv, param) + // } + private predicate guardChecksAdjTypes(Guards::Guards_v3::Guard g, Expr e, boolean branch, P param) { + guardChecks(g, e, branch, param) + } + + private predicate guardChecksWithWrappers( + DataFlowIntegrationInput::Guard g, Definition def, Guards::GuardValue val, P param + ) { + Guards::Guards_v3::ValidationWrapperWithState::guardChecksDef(g, def, val, param) + } + + private Node getABarrierNodeImpl(P param) { + result = + DataFlowIntegrationImpl::BarrierGuardDefWithState::getABarrierNode(param) + } + + predicate getABarrierNode = getABarrierNodeImpl/1; + } } cached diff --git a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll index 206e846b88cb..28b007fcb9cc 100644 --- a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll @@ -215,6 +215,35 @@ module Make< ] } + class AcceptingValue extends string { + AcceptingValue() { + this = + [ + "true", + "false", + "no-exception", + "zero", + "not-zero", + "null", + "not-null", + ] + } + + predicate isTrue() { this = "true" } + + predicate isFalse() { this = "false" } + + predicate isNoException() { this = "no-exception" } + + predicate isZero() { this = "zero" } + + predicate isNotZero() { this = "not-zero" } + + predicate isNull() { this = "null" } + + predicate isNotNull() { this = "not-null" } + } + /** * A class used to represent provenance values for MaD models. * @@ -2015,6 +2044,12 @@ module Make< not exists(interpretComponent(c)) } + /** Holds if `acceptingvalue` is not a valid barrier guard accepting-value. */ + bindingset[acceptingvalue] + predicate invalidAcceptingValue(string acceptingvalue) { + not acceptingvalue instanceof AcceptingValue + } + /** Holds if `provenance` is not a valid provenance value. */ bindingset[provenance] predicate invalidProvenance(string provenance) { not provenance instanceof Provenance } @@ -2060,6 +2095,15 @@ module Make< Element n, string output, string kind, Provenance provenance, string model ); + /** + * Holds if an external barrier guard specification exists for `n` with input + * specification `input`, accepting value `acceptingvalue`, and kind `kind`. + */ + predicate barrierGuardElement( + Element n, string input, AcceptingValue acceptingvalue, string kind, + Provenance provenance, string model + ); + class SourceOrSinkElement extends Element; /** An entity used to interpret a source/sink specification. */ @@ -2114,7 +2158,8 @@ module Make< private predicate sourceSinkSpec(string spec) { sourceElement(_, spec, _, _, _) or sinkElement(_, spec, _, _, _) or - barrierElement(_, spec, _, _, _) + barrierElement(_, spec, _, _, _) or + barrierGuardElement(_, spec, _, _, _, _) } private module AccessPath = AccessPathSyntax::AccessPath; @@ -2180,6 +2225,18 @@ module Make< ) } + private predicate barrierGuardElementRef( + InterpretNode ref, SourceSinkAccessPath input, AcceptingValue acceptingvalue, string kind, + string model + ) { + exists(SourceOrSinkElement e | + barrierGuardElement(e, input, acceptingvalue, kind, _, model) and + if inputNeedsReferenceExt(input.getToken(0)) + then e = ref.getCallTarget() + else e = ref.asElement() + ) + } + /** Holds if the first `n` tokens of `output` resolve to the given interpretation. */ private predicate interpretOutput( SourceSinkAccessPath output, int n, InterpretNode ref, InterpretNode node @@ -2311,12 +2368,19 @@ module Make< ) } - // predicate isBarrierGuardNode(InterpretNode node, GuardAcceptingValue gv, string kind, string model) { - // exists(InterpretNode ref, SourceSinkAccessPath input | - // barrierGuardElementRef(ref, input, kind, model) and - // interpretInput(input, input.getNumToken(), ref, node) - // ) - // } + /** + * Holds if `node` is specified as a barrier guard argument with the + * given kind in a MaD flow model. + */ + predicate isBarrierGuardNode( + InterpretNode node, AcceptingValue acceptingvalue, string kind, string model + ) { + exists(InterpretNode ref, SourceSinkAccessPath input | + barrierGuardElementRef(ref, input, acceptingvalue, kind, model) and + interpretInput(input, input.getNumToken(), ref, node) + ) + } + final private class SourceOrSinkElementFinal = SourceOrSinkElement; signature predicate sourceOrSinkElementSig( From b92d0b4cacf96c85b934ee2b0fe816d986d0bb16 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 9 Dec 2025 12:52:38 +0100 Subject: [PATCH 3/8] Java: Add empty extensible data. --- java/ql/lib/ext/empty.model.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/java/ql/lib/ext/empty.model.yml b/java/ql/lib/ext/empty.model.yml index 43028e7cc14e..a0ea3cf753ba 100644 --- a/java/ql/lib/ext/empty.model.yml +++ b/java/ql/lib/ext/empty.model.yml @@ -13,6 +13,14 @@ extensions: pack: codeql/java-all extensible: summaryModel data: [] + - addsTo: + pack: codeql/java-all + extensible: barrierModel + data: [] + - addsTo: + pack: codeql/java-all + extensible: barrierGuardModel + data: [] - addsTo: pack: codeql/java-all extensible: neutralModel From d244d06fcc3ba295e291c7549b11b96b5f31f409 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 9 Dec 2025 12:24:43 +0000 Subject: [PATCH 4/8] Convert trust boundary violation barrier and barrier guard to MaD --- java/ql/lib/ext/org.owasp.esapi.model.yml | 38 ++++++++++++++++- .../code/java/frameworks/owasp/Esapi.qll | 42 ------------------- .../security/TrustBoundaryViolationQuery.qll | 22 +--------- 3 files changed, 39 insertions(+), 63 deletions(-) delete mode 100644 java/ql/lib/semmle/code/java/frameworks/owasp/Esapi.qll diff --git a/java/ql/lib/ext/org.owasp.esapi.model.yml b/java/ql/lib/ext/org.owasp.esapi.model.yml index 30578debe580..70890d7e03b3 100644 --- a/java/ql/lib/ext/org.owasp.esapi.model.yml +++ b/java/ql/lib/ext/org.owasp.esapi.model.yml @@ -1,6 +1,42 @@ extensions: + - addsTo: + pack: codeql/java-all + extensible: barrierGuardModel + data: + - ["org.owasp.esapi", "Validator", true, "isValidCreditCard", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidDate", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidDirectoryPath", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidDouble", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidFileContent", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidFileName", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidInput", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidInteger", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidListItem", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidNumber", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidPrintable", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidRedirectLocation", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidSafeHTML", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "isValidURI", "", "", "Argument[1]", "true", "trust-boundary-violation", "manual"] + - addsTo: + pack: codeql/java-all + extensible: barrierModel + data: + - ["org.owasp.esapi", "Validator", true, "getValidCreditCard", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidDate", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidDirectoryPath", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidDouble", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidFileContent", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidFileName", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidInput", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidInteger", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidListItem", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidNumber", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidPrintable", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidRedirectLocation", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidSafeHTML", "", "", "ReturnValue", "trust-boundary-violation", "manual"] + - ["org.owasp.esapi", "Validator", true, "getValidURI", "", "", "ReturnValue", "trust-boundary-violation", "manual"] - addsTo: pack: codeql/java-all extensible: summaryModel data: - - ["org.owasp.esapi", "Encoder", true, "encodeForHTML", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"] \ No newline at end of file + - ["org.owasp.esapi", "Encoder", true, "encodeForHTML", "(String)", "", "Argument[0]", "ReturnValue", "taint", "manual"] diff --git a/java/ql/lib/semmle/code/java/frameworks/owasp/Esapi.qll b/java/ql/lib/semmle/code/java/frameworks/owasp/Esapi.qll deleted file mode 100644 index fe95cd0d39d3..000000000000 --- a/java/ql/lib/semmle/code/java/frameworks/owasp/Esapi.qll +++ /dev/null @@ -1,42 +0,0 @@ -/** Classes and predicates for reasoning about the `owasp.easpi` package. */ -overlay[local?] -module; - -import java - -/** - * The `org.owasp.esapi.Validator` interface. - */ -class EsapiValidator extends RefType { - EsapiValidator() { this.hasQualifiedName("org.owasp.esapi", "Validator") } -} - -/** - * The methods of `org.owasp.esapi.Validator` which validate data. - */ -class EsapiIsValidMethod extends Method { - EsapiIsValidMethod() { - this.getDeclaringType() instanceof EsapiValidator and - this.hasName([ - "isValidCreditCard", "isValidDate", "isValidDirectoryPath", "isValidDouble", - "isValidFileContent", "isValidFileName", "isValidInput", "isValidInteger", - "isValidListItem", "isValidNumber", "isValidPrintable", "isValidRedirectLocation", - "isValidSafeHTML", "isValidURI" - ]) - } -} - -/** - * The methods of `org.owasp.esapi.Validator` which return validated data. - */ -class EsapiGetValidMethod extends Method { - EsapiGetValidMethod() { - this.getDeclaringType() instanceof EsapiValidator and - this.hasName([ - "getValidCreditCard", "getValidDate", "getValidDirectoryPath", "getValidDouble", - "getValidFileContent", "getValidFileName", "getValidInput", "getValidInteger", - "getValidListItem", "getValidNumber", "getValidPrintable", "getValidRedirectLocation", - "getValidSafeHTML", "getValidURI" - ]) - } -} diff --git a/java/ql/lib/semmle/code/java/security/TrustBoundaryViolationQuery.qll b/java/ql/lib/semmle/code/java/security/TrustBoundaryViolationQuery.qll index b2f49834b5ab..477aeb48b64e 100644 --- a/java/ql/lib/semmle/code/java/security/TrustBoundaryViolationQuery.qll +++ b/java/ql/lib/semmle/code/java/security/TrustBoundaryViolationQuery.qll @@ -5,7 +5,6 @@ private import semmle.code.java.dataflow.DataFlow private import semmle.code.java.controlflow.Guards private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSources -private import semmle.code.java.frameworks.owasp.Esapi private import semmle.code.java.security.Sanitizers /** @@ -28,25 +27,8 @@ class TrustBoundaryViolationSink extends DataFlow::Node { */ abstract class TrustBoundaryValidationSanitizer extends DataFlow::Node { } -/** - * A node validated by an OWASP ESAPI validation method. - */ -private class EsapiValidatedInputSanitizer extends TrustBoundaryValidationSanitizer { - EsapiValidatedInputSanitizer() { - this = DataFlow::BarrierGuard::getABarrierNode() or - this.asExpr().(MethodCall).getMethod() instanceof EsapiGetValidMethod - } -} - -/** - * Holds if `g` is a guard that checks that `e` is valid data according to an OWASP ESAPI validation method. - */ -private predicate esapiIsValidData(Guard g, Expr e, boolean branch) { - branch = true and - exists(MethodCall ma | ma.getMethod() instanceof EsapiIsValidMethod | - g = ma and - e = ma.getArgument(1) - ) +private class DefaultTrustBoundaryValidationSanitizer extends TrustBoundaryValidationSanitizer { + DefaultTrustBoundaryValidationSanitizer() { barrierNode(this, "trust-boundary-violation") } } /** From afc0116537792ac192004decb9f7918758792342 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Tue, 9 Dec 2025 12:55:04 +0000 Subject: [PATCH 5/8] Convert path injection barrier to MaD --- java/ql/lib/ext/java.io.model.yml | 6 ++++++ .../semmle/code/java/security/PathSanitizer.qll | 16 +++------------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/java/ql/lib/ext/java.io.model.yml b/java/ql/lib/ext/java.io.model.yml index 3582e2b78ac6..88dbd45dd2c7 100644 --- a/java/ql/lib/ext/java.io.model.yml +++ b/java/ql/lib/ext/java.io.model.yml @@ -162,3 +162,9 @@ extensions: extensible: sourceModel data: - ["java.io", "FileInputStream", True, "FileInputStream", "", "", "Argument[this]", "file", "manual"] +extensions: + - addsTo: + pack: codeql/java-all + extensible: barrierModel + data: + - ["java.io", "File", True, "getName", "()", "", "ReturnValue", "path-injection", "manual"] diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll index da6f242bde52..2018004a3fb5 100644 --- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll +++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll @@ -4,6 +4,7 @@ module; import java private import semmle.code.java.controlflow.Guards +private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSources private import semmle.code.java.dataflow.SSA private import semmle.code.java.frameworks.kotlin.IO @@ -288,19 +289,8 @@ private Method getSourceMethod(Method m) { result = m } -/** - * A sanitizer that protects against path injection vulnerabilities - * by extracting the final component of the user provided path. - * - * TODO: convert this class to models-as-data if sanitizer support is added - */ -private class FileGetNameSanitizer extends PathInjectionSanitizer { - FileGetNameSanitizer() { - exists(MethodCall mc | - mc.getMethod().hasQualifiedName("java.io", "File", "getName") and - this.asExpr() = mc - ) - } +private class DefaultPathInjectionSanitizer extends PathInjectionSanitizer { + DefaultPathInjectionSanitizer() { barrierNode(this, "path-injection") } } /** Holds if `g` is a guard that checks for `..` components. */ From 7e6d189337f98df352f155c3aaebe11ae2e38729 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 9 Dec 2025 13:59:01 +0100 Subject: [PATCH 6/8] bugfix --- shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll index 28b007fcb9cc..4ab2eb1650c6 100644 --- a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll @@ -2241,7 +2241,7 @@ module Make< private predicate interpretOutput( SourceSinkAccessPath output, int n, InterpretNode ref, InterpretNode node ) { - sourceElementRef(ref, output, _, _) and + (sourceElementRef(ref, output, _, _) or barrierElementRef(ref, output, _, _)) and n = 0 and ( if output = "" @@ -2297,7 +2297,7 @@ module Make< private predicate interpretInput( SourceSinkAccessPath input, int n, InterpretNode ref, InterpretNode node ) { - sinkElementRef(ref, input, _, _) and + (sinkElementRef(ref, input, _, _) or barrierGuardElementRef(ref, input, _, _, _)) and n = 0 and ( if input = "" From cb83fd9491f3c8e5c18221a9f619259cc3096c05 Mon Sep 17 00:00:00 2001 From: yoff Date: Wed, 10 Dec 2025 01:37:39 +0100 Subject: [PATCH 7/8] python: remove barrier that can be expressed in MaD --- .../lib/semmle/python/frameworks/Django.qll | 32 ------------------- .../CWE-601-UrlRedirect/UrlRedirect.expected | 3 ++ 2 files changed, 3 insertions(+), 32 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/Django.qll b/python/ql/lib/semmle/python/frameworks/Django.qll index 4aa5776ad54b..ee0ed4a84dd0 100644 --- a/python/ql/lib/semmle/python/frameworks/Django.qll +++ b/python/ql/lib/semmle/python/frameworks/Django.qll @@ -2965,38 +2965,6 @@ module PrivateDjango { override predicate csrfEnabled() { decoratorName in ["csrf_protect", "requires_csrf_token"] } } - private predicate djangoUrlHasAllowedHostAndScheme( - DataFlow::GuardNode g, ControlFlowNode node, boolean branch - ) { - exists(API::CallNode call | - call = - API::moduleImport("django") - .getMember("utils") - .getMember("http") - .getMember("url_has_allowed_host_and_scheme") - .getACall() and - g = call.asCfgNode() and - node = call.getParameter(0, "url").asSink().asCfgNode() and - branch = true - ) - } - - /** - * A call to `django.utils.http.url_has_allowed_host_and_scheme`, considered as a sanitizer-guard for URL redirection. - * - * See https://docs.djangoproject.com/en/4.2/_modules/django/utils/http/ - */ - private class DjangoAllowedUrl extends UrlRedirect::Sanitizer { - DjangoAllowedUrl() { - this = DataFlow::BarrierGuard::getABarrierNode() - } - - override predicate sanitizes(UrlRedirect::FlowState state) { - // sanitize all flow states - any() - } - } - // --------------------------------------------------------------------------- // Templates // --------------------------------------------------------------------------- diff --git a/python/ql/test/query-tests/Security/CWE-601-UrlRedirect/UrlRedirect.expected b/python/ql/test/query-tests/Security/CWE-601-UrlRedirect/UrlRedirect.expected index 551299a64dc4..d7c891b46349 100644 --- a/python/ql/test/query-tests/Security/CWE-601-UrlRedirect/UrlRedirect.expected +++ b/python/ql/test/query-tests/Security/CWE-601-UrlRedirect/UrlRedirect.expected @@ -52,6 +52,7 @@ edges | test.py:81:17:81:46 | ControlFlowNode for Attribute() | test.py:81:5:81:13 | ControlFlowNode for untrusted | provenance | | | test.py:82:5:82:10 | ControlFlowNode for unsafe | test.py:83:21:83:26 | ControlFlowNode for unsafe | provenance | | | test.py:90:5:90:13 | ControlFlowNode for untrusted | test.py:93:18:93:26 | ControlFlowNode for untrusted | provenance | | +| test.py:90:5:90:13 | ControlFlowNode for untrusted | test.py:95:25:95:33 | ControlFlowNode for untrusted | provenance | | | test.py:90:17:90:23 | ControlFlowNode for request | test.py:90:17:90:28 | ControlFlowNode for Attribute | provenance | AdditionalTaintStep | | test.py:90:17:90:28 | ControlFlowNode for Attribute | test.py:90:17:90:46 | ControlFlowNode for Attribute() | provenance | dict.get | | test.py:90:17:90:46 | ControlFlowNode for Attribute() | test.py:90:5:90:13 | ControlFlowNode for untrusted | provenance | | @@ -122,6 +123,7 @@ nodes | test.py:90:17:90:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | | test.py:90:17:90:46 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | test.py:93:18:93:26 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted | +| test.py:95:25:95:33 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted | | test.py:111:5:111:13 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted | | test.py:111:17:111:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | | test.py:111:17:111:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | @@ -148,6 +150,7 @@ subpaths | test.py:76:21:76:26 | ControlFlowNode for unsafe | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:76:21:76:26 | ControlFlowNode for unsafe | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | | test.py:83:21:83:26 | ControlFlowNode for unsafe | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:83:21:83:26 | ControlFlowNode for unsafe | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | | test.py:93:18:93:26 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:93:18:93:26 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | +| test.py:95:25:95:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:95:25:95:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | | test.py:114:25:114:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:114:25:114:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | | test.py:140:25:140:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:140:25:140:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | | test.py:148:25:148:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:148:25:148:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | From 08e719a37cdcb90071d9b5e3c0317d4ecc558a06 Mon Sep 17 00:00:00 2001 From: yoff Date: Wed, 10 Dec 2025 01:43:14 +0100 Subject: [PATCH 8/8] python: add machinery for MaD barriers and reinstate previously removed barrier now as a MaD row --- .../semmle/python/frameworks/Django.model.yml | 6 ++ .../data/internal/ApiGraphModels.qll | 66 ++++++++++++++++++- .../internal/ApiGraphModelsExtensions.qll | 20 ++++++ .../frameworks/data/internal/empty.model.yml | 10 +++ .../dataflow/UrlRedirectCustomizations.qll | 20 ++++++ .../CWE-601-UrlRedirect/UrlRedirect.expected | 3 - 6 files changed, 120 insertions(+), 5 deletions(-) create mode 100644 python/ql/lib/semmle/python/frameworks/Django.model.yml diff --git a/python/ql/lib/semmle/python/frameworks/Django.model.yml b/python/ql/lib/semmle/python/frameworks/Django.model.yml new file mode 100644 index 000000000000..4e472af6c117 --- /dev/null +++ b/python/ql/lib/semmle/python/frameworks/Django.model.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/python-all + extensible: barrierGuardModel + data: + - ['django', 'Member[utils].Member[http].Member[url_has_allowed_host_and_scheme].Argument[0,url:]', "true", 'url-redirection'] diff --git a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll index 80ec45a3cf17..ee2cd8f57bfe 100644 --- a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll +++ b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll @@ -344,6 +344,26 @@ private predicate sinkModel(string type, string path, string kind, string model) ) } +/** Holds if a barrier model exists for the given parameters. */ +private predicate barrierModel(string type, string path, string kind, string model) { + // No deprecation adapter for barrier models, they were not around back then. + exists(QlBuiltins::ExtensionId madId | + Extensions::barrierModel(type, path, kind, madId) and + model = "MaD:" + madId.toString() + ) +} + +/** Holds if a barrier guard model exists for the given parameters. */ +private predicate barrierGuardModel( + string type, string path, string branch, string kind, string model +) { + // No deprecation adapter for barrier models, they were not around back then. + exists(QlBuiltins::ExtensionId madId | + Extensions::barrierGuardModel(type, path, branch, kind, madId) and + model = "MaD:" + madId.toString() + ) +} + /** Holds if a summary model `row` exists for the given parameters. */ private predicate summaryModel( string type, string path, string input, string output, string kind, string model @@ -400,6 +420,8 @@ predicate isRelevantType(string type) { ( sourceModel(type, _, _, _) or sinkModel(type, _, _, _) or + barrierModel(type, _, _, _) or + barrierGuardModel(type, _, _, _, _) or summaryModel(type, _, _, _, _, _) or typeModel(_, type, _) ) and @@ -427,6 +449,8 @@ predicate isRelevantFullPath(string type, string path) { ( sourceModel(type, path, _, _) or sinkModel(type, path, _, _) or + barrierModel(type, path, _, _) or + barrierGuardModel(type, path, _, _, _) or summaryModel(type, path, _, _, _, _) or typeModel(_, type, path) ) @@ -745,6 +769,32 @@ module ModelOutput { ) } + /** + * Holds if a barrier model contributed `barrier` with the given `kind`. + */ + cached + API::Node getABarrierNode(string kind, string model) { + exists(string type, string path | + barrierModel(type, path, kind, model) and + result = getNodeFromPath(type, path) + ) + } + + /** + * Holds if a barrier model contributed `barrier` with the given `kind`. + */ + cached + API::Node getABarrierGuardNode(string kind, boolean branch, string model) { + exists(string type, string path, string branch_str | + branch = true and branch_str = "true" + or + branch = false and branch_str = "false" + | + barrierGuardModel(type, path, branch_str, kind, model) and + result = getNodeFromPath(type, path) + ) + } + /** * Holds if a relevant summary exists for these parameters. */ @@ -787,15 +837,27 @@ module ModelOutput { private import codeql.mad.ModelValidation as SharedModelVal /** - * Holds if a CSV source model contributed `source` with the given `kind`. + * Holds if an external model contributed `source` with the given `kind`. */ API::Node getASourceNode(string kind) { result = getASourceNode(kind, _) } /** - * Holds if a CSV sink model contributed `sink` with the given `kind`. + * Holds if an external model contributed `sink` with the given `kind`. */ API::Node getASinkNode(string kind) { result = getASinkNode(kind, _) } + /** + * Holds if an external model contributed `barrier` with the given `kind`. + */ + API::Node getABarrierNode(string kind) { result = getABarrierNode(kind, _) } + + /** + * Holds if an external model contributed `barrier-guard` with the given `kind` and `branch`. + */ + API::Node getABarrierGuardNode(string kind, boolean branch) { + result = getABarrierGuardNode(kind, branch, _) + } + private module KindValConfig implements SharedModelVal::KindValidationConfigSig { predicate summaryKind(string kind) { summaryModel(_, _, _, _, kind, _) } diff --git a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModelsExtensions.qll b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModelsExtensions.qll index 3f38c498f324..2a644aabb95d 100644 --- a/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModelsExtensions.qll +++ b/python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModelsExtensions.qll @@ -20,6 +20,26 @@ extensible predicate sourceModel( */ extensible predicate sinkModel(string type, string path, string kind, QlBuiltins::ExtensionId madId); +/** + * Holds if the value at `(type, path)` should be seen as a barrier + * of the given `kind` and `madId` is the data extension row number. + */ +extensible predicate barrierModel( + string type, string path, string kind, QlBuiltins::ExtensionId madId +); + +/** + * Holds if the value at `(type, path)` should be seen as a barrier guard + * of the given `kind` and `madId` is the data extension row number. + * `path` is assumed to lead to a parameter of a call (possibly `self`), and + * the call is guarding the parameter. + * `branch` is either `true` or `false`, indicating which branch of the guard + * is protecting the parameter. + */ +extensible predicate barrierGuardModel( + string type, string path, string branch, string kind, QlBuiltins::ExtensionId madId +); + /** * Holds if in calls to `(type, path)`, the value referred to by `input` * can flow to the value referred to by `output` and `madId` is the data diff --git a/python/ql/lib/semmle/python/frameworks/data/internal/empty.model.yml b/python/ql/lib/semmle/python/frameworks/data/internal/empty.model.yml index ea9b9fce546b..a7529031c29f 100644 --- a/python/ql/lib/semmle/python/frameworks/data/internal/empty.model.yml +++ b/python/ql/lib/semmle/python/frameworks/data/internal/empty.model.yml @@ -11,6 +11,16 @@ extensions: extensible: sinkModel data: [] + - addsTo: + pack: codeql/python-all + extensible: barrierModel + data: [] + + - addsTo: + pack: codeql/python-all + extensible: barrierGuardModel + data: [] + - addsTo: pack: codeql/python-all extensible: summaryModel diff --git a/python/ql/lib/semmle/python/security/dataflow/UrlRedirectCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UrlRedirectCustomizations.qll index f5810944f8d9..2d92f6cb880f 100644 --- a/python/ql/lib/semmle/python/security/dataflow/UrlRedirectCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/UrlRedirectCustomizations.qll @@ -7,6 +7,7 @@ private import python private import semmle.python.dataflow.new.DataFlow private import semmle.python.Concepts +private import semmle.python.ApiGraphs private import semmle.python.dataflow.new.RemoteFlowSources private import semmle.python.dataflow.new.BarrierGuards private import semmle.python.frameworks.data.ModelsAsData @@ -156,4 +157,23 @@ module UrlRedirect { /** DEPRECATED: Use ConstCompareAsSanitizerGuard instead. */ deprecated class StringConstCompareAsSanitizerGuard = ConstCompareAsSanitizerGuard; + + private predicate urlCheck(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) { + exists(API::CallNode call, API::Node parameter | + parameter = call.getAParameter() and + parameter = ModelOutput::getABarrierGuardNode("url-redirection", branch) + | + g = call.asCfgNode() and + node = parameter.asSink().asCfgNode() + ) + } + + class SanitizerFromModel extends Sanitizer { + SanitizerFromModel() { this = DataFlow::BarrierGuard::getABarrierNode() } + + override predicate sanitizes(FlowState state) { + // sanitize all flow states + any() + } + } } diff --git a/python/ql/test/query-tests/Security/CWE-601-UrlRedirect/UrlRedirect.expected b/python/ql/test/query-tests/Security/CWE-601-UrlRedirect/UrlRedirect.expected index d7c891b46349..551299a64dc4 100644 --- a/python/ql/test/query-tests/Security/CWE-601-UrlRedirect/UrlRedirect.expected +++ b/python/ql/test/query-tests/Security/CWE-601-UrlRedirect/UrlRedirect.expected @@ -52,7 +52,6 @@ edges | test.py:81:17:81:46 | ControlFlowNode for Attribute() | test.py:81:5:81:13 | ControlFlowNode for untrusted | provenance | | | test.py:82:5:82:10 | ControlFlowNode for unsafe | test.py:83:21:83:26 | ControlFlowNode for unsafe | provenance | | | test.py:90:5:90:13 | ControlFlowNode for untrusted | test.py:93:18:93:26 | ControlFlowNode for untrusted | provenance | | -| test.py:90:5:90:13 | ControlFlowNode for untrusted | test.py:95:25:95:33 | ControlFlowNode for untrusted | provenance | | | test.py:90:17:90:23 | ControlFlowNode for request | test.py:90:17:90:28 | ControlFlowNode for Attribute | provenance | AdditionalTaintStep | | test.py:90:17:90:28 | ControlFlowNode for Attribute | test.py:90:17:90:46 | ControlFlowNode for Attribute() | provenance | dict.get | | test.py:90:17:90:46 | ControlFlowNode for Attribute() | test.py:90:5:90:13 | ControlFlowNode for untrusted | provenance | | @@ -123,7 +122,6 @@ nodes | test.py:90:17:90:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | | test.py:90:17:90:46 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | test.py:93:18:93:26 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted | -| test.py:95:25:95:33 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted | | test.py:111:5:111:13 | ControlFlowNode for untrusted | semmle.label | ControlFlowNode for untrusted | | test.py:111:17:111:23 | ControlFlowNode for request | semmle.label | ControlFlowNode for request | | test.py:111:17:111:28 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute | @@ -150,7 +148,6 @@ subpaths | test.py:76:21:76:26 | ControlFlowNode for unsafe | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:76:21:76:26 | ControlFlowNode for unsafe | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | | test.py:83:21:83:26 | ControlFlowNode for unsafe | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:83:21:83:26 | ControlFlowNode for unsafe | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | | test.py:93:18:93:26 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:93:18:93:26 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | -| test.py:95:25:95:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:95:25:95:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | | test.py:114:25:114:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:114:25:114:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | | test.py:140:25:140:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:140:25:140:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value | | test.py:148:25:148:33 | ControlFlowNode for untrusted | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:148:25:148:33 | ControlFlowNode for untrusted | Untrusted URL redirection depends on a $@. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |