diff --git a/cpp/ql/lib/Customizations.qll b/cpp/ql/lib/Customizations.qll new file mode 100644 index 00000000000..c9d899e07e0 --- /dev/null +++ b/cpp/ql/lib/Customizations.qll @@ -0,0 +1,11 @@ +/** + * Contains customizations to the standard library. + * + * This module is imported by `cpp.qll`, so any customizations defined here automatically + * apply to all queries. + * + * Typical examples of customizations include adding new subclasses of abstract classes such as + * the `RemoteFlowSource` class to model frameworks that are not covered by the standard library. + */ + +import cpp diff --git a/cpp/ql/lib/change-notes/2025-08-19-virtual-dispatch.md b/cpp/ql/lib/change-notes/2025-08-19-virtual-dispatch.md new file mode 100644 index 00000000000..4342bb7f62d --- /dev/null +++ b/cpp/ql/lib/change-notes/2025-08-19-virtual-dispatch.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The new dataflow/taint-tracking library (`semmle.code.cpp.dataflow.new.DataFlow` and `semmle.code.cpp.dataflow.new.TaintTracking`) now resolves virtual function calls more precisely. This results in fewer false positives when running dataflow/taint-tracking queries on C++ projects. \ No newline at end of file diff --git a/cpp/ql/lib/cpp.qll b/cpp/ql/lib/cpp.qll index ccd32c368e4..c8afac1c7ae 100644 --- a/cpp/ql/lib/cpp.qll +++ b/cpp/ql/lib/cpp.qll @@ -13,6 +13,7 @@ * https://github.com/cplusplus/draft/raw/master/papers/n4140.pdf */ +import Customizations import semmle.code.cpp.File import semmle.code.cpp.Linkage import semmle.code.cpp.Location diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll index b5e899bf0aa..0d63558c956 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll @@ -1,218 +1,21 @@ private import cpp private import semmle.code.cpp.ir.IR -private import DataFlowPrivate +private import semmle.code.cpp.ir.dataflow.DataFlow +private import DataFlowPrivate as DataFlowPrivate private import DataFlowUtil private import DataFlowImplCommon as DataFlowImplCommon +private import codeql.typetracking.TypeTracking +private import SsaImpl as SsaImpl /** - * Gets a function that might be called by `call`. - * - * This predicate does not take additional call targets - * from `AdditionalCallTarget` into account. + * Holds if `f` has name `qualifiedName` and `nparams` parameter count. This is + * an approximation of its signature for the purpose of matching functions that + * might be the same across link targets. */ -cached -DataFlowCallable defaultViableCallable(DataFlowCall call) { - DataFlowImplCommon::forceCachingInSameStage() and - result = call.getStaticCallTarget() - or - // If the target of the call does not have a body in the snapshot, it might - // be because the target is just a header declaration, and the real target - // will be determined at run time when the caller and callee are linked - // together by the operating system's dynamic linker. In case a _unique_ - // function with the right signature is present in the database, we return - // that as a potential callee. - exists(string qualifiedName, int nparams | - callSignatureWithoutBody(qualifiedName, nparams, call.asCallInstruction()) and - functionSignatureWithBody(qualifiedName, nparams, result.getUnderlyingCallable()) and - strictcount(Function other | functionSignatureWithBody(qualifiedName, nparams, other)) = 1 - ) - or - // Virtual dispatch - result.asSourceCallable() = call.(VirtualDispatch::DataSensitiveCall).resolve() -} - -/** - * Gets a function that might be called by `call`. - */ -cached -DataFlowCallable viableCallable(DataFlowCall call) { - result = defaultViableCallable(call) - or - // Additional call targets - result.getUnderlyingCallable() = - any(AdditionalCallTarget additional) - .viableTarget(call.asCallInstruction().getUnconvertedResultExpression()) -} - -/** - * Provides virtual dispatch support compatible with the original - * implementation of `semmle.code.cpp.security.TaintTracking`. - */ -private module VirtualDispatch { - /** A call that may dispatch differently depending on the qualifier value. */ - abstract class DataSensitiveCall extends DataFlowCall { - /** - * Gets the node whose value determines the target of this call. This node - * could be the qualifier of a virtual dispatch or the function-pointer - * expression in a call to a function pointer. What they have in common is - * that we need to find out which data flows there, and then it's up to the - * `resolve` predicate to stitch that information together and resolve the - * call. - */ - abstract Node getDispatchValue(); - - /** Gets a candidate target for this call. */ - abstract Function resolve(); - - /** - * Whether `src` can flow to this call. - * - * Searches backwards from `getDispatchValue()` to `src`. The `allowFromArg` - * parameter is true when the search is allowed to continue backwards into - * a parameter; non-recursive callers should pass `_` for `allowFromArg`. - */ - predicate flowsFrom(Node src, boolean allowFromArg) { - src = this.getDispatchValue() and allowFromArg = true - or - exists(Node other, boolean allowOtherFromArg | this.flowsFrom(other, allowOtherFromArg) | - // Call argument - exists(DataFlowCall call, Position i | - other.(ParameterNode).isParameterOf(pragma[only_bind_into](call).getStaticCallTarget(), i) and - src.(ArgumentNode).argumentOf(call, pragma[only_bind_into](pragma[only_bind_out](i))) - ) and - allowOtherFromArg = true and - allowFromArg = true - or - // Call return - exists(DataFlowCall call, ReturnKind returnKind | - other = getAnOutNode(call, returnKind) and - returnNodeWithKindAndEnclosingCallable(src, returnKind, call.getStaticCallTarget()) - ) and - allowFromArg = false - or - // Local flow - localFlowStep(src, other) and - allowFromArg = allowOtherFromArg - or - // Flow from global variable to load. - exists(LoadInstruction load, GlobalOrNamespaceVariable var | - var = src.asVariable() and - other.asInstruction() = load and - addressOfGlobal(load.getSourceAddress(), var) and - // The `allowFromArg` concept doesn't play a role when `src` is a - // global variable, so we just set it to a single arbitrary value for - // performance. - allowFromArg = true - ) - or - // Flow from store to global variable. - exists(StoreInstruction store, GlobalOrNamespaceVariable var | - var = other.asVariable() and - store = src.asInstruction() and - storeIntoGlobal(store, var) and - // Setting `allowFromArg` to `true` like in the base case means we - // treat a store to a global variable like the dispatch itself: flow - // may come from anywhere. - allowFromArg = true - ) - ) - } - } - - pragma[noinline] - private predicate storeIntoGlobal(StoreInstruction store, GlobalOrNamespaceVariable var) { - addressOfGlobal(store.getDestinationAddress(), var) - } - - /** Holds if `addressInstr` is an instruction that produces the address of `var`. */ - private predicate addressOfGlobal(Instruction addressInstr, GlobalOrNamespaceVariable var) { - // Access directly to the global variable - addressInstr.(VariableAddressInstruction).getAstVariable() = var - or - // Access to a field on a global union - exists(FieldAddressInstruction fa | - fa = addressInstr and - fa.getObjectAddress().(VariableAddressInstruction).getAstVariable() = var and - fa.getField().getDeclaringType() instanceof Union - ) - } - - /** - * A ReturnNode with its ReturnKind and its enclosing callable. - * - * Used to fix a join ordering issue in flowsFrom. - */ - pragma[noinline] - private predicate returnNodeWithKindAndEnclosingCallable( - ReturnNode node, ReturnKind kind, DataFlowCallable callable - ) { - node.getKind() = kind and - node.getFunction() = callable.getUnderlyingCallable() - } - - /** Call through a function pointer. */ - private class DataSensitiveExprCall extends DataSensitiveCall { - DataSensitiveExprCall() { not exists(this.getStaticCallTarget()) } - - override Node getDispatchValue() { result.asOperand() = this.getCallTargetOperand() } - - override Function resolve() { - exists(FunctionInstruction fi | - this.flowsFrom(instructionNode(fi), _) and - result = fi.getFunctionSymbol() - ) and - ( - this.getNumberOfArguments() <= result.getEffectiveNumberOfParameters() and - this.getNumberOfArguments() >= result.getEffectiveNumberOfParameters() - or - result.isVarargs() - ) - } - } - - /** Call to a virtual function. */ - private class DataSensitiveOverriddenFunctionCall extends DataSensitiveCall { - DataSensitiveOverriddenFunctionCall() { - exists( - this.getStaticCallTarget() - .getUnderlyingCallable() - .(VirtualFunction) - .getAnOverridingFunction() - ) - } - - override Node getDispatchValue() { result.asInstruction() = this.getArgument(-1) } - - override MemberFunction resolve() { - exists(Class overridingClass | - this.overrideMayAffectCall(overridingClass, result) and - this.hasFlowFromCastFrom(overridingClass) - ) - } - - /** - * Holds if `this` is a virtual function call whose static target is - * overridden by `overridingFunction` in `overridingClass`. - */ - pragma[noinline] - private predicate overrideMayAffectCall(Class overridingClass, MemberFunction overridingFunction) { - overridingFunction.getAnOverriddenFunction+() = - this.getStaticCallTarget().getUnderlyingCallable().(VirtualFunction) and - overridingFunction.getDeclaringType() = overridingClass - } - - /** - * Holds if the qualifier of `this` has flow from an upcast from - * `derivedClass`. - */ - pragma[noinline] - private predicate hasFlowFromCastFrom(Class derivedClass) { - exists(ConvertToBaseInstruction toBase | - this.flowsFrom(instructionNode(toBase), _) and - derivedClass = toBase.getDerivedClass() - ) - } - } +private predicate functionSignature(Function f, string qualifiedName, int nparams) { + qualifiedName = f.getQualifiedName() and + nparams = f.getNumberOfParameters() and + not f.isStatic() } /** @@ -238,34 +41,319 @@ private predicate callSignatureWithoutBody(string qualifiedName, int nparams, Ca } /** - * Holds if `f` has name `qualifiedName` and `nparams` parameter count. This is - * an approximation of its signature for the purpose of matching functions that - * might be the same across link targets. + * Gets a function that might be called by `call`. + * + * This predicate does not take additional call targets + * from `AdditionalCallTarget` into account. */ -private predicate functionSignature(Function f, string qualifiedName, int nparams) { - qualifiedName = f.getQualifiedName() and - nparams = f.getNumberOfParameters() and - not f.isStatic() +cached +DataFlowPrivate::DataFlowCallable defaultViableCallable(DataFlowPrivate::DataFlowCall call) { + result = defaultViableCallableWithoutLambda(call) + or + result = DataFlowImplCommon::viableCallableLambda(call, _) +} + +private DataFlowPrivate::DataFlowCallable defaultViableCallableWithoutLambda( + DataFlowPrivate::DataFlowCall call +) { + DataFlowImplCommon::forceCachingInSameStage() and + result = call.getStaticCallTarget() + or + // If the target of the call does not have a body in the snapshot, it might + // be because the target is just a header declaration, and the real target + // will be determined at run time when the caller and callee are linked + // together by the operating system's dynamic linker. In case a _unique_ + // function with the right signature is present in the database, we return + // that as a potential callee. + exists(string qualifiedName, int nparams | + callSignatureWithoutBody(qualifiedName, nparams, call.asCallInstruction()) and + functionSignatureWithBody(qualifiedName, nparams, result.getUnderlyingCallable()) and + strictcount(Function other | functionSignatureWithBody(qualifiedName, nparams, other)) = 1 + ) +} + +/** + * Gets a function that might be called by `call`. + */ +private DataFlowPrivate::DataFlowCallable nonVirtualDispatch(DataFlowPrivate::DataFlowCall call) { + result = defaultViableCallableWithoutLambda(call) + or + // Additional call targets + result.getUnderlyingCallable() = + any(AdditionalCallTarget additional) + .viableTarget(call.asCallInstruction().getUnconvertedResultExpression()) +} + +private class RelevantNode extends Node { + RelevantNode() { this.getType().stripType() instanceof Class } +} + +private signature DataFlowPrivate::DataFlowCallable methodDispatchSig( + DataFlowPrivate::DataFlowCall c +); + +private predicate ignoreConstructor(Expr e) { + e instanceof ConstructorDirectInit or + e instanceof ConstructorVirtualInit or + e instanceof ConstructorDelegationInit or + exists(ConstructorFieldInit init | init.getExpr() = e) +} + +/** + * Holds if `n` is either: + * - the post-update node of a qualifier after a call to a constructor which + * constructs an object containing at least one virtual function. + * - a node which represents a derived-to-base instruction that converts from `c`. + */ +private predicate qualifierSourceImpl(RelevantNode n, Class c) { + // Object construction + exists(CallInstruction call, ThisArgumentOperand qualifier, Call e | + qualifier = call.getThisArgumentOperand() and + n.(PostUpdateNode).getPreUpdateNode().asOperand() = qualifier and + call.getStaticCallTarget() instanceof Constructor and + qualifier.getType().stripType() = c and + c.getABaseClass*().getAMemberFunction().isVirtual() and + e = call.getUnconvertedResultExpression() and + not ignoreConstructor(e) + | + exists(c.getABaseClass()) + or + exists(c.getADerivedClass()) + ) + or + // Conversion to a base class + exists(ConvertToBaseInstruction convert | + // Only keep the most specific cast + not convert.getUnary() instanceof ConvertToBaseInstruction and + n.asInstruction() = convert and + convert.getDerivedClass() = c and + c.getABaseClass*().getAMemberFunction().isVirtual() + ) +} + +private module TrackVirtualDispatch { + /** + * Gets a possible runtime target of `c` using both static call-target + * information, and call-target resolution from `virtualDispatch0`. + */ + private DataFlowPrivate::DataFlowCallable dispatch(DataFlowPrivate::DataFlowCall c) { + result = nonVirtualDispatch(c) or + result = virtualDispatch0(c) + } + + private module TtInput implements TypeTrackingInput { + final class Node = RelevantNode; + + class LocalSourceNode extends Node { + LocalSourceNode() { + this instanceof ParameterNode + or + this instanceof DataFlowPrivate::OutNode + or + DataFlowPrivate::readStep(_, _, this) + or + DataFlowPrivate::storeStep(_, _, this) + or + DataFlowPrivate::jumpStep(_, this) + or + qualifierSourceImpl(this, _) + } + } + + final private class ContentSetFinal = ContentSet; + + class Content extends ContentSetFinal { + Content() { + exists(DataFlow::Content c | + this.isSingleton(c) and + c.getIndirectionIndex() = 1 + ) + } + } + + class ContentFilter extends Content { + Content getAMatchingContent() { result = this } + } + + predicate compatibleContents(Content storeContents, Content loadContents) { + storeContents = loadContents + } + + predicate simpleLocalSmallStep(Node nodeFrom, Node nodeTo) { + nodeFrom.getFunction() instanceof Function and + simpleLocalFlowStep(nodeFrom, nodeTo, _) + } + + predicate levelStepNoCall(Node n1, LocalSourceNode n2) { none() } + + predicate levelStepCall(Node n1, LocalSourceNode n2) { none() } + + predicate storeStep(Node n1, Node n2, Content f) { DataFlowPrivate::storeStep(n1, f, n2) } + + predicate callStep(Node n1, LocalSourceNode n2) { + exists(DataFlowPrivate::DataFlowCall call, DataFlowPrivate::Position pos | + n1.(DataFlowPrivate::ArgumentNode).argumentOf(call, pos) and + n2.(ParameterNode).isParameterOf(dispatch(call), pos) + ) + } + + predicate returnStep(Node n1, LocalSourceNode n2) { + exists(DataFlowPrivate::DataFlowCallable callable, DataFlowPrivate::DataFlowCall call | + n1.(DataFlowPrivate::ReturnNode).getEnclosingCallable() = callable and + callable = dispatch(call) and + n2 = DataFlowPrivate::getAnOutNode(call, n1.(DataFlowPrivate::ReturnNode).getKind()) + ) + } + + predicate loadStep(Node n1, LocalSourceNode n2, Content f) { + DataFlowPrivate::readStep(n1, f, n2) + } + + predicate loadStoreStep(Node nodeFrom, Node nodeTo, Content f1, Content f2) { none() } + + predicate withContentStep(Node nodeFrom, LocalSourceNode nodeTo, ContentFilter f) { none() } + + predicate withoutContentStep(Node nodeFrom, LocalSourceNode nodeTo, ContentFilter f) { none() } + + predicate jumpStep(Node n1, LocalSourceNode n2) { DataFlowPrivate::jumpStep(n1, n2) } + + predicate hasFeatureBacktrackStoreTarget() { none() } + } + + private predicate qualifierSource(RelevantNode n) { qualifierSourceImpl(n, _) } + + /** + * Holds if `n` is the qualifier of `call` which targets the virtual member + * function `mf`. + */ + private predicate qualifierOfVirtualCallImpl( + RelevantNode n, CallInstruction call, MemberFunction mf + ) { + n.asOperand() = call.getThisArgumentOperand() and + call.getStaticCallTarget() = mf and + mf.isVirtual() + } + + private predicate qualifierOfVirtualCall(RelevantNode n) { qualifierOfVirtualCallImpl(n, _, _) } + + private import TypeTracking::TypeTrack::Graph + + private predicate edgePlus(PathNode n1, PathNode n2) = fastTC(edges/2)(n1, n2) + + /** + * Gets the most specific implementation of `mf` that may be called when the + * qualifier has runtime type `c`. + */ + private MemberFunction mostSpecific(MemberFunction mf, Class c) { + qualifierOfVirtualCallImpl(_, _, mf) and + mf.getAnOverridingFunction*() = result and + ( + result.getDeclaringType() = c + or + not c.getAMemberFunction().getAnOverriddenFunction*() = mf and + result = mostSpecific(mf, c.getABaseClass()) + ) + } + + /** + * Gets a possible pair of end-points `(p1, p2)` where: + * - `p1` is a derived-to-base conversion that converts from some + * class `derived`, and + * - `p2` is the qualifier of a call to a virtual function that may + * target `callable`, and + * - `callable` is the most specific implementation that may be called when + * the qualifier has type `derived`. + */ + private predicate pairCand( + PathNode p1, PathNode p2, DataFlowPrivate::DataFlowCallable callable, + DataFlowPrivate::DataFlowCall call + ) { + exists(Class derived, MemberFunction mf | + qualifierSourceImpl(p1.getNode(), derived) and + qualifierOfVirtualCallImpl(p2.getNode(), call.asCallInstruction(), mf) and + p1.isSource() and + p2.isSink() and + callable.asSourceCallable() = mostSpecific(mf, derived) + ) + } + + /** Gets a possible run-time target of `call`. */ + DataFlowPrivate::DataFlowCallable virtualDispatch(DataFlowPrivate::DataFlowCall call) { + exists(PathNode p1, PathNode p2 | p1 = p2 or edgePlus(p1, p2) | pairCand(p1, p2, result, call)) + } +} + +private DataFlowPrivate::DataFlowCallable noDisp(DataFlowPrivate::DataFlowCall call) { none() } + +pragma[nomagic] +private DataFlowPrivate::DataFlowCallable d1(DataFlowPrivate::DataFlowCall call) { + result = TrackVirtualDispatch::virtualDispatch(call) +} + +pragma[nomagic] +private DataFlowPrivate::DataFlowCallable d2(DataFlowPrivate::DataFlowCall call) { + result = TrackVirtualDispatch::virtualDispatch(call) +} + +pragma[nomagic] +private DataFlowPrivate::DataFlowCallable d3(DataFlowPrivate::DataFlowCall call) { + result = TrackVirtualDispatch::virtualDispatch(call) +} + +pragma[nomagic] +private DataFlowPrivate::DataFlowCallable d4(DataFlowPrivate::DataFlowCall call) { + result = TrackVirtualDispatch::virtualDispatch(call) +} + +pragma[nomagic] +private DataFlowPrivate::DataFlowCallable d5(DataFlowPrivate::DataFlowCall call) { + result = TrackVirtualDispatch::virtualDispatch(call) +} + +pragma[nomagic] +private DataFlowPrivate::DataFlowCallable d6(DataFlowPrivate::DataFlowCall call) { + result = TrackVirtualDispatch::virtualDispatch(call) +} + +/** Gets a function that might be called by `call`. */ +cached +DataFlowPrivate::DataFlowCallable viableCallable(DataFlowPrivate::DataFlowCall call) { + not exists(d6(call)) and + result = nonVirtualDispatch(call) + or + result = d6(call) } /** * Holds if the set of viable implementations that can be called by `call` * might be improved by knowing the call context. */ -predicate mayBenefitFromCallContext(DataFlowCall call) { mayBenefitFromCallContext(call, _, _) } +predicate mayBenefitFromCallContext(DataFlowPrivate::DataFlowCall call) { + mayBenefitFromCallContext(call, _, _) +} + +private predicate localLambdaFlowStep(Node nodeFrom, Node nodeTo) { + localFlowStep(nodeFrom, nodeTo) + or + DataFlowPrivate::additionalLambdaFlowStep(nodeFrom, nodeTo, _) +} /** * Holds if `call` is a call through a function pointer, and the pointer * value is given as the `arg`'th argument to `f`. */ private predicate mayBenefitFromCallContext( - VirtualDispatch::DataSensitiveCall call, DataFlowCallable f, int arg + DataFlowPrivate::DataFlowCall call, DataFlowPrivate::DataFlowCallable f, int arg ) { f = pragma[only_bind_out](call).getEnclosingCallable() and exists(InitializeParameterInstruction init | - not exists(call.getStaticCallTarget()) and + not exists(call.getStaticCallTarget()) + or + exists(call.getStaticCallSourceTarget().(VirtualFunction).getAnOverridingFunction()) + | init.getEnclosingFunction() = f.getUnderlyingCallable() and - call.flowsFrom(instructionNode(init), _) and + localLambdaFlowStep+(instructionNode(init), + operandNode(call.asCallInstruction().getCallTargetOperand())) and init.getParameter().getIndex() = arg ) } @@ -274,9 +362,11 @@ private predicate mayBenefitFromCallContext( * Gets a viable dispatch target of `call` in the context `ctx`. This is * restricted to those `call`s for which a context might make a difference. */ -DataFlowCallable viableImplInCallContext(DataFlowCall call, DataFlowCall ctx) { +DataFlowPrivate::DataFlowCallable viableImplInCallContext( + DataFlowPrivate::DataFlowCall call, DataFlowPrivate::DataFlowCall ctx +) { result = viableCallable(call) and - exists(int i, DataFlowCallable f | + exists(int i, DataFlowPrivate::DataFlowCallable f | mayBenefitFromCallContext(pragma[only_bind_into](call), f, i) and f = ctx.getStaticCallTarget() and result.asSourceCallable() = @@ -286,4 +376,8 @@ DataFlowCallable viableImplInCallContext(DataFlowCall call, DataFlowCall ctx) { /** Holds if arguments at position `apos` match parameters at position `ppos`. */ pragma[inline] -predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos } +predicate parameterMatch( + DataFlowPrivate::ParameterPosition ppos, DataFlowPrivate::ArgumentPosition apos +) { + ppos = apos +} diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll index a03042a77ff..3aa8994a449 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll @@ -1492,7 +1492,14 @@ predicate lambdaCall(DataFlowCall call, LambdaCallKind kind, Node receiver) { } /** Extra data-flow steps needed for lambda flow analysis. */ -predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preservesValue) { none() } +predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preservesValue) { + preservesValue = false and + exists(ContentSet cs | cs.isSingleton(any(UnionContent uc)) | + storeStep(nodeFrom, cs, nodeTo) + or + readStep(nodeFrom, cs, nodeTo) + ) +} predicate knownSourceModel(Node source, string model) { External::sourceNode(source, _, model) } diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index a0a99711552..ef4051171af 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -795,7 +795,7 @@ class FinalGlobalValue extends Node, TFinalGlobalValue { override DataFlowType getType() { exists(int indirectionIndex | indirectionIndex = globalUse.getIndirectionIndex() and - result = getTypeImpl(globalUse.getUnderlyingType(), indirectionIndex - 1) + result = getTypeImpl(globalUse.getUnderlyingType(), indirectionIndex) ) } diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll index b9f320e57b2..1cf45439d8b 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll @@ -756,9 +756,9 @@ private predicate modeledFlowBarrier(Node n) { partialFlowFunc = call.getStaticCallTarget() and not partialFlowFunc.isPartialWrite(output) | - call.getStaticCallTarget().(DataFlow::DataFlowFunction).hasDataFlow(_, output) + partialFlowFunc.(DataFlow::DataFlowFunction).hasDataFlow(_, output) or - call.getStaticCallTarget().(Taint::TaintFunction).hasTaintFlow(_, output) + partialFlowFunc.(Taint::TaintFunction).hasTaintFlow(_, output) ) or exists(Operand operand, Instruction instr, Node n0, int indirectionIndex | diff --git a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll index ce65a65319a..757db13fe8c 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -170,6 +170,16 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction { output.isParameterDeref(this.getOutputParameterIndex(_)) ) } + + final override predicate isPartialWrite(FunctionOutput output) { + exists(int outputParameterIndex | + output.isParameterDeref(outputParameterIndex) and + // We require the output to be a stream since that definitely means that + // it's a partial write. If it's not a stream then it will most likely + // fill the whole buffer. + outputParameterIndex = this.getOutputParameterIndex(true) + ) + } } /** diff --git a/cpp/ql/src/change-notes/2025-08-20-add-customizations.md b/cpp/ql/src/change-notes/2025-08-20-add-customizations.md new file mode 100644 index 00000000000..3a01298283c --- /dev/null +++ b/cpp/ql/src/change-notes/2025-08-20-add-customizations.md @@ -0,0 +1,4 @@ +--- +category: fix +--- +* Fixed an inconsistency across languages where most have a `Customizations.qll` file for adding customizations, but not all did. \ No newline at end of file diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dispatch.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dispatch.cpp index 105212ccca6..63528d712c0 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dispatch.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dispatch.cpp @@ -8,7 +8,7 @@ struct Top { virtual void isSink(int x) { } virtual int notSource1() { return source(); } virtual int notSource2() { return source(); } - virtual void notSink(int x) { sink(x); } // $ SPURIOUS: ast,ir=37:19 ast,ir=45:18 + virtual void notSink(int x) { sink(x); } // $ SPURIOUS: ast=37:19 ast=45:18 }; // This class has the correct behavior for just the functions ending in 2. @@ -32,16 +32,16 @@ void VirtualDispatch(Bottom *bottomPtr, Bottom &bottomRef) { // $ ast-def=bottom sink(topPtr->isSource2()); // $ ir MISSING: ast topPtr->isSink(source()); // causing a MISSING for ast - sink(topPtr->notSource1()); // $ SPURIOUS: ast,ir - sink(topPtr->notSource2()); // $ SPURIOUS: ast,ir + sink(topPtr->notSource1()); // $ SPURIOUS: ast + sink(topPtr->notSource2()); // $ SPURIOUS: ast topPtr->notSink(source()); // causing SPURIOUS for ast,ir sink(topRef.isSource1()); // $ ir MISSING: ast sink(topRef.isSource2()); // $ ir MISSING: ast topRef.isSink(source()); // causing a MISSING for ast - sink(topRef.notSource1()); // $ SPURIOUS: ast,ir - sink(topRef.notSource2()); // $ SPURIOUS: ast,ir + sink(topRef.notSource1()); // $ SPURIOUS: ast + sink(topRef.notSource2()); // $ SPURIOUS: ast topRef.notSink(source()); // causing SPURIOUS for ast,ir } @@ -126,8 +126,8 @@ namespace virtual_inheritance { // get flow from a `Middle` value to the call qualifier. Top *topPtr = bottomPtr, &topRef = bottomRef; - sink(topPtr->isSource()); // $ MISSING: ast,ir - sink(topRef.isSource()); // $ MISSING: ast,ir + sink(topPtr->isSource()); // $ ir MISSING: ast + sink(topRef.isSource()); // $ ir MISSING: ast } } diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-source-sink.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-source-sink.expected index 6e0b03be9c6..8c009241734 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-source-sink.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-source-sink.expected @@ -169,10 +169,6 @@ irFlow | clang.cpp:50:35:50:40 | call to source | clang.cpp:53:17:53:26 | *stackArray | | clang.cpp:51:19:51:24 | call to source | clang.cpp:53:17:53:26 | *stackArray | | clang.cpp:57:21:57:28 | call to source | clang.cpp:59:8:59:8 | d | -| dispatch.cpp:9:37:9:42 | call to source | dispatch.cpp:35:16:35:25 | call to notSource1 | -| dispatch.cpp:9:37:9:42 | call to source | dispatch.cpp:43:15:43:24 | call to notSource1 | -| dispatch.cpp:10:37:10:42 | call to source | dispatch.cpp:36:16:36:25 | call to notSource2 | -| dispatch.cpp:10:37:10:42 | call to source | dispatch.cpp:44:15:44:24 | call to notSource2 | | dispatch.cpp:16:37:16:42 | call to source | dispatch.cpp:32:16:32:24 | call to isSource2 | | dispatch.cpp:16:37:16:42 | call to source | dispatch.cpp:40:15:40:23 | call to isSource2 | | dispatch.cpp:22:37:22:42 | call to source | dispatch.cpp:31:16:31:24 | call to isSource1 | @@ -180,13 +176,13 @@ irFlow | dispatch.cpp:22:37:22:42 | call to source | dispatch.cpp:55:22:55:30 | call to isSource1 | | dispatch.cpp:22:37:22:42 | call to source | dispatch.cpp:58:28:58:36 | call to isSource1 | | dispatch.cpp:33:18:33:23 | call to source | dispatch.cpp:23:38:23:38 | x | -| dispatch.cpp:37:19:37:24 | call to source | dispatch.cpp:11:38:11:38 | x | | dispatch.cpp:41:17:41:22 | call to source | dispatch.cpp:23:38:23:38 | x | -| dispatch.cpp:45:18:45:23 | call to source | dispatch.cpp:11:38:11:38 | x | | dispatch.cpp:69:15:69:20 | call to source | dispatch.cpp:23:38:23:38 | x | | dispatch.cpp:73:14:73:19 | call to source | dispatch.cpp:23:38:23:38 | x | | dispatch.cpp:81:13:81:18 | call to source | dispatch.cpp:23:38:23:38 | x | | dispatch.cpp:107:17:107:22 | call to source | dispatch.cpp:96:8:96:8 | x | +| dispatch.cpp:117:38:117:43 | call to source | dispatch.cpp:129:18:129:25 | call to isSource | +| dispatch.cpp:117:38:117:43 | call to source | dispatch.cpp:130:17:130:24 | call to isSource | | dispatch.cpp:140:8:140:13 | call to source | dispatch.cpp:96:8:96:8 | x | | dispatch.cpp:144:8:144:13 | call to source | dispatch.cpp:96:8:96:8 | x | | flowOut.cpp:5:16:5:21 | call to source | flowOut.cpp:31:9:31:9 | x | diff --git a/cpp/ql/test/library-tests/dataflow/dispatch/test.cpp b/cpp/ql/test/library-tests/dataflow/dispatch/test.cpp new file mode 100644 index 00000000000..f243b76ad14 --- /dev/null +++ b/cpp/ql/test/library-tests/dataflow/dispatch/test.cpp @@ -0,0 +1,127 @@ +struct Base { + void f(); + virtual void virtual_f(); +}; + +struct Derived : Base { + void f(); + void virtual_f(); +}; + +void test_simple() { + Base b; + b.f(); // $ target=2 + b.virtual_f(); // $ target=3 + + Derived d; + d.f(); // $ target=7 + d.virtual_f(); // $ target=8 + + Base* b_ptr = &d; + b_ptr->f(); // $ target=2 + b_ptr->virtual_f(); // $ target=8 + + Base& b_ref = d; + b_ref.f(); // $ target=2 + b_ref.virtual_f(); // $ target=8 + + Base* b_null = nullptr; + b_null->f(); // $ target=2 + b_null->virtual_f(); // $ target=3 + + Base* base_is_derived = new Derived(); + base_is_derived->f(); // $ target=2 + base_is_derived->virtual_f(); // $ target=8 + + Base* base_is_base = new Base(); + base_is_base->f(); // $ target=2 + base_is_base->virtual_f(); // $ target=3 + + Derived* derived_is_derived = new Derived(); + derived_is_derived->f(); // $ target=7 + derived_is_derived->virtual_f(); // $ target=8 + + Base& b_ref2 = b; + b_ref2 = d; + b_ref2.f(); // $ target=2 + b_ref2.virtual_f(); // $ target=3 +} + +struct S { + Base* b1; + Base* b2; +}; + +void test_fields() { + S s; + + s.b1 = new Base(); + s.b2 = new Derived(); + + s.b1->virtual_f(); // $ target=3 + s.b2->virtual_f(); // $ target=8 + + s.b1 = new Derived(); + s.b2 = new Base(); + s.b1->virtual_f(); // $ target=8 SPURIOUS: target=3 // type-tracking has no 'clearsContent' feature and C/C++ doesn't have field-based SSA + s.b2->virtual_f(); // $ target=3 SPURIOUS: target=8 // type-tracking has no 'clearsContent' feature and C/C++ doesn't have field-based SSA +} + +Base* getDerived() { + return new Derived(); +} + +void test_getDerived() { + Base* b = getDerived(); + b->virtual_f(); // $ target=8 + + Derived d = *(Derived*)getDerived(); + d.virtual_f(); // $ target=8 +} + +void write_to_arg(Base* b) { + *b = Derived(); +} + +void write_to_arg_2(Base** b) { + Derived* d = new Derived(); + *b = d; +} + +void test_write_to_arg() { + { + Base b; + write_to_arg(&b); + b.virtual_f(); // $ SPURIOUS: target=3 MISSING: target=8 // missing flow through the copy-constructor in write_to_arg + } + + { + Base* b; + write_to_arg_2(&b); + b->virtual_f(); // $ target=8 + } +} + +Base* global_derived; + +void set_global_to_derived() { + global_derived = new Derived(); +} + +void read_global() { + global_derived->virtual_f(); // $ target=8 +} + +Base* global_base_or_derived; + +void set_global_base_or_derived_1() { + global_base_or_derived = new Base(); +} + +void set_global_base_or_derived_2() { + global_base_or_derived = new Derived(); +} + +void read_global_base_or_derived() { + global_base_or_derived->virtual_f(); // $ target=3 target=8 +} \ No newline at end of file diff --git a/cpp/ql/test/library-tests/dataflow/dispatch/test.expected b/cpp/ql/test/library-tests/dataflow/dispatch/test.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/cpp/ql/test/library-tests/dataflow/dispatch/test.ql b/cpp/ql/test/library-tests/dataflow/dispatch/test.ql new file mode 100644 index 00000000000..de16d6da1ef --- /dev/null +++ b/cpp/ql/test/library-tests/dataflow/dispatch/test.ql @@ -0,0 +1,22 @@ +import cpp +import utils.test.InlineExpectationsTest +import semmle.code.cpp.ir.dataflow.internal.DataFlowDispatch +import semmle.code.cpp.ir.dataflow.internal.DataFlowPrivate + +module ResolveDispatchTest implements TestSig { + string getARelevantTag() { result = "target" } + + predicate hasActualResult(Location location, string element, string tag, string value) { + exists(DataFlowCall call, SourceCallable callable, MemberFunction mf | + mf = callable.asSourceCallable() and + not mf.isCompilerGenerated() and + callable = viableCallable(call) and + location = call.getLocation() and + element = call.toString() and + tag = "target" and + value = callable.getLocation().getStartLine().toString() + ) + } +} + +import MakeTest diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index e19f34eb217..76eaebb52cd 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -7767,6 +7767,10 @@ WARNING: module 'TaintTracking' has been deprecated and may be removed in future | taint.cpp:830:20:830:34 | call to indirect_source | taint.cpp:832:23:832:24 | in | | | taint.cpp:831:15:831:17 | out | taint.cpp:832:18:832:20 | out | | | taint.cpp:831:15:831:17 | out | taint.cpp:833:8:833:10 | out | | +| taint.cpp:841:21:841:35 | call to indirect_source | taint.cpp:842:11:842:12 | fp | | +| taint.cpp:841:21:841:35 | call to indirect_source | taint.cpp:843:16:843:17 | fp | | +| taint.cpp:842:11:842:12 | ref arg fp | taint.cpp:843:16:843:17 | fp | | +| taint.cpp:842:15:842:16 | | taint.cpp:842:11:842:12 | ref arg fp | TAINT | | thread.cpp:10:27:10:27 | s | thread.cpp:10:27:10:27 | s | | | thread.cpp:10:27:10:27 | s | thread.cpp:11:8:11:8 | s | | | thread.cpp:14:26:14:26 | s | thread.cpp:15:8:15:8 | s | | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp index 74bcf26ea7d..0c09665de1c 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp @@ -831,4 +831,15 @@ void test_write_to_const_ptr_ptr() { const char* out; take_const_ptr(out, in); sink(out); // $ SPURIOUS: ast +} + +void indirect_sink(FILE *fp); +int fprintf(FILE *fp, const char *format, ...); + +int f7(void) +{ + FILE* fp = (FILE*)indirect_source(); + fprintf(fp, ""); + indirect_sink(fp); // $ ir MISSING: ast + return 0; } \ No newline at end of file diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.ql b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.ql index f5f483cdf1b..3bde6ca03f0 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.ql +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.ql @@ -117,6 +117,11 @@ module IRTest { call.getTarget().getName() = "sink" and [sink.asExpr(), sink.asIndirectExpr()] = call.getAnArgument() ) + or + exists(FunctionCall call | + call.getTarget().getName() = "indirect_sink" and + sink.asIndirectExpr() = call.getAnArgument() + ) } predicate isBarrier(DataFlow::Node barrier) { diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected index 239ed2ec607..f5342ea7694 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_mad-signatures.expected @@ -17670,6 +17670,55 @@ signatureMatches | taint.cpp:822:6:822:19 | take_const_ptr | (unsigned long *,const char *) | | set_cert_ex | 1 | | taint.cpp:822:6:822:19 | take_const_ptr | (unsigned long *,const char *) | | set_name_ex | 1 | | taint.cpp:822:6:822:19 | take_const_ptr | (uv_pipe_t *,const char *) | | uv_pipe_bind | 1 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_default_uflow | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_feof | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_ferror | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_file_close_mmap | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_file_underflow_mmap | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_ftell | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_getc | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_getwc | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_new_file_underflow | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_peekc_locked | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_str_count | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_str_underflow | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_sungetc | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_sungetwc | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_wdefault_uflow | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_wfile_underflow | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_wfile_underflow_mmap | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_wstr_count | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | _IO_wstr_underflow | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __fbufsize | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __feof_unlocked | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __ferror_unlocked | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __fileno | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __flbf | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __fopen_maybe_mmap | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __fpending | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __ftello | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __fwriting | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __getc_unlocked | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __getwc_unlocked | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __uflow | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __underflow | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __wuflow | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | __wunderflow | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | feof_unlocked | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | ferror_unlocked | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | fgetc_unlocked | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | fgetgrent | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | fgetpwent | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | fgetsgent | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | fgetspent | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | getc_unlocked | 0 | +| taint.cpp:836:6:836:18 | indirect_sink | (FILE *) | | getmntent | 0 | +| taint.cpp:837:5:837:11 | fprintf | (CURLSH *,CURLSHoption,...) | | curl_share_setopt | 2 | +| taint.cpp:837:5:837:11 | fprintf | (Jim_Interp *,const char *,...) | | Jim_SetResultFormatted | 1 | +| taint.cpp:837:5:837:11 | fprintf | (Jim_Interp *,const char *,...) | | Jim_SetResultFormatted | 2 | +| taint.cpp:837:5:837:11 | fprintf | (char **,const char *,...) | | ___asprintf | 1 | +| taint.cpp:837:5:837:11 | fprintf | (char **,const char *,...) | | ___asprintf | 2 | +| taint.cpp:837:5:837:11 | fprintf | (curl_httppost **,curl_httppost **,...) | | curl_formadd | 2 | | thread.cpp:4:6:4:9 | sink | (int) | | ASN1_STRING_type_new | 0 | | thread.cpp:4:6:4:9 | sink | (int) | | ASN1_tag2bit | 0 | | thread.cpp:4:6:4:9 | sink | (int) | | ASN1_tag2str | 0 | @@ -47191,6 +47240,10 @@ getParameterTypeName | taint.cpp:817:6:817:27 | write_to_const_ptr_ptr | 1 | const char ** | | taint.cpp:822:6:822:19 | take_const_ptr | 0 | const char * | | taint.cpp:822:6:822:19 | take_const_ptr | 1 | const char * | +| taint.cpp:836:6:836:18 | indirect_sink | 0 | FILE * | +| taint.cpp:837:5:837:11 | fprintf | 0 | FILE * | +| taint.cpp:837:5:837:11 | fprintf | 1 | const char * | +| taint.cpp:837:5:837:11 | fprintf | 2 | ... | | thread.cpp:4:6:4:9 | sink | 0 | int | | thread.cpp:6:8:6:8 | operator= | 0 | S && | | thread.cpp:6:8:6:8 | operator= | 0 | const S & | diff --git a/csharp/ql/lib/change-notes/2025-08-18-implicit-reads-at-sinks.md b/csharp/ql/lib/change-notes/2025-08-18-implicit-reads-at-sinks.md new file mode 100644 index 00000000000..d66e982e6ae --- /dev/null +++ b/csharp/ql/lib/change-notes/2025-08-18-implicit-reads-at-sinks.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The default taint tracking configuration now allows implicit reads from collections at sinks and in additional flow steps. This increases flow coverage for many taint tracking queries and helps reduce false negatives. diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll index b7681994e2c..3146720efe8 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll @@ -29,7 +29,10 @@ predicate defaultTaintSanitizer(DataFlow::Node node) { * of `c` at sinks and inputs to additional taint steps. */ bindingset[node] -predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c) { none() } +predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c) { + exists(node) and + c.isElement() +} private class LocalTaintExprStepConfiguration extends ControlFlowReachabilityConfiguration { LocalTaintExprStepConfiguration() { this = "LocalTaintExprStepConfiguration" } diff --git a/csharp/ql/src/Security Features/CWE-090/LDAPInjection.qhelp b/csharp/ql/src/Security Features/CWE-090/LDAPInjection.qhelp index 04f01720ce6..4af37eadfd7 100644 --- a/csharp/ql/src/Security Features/CWE-090/LDAPInjection.qhelp +++ b/csharp/ql/src/Security Features/CWE-090/LDAPInjection.qhelp @@ -12,7 +12,7 @@ is likely to be able to run malicious LDAP queries.

If user input must be included in an LDAP query, it should be escaped to avoid a malicious user providing special characters that change the meaning of the query. If possible, use an existing library, such as the AntiXSS -library.

+library. One may also make their own encoder filter `LdapEncode` following RFC 4515 standards.

@@ -35,7 +35,6 @@ the query cannot be changed by a malicious user.

  • OWASP: LDAP Injection Prevention Cheat Sheet.
  • OWASP: Preventing LDAP Injection in Java.
    • -
  • AntiXSS doc: LdapFilterEncode.
    • -
  • AntiXSS doc: LdapDistinguishedNameEncode.
    • +
  • RFC 4515: String Search Filter Definition.
    •  diff --git a/csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql b/csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql index f18798c8b08..f175723c099 100644 --- a/csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql +++ b/csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql @@ -10,6 +10,7 @@ */ import csharp +import semmle.code.csharp.frameworks.system.Collections import HashWithoutSalt::PathGraph /** The C# class `Windows.Security.Cryptography.Core.HashAlgorithmProvider`. */ @@ -93,12 +94,17 @@ predicate hasAnotherHashCall(MethodCall mc) { /** Holds if a password hash without salt is further processed in another method call. */ predicate hasFurtherProcessing(MethodCall mc) { - mc.getTarget().fromLibrary() and - ( - mc.getTarget().hasFullyQualifiedName("System", "Array", "Copy") or // Array.Copy(passwordHash, 0, password.Length), 0, key, 0, keyLen); - mc.getTarget().hasFullyQualifiedName("System", "String", "Concat") or // string.Concat(passwordHash, saltkey) - mc.getTarget().hasFullyQualifiedName("System", "Buffer", "BlockCopy") or // Buffer.BlockCopy(passwordHash, 0, allBytes, 0, 20) - mc.getTarget().hasFullyQualifiedName("System", "String", "Format") // String.Format("{0}:{1}:{2}", username, salt, password) + exists(Method m | m = mc.getTarget() and m.fromLibrary() | + m.hasFullyQualifiedName("System", "Array", "Copy") // Array.Copy(passwordHash, 0, password.Length), 0, key, 0, keyLen); + or + m.hasFullyQualifiedName("System", "String", "Concat") // string.Concat(passwordHash, saltkey) + or + m.hasFullyQualifiedName("System", "Buffer", "BlockCopy") // Buffer.BlockCopy(passwordHash, 0, allBytes, 0, 20) + or + m.hasFullyQualifiedName("System", "String", "Format") // String.Format("{0}:{1}:{2}", username, salt, password) + or + m.getName() = "CopyTo" and + m.getDeclaringType().getABaseType*() instanceof SystemCollectionsICollectionInterface // passBytes.CopyTo(rawSalted, 0); ) } diff --git a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs index 705efd35e38..d7552376c0f 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs @@ -116,7 +116,7 @@ namespace My.Qltest { var a = new object[] { new object() }; var b = Reverse(a); - Sink(b); // No flow + Sink(b); // Flow Sink(b[0]); // Flow } diff --git a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected index 7254208be18..3099a3fec7e 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected @@ -104,6 +104,7 @@ edges | ExternalFlow.cs:117:17:117:17 | access to local variable a : null [element] : Object | ExternalFlow.cs:118:29:118:29 | access to local variable a : null [element] : Object | provenance | | | ExternalFlow.cs:117:34:117:49 | { ..., ... } : null [element] : Object | ExternalFlow.cs:117:17:117:17 | access to local variable a : null [element] : Object | provenance | | | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | ExternalFlow.cs:117:34:117:49 | { ..., ... } : null [element] : Object | provenance | | +| ExternalFlow.cs:118:17:118:17 | access to local variable b : null [element] : Object | ExternalFlow.cs:119:18:119:18 | access to local variable b | provenance | | | ExternalFlow.cs:118:17:118:17 | access to local variable b : null [element] : Object | ExternalFlow.cs:120:18:120:18 | access to local variable b : null [element] : Object | provenance | | | ExternalFlow.cs:118:21:118:30 | call to method Reverse : null [element] : Object | ExternalFlow.cs:118:17:118:17 | access to local variable b : null [element] : Object | provenance | | | ExternalFlow.cs:118:29:118:29 | access to local variable a : null [element] : Object | ExternalFlow.cs:118:21:118:30 | call to method Reverse : null [element] : Object | provenance | MaD:7 | @@ -240,6 +241,7 @@ nodes | ExternalFlow.cs:118:17:118:17 | access to local variable b : null [element] : Object | semmle.label | access to local variable b : null [element] : Object | | ExternalFlow.cs:118:21:118:30 | call to method Reverse : null [element] : Object | semmle.label | call to method Reverse : null [element] : Object | | ExternalFlow.cs:118:29:118:29 | access to local variable a : null [element] : Object | semmle.label | access to local variable a : null [element] : Object | +| ExternalFlow.cs:119:18:119:18 | access to local variable b | semmle.label | access to local variable b | | ExternalFlow.cs:120:18:120:18 | access to local variable b : null [element] : Object | semmle.label | access to local variable b : null [element] : Object | | ExternalFlow.cs:120:18:120:21 | access to array element | semmle.label | access to array element | | ExternalFlow.cs:205:17:205:18 | access to local variable o2 : Object | semmle.label | access to local variable o2 : Object | @@ -315,6 +317,7 @@ invalidModelRow | ExternalFlow.cs:102:22:102:22 | access to parameter d | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | ExternalFlow.cs:102:22:102:22 | access to parameter d | $@ | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:104:18:104:25 | access to field Field | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | ExternalFlow.cs:104:18:104:25 | access to field Field | $@ | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:112:18:112:25 | access to property MyProp | ExternalFlow.cs:111:24:111:35 | object creation of type Object : Object | ExternalFlow.cs:112:18:112:25 | access to property MyProp | $@ | ExternalFlow.cs:111:24:111:35 | object creation of type Object : Object | object creation of type Object : Object | +| ExternalFlow.cs:119:18:119:18 | access to local variable b | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | ExternalFlow.cs:119:18:119:18 | access to local variable b | $@ | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:120:18:120:21 | access to array element | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | ExternalFlow.cs:120:18:120:21 | access to array element | $@ | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:206:18:206:48 | call to method MixedFlowArgs | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | ExternalFlow.cs:206:18:206:48 | call to method MixedFlowArgs | $@ | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:212:18:212:62 | call to method GeneratedFlowWithGeneratedNeutral | ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | ExternalFlow.cs:212:18:212:62 | call to method GeneratedFlowWithGeneratedNeutral | $@ | ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | object creation of type Object : Object | diff --git a/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.cs b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.cs new file mode 100644 index 00000000000..d4177a57661 --- /dev/null +++ b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.cs @@ -0,0 +1,13 @@ +public class CollectionTaintTracking +{ + public void ImplicitCollectionReadAtSink() + { + var tainted = Source(1); + var arr = new object[] { tainted }; + Sink(arr); // $ hasTaintFlow=1 + } + + static T Source(object source) => throw null; + + public static void Sink(T t) { } +} diff --git a/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected new file mode 100644 index 00000000000..6d93e7f5ef9 --- /dev/null +++ b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected @@ -0,0 +1,18 @@ +models +edges +| CollectionTaintTracking.cs:5:13:5:19 | access to local variable tainted : Object | CollectionTaintTracking.cs:6:34:6:40 | access to local variable tainted : Object | provenance | | +| CollectionTaintTracking.cs:5:23:5:39 | call to method Source : Object | CollectionTaintTracking.cs:5:13:5:19 | access to local variable tainted : Object | provenance | | +| CollectionTaintTracking.cs:6:13:6:15 | access to local variable arr : null [element] : Object | CollectionTaintTracking.cs:7:14:7:16 | access to local variable arr | provenance | | +| CollectionTaintTracking.cs:6:32:6:42 | { ..., ... } : null [element] : Object | CollectionTaintTracking.cs:6:13:6:15 | access to local variable arr : null [element] : Object | provenance | | +| CollectionTaintTracking.cs:6:34:6:40 | access to local variable tainted : Object | CollectionTaintTracking.cs:6:32:6:42 | { ..., ... } : null [element] : Object | provenance | | +nodes +| CollectionTaintTracking.cs:5:13:5:19 | access to local variable tainted : Object | semmle.label | access to local variable tainted : Object | +| CollectionTaintTracking.cs:5:23:5:39 | call to method Source : Object | semmle.label | call to method Source : Object | +| CollectionTaintTracking.cs:6:13:6:15 | access to local variable arr : null [element] : Object | semmle.label | access to local variable arr : null [element] : Object | +| CollectionTaintTracking.cs:6:32:6:42 | { ..., ... } : null [element] : Object | semmle.label | { ..., ... } : null [element] : Object | +| CollectionTaintTracking.cs:6:34:6:40 | access to local variable tainted : Object | semmle.label | access to local variable tainted : Object | +| CollectionTaintTracking.cs:7:14:7:16 | access to local variable arr | semmle.label | access to local variable arr | +subpaths +testFailures +#select +| CollectionTaintTracking.cs:7:14:7:16 | access to local variable arr | CollectionTaintTracking.cs:5:23:5:39 | call to method Source : Object | CollectionTaintTracking.cs:7:14:7:16 | access to local variable arr | $@ | CollectionTaintTracking.cs:5:23:5:39 | call to method Source : Object | call to method Source : Object | diff --git a/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.ql b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.ql new file mode 100644 index 00000000000..0af8971a13b --- /dev/null +++ b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.ql @@ -0,0 +1,12 @@ +/** + * @kind path-problem + */ + +import csharp +import utils.test.InlineFlowTest +import TaintFlowTest +import PathGraph + +from PathNode source, PathNode sink +where flowPath(source, sink) +select sink, source, sink, "$@", source, source.toString() diff --git a/csharp/ql/test/library-tests/tainttracking/collections/options b/csharp/ql/test/library-tests/tainttracking/collections/options new file mode 100644 index 00000000000..75c39b4541b --- /dev/null +++ b/csharp/ql/test/library-tests/tainttracking/collections/options @@ -0,0 +1,2 @@ +semmle-extractor-options: /nostdlib /noconfig +semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj diff --git a/go/extractor/go.mod b/go/extractor/go.mod index 8ec7ec72fc2..a23183400d0 100644 --- a/go/extractor/go.mod +++ b/go/extractor/go.mod @@ -9,8 +9,8 @@ toolchain go1.25.0 // when adding or removing dependencies, run // bazel mod tidy require ( - golang.org/x/mod v0.26.0 - golang.org/x/tools v0.35.0 + golang.org/x/mod v0.27.0 + golang.org/x/tools v0.36.0 ) require golang.org/x/sync v0.16.0 // indirect diff --git a/go/extractor/go.sum b/go/extractor/go.sum index 58f0d0b933b..e1a8435ba51 100644 --- a/go/extractor/go.sum +++ b/go/extractor/go.sum @@ -1,8 +1,8 @@ github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= -golang.org/x/mod v0.26.0 h1:EGMPT//Ezu+ylkCijjPc+f4Aih7sZvaAr+O3EHBxvZg= -golang.org/x/mod v0.26.0/go.mod h1:/j6NAhSk8iQ723BGAUyoAcn7SlD7s15Dp9Nd/SfeaFQ= +golang.org/x/mod v0.27.0 h1:kb+q2PyFnEADO2IEF935ehFUXlWiNjJWtRNgBLSfbxQ= +golang.org/x/mod v0.27.0/go.mod h1:rWI627Fq0DEoudcK+MBkNkCe0EetEaDSwJJkCcjpazc= golang.org/x/sync v0.16.0 h1:ycBJEhp9p4vXvUZNszeOq0kGTPghopOL8q0fq3vstxw= golang.org/x/sync v0.16.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= -golang.org/x/tools v0.35.0 h1:mBffYraMEf7aa0sB+NuKnuCy8qI/9Bughn8dC2Gu5r0= -golang.org/x/tools v0.35.0/go.mod h1:NKdj5HkL/73byiZSJjqJgKn3ep7KjFkBOkR/Hps3VPw= +golang.org/x/tools v0.36.0 h1:kWS0uv/zsvHEle1LbV5LE8QujrxB3wfQyxHfhOk0Qkg= +golang.org/x/tools v0.36.0/go.mod h1:WBDiHKJK8YgLHlcQPYQzNCkUxUypCaa5ZegCVutKm+s= diff --git a/java/documentation/library-coverage/coverage.csv b/java/documentation/library-coverage/coverage.csv index 7ec0b4c5f0f..1d1f9a1545e 100644 --- a/java/documentation/library-coverage/coverage.csv +++ b/java/documentation/library-coverage/coverage.csv @@ -76,7 +76,7 @@ jakarta.activation,2,,2,,,,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,1,,,,,,,,,,,,,,,,2, jakarta.faces.context,4,7,,,,,,,,,,,,,,2,,,,,,,,,,,2,,,,,,,,,,,,,,,,,,,,,,,,7,, jakarta.json,,,123,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,100,23 jakarta.persistence,2,,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,,,,,,,1, -jakarta.servlet,2,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,,1,, +jakarta.servlet,2,19,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,,19,, jakarta.ws.rs.client,1,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1,,,,,,,,,,,,,,,,, jakarta.ws.rs.container,,9,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,9,, jakarta.ws.rs.core,2,,149,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,2,,,,,,,,,94,55 diff --git a/java/documentation/library-coverage/coverage.rst b/java/documentation/library-coverage/coverage.rst index aa6a3f2c171..d70f9c2f463 100644 --- a/java/documentation/library-coverage/coverage.rst +++ b/java/documentation/library-coverage/coverage.rst @@ -19,9 +19,9 @@ Java framework & library support JBoss Logging,``org.jboss.logging``,,,324,,,,,, `JSON-java `_,``org.json``,,236,,,,,,, Java Standard Library,``java.*``,10,4621,260,99,,9,,,26 - Java extensions,"``javax.*``, ``jakarta.*``",69,4159,90,10,4,2,1,1,4 + Java extensions,"``javax.*``, ``jakarta.*``",87,4159,90,10,4,2,1,1,4 Kotlin Standard Library,``kotlin*``,,1849,16,14,,,,,2 `Spring `_,``org.springframework.*``,38,486,143,26,,28,14,,35 Others,"``actions.osgi``, ``antlr``, ``ch.ethz.ssh2``, ``cn.hutool.core.codec``, ``com.alibaba.com.caucho.hessian.io``, ``com.alibaba.druid.sql``, ``com.alibaba.fastjson2``, ``com.amazonaws.auth``, ``com.auth0.jwt.algorithms``, ``com.azure.identity``, ``com.caucho.burlap.io``, ``com.caucho.hessian.io``, ``com.cedarsoftware.util.io``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.esotericsoftware.yamlbeans``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.google.gson``, ``com.hubspot.jinjava``, ``com.jcraft.jsch``, ``com.microsoft.sqlserver.jdbc``, ``com.mitchellbosecke.pebble``, ``com.mongodb``, ``com.opensymphony.xwork2``, ``com.rabbitmq.client``, ``com.sshtools.j2ssh.authentication``, ``com.sun.crypto.provider``, ``com.sun.jndi.ldap``, ``com.sun.net.httpserver``, ``com.sun.net.ssl``, ``com.sun.rowset``, ``com.sun.security.auth.module``, ``com.sun.security.ntlm``, ``com.sun.security.sasl.digest``, ``com.thoughtworks.xstream``, ``com.trilead.ssh2``, ``com.unboundid.ldap.sdk``, ``com.zaxxer.hikari``, ``flexjson``, ``freemarker.cache``, ``freemarker.template``, ``groovy.lang``, ``groovy.text``, ``groovy.util``, ``hudson``, ``io.jsonwebtoken``, ``io.netty.bootstrap``, ``io.netty.buffer``, ``io.netty.channel``, ``io.netty.handler.codec``, ``io.netty.handler.ssl``, ``io.netty.handler.stream``, ``io.netty.resolver``, ``io.netty.util``, ``io.undertow.server.handlers.resource``, ``javafx.scene.web``, ``jenkins``, ``jodd.json``, ``liquibase.database.jvm``, ``liquibase.statement.core``, ``net.lingala.zip4j``, ``net.schmizz.sshj``, ``net.sf.json``, ``net.sf.saxon.s9api``, ``ognl``, ``okhttp3``, ``org.acegisecurity``, ``org.antlr.runtime``, ``org.apache.commons.codec``, ``org.apache.commons.compress.archivers.tar``, ``org.apache.commons.exec``, ``org.apache.commons.httpclient.util``, ``org.apache.commons.jelly``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.lang``, ``org.apache.commons.logging``, ``org.apache.commons.net``, ``org.apache.commons.ognl``, ``org.apache.cxf.catalog``, ``org.apache.cxf.common.classloader``, ``org.apache.cxf.common.jaxb``, ``org.apache.cxf.common.logging``, ``org.apache.cxf.configuration.jsse``, ``org.apache.cxf.helpers``, ``org.apache.cxf.resource``, ``org.apache.cxf.staxutils``, ``org.apache.cxf.tools.corba.utils``, ``org.apache.cxf.tools.util``, ``org.apache.cxf.transform``, ``org.apache.directory.ldap.client.api``, ``org.apache.hadoop.fs``, ``org.apache.hadoop.hive.metastore``, ``org.apache.hadoop.hive.ql.exec``, ``org.apache.hadoop.hive.ql.metadata``, ``org.apache.hc.client5.http.async.methods``, ``org.apache.hc.client5.http.classic.methods``, ``org.apache.hc.client5.http.fluent``, ``org.apache.hive.hcatalog.templeton``, ``org.apache.ibatis.jdbc``, ``org.apache.ibatis.mapping``, ``org.apache.log4j``, ``org.apache.shiro.authc``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.apache.shiro.mgt``, ``org.apache.sshd.client.session``, ``org.apache.struts.beanvalidation.validation.interceptor``, ``org.apache.struts2``, ``org.apache.tools.ant``, ``org.apache.tools.zip``, ``org.apache.velocity.app``, ``org.apache.velocity.runtime``, ``org.codehaus.cargo.container.installer``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.eclipse.jetty.client``, ``org.exolab.castor.xml``, ``org.fusesource.leveldbjni``, ``org.geogebra.web.full.main``, ``org.gradle.api.file``, ``org.hibernate``, ``org.ho.yaml``, ``org.influxdb``, ``org.jabsorb``, ``org.jboss.vfs``, ``org.jdbi.v3.core``, ``org.jenkins.ui.icon``, ``org.jenkins.ui.symbol``, ``org.jooq``, ``org.keycloak.models.map.storage``, ``org.kohsuke.stapler``, ``org.lastaflute.web``, ``org.mvel2``, ``org.openjdk.jmh.runner.options``, ``org.owasp.esapi``, ``org.pac4j.jwt.config.encryption``, ``org.pac4j.jwt.config.signature``, ``org.scijava.log``, ``org.slf4j``, ``org.thymeleaf``, ``org.xml.sax``, ``org.xmlpull.v1``, ``org.yaml.snakeyaml``, ``play.libs.ws``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``, ``retrofit2``, ``software.amazon.awssdk.transfer.s3.model``, ``sun.jvmstat.perfdata.monitor.protocol.local``, ``sun.jvmstat.perfdata.monitor.protocol.rmi``, ``sun.misc``, ``sun.net.ftp``, ``sun.net.www.protocol.http``, ``sun.security.acl``, ``sun.security.jgss.krb5``, ``sun.security.krb5``, ``sun.security.pkcs``, ``sun.security.pkcs11``, ``sun.security.provider``, ``sun.security.ssl``, ``sun.security.x509``, ``sun.tools.jconsole``",133,10525,927,140,6,22,18,,208 - Totals,,312,26328,2656,404,16,128,33,1,409 + Totals,,330,26328,2656,404,16,128,33,1,409 diff --git a/java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected b/java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected index 7a1a986b2aa..4a736437995 100644 --- a/java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected +++ b/java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected @@ -80,6 +80,7 @@ ql/java/ql/src/Violations of Best Practice/Naming Conventions/LocalShadowsFieldC ql/java/ql/src/Violations of Best Practice/Naming Conventions/SameNameAsSuper.ql ql/java/ql/src/Violations of Best Practice/Records/IgnoredSerializationMembersOfRecordClass.ql ql/java/ql/src/Violations of Best Practice/SpecialCharactersInLiterals/NonExplicitControlAndWhitespaceCharsInLiterals.ql +ql/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToRunFinalizersOnExit.ql ql/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToStringToString.ql ql/java/ql/src/Violations of Best Practice/Undesirable Calls/DefaultToString.ql ql/java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.ql diff --git a/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToRunFinalizersOnExit.ql b/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToRunFinalizersOnExit.ql index c2ffe45b520..568be1805e6 100644 --- a/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToRunFinalizersOnExit.ql +++ b/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToRunFinalizersOnExit.ql @@ -7,8 +7,11 @@ * @problem.severity error * @precision medium * @id java/run-finalizers-on-exit - * @tags reliability - * maintainability + * @previous-id java/do-not-use-finalizers + * @tags quality + * reliability + * correctness + * performance */ import java diff --git a/java/ql/src/Violations of Best Practice/Undesirable Calls/GarbageCollection.ql b/java/ql/src/Violations of Best Practice/Undesirable Calls/GarbageCollection.ql index 1067bdcb6dc..620177cc58c 100644 --- a/java/ql/src/Violations of Best Practice/Undesirable Calls/GarbageCollection.ql +++ b/java/ql/src/Violations of Best Practice/Undesirable Calls/GarbageCollection.ql @@ -6,8 +6,10 @@ * @problem.severity recommendation * @precision low * @id java/garbage-collection - * @tags reliability - * maintainability + * @previous-id java/do-not-use-finalizers + * @tags quality + * reliability + * correctness */ import java diff --git a/java/ql/src/change-notes/2025-07-19-adjust-tags.md b/java/ql/src/change-notes/2025-07-19-adjust-tags.md new file mode 100644 index 00000000000..0067adebdaf --- /dev/null +++ b/java/ql/src/change-notes/2025-07-19-adjust-tags.md @@ -0,0 +1,5 @@ +--- +category: queryMetadata +--- +* The tag `maintainability` has been removed from `java/run-finalizers-on-exit` and the tags `quality`, `correctness`, and `performance` have been added. +* The tag `maintainability` has been removed from `java/garbage-collection` and the tags `quality` and `correctness` have been added. diff --git a/rust/README.md b/rust/README.md index 8be853bd5f2..4f462f1b70f 100644 --- a/rust/README.md +++ b/rust/README.md @@ -1,8 +1,5 @@ # Rust on CodeQL -> [!WARNING] -> Rust support for CodeQL is experimental. No support is offered. QL and database interfaces will change and break without notice or deprecation periods. - ## Development ### Dependencies diff --git a/rust/ql/lib/Customizations.qll b/rust/ql/lib/Customizations.qll new file mode 100644 index 00000000000..8fc6bbea911 --- /dev/null +++ b/rust/ql/lib/Customizations.qll @@ -0,0 +1,11 @@ +/** + * Contains customizations to the standard library. + * + * This module is imported by `rust.qll`, so any customizations defined here automatically + * apply to all queries. + * + * Typical examples of customizations include adding new subclasses of abstract classes such as + * the `RemoteFlowSource` class to model frameworks that are not covered by the standard library. + */ + +import rust diff --git a/rust/ql/lib/codeql/rust/frameworks/rustcrypto/RustCrypto.qll b/rust/ql/lib/codeql/rust/frameworks/rustcrypto/RustCrypto.qll index 70b92a3f7ea..51d00f795d7 100644 --- a/rust/ql/lib/codeql/rust/frameworks/rustcrypto/RustCrypto.qll +++ b/rust/ql/lib/codeql/rust/frameworks/rustcrypto/RustCrypto.qll @@ -5,6 +5,8 @@ private import rust private import codeql.rust.Concepts private import codeql.rust.dataflow.DataFlow +private import codeql.rust.internal.TypeInference +private import codeql.rust.internal.Type bindingset[algorithmName] private string simplifyAlgorithmName(string algorithmName) { @@ -21,28 +23,20 @@ class StreamCipherInit extends Cryptography::CryptographicOperation::Range { StreamCipherInit() { // a call to `cipher::KeyInit::new`, `cipher::KeyInit::new_from_slice`, - // `cipher::KeyIvInit::new`, `cipher::KeyIvInit::new_from_slices` or `rc2::Rc2::new_with_eff_key_len`. - exists(PathExpr p, string rawAlgorithmName | - this.asExpr().getExpr().(CallExpr).getFunction() = p and - p.getResolvedCrateOrigin().matches("%/RustCrypto%") and - p.getPath().getText() = ["new", "new_from_slice", "new_from_slices", "new_with_eff_key_len"] and - ( - rawAlgorithmName = p.getPath().getQualifier().getText() or + // `cipher::KeyIvInit::new`, `cipher::KeyIvInit::new_from_slices`, `rc2::Rc2::new_with_eff_key_len` or similar. + exists(CallExprBase ce, string rawAlgorithmName | + ce = this.asExpr().getExpr() and + ce.getStaticTarget().getName().getText() = + ["new", "new_from_slice", "new_with_eff_key_len", "new_from_slices"] and + // extract the algorithm name from the type of `ce` or its receiver. + exists(Type t, TypePath tp | + t = inferType([ce, ce.(MethodCallExpr).getReceiver()], tp) and rawAlgorithmName = - p.getPath() - .getQualifier() - .getSegment() - .getGenericArgList() - .getGenericArg(0) - .(TypeArg) - .getTypeRepr() - .(PathTypeRepr) - .getPath() - .getSegment() - .getIdentifier() - .getText() + t.(StructType).asItemNode().(Addressable).getCanonicalPath().splitAt("::") ) and - algorithmName = simplifyAlgorithmName(rawAlgorithmName) + algorithmName = simplifyAlgorithmName(rawAlgorithmName) and + // only match a known cryptographic algorithm + any(Cryptography::CryptographicAlgorithm alg).matchesName(algorithmName) ) } diff --git a/rust/ql/lib/codeql/rust/internal/CachedStages.qll b/rust/ql/lib/codeql/rust/internal/CachedStages.qll index ad011a753c8..c2dc2166178 100644 --- a/rust/ql/lib/codeql/rust/internal/CachedStages.qll +++ b/rust/ql/lib/codeql/rust/internal/CachedStages.qll @@ -120,9 +120,7 @@ module Stages { or exists(resolvePath(_)) or - exists(any(ItemNode i).getASuccessor(_)) - or - exists(any(ItemNode i).getASuccessorRec(_)) + exists(any(ItemNode i).getASuccessor(_, _)) or exists(any(ImplOrTraitItemNode i).getASelfPath()) or diff --git a/rust/ql/lib/codeql/rust/internal/Definitions.qll b/rust/ql/lib/codeql/rust/internal/Definitions.qll index c03ece9d56d..b1b3e475c46 100644 --- a/rust/ql/lib/codeql/rust/internal/Definitions.qll +++ b/rust/ql/lib/codeql/rust/internal/Definitions.qll @@ -135,10 +135,10 @@ private class PositionalFormatArgumentUse extends Use instanceof PositionalForma override string getUseType() { result = "format argument" } } -private class PathUse extends Use instanceof PathSegment { +private class PathUse extends Use instanceof NameRef { private Path path; - PathUse() { this = path.getSegment() } + PathUse() { this = path.getSegment().getIdentifier() } private CallExpr getCall() { result.getFunction().(PathExpr).getPath() = path } diff --git a/rust/ql/lib/codeql/rust/internal/PathResolution.qll b/rust/ql/lib/codeql/rust/internal/PathResolution.qll index 354cab5a6de..a0d52a34ece 100644 --- a/rust/ql/lib/codeql/rust/internal/PathResolution.qll +++ b/rust/ql/lib/codeql/rust/internal/PathResolution.qll @@ -32,24 +32,102 @@ final class Namespace extends TNamespace { } } +private newtype TSuccessorKind = + TInternal() or + TExternal() or + TBoth() + +/** A successor kind. */ +class SuccessorKind extends TSuccessorKind { + predicate isInternal() { this = TInternal() } + + predicate isExternal() { this = TExternal() } + + predicate isBoth() { this = TBoth() } + + predicate isInternalOrBoth() { this.isInternal() or this.isBoth() } + + predicate isExternalOrBoth() { this.isExternal() or this.isBoth() } + + string toString() { + this.isInternal() and result = "internal" + or + this.isExternal() and result = "external" + or + this.isBoth() and result = "both" + } +} + +pragma[nomagic] +private ItemNode getAChildSuccessor(ItemNode item, string name, SuccessorKind kind) { + item = result.getImmediateParent() and + name = result.getName() and + ( + // type parameters are only available inside the declaring item + if result instanceof TypeParam + then kind.isInternal() + else + // associated items must always be qualified, also within the declaring + // item (using `Self`) + if item instanceof ImplOrTraitItemNode and result instanceof AssocItem + then kind.isExternal() + else + if result instanceof Use + then kind.isInternal() + else kind.isBoth() + ) +} + /** * An item that may be referred to by a path, and which is a node in * the _item graph_. * * The item graph is a labeled directed graph, where an edge - * `item1 --name--> item2` means that `item2` is available inside the - * scope of `item1` under the name `name`. For example, if we have + * + * ``` + * item1 --name,kind--> item2 + * ``` + * + * means that: + * + * - `item2` is available _inside_ the scope of `item1` under the name `name`, + * when `kind` is either `internal` or `both`, and + * + * - `item2` is available _externally_ from `item1` under the name `name`, when + * `kind` is either `external` or `both`. + * + * For example, if we have * * ```rust - * mod m1 { - * mod m2 { } + * pub mod m1 { + * pub mod m2 { } * } * ``` * - * then there is an edge `m1 --m2--> m1::m2`. + * then there is an edge `mod m1 --m2,both--> mod m2`. + * + * Associated items are example of externally visible items (inside the + * declaring item they must be `Self` prefixed), while type parameters are + * examples of internally visible items. For example, for + * + * ```rust + * mod m { + * pub trait Trait { + * fn foo(&self) -> T; + * } + * } + * ``` + * + * we have the following edges + * + * ``` + * mod m --Trait,both--> trait Trait + * trait Trait --foo,external --> fn foo + * trait Trait --T,internal --> T + * ``` * * Source files are also considered nodes in the item graph, and for - * each source file `f` there is an edge `f --name--> item` when `f` + * each source file `f` there is an edge `f --name,both--> item` when `f` * declares `item` with the name `name`. * * For imports like @@ -61,11 +139,13 @@ final class Namespace extends TNamespace { * } * ``` * - * we first generate an edge `m1::m2 --name--> f::item`, where `item` is - * any item (named `name`) inside the imported source file `f`. Using this - * edge, `m2::foo` can resolve to `f::foo`, which results in the edge - * `m1::use m2 --foo--> f::foo`. Lastly, all edges out of `use` nodes are - * lifted to predecessors in the graph, so we get an edge `m1 --foo--> f::foo`. + * we first generate an edge `mod m2 --name,kind--> f::item`, where `item` is + * any item (named `name`) inside the imported source file `f`, and `kind` is + * either `external` or `both`. Using this edge, `m2::foo` can resolve to + * `f::foo`, which results in the edge `use m2 --foo,internal--> f::foo` + * (would have been `external` if it was `pub use m2::foo`). Lastly, all edges + * out of `use` nodes are lifted to predecessors in the graph, so we get + * an edge `mod m1 --foo,internal--> f::foo`. * * * References: @@ -112,42 +192,40 @@ abstract class ItemNode extends Locatable { result = this.(SourceFileItemNode).getSuper() } - pragma[nomagic] - private ItemNode getAChildSuccessor(string name) { - this = result.getImmediateParent() and - name = result.getName() - } - + /** Gets a successor named `name` of the given `kind`, if any. */ cached - ItemNode getASuccessorRec(string name) { + ItemNode getASuccessor(string name, SuccessorKind kind) { Stages::PathResolutionStage::ref() and - sourceFileEdge(this, name, result) + sourceFileEdge(this, name, result) and + kind.isBoth() or - result = this.getAChildSuccessor(name) + result = getAChildSuccessor(this, name, kind) or - fileImportEdge(this, name, result) + fileImportEdge(this, name, result, kind) or - useImportEdge(this, name, result) + useImportEdge(this, name, result, kind) or - crateDefEdge(this, name, result) + crateDefEdge(this, name, result, kind) or - crateDependencyEdge(this, name, result) + crateDependencyEdge(this, name, result) and + kind.isInternal() or - externCrateEdge(this, name, result) + externCrateEdge(this, name, result) and + kind.isInternal() or // items made available through `use` are available to nodes that contain the `use` exists(UseItemNode use | - use = this.getASuccessorRec(_) and - result = use.(ItemNode).getASuccessorRec(name) + use = this.getASuccessor(_, _) and + result = use.(ItemNode).getASuccessor(name, kind) ) or - exists(ExternCrateItemNode ec | result = ec.(ItemNode).getASuccessorRec(name) | - ec = this.getASuccessorRec(_) + exists(ExternCrateItemNode ec | result = ec.(ItemNode).getASuccessor(name, kind) | + ec = this.getASuccessor(_, _) or // if the extern crate appears in the crate root, then the crate name is also added // to the 'extern prelude', see https://doc.rust-lang.org/reference/items/extern-crates.html exists(Crate c | - ec = c.getSourceFile().(ItemNode).getASuccessorRec(_) and + ec = c.getSourceFile().(ItemNode).getASuccessor(_, _) and this = c.getASourceFile() ) ) @@ -155,7 +233,8 @@ abstract class ItemNode extends Locatable { // a trait has access to the associated items of its supertraits this = any(TraitItemNode trait | - result = trait.resolveABound().getASuccessorRec(name) and + result = trait.resolveABound().getASuccessor(name, kind) and + kind.isExternalOrBoth() and result instanceof AssocItemNode and not trait.hasAssocItem(name) ) @@ -163,50 +242,39 @@ abstract class ItemNode extends Locatable { // items made available by an implementation where `this` is the implementing type exists(ItemNode node | this = node.(ImplItemNode).resolveSelfTy() and - result = node.getASuccessorRec(name) and - result instanceof AssocItemNode and - not result instanceof TypeAlias + result = node.getASuccessor(name, kind) and + kind.isExternalOrBoth() and + result instanceof AssocItemNode ) or // trait items with default implementations made available in an implementation exists(ImplItemNode impl, ItemNode trait | this = impl and trait = impl.resolveTraitTy() and - result = trait.getASuccessorRec(name) and + result = trait.getASuccessor(name, kind) and result.(AssocItemNode).hasImplementation() and + kind.isExternalOrBoth() and not impl.hasAssocItem(name) ) or // type parameters have access to the associated items of its bounds - result = this.(TypeParamItemNode).resolveABound().getASuccessorRec(name).(AssocItemNode) + result = this.(TypeParamItemNode).resolveABound().getASuccessor(name, kind).(AssocItemNode) and + kind.isExternalOrBoth() or - result = this.(ImplTraitTypeReprItemNode).resolveABound().getASuccessorRec(name).(AssocItemNode) + result = + this.(ImplTraitTypeReprItemNode).resolveABound().getASuccessor(name, kind).(AssocItemNode) and + kind.isExternalOrBoth() or - result = this.(TypeAliasItemNode).resolveAlias().getASuccessorRec(name) and - // type parameters defined in the RHS are not available in the LHS - not result instanceof TypeParam - } - - /** - * Gets a successor named `name` of this item, if any. - * - * Whenever a function exists in both source code and in library code, - * both are included - */ - cached - ItemNode getASuccessor(string name) { - Stages::PathResolutionStage::ref() and - result = this.getASuccessorRec(name) - or - preludeEdge(this, name, result) - or - this instanceof SourceFile and - builtin(name, result) + result = this.(TypeAliasItemNode).resolveAlias().getASuccessor(name, kind) and + kind.isExternalOrBoth() or name = "super" and if this instanceof Module or this instanceof SourceFile - then result = this.getImmediateParentModule() - else result = this.getImmediateParentModule().getImmediateParentModule() + then ( + kind.isBoth() and result = this.getImmediateParentModule() + ) else ( + kind.isInternal() and result = this.getImmediateParentModule().getImmediateParentModule() + ) or name = "self" and if @@ -214,51 +282,40 @@ abstract class ItemNode extends Locatable { this instanceof Enum or this instanceof Struct or this instanceof Crate - then result = this - else result = this.getImmediateParentModule() + then ( + kind.isBoth() and + result = this + ) else ( + kind.isInternal() and + result = this.getImmediateParentModule() + ) or - name = "Self" and - this = result.(ImplOrTraitItemNode).getAnItemInSelfScope() - or - name = "crate" and - this = result.(CrateItemNode).getASourceFile() - or - // todo: implement properly - name = "$crate" and - result = any(CrateItemNode crate | this = crate.getASourceFile()).(Crate).getADependency*() and - result.(CrateItemNode).isPotentialDollarCrateTarget() + kind.isInternal() and + ( + preludeEdge(this, name, result) + or + this instanceof SourceFile and + builtin(name, result) + or + name = "Self" and + this = result.(ImplOrTraitItemNode).getAnItemInSelfScope() + or + name = "crate" and + this = result.(CrateItemNode).getASourceFile() + or + // todo: implement properly + name = "$crate" and + result = any(CrateItemNode crate | this = crate.getASourceFile()).(Crate).getADependency*() and + result.(CrateItemNode).isPotentialDollarCrateTarget() + ) } - /** - * Holds if the successor `item` with the name `name` is not available locally - * for unqualified paths. - * - * This has the effect that a path of the form `name` inside `this` will not - * resolve to `item`. - */ - pragma[nomagic] - predicate excludedLocally(string name, ItemNode item) { - // Associated items in an impl or trait block are not directly available - // inside the block, they require a qualified path with a `Self` prefix. - item = this.getAChildSuccessor(name) and - this instanceof ImplOrTraitItemNode and - item instanceof AssocItemNode - } - - /** - * Holds if the successor `item` with the name `name` is not available - * externally for qualified paths that resolve to this item. - * - * This has the effect that a path of the form `Qualifier::name`, where - * `Qualifier` resolves to this item, will not resolve to `item`. - */ - pragma[nomagic] - predicate excludedExternally(string name, ItemNode item) { - // Type parameters for an `impl` or trait block are not available outside of - // the block. - item = this.getAChildSuccessor(name) and - this instanceof ImplOrTraitItemNode and - item instanceof TypeParamItemNode + /** Gets an _external_ successor named `name`, if any. */ + ItemNode getASuccessor(string name) { + exists(SuccessorKind kind | + result = this.getASuccessor(name, kind) and + kind.isExternalOrBoth() + ) } /** Holds if this item has a canonical path belonging to the crate `c`. */ @@ -552,7 +609,7 @@ abstract class ImplOrTraitItemNode extends ItemNode { Path getASelfPath() { Stages::PathResolutionStage::ref() and isUnqualifiedSelfPath(result) and - this = unqualifiedPathLookup(result, _) + this = unqualifiedPathLookup(result, _, _) } /** Gets an associated item belonging to this trait or `impl` block. */ @@ -580,7 +637,7 @@ class ImplItemNode extends ImplOrTraitItemNode instanceof Impl { Path getTraitPath() { result = super.getTrait().(PathTypeRepr).getPath() } - ItemNode resolveSelfTy() { result = resolvePath(this.getSelfPath()) } + TypeItemNode resolveSelfTy() { result = resolvePath(this.getSelfPath()) } TraitItemNode resolveTraitTy() { result = resolvePath(this.getTraitPath()) } @@ -615,7 +672,7 @@ class ImplItemNode extends ImplOrTraitItemNode instanceof Impl { if this.hasCanonicalPath(c2) then c1 = c2 else ( - c2 = c1.getADependency() or c1 = c2.getADependency() + c2 = c1.getADependency+() or c1 = c2.getADependency+() ) ) } @@ -669,7 +726,7 @@ class ImplItemNode extends ImplOrTraitItemNode instanceof Impl { } } -private class ImplTraitTypeReprItemNode extends ItemNode instanceof ImplTraitTypeRepr { +private class ImplTraitTypeReprItemNode extends TypeItemNode instanceof ImplTraitTypeRepr { pragma[nomagic] Path getABoundPath() { result = super.getTypeBoundList().getABound().getTypeRepr().(PathTypeRepr).getPath() @@ -1090,10 +1147,10 @@ predicate fileImport(Module m, SourceFile f) { * in scope under the name `name`. */ pragma[nomagic] -private predicate fileImportEdge(Module mod, string name, ItemNode item) { +private predicate fileImportEdge(Module mod, string name, ItemNode item, SuccessorKind kind) { exists(SourceFileItemNode f | fileImport(mod, f) and - item = f.getASuccessorRec(name) + item = f.getASuccessor(name, kind) ) } @@ -1101,9 +1158,9 @@ private predicate fileImportEdge(Module mod, string name, ItemNode item) { * Holds if crate `c` defines the item `i` named `name`. */ pragma[nomagic] -private predicate crateDefEdge(CrateItemNode c, string name, ItemNode i) { - i = c.getSourceFile().getASuccessorRec(name) and - not i instanceof Crate +private predicate crateDefEdge(CrateItemNode c, string name, ItemNode i, SuccessorKind kind) { + i = c.getSourceFile().getASuccessor(name, kind) and + kind.isExternalOrBoth() } private class BuiltinSourceFile extends SourceFileItemNode { @@ -1116,11 +1173,6 @@ private class BuiltinSourceFile extends SourceFileItemNode { pragma[nomagic] private predicate crateDependencyEdge(SourceFileItemNode file, string name, CrateItemNode dep) { exists(CrateItemNode c | dep = c.(Crate).getDependency(name) | file = c.getASourceFile()) - or - // Give builtin files access to `std` - file instanceof BuiltinSourceFile and - dep.getName() = name and - name = "std" } private predicate useTreeDeclares(UseTree tree, string name) { @@ -1147,12 +1199,13 @@ private predicate useTreeDeclares(UseTree tree, string name) { */ pragma[nomagic] private predicate declares(ItemNode item, Namespace ns, string name) { - exists(ItemNode child | child.getImmediateParent() = item | - child.getName() = name and + exists(ItemNode child, SuccessorKind kind | child = getAChildSuccessor(item, name, kind) | child.getNamespace() = ns and - // If `item` is excluded locally then it does not declare `name`. - not item.excludedLocally(name, child) - or + kind.isInternalOrBoth() + ) + or + exists(ItemNode child | + child.getImmediateParent() = item and useTreeDeclares(child.(Use).getUseTree(), name) and exists(ns) // `use foo::bar` can refer to both a value and a type ) @@ -1224,8 +1277,8 @@ private predicate unqualifiedPathLookup(ItemNode encl, string name, Namespace ns } pragma[nomagic] -private ItemNode getASuccessor(ItemNode pred, string name, Namespace ns) { - result = pred.getASuccessor(name) and +private ItemNode getASuccessor(ItemNode pred, string name, Namespace ns, SuccessorKind kind) { + result = pred.getASuccessor(name, kind) and ns = result.getNamespace() } @@ -1258,9 +1311,10 @@ private predicate keywordLookup(ItemNode encl, string name, RelevantPath p) { } pragma[nomagic] -private ItemNode unqualifiedPathLookup(RelevantPath p, Namespace ns) { +private ItemNode unqualifiedPathLookup(RelevantPath p, Namespace ns, SuccessorKind kind) { exists(ItemNode encl, string name | - result = getASuccessor(encl, name, ns) and not encl.excludedLocally(name, result) + result = getASuccessor(encl, name, ns, kind) and + kind.isInternalOrBoth() | unqualifiedPathLookup(encl, name, ns, p) or @@ -1272,9 +1326,9 @@ pragma[nomagic] private predicate isUnqualifiedSelfPath(RelevantPath path) { path.isUnqualified("Self") } pragma[nomagic] -private ItemNode resolvePath0(RelevantPath path, Namespace ns) { +private ItemNode resolvePath0(RelevantPath path, Namespace ns, SuccessorKind kind) { exists(ItemNode res | - res = unqualifiedPathLookup(path, ns) and + res = unqualifiedPathLookup(path, ns, kind) and if not any(RelevantPath parent).getQualifier() = path and isUnqualifiedSelfPath(path) and @@ -1285,11 +1339,11 @@ private ItemNode resolvePath0(RelevantPath path, Namespace ns) { or exists(ItemNode q, string name | q = resolvePathQualifier(path, name) and - result = getASuccessor(q, name, ns) and - not q.excludedExternally(name, result) + result = getASuccessor(q, name, ns, kind) and + kind.isExternalOrBoth() ) or - result = resolveUseTreeListItem(_, _, path) and + result = resolveUseTreeListItem(_, _, path, kind) and ns = result.getNamespace() } @@ -1326,7 +1380,17 @@ private predicate pathUsesNamespace(Path p, Namespace n) { /** Gets the item that `path` resolves to, if any. */ cached ItemNode resolvePath(RelevantPath path) { - exists(Namespace ns | result = resolvePath0(path, ns) | + exists(Namespace ns | + result = resolvePath0(path, ns, _) and + if path = any(ImplItemNode i).getSelfPath() + then + result instanceof TypeItemNode and + not result instanceof TraitItemNode + else + if path = any(ImplItemNode i).getTraitPath() + then result instanceof TraitItemNode + else any() + | pathUsesNamespace(path, ns) or not pathUsesNamespace(path, _) and @@ -1357,17 +1421,20 @@ private predicate isUseTreeSubPathUnqualified(UseTree tree, RelevantPath path, s } pragma[nomagic] -private ItemNode resolveUseTreeListItem(Use use, UseTree tree, RelevantPath path) { - exists(UseTree midTree, ItemNode mid, string name | - mid = resolveUseTreeListItem(use, midTree) and - tree = midTree.getUseTreeList().getAUseTree() and - isUseTreeSubPathUnqualified(tree, path, pragma[only_bind_into](name)) and - result = mid.getASuccessor(pragma[only_bind_into](name)) - ) - or - exists(ItemNode q, string name | - q = resolveUseTreeListItemQualifier(use, tree, path, name) and - result = q.getASuccessor(name) +private ItemNode resolveUseTreeListItem(Use use, UseTree tree, RelevantPath path, SuccessorKind kind) { + kind.isExternalOrBoth() and + ( + exists(UseTree midTree, ItemNode mid, string name | + mid = resolveUseTreeListItem(use, midTree) and + tree = midTree.getUseTreeList().getAUseTree() and + isUseTreeSubPathUnqualified(tree, path, pragma[only_bind_into](name)) and + result = mid.getASuccessor(pragma[only_bind_into](name), kind) + ) + or + exists(ItemNode q, string name | + q = resolveUseTreeListItemQualifier(use, tree, path, name) and + result = q.getASuccessor(name, kind) + ) ) } @@ -1375,7 +1442,7 @@ pragma[nomagic] private ItemNode resolveUseTreeListItemQualifier( Use use, UseTree tree, RelevantPath path, string name ) { - result = resolveUseTreeListItem(use, tree, path.getQualifier()) and + result = resolveUseTreeListItem(use, tree, path.getQualifier(), _) and name = path.getText() } @@ -1384,23 +1451,25 @@ private ItemNode resolveUseTreeListItem(Use use, UseTree tree) { tree = use.getUseTree() and result = resolvePath(tree.getPath()) or - result = resolveUseTreeListItem(use, tree, tree.getPath()) + result = resolveUseTreeListItem(use, tree, tree.getPath(), _) } /** Holds if `use` imports `item` as `name`. */ pragma[nomagic] -private predicate useImportEdge(Use use, string name, ItemNode item) { +private predicate useImportEdge(Use use, string name, ItemNode item, SuccessorKind kind) { + (if use.hasVisibility() then kind.isBoth() else kind.isInternal()) and exists(UseTree tree, ItemNode used | used = resolveUseTreeListItem(use, tree) and not tree.hasUseTreeList() and if tree.isGlob() then - exists(ItemNode encl, Namespace ns | + exists(ItemNode encl, Namespace ns, SuccessorKind kind1 | encl.getADescendant() = use and - item = getASuccessor(used, name, ns) and + item = getASuccessor(used, name, ns, kind1) and + kind1.isExternalOrBoth() and // glob imports can be shadowed not declares(encl, ns, name) and - not name = ["super", "self", "Self", "$crate", "crate"] + not name = ["super", "self"] ) else ( item = used and @@ -1433,13 +1502,21 @@ private predicate externCrateEdge(ExternCrateItemNode ec, string name, CrateItem pragma[nomagic] private predicate preludeItem(string name, ItemNode i) { - exists(Crate stdOrCore, ModuleLikeNode mod, ModuleItemNode prelude, ModuleItemNode rust | - stdOrCore.getName() = ["std", "core"] and + exists( + Crate stdOrCore, string stdOrCoreName, ModuleLikeNode mod, ModuleItemNode prelude, + ModuleItemNode rust + | + stdOrCore.getName() = stdOrCoreName and + stdOrCoreName = ["std", "core"] and mod = stdOrCore.getSourceFile() and - prelude = mod.getASuccessorRec("prelude") and - rust = prelude.getASuccessorRec(["rust_2015", "rust_2018", "rust_2021", "rust_2024"]) and - i = rust.getASuccessorRec(name) and - not i instanceof Use + prelude = mod.getASuccessor("prelude") and + rust = prelude.getASuccessor(["rust_2015", "rust_2018", "rust_2021", "rust_2024"]) + | + i = rust.getASuccessor(name) and + not name = ["super", "self"] + or + name = stdOrCoreName and + i = stdOrCore ) } @@ -1463,7 +1540,7 @@ pragma[nomagic] private predicate builtin(string name, ItemNode i) { exists(BuiltinSourceFile builtins | builtins.getFile().getBaseName() = "types.rs" and - i = builtins.getASuccessorRec(name) + i = builtins.getASuccessor(name) ) } @@ -1490,19 +1567,19 @@ private module Debug { result = resolvePath(path) } - predicate debugUseImportEdge(Use use, string name, ItemNode item) { + predicate debugUseImportEdge(Use use, string name, ItemNode item, SuccessorKind kind) { use = getRelevantLocatable() and - useImportEdge(use, name, item) + useImportEdge(use, name, item, kind) } - ItemNode debugGetASuccessorRec(ItemNode i, string name) { + ItemNode debuggetASuccessor(ItemNode i, string name, SuccessorKind kind) { i = getRelevantLocatable() and - result = i.getASuccessor(name) + result = i.getASuccessor(name, kind) } - predicate debugFileImportEdge(Module mod, string name, ItemNode item) { + predicate debugFileImportEdge(Module mod, string name, ItemNode item, SuccessorKind kind) { mod = getRelevantLocatable() and - fileImportEdge(mod, name, item) + fileImportEdge(mod, name, item, kind) } predicate debugFileImport(Module m, SourceFile f) { diff --git a/rust/ql/lib/codeql/rust/internal/TypeInference.qll b/rust/ql/lib/codeql/rust/internal/TypeInference.qll index 07f1180f1bd..d9a5bef9a65 100644 --- a/rust/ql/lib/codeql/rust/internal/TypeInference.qll +++ b/rust/ql/lib/codeql/rust/internal/TypeInference.qll @@ -224,8 +224,9 @@ private import M2 module Consistency { import M2::Consistency - query predicate nonUniqueCertainType(AstNode n, TypePath path) { - strictcount(CertainTypeInference::inferCertainType(n, path)) > 1 + predicate nonUniqueCertainType(AstNode n, TypePath path, Type t) { + strictcount(CertainTypeInference::inferCertainType(n, path)) > 1 and + t = CertainTypeInference::inferCertainType(n, path) } } @@ -2513,7 +2514,6 @@ private module Debug { Type debugInferCertainNonUniqueType(AstNode n, TypePath path) { n = getRelevantLocatable() and - Consistency::nonUniqueCertainType(n, path) and - result = CertainTypeInference::inferCertainType(n, path) + Consistency::nonUniqueCertainType(n, path, result) } } diff --git a/rust/ql/lib/codeql/rust/internal/TypeInferenceConsistency.qll b/rust/ql/lib/codeql/rust/internal/TypeInferenceConsistency.qll index fd6257fc260..16eaff92bb6 100644 --- a/rust/ql/lib/codeql/rust/internal/TypeInferenceConsistency.qll +++ b/rust/ql/lib/codeql/rust/internal/TypeInferenceConsistency.qll @@ -17,6 +17,10 @@ query predicate illFormedTypeMention(TypeMention tm) { tm.fromSource() } +query predicate nonUniqueCertainType(AstNode n, TypePath path) { + Consistency::nonUniqueCertainType(n, path, _) +} + int getTypeInferenceInconsistencyCounts(string type) { type = "Missing type parameter ID" and result = count(TypeParameter tp | missingTypeParameterId(tp) | tp) diff --git a/rust/ql/lib/rust.qll b/rust/ql/lib/rust.qll index e7d02adea32..b46e96868f6 100644 --- a/rust/ql/lib/rust.qll +++ b/rust/ql/lib/rust.qll @@ -1,5 +1,6 @@ /** Top-level import for the Rust language pack */ +import Customizations import codeql.rust.elements import codeql.Locations import codeql.files.FileSystem diff --git a/rust/ql/src/change-notes/2025-08-20-add-customizations.md b/rust/ql/src/change-notes/2025-08-20-add-customizations.md new file mode 100644 index 00000000000..3a01298283c --- /dev/null +++ b/rust/ql/src/change-notes/2025-08-20-add-customizations.md @@ -0,0 +1,4 @@ +--- +category: fix +--- +* Fixed an inconsistency across languages where most have a `Customizations.qll` file for adding customizations, but not all did. \ No newline at end of file diff --git a/rust/ql/test/library-tests/definitions/Definitions.expected b/rust/ql/test/library-tests/definitions/Definitions.expected index f3cde895068..b6f8201240a 100644 --- a/rust/ql/test/library-tests/definitions/Definitions.expected +++ b/rust/ql/test/library-tests/definitions/Definitions.expected @@ -2,28 +2,39 @@ | main.rs:9:14:9:14 | S | main.rs:7:9:7:21 | struct S | path | | main.rs:10:36:10:39 | Self | main.rs:7:9:7:21 | struct S | path | | main.rs:11:17:11:17 | S | main.rs:7:9:7:21 | struct S | path | -| main.rs:21:22:21:26 | value | main.rs:21:50:21:54 | value | format argument | -| main.rs:21:29:21:33 | width | main.rs:18:9:18:13 | width | local variable | -| main.rs:21:36:21:44 | precision | main.rs:19:9:19:17 | precision | local variable | -| main.rs:22:22:22:22 | 0 | main.rs:22:34:22:38 | value | format argument | -| main.rs:22:25:22:25 | 1 | main.rs:22:41:22:45 | width | format argument | -| main.rs:22:28:22:28 | 2 | main.rs:22:48:22:56 | precision | format argument | -| main.rs:22:34:22:38 | value | main.rs:20:9:20:13 | value | local variable | -| main.rs:22:41:22:45 | width | main.rs:18:9:18:13 | width | local variable | -| main.rs:22:48:22:56 | precision | main.rs:19:9:19:17 | precision | local variable | -| main.rs:23:21:23:22 | {} | main.rs:23:29:23:33 | value | format argument | -| main.rs:23:24:23:25 | {} | main.rs:23:36:23:40 | width | format argument | -| main.rs:23:29:23:33 | value | main.rs:20:9:20:13 | value | local variable | -| main.rs:23:36:23:40 | width | main.rs:18:9:18:13 | width | local variable | -| main.rs:25:22:25:27 | people | main.rs:24:9:24:14 | people | local variable | -| main.rs:26:16:26:16 | 1 | main.rs:26:34:26:34 | 2 | format argument | -| main.rs:26:19:26:20 | {} | main.rs:26:31:26:31 | 1 | format argument | -| main.rs:26:23:26:23 | 0 | main.rs:26:31:26:31 | 1 | format argument | -| main.rs:26:26:26:27 | {} | main.rs:26:34:26:34 | 2 | format argument | -| main.rs:27:31:27:35 | {:<5} | main.rs:27:40:27:42 | "x" | format argument | -| main.rs:28:13:28:13 | S | main.rs:1:1:1:9 | struct S | path | -| main.rs:29:13:29:14 | M1 | main.rs:5:1:15:1 | mod M1 | path | -| main.rs:29:17:29:18 | M2 | main.rs:6:5:14:5 | mod M2 | path | -| main.rs:29:21:29:21 | S | main.rs:7:9:7:21 | struct S | path | -| main.rs:30:5:30:5 | s | main.rs:29:9:29:9 | s | local variable | -| main.rs:30:7:30:12 | method | main.rs:10:13:12:13 | fn method | method | +| main.rs:16:22:16:22 | T | main.rs:16:19:16:19 | T | path | +| main.rs:18:13:18:14 | S2 | main.rs:16:5:16:24 | struct S2 | path | +| main.rs:18:16:18:16 | T | main.rs:18:10:18:10 | T | path | +| main.rs:19:23:19:23 | T | main.rs:18:10:18:10 | T | path | +| main.rs:19:29:19:32 | Self | main.rs:16:5:16:24 | struct S2 | path | +| main.rs:20:16:20:16 | x | main.rs:19:20:19:20 | x | local variable | +| main.rs:29:22:29:26 | value | main.rs:29:50:29:54 | value | format argument | +| main.rs:29:29:29:33 | width | main.rs:26:9:26:13 | width | local variable | +| main.rs:29:36:29:44 | precision | main.rs:27:9:27:17 | precision | local variable | +| main.rs:30:22:30:22 | 0 | main.rs:30:34:30:38 | value | format argument | +| main.rs:30:25:30:25 | 1 | main.rs:30:41:30:45 | width | format argument | +| main.rs:30:28:30:28 | 2 | main.rs:30:48:30:56 | precision | format argument | +| main.rs:30:34:30:38 | value | main.rs:28:9:28:13 | value | local variable | +| main.rs:30:41:30:45 | width | main.rs:26:9:26:13 | width | local variable | +| main.rs:30:48:30:56 | precision | main.rs:27:9:27:17 | precision | local variable | +| main.rs:31:21:31:22 | {} | main.rs:31:29:31:33 | value | format argument | +| main.rs:31:24:31:25 | {} | main.rs:31:36:31:40 | width | format argument | +| main.rs:31:29:31:33 | value | main.rs:28:9:28:13 | value | local variable | +| main.rs:31:36:31:40 | width | main.rs:26:9:26:13 | width | local variable | +| main.rs:33:22:33:27 | people | main.rs:32:9:32:14 | people | local variable | +| main.rs:34:16:34:16 | 1 | main.rs:34:34:34:34 | 2 | format argument | +| main.rs:34:19:34:20 | {} | main.rs:34:31:34:31 | 1 | format argument | +| main.rs:34:23:34:23 | 0 | main.rs:34:31:34:31 | 1 | format argument | +| main.rs:34:26:34:27 | {} | main.rs:34:34:34:34 | 2 | format argument | +| main.rs:35:31:35:35 | {:<5} | main.rs:35:40:35:42 | "x" | format argument | +| main.rs:36:13:36:13 | S | main.rs:1:1:1:9 | struct S | path | +| main.rs:37:13:37:14 | M1 | main.rs:5:1:23:1 | mod M1 | path | +| main.rs:37:17:37:18 | M2 | main.rs:6:5:14:5 | mod M2 | path | +| main.rs:37:21:37:21 | S | main.rs:7:9:7:21 | struct S | path | +| main.rs:38:5:38:5 | s | main.rs:37:9:37:9 | s | local variable | +| main.rs:38:7:38:12 | method | main.rs:10:13:12:13 | fn method | method | +| main.rs:39:5:39:6 | M1 | main.rs:5:1:23:1 | mod M1 | path | +| main.rs:39:9:39:10 | S2 | main.rs:16:5:16:24 | struct S2 | path | +| main.rs:39:14:39:14 | S | main.rs:1:1:1:9 | struct S | path | +| main.rs:39:18:39:20 | new | main.rs:19:9:21:9 | fn new | path | +| main.rs:39:22:39:22 | S | main.rs:1:1:1:9 | struct S | path | diff --git a/rust/ql/test/library-tests/definitions/main.rs b/rust/ql/test/library-tests/definitions/main.rs index 35acea6858e..89742adf8f9 100644 --- a/rust/ql/test/library-tests/definitions/main.rs +++ b/rust/ql/test/library-tests/definitions/main.rs @@ -12,6 +12,14 @@ mod M1 { } } } + + pub struct S2(T); + + impl S2 { + pub fn new(x: T) -> Self { + S2(x) + } + } } fn main() { @@ -28,4 +36,5 @@ fn main() { let x = S; let s = M1::M2::S; s.method(); + M1::S2::::new(S); } diff --git a/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/TypeInferenceConsistency.expected b/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/TypeInferenceConsistency.expected deleted file mode 100644 index 6a60922b6f0..00000000000 --- a/rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/TypeInferenceConsistency.expected +++ /dev/null @@ -1,3 +0,0 @@ -nonUniqueCertainType -| sqlx.rs:158:13:158:81 | { ... } | E | -| sqlx.rs:160:17:160:86 | { ... } | E | diff --git a/rust/ql/test/query-tests/security/CWE-312/CONSISTENCY/TypeInferenceConsistency.expected b/rust/ql/test/query-tests/security/CWE-312/CONSISTENCY/TypeInferenceConsistency.expected deleted file mode 100644 index 952a4afe86b..00000000000 --- a/rust/ql/test/query-tests/security/CWE-312/CONSISTENCY/TypeInferenceConsistency.expected +++ /dev/null @@ -1,4 +0,0 @@ -nonUniqueCertainType -| test_logging.rs:17:8:19:12 | { ... } | E | -| test_logging.rs:29:8:31:12 | { ... } | E | -| test_storage.rs:177:8:179:9 | { ... } | E | diff --git a/rust/ql/test/query-tests/security/CWE-327/BrokenCryptoAlgorithm.expected b/rust/ql/test/query-tests/security/CWE-327/BrokenCryptoAlgorithm.expected index f1395ff39ec..ef0a9e0d806 100644 --- a/rust/ql/test/query-tests/security/CWE-327/BrokenCryptoAlgorithm.expected +++ b/rust/ql/test/query-tests/security/CWE-327/BrokenCryptoAlgorithm.expected @@ -2,20 +2,22 @@ | test_cipher.rs:23:27:23:60 | ...::new_from_slice(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:23:27:23:60 | ...::new_from_slice(...) | The cryptographic algorithm RC4 | | test_cipher.rs:26:27:26:48 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:26:27:26:48 | ...::new(...) | The cryptographic algorithm RC4 | | test_cipher.rs:29:27:29:48 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:29:27:29:48 | ...::new(...) | The cryptographic algorithm RC4 | -| test_cipher.rs:59:23:59:42 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:59:23:59:42 | ...::new(...) | The cryptographic algorithm DES | -| test_cipher.rs:63:23:63:47 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:63:23:63:47 | ...::new(...) | The cryptographic algorithm DES | -| test_cipher.rs:67:23:67:46 | ...::new_from_slice(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:67:23:67:46 | ...::new_from_slice(...) | The cryptographic algorithm DES | -| test_cipher.rs:71:23:71:42 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:71:23:71:42 | ...::new(...) | The cryptographic algorithm DES | -| test_cipher.rs:75:27:75:46 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:75:27:75:46 | ...::new(...) | The cryptographic algorithm DES | -| test_cipher.rs:80:24:80:48 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:80:24:80:48 | ...::new(...) | The cryptographic algorithm 3DES | +| test_cipher.rs:59:29:59:45 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:59:29:59:45 | ...::new(...) | The cryptographic algorithm DES | +| test_cipher.rs:63:23:63:42 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:63:23:63:42 | ...::new(...) | The cryptographic algorithm DES | +| test_cipher.rs:67:23:67:47 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:67:23:67:47 | ...::new(...) | The cryptographic algorithm DES | +| test_cipher.rs:71:23:71:46 | ...::new_from_slice(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:71:23:71:46 | ...::new_from_slice(...) | The cryptographic algorithm DES | +| test_cipher.rs:75:23:75:42 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:75:23:75:42 | ...::new(...) | The cryptographic algorithm DES | +| test_cipher.rs:79:27:79:46 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:79:27:79:46 | ...::new(...) | The cryptographic algorithm DES | | test_cipher.rs:84:24:84:48 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:84:24:84:48 | ...::new(...) | The cryptographic algorithm 3DES | +| test_cipher.rs:84:24:84:48 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:84:24:84:48 | ...::new(...) | The cryptographic algorithm DES | | test_cipher.rs:88:24:88:48 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:88:24:88:48 | ...::new(...) | The cryptographic algorithm 3DES | -| test_cipher.rs:92:24:92:52 | ...::new_from_slice(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:92:24:92:52 | ...::new_from_slice(...) | The cryptographic algorithm 3DES | -| test_cipher.rs:97:23:97:42 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:97:23:97:42 | ...::new(...) | The cryptographic algorithm RC2 | -| test_cipher.rs:101:23:101:46 | ...::new_from_slice(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:101:23:101:46 | ...::new_from_slice(...) | The cryptographic algorithm RC2 | -| test_cipher.rs:105:23:105:56 | ...::new_with_eff_key_len(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:105:23:105:56 | ...::new_with_eff_key_len(...) | The cryptographic algorithm RC2 | -| test_cipher.rs:110:23:110:50 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:110:23:110:50 | ...::new(...) | The cryptographic algorithm RC5 | -| test_cipher.rs:114:23:114:55 | ...::new_from_slice(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:114:23:114:55 | ...::new_from_slice(...) | The cryptographic algorithm RC5 | -| test_cipher.rs:132:23:132:76 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:132:23:132:76 | ...::new(...) | The cryptographic algorithm DES | -| test_cipher.rs:138:23:138:76 | ...::new_from_slices(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:138:23:138:76 | ...::new_from_slices(...) | The cryptographic algorithm DES | -| test_cipher.rs:141:23:141:76 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:141:23:141:76 | ...::new(...) | The cryptographic algorithm DES | +| test_cipher.rs:88:24:88:48 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:88:24:88:48 | ...::new(...) | The cryptographic algorithm DES | +| test_cipher.rs:92:24:92:48 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:92:24:92:48 | ...::new(...) | The cryptographic algorithm 3DES | +| test_cipher.rs:92:24:92:48 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:92:24:92:48 | ...::new(...) | The cryptographic algorithm DES | +| test_cipher.rs:96:24:96:52 | ...::new_from_slice(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:96:24:96:52 | ...::new_from_slice(...) | The cryptographic algorithm 3DES | +| test_cipher.rs:96:24:96:52 | ...::new_from_slice(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:96:24:96:52 | ...::new_from_slice(...) | The cryptographic algorithm DES | +| test_cipher.rs:101:23:101:42 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:101:23:101:42 | ...::new(...) | The cryptographic algorithm RC2 | +| test_cipher.rs:105:23:105:46 | ...::new_from_slice(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:105:23:105:46 | ...::new_from_slice(...) | The cryptographic algorithm RC2 | +| test_cipher.rs:109:23:109:56 | ...::new_with_eff_key_len(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:109:23:109:56 | ...::new_with_eff_key_len(...) | The cryptographic algorithm RC2 | +| test_cipher.rs:114:23:114:50 | ...::new(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:114:23:114:50 | ...::new(...) | The cryptographic algorithm RC5 | +| test_cipher.rs:118:23:118:55 | ...::new_from_slice(...) | $@ is broken or weak, and should not be used. | test_cipher.rs:118:23:118:55 | ...::new_from_slice(...) | The cryptographic algorithm RC5 | diff --git a/rust/ql/test/query-tests/security/CWE-327/CONSISTENCY/PathResolutionConsistency.expected b/rust/ql/test/query-tests/security/CWE-327/CONSISTENCY/PathResolutionConsistency.expected index 3d73ede26c5..50dd9e9f0ed 100644 --- a/rust/ql/test/query-tests/security/CWE-327/CONSISTENCY/PathResolutionConsistency.expected +++ b/rust/ql/test/query-tests/security/CWE-327/CONSISTENCY/PathResolutionConsistency.expected @@ -4,4 +4,4 @@ multipleCallTargets | test_cipher.rs:29:27:29:48 | ...::new(...) | | test_cipher.rs:36:30:36:59 | ...::new(...) | | test_cipher.rs:39:30:39:63 | ...::new(...) | -| test_cipher.rs:110:23:110:50 | ...::new(...) | +| test_cipher.rs:114:23:114:50 | ...::new(...) | diff --git a/rust/ql/test/query-tests/security/CWE-327/test_cipher.rs b/rust/ql/test/query-tests/security/CWE-327/test_cipher.rs index 0cf20c4c278..61471ac99ec 100644 --- a/rust/ql/test/query-tests/security/CWE-327/test_cipher.rs +++ b/rust/ql/test/query-tests/security/CWE-327/test_cipher.rs @@ -42,7 +42,7 @@ fn test_stream_cipher( fn test_block_cipher( key: &[u8], key128: &[u8;16], key192: &[u8;24], key256: &[u8;32], - data: &mut [u8], input: &[u8], block128: &mut [u8;16] + data: &mut [u8], input: &[u8], block128: &mut [u8;16], des_key : &cipher::Key ) { // aes let aes_cipher1 = Aes128::new(key128.into()); @@ -56,6 +56,10 @@ fn test_block_cipher( aes_cipher3.decrypt_block(block128.into()); // des (broken) + let des_cipher0 : Des = Des::new(des_key); // $ Alert[rust/weak-cryptographic-algorithm] + des_cipher0.encrypt_block(data.into()); + des_cipher0.decrypt_block(data.into()); + let des_cipher1 = Des::new(key.into()); // $ Alert[rust/weak-cryptographic-algorithm] des_cipher1.encrypt_block(data.into()); des_cipher1.decrypt_block(data.into()); @@ -129,15 +133,15 @@ fn test_cbc( _ = aes_cipher1.encrypt_padded_mut::(data, data_len).unwrap(); // des (broken) - let des_cipher1 = cbc::Encryptor::::new(key.into(), iv.into()); // $ Alert[rust/weak-cryptographic-algorithm] + let des_cipher1 = cbc::Encryptor::::new(key.into(), iv.into()); // $ MISSING: Alert[rust/weak-cryptographic-algorithm] _ = des_cipher1.encrypt_padded_mut::(data, data_len).unwrap(); let des_cipher2 = MyDesEncryptor::new(key.into(), iv.into()); // $ MISSING: Alert[rust/weak-cryptographic-algorithm] _ = des_cipher2.encrypt_padded_mut::(data, data_len).unwrap(); - let des_cipher3 = cbc::Encryptor::::new_from_slices(&key, &iv).unwrap(); // $ Alert[rust/weak-cryptographic-algorithm] + let des_cipher3 = cbc::Encryptor::::new_from_slices(&key, &iv).unwrap(); // $ MISSING: Alert[rust/weak-cryptographic-algorithm] _ = des_cipher3.encrypt_padded_mut::(data, data_len).unwrap(); - let des_cipher4 = cbc::Encryptor::::new(key.into(), iv.into()); // $ Alert[rust/weak-cryptographic-algorithm] + let des_cipher4 = cbc::Encryptor::::new(key.into(), iv.into()); // $ MISSING: Alert[rust/weak-cryptographic-algorithm] _ = des_cipher4.encrypt_padded_b2b_mut::(input, data).unwrap(); } diff --git a/rust/ql/test/query-tests/security/CWE-770/CONSISTENCY/PathResolutionConsistency.expected b/rust/ql/test/query-tests/security/CWE-770/CONSISTENCY/PathResolutionConsistency.expected index 6e88d5bdf86..6c3b9efca14 100644 --- a/rust/ql/test/query-tests/security/CWE-770/CONSISTENCY/PathResolutionConsistency.expected +++ b/rust/ql/test/query-tests/security/CWE-770/CONSISTENCY/PathResolutionConsistency.expected @@ -1,18 +1,3 @@ multipleCallTargets -| main.rs:218:14:218:30 | ...::malloc(...) | -| main.rs:219:13:219:27 | ...::malloc(...) | -| main.rs:220:13:220:37 | ...::aligned_alloc(...) | -| main.rs:221:13:221:37 | ...::aligned_alloc(...) | -| main.rs:222:13:222:31 | ...::calloc(...) | -| main.rs:223:13:223:55 | ...::calloc(...) | -| main.rs:224:13:224:32 | ...::realloc(...) | | main.rs:229:13:229:40 | ...::with_capacity(...) | | main.rs:233:18:233:47 | ...::with_capacity(...) | -multiplePathResolutions -| main.rs:218:14:218:17 | libc | -| main.rs:219:13:219:16 | libc | -| main.rs:220:13:220:16 | libc | -| main.rs:221:13:221:16 | libc | -| main.rs:222:13:222:16 | libc | -| main.rs:223:13:223:16 | libc | -| main.rs:224:13:224:16 | libc | diff --git a/rust/ql/test/query-tests/security/CWE-825/CONSISTENCY/PathResolutionConsistency.expected b/rust/ql/test/query-tests/security/CWE-825/CONSISTENCY/PathResolutionConsistency.expected index 22e607bfa48..d13271f5fe6 100644 --- a/rust/ql/test/query-tests/security/CWE-825/CONSISTENCY/PathResolutionConsistency.expected +++ b/rust/ql/test/query-tests/security/CWE-825/CONSISTENCY/PathResolutionConsistency.expected @@ -1,6 +1,4 @@ multipleCallTargets -| deallocation.rs:106:16:106:32 | ...::malloc(...) | -| deallocation.rs:112:3:112:41 | ...::free(...) | | deallocation.rs:260:11:260:29 | ...::from(...) | | deallocation.rs:261:11:261:29 | ...::from(...) | | lifetime.rs:610:13:610:31 | ...::from(...) | @@ -9,7 +7,3 @@ multipleCallTargets | lifetime.rs:612:41:612:52 | bar.as_str() | | lifetime.rs:628:13:628:31 | ...::from(...) | | lifetime.rs:629:32:629:43 | baz.as_str() | -multiplePathResolutions -| deallocation.rs:106:16:106:19 | libc | -| deallocation.rs:112:3:112:6 | libc | -| deallocation.rs:112:29:112:32 | libc | diff --git a/shared/controlflow/codeql/controlflow/Guards.qll b/shared/controlflow/codeql/controlflow/Guards.qll index ab500456058..917f95569ed 100644 --- a/shared/controlflow/codeql/controlflow/Guards.qll +++ b/shared/controlflow/codeql/controlflow/Guards.qll @@ -873,77 +873,85 @@ module Make Input> { private signature predicate baseGuardValueSig(Guard guard, GuardValue v); - /** - * Calculates the transitive closure of all the guard implication steps - * starting from a given set of base cases. - */ cached - private module ImpliesTC { + private module Cached { /** - * Holds if `tgtGuard` evaluating to `tgtVal` implies that `guard` - * evaluates to `v`. + * Calculates the transitive closure of all the guard implication steps + * starting from a given set of base cases. */ - pragma[nomagic] cached - predicate guardControls(Guard guard, GuardValue v, Guard tgtGuard, GuardValue tgtVal) { - baseGuardValue(tgtGuard, tgtVal) and - guard = tgtGuard and - v = tgtVal - or - exists(Guard g0, GuardValue v0 | - guardControls(g0, v0, tgtGuard, tgtVal) and - impliesStep2(g0, v0, guard, v) - ) - or - exists(Guard g0, GuardValue v0 | - guardControls(g0, v0, tgtGuard, tgtVal) and - unboundImpliesStep(g0, v0, guard, v) - ) - or - exists(SsaDefinition def0, GuardValue v0 | - ssaControls(def0, v0, tgtGuard, tgtVal) and - impliesStepSsaGuard(def0, v0, guard, v) - ) - or - exists(Guard g0, GuardValue v0 | - guardControls(g0, v0, tgtGuard, tgtVal) and - WrapperGuard::wrapperImpliesStep(g0, v0, guard, v) - ) - or - exists(Guard g0, GuardValue v0 | - guardControls(g0, v0, tgtGuard, tgtVal) and - additionalImpliesStep(g0, v0, guard, v) - ) + module ImpliesTC { + /** + * Holds if `tgtGuard` evaluating to `tgtVal` implies that `guard` + * evaluates to `v`. + */ + pragma[nomagic] + cached + predicate guardControls(Guard guard, GuardValue v, Guard tgtGuard, GuardValue tgtVal) { + baseGuardValue(tgtGuard, tgtVal) and + guard = tgtGuard and + v = tgtVal + or + exists(Guard g0, GuardValue v0 | + guardControls(g0, v0, tgtGuard, tgtVal) and + impliesStep2(g0, v0, guard, v) + ) + or + exists(Guard g0, GuardValue v0 | + guardControls(g0, v0, tgtGuard, tgtVal) and + unboundImpliesStep(g0, v0, guard, v) + ) + or + exists(SsaDefinition def0, GuardValue v0 | + ssaControls(def0, v0, tgtGuard, tgtVal) and + impliesStepSsaGuard(def0, v0, guard, v) + ) + or + exists(Guard g0, GuardValue v0 | + guardControls(g0, v0, tgtGuard, tgtVal) and + WrapperGuard::wrapperImpliesStep(g0, v0, guard, v) + ) + or + exists(Guard g0, GuardValue v0 | + guardControls(g0, v0, tgtGuard, tgtVal) and + additionalImpliesStep(g0, v0, guard, v) + ) + } + + /** + * Holds if `tgtGuard` evaluating to `tgtVal` implies that `def` + * evaluates to `v`. + */ + pragma[nomagic] + cached + predicate ssaControls(SsaDefinition def, GuardValue v, Guard tgtGuard, GuardValue tgtVal) { + exists(Guard g0 | + guardControls(g0, v, tgtGuard, tgtVal) and + guardReadsSsaVar(g0, def) + ) + or + exists(SsaDefinition def0 | + ssaControls(def0, v, tgtGuard, tgtVal) and + impliesStepSsa(def0, v, def) + ) + } } /** - * Holds if `tgtGuard` evaluating to `tgtVal` implies that `def` - * evaluates to `v`. + * Holds if `guard` evaluating to `v` implies that `e` is guaranteed to be + * null if `isNull` is true, and non-null if `isNull` is false. */ - pragma[nomagic] cached - predicate ssaControls(SsaDefinition def, GuardValue v, Guard tgtGuard, GuardValue tgtVal) { - exists(Guard g0 | - guardControls(g0, v, tgtGuard, tgtVal) and - guardReadsSsaVar(g0, def) - ) - or - exists(SsaDefinition def0 | - ssaControls(def0, v, tgtGuard, tgtVal) and - impliesStepSsa(def0, v, def) - ) + predicate nullGuard(Guard guard, GuardValue v, Expr e, boolean isNull) { + impliesStep2(guard, v, e, any(GuardValue gv | gv.isNullness(isNull))) or + WrapperGuard::wrapperImpliesStep(guard, v, e, any(GuardValue gv | gv.isNullness(isNull))) or + additionalImpliesStep(guard, v, e, any(GuardValue gv | gv.isNullness(isNull))) } } - /** - * Holds if `guard` evaluating to `v` implies that `e` is guaranteed to be - * null if `isNull` is true, and non-null if `isNull` is false. - */ - predicate nullGuard(Guard guard, GuardValue v, Expr e, boolean isNull) { - impliesStep2(guard, v, e, any(GuardValue gv | gv.isNullness(isNull))) or - WrapperGuard::wrapperImpliesStep(guard, v, e, any(GuardValue gv | gv.isNullness(isNull))) or - additionalImpliesStep(guard, v, e, any(GuardValue gv | gv.isNullness(isNull))) - } + private import Cached + + predicate nullGuard = Cached::nullGuard/4; private predicate hasAValueBranchEdge(Guard guard, GuardValue v) { guard.hasValueBranchEdge(_, _, v) diff --git a/shared/typeinference/codeql/typeinference/internal/TypeInference.qll b/shared/typeinference/codeql/typeinference/internal/TypeInference.qll index c42a424f3e3..32615dc46cd 100644 --- a/shared/typeinference/codeql/typeinference/internal/TypeInference.qll +++ b/shared/typeinference/codeql/typeinference/internal/TypeInference.qll @@ -731,20 +731,24 @@ module Make1 Input1> { IsInstantiationOfInputSig { pragma[nomagic] - private predicate typeCondition(Type type, TypeAbstraction abs, TypeMentionTypeTree lhs) { - conditionSatisfiesConstraint(abs, lhs, _) and type = resolveTypeMentionRoot(lhs) + private predicate typeCondition( + Type type, TypeAbstraction abs, TypeMentionTypeTree condition + ) { + conditionSatisfiesConstraint(abs, condition, _) and + type = resolveTypeMentionRoot(condition) } pragma[nomagic] - private predicate typeConstraint(Type type, TypeMentionTypeTree rhs) { - conditionSatisfiesConstraint(_, _, rhs) and type = resolveTypeMentionRoot(rhs) + private predicate typeConstraint(Type type, TypeMentionTypeTree constraint) { + conditionSatisfiesConstraint(_, _, constraint) and + type = resolveTypeMentionRoot(constraint) } predicate potentialInstantiationOf( - TypeMentionTypeTree condition, TypeAbstraction abs, TypeMention constraint + TypeMentionTypeTree constraint, TypeAbstraction abs, TypeMention condition ) { exists(Type type | - typeConstraint(type, condition) and typeCondition(type, abs, constraint) + typeConstraint(type, constraint) and typeCondition(type, abs, condition) ) } } @@ -761,20 +765,20 @@ module Make1 Input1> { constraint.resolveTypeAt(path) = t or // recursive case - exists(TypeAbstraction midAbs, TypeMention midSup, TypeMention midSub | - conditionSatisfiesConstraint(abs, condition, midSup) and - // NOTE: `midAbs` describe the free type variables in `midSub`, hence + exists(TypeAbstraction midAbs, TypeMention midConstraint, TypeMention midCondition | + conditionSatisfiesConstraint(abs, condition, midConstraint) and + // NOTE: `midAbs` describe the free type variables in `midCondition`, hence // we use that for instantiation check. - IsInstantiationOf::isInstantiationOf(midSup, - midAbs, midSub) + IsInstantiationOf::isInstantiationOf(midConstraint, + midAbs, midCondition) | - conditionSatisfiesConstraintTypeAt(midAbs, midSub, constraint, path, t) and + conditionSatisfiesConstraintTypeAt(midAbs, midCondition, constraint, path, t) and not t = midAbs.getATypeParameter() or exists(TypePath prefix, TypePath suffix, TypeParameter tp | tp = midAbs.getATypeParameter() and - conditionSatisfiesConstraintTypeAt(midAbs, midSub, constraint, prefix, tp) and - instantiatesWith(midSup, midSub, tp, suffix, t) and + conditionSatisfiesConstraintTypeAt(midAbs, midCondition, constraint, prefix, tp) and + instantiatesWith(midConstraint, midCondition, tp, suffix, t) and path = prefix.append(suffix) ) ) @@ -949,23 +953,24 @@ module Make1 Input1> { */ pragma[nomagic] private predicate hasConstraintMention( - HasTypeTree tt, TypeAbstraction abs, TypeMention sub, Type constraint, + HasTypeTree tt, TypeAbstraction abs, TypeMention condition, Type constraint, TypeMention constraintMention ) { exists(Type type | hasTypeConstraint(tt, type, constraint) | not exists(countConstraintImplementations(type, constraint)) and - conditionSatisfiesConstraintTypeAt(abs, sub, constraintMention, _, _) and - resolveTypeMentionRoot(sub) = abs.getATypeParameter() and + conditionSatisfiesConstraintTypeAt(abs, condition, constraintMention, _, _) and + resolveTypeMentionRoot(condition) = abs.getATypeParameter() and constraint = resolveTypeMentionRoot(constraintMention) or countConstraintImplementations(type, constraint) > 0 and - rootTypesSatisfaction(type, constraint, abs, sub, constraintMention) and + rootTypesSatisfaction(type, constraint, abs, condition, constraintMention) and // When there are multiple ways the type could implement the // constraint we need to find the right implementation, which is the // one where the type instantiates the precondition. if multipleConstraintImplementations(type, constraint) then - IsInstantiationOf::isInstantiationOf(tt, abs, sub) + IsInstantiationOf::isInstantiationOf(tt, abs, + condition) else any() ) } diff --git a/swift/ql/lib/Customizations.qll b/swift/ql/lib/Customizations.qll new file mode 100644 index 00000000000..bf9e66de70b --- /dev/null +++ b/swift/ql/lib/Customizations.qll @@ -0,0 +1,11 @@ +/** + * Contains customizations to the standard library. + * + * This module is imported by `swift.qll`, so any customizations defined here automatically + * apply to all queries. + * + * Typical examples of customizations include adding new subclasses of abstract classes such as + * the `RemoteFlowSource` class to model frameworks that are not covered by the standard library. + */ + +import swift diff --git a/swift/ql/lib/swift.qll b/swift/ql/lib/swift.qll index 901d9e895e0..54f2abf9092 100644 --- a/swift/ql/lib/swift.qll +++ b/swift/ql/lib/swift.qll @@ -1,5 +1,6 @@ /** Top-level import for the Swift language pack */ +import Customizations import codeql.swift.elements import codeql.swift.elements.expr.ArithmeticOperation import codeql.swift.elements.expr.Assignment diff --git a/swift/ql/src/change-notes/2025-08-20-add-customizations.md b/swift/ql/src/change-notes/2025-08-20-add-customizations.md new file mode 100644 index 00000000000..3a01298283c --- /dev/null +++ b/swift/ql/src/change-notes/2025-08-20-add-customizations.md @@ -0,0 +1,4 @@ +--- +category: fix +--- +* Fixed an inconsistency across languages where most have a `Customizations.qll` file for adding customizations, but not all did. \ No newline at end of file