From 6864e3a1cd5d97c0db669d956e8d83efe377d496 Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Thu, 24 Sep 2026 12:22:15 +0200 Subject: [PATCH 1/2] STR30-C: use new DataFlow module --- .../DoNotAttemptToModifyStringLiterals.ql | 142 ++++++------------ ...oNotAttemptToModifyStringLiterals.expected | 15 -- 2 files changed, 43 insertions(+), 114 deletions(-) diff --git a/c/cert/src/rules/STR30-C/DoNotAttemptToModifyStringLiterals.ql b/c/cert/src/rules/STR30-C/DoNotAttemptToModifyStringLiterals.ql index 397e1bfc9e..b4ef7820aa 100644 --- a/c/cert/src/rules/STR30-C/DoNotAttemptToModifyStringLiterals.ql +++ b/c/cert/src/rules/STR30-C/DoNotAttemptToModifyStringLiterals.ql @@ -19,141 +19,85 @@ import cpp import codingstandards.c.cert import semmle.code.cpp.security.BufferWrite -import semmle.code.cpp.dataflow.DataFlow - -/** - * Class that includes into `BufferWrite` functions that will modify their - * first argument. This is an extension of `BufferWrite` which covers the case - * of opaque writes via library functions. - */ -class ModifiesFirstArgFunction extends BufferWrite, FunctionCall { - Expr modifiedExpr; +import semmle.code.cpp.dataflow.new.DataFlow +/** A modeled buffer write through the first argument of a library call. */ +private class ModifiesFirstArgFunction extends BufferWrite, FunctionCall { ModifiesFirstArgFunction() { - getTarget().getName() = ["mkstemp", "memset", "memcpy", "memmove"] and - getArgument(0) = modifiedExpr + getTarget().getName() = ["mkstemp", "memset", "memcpy", "memmove"] } override Type getBufferType() { none() } - override Expr getDest() { result = modifiedExpr } + override Expr getDest() { result = getArgument(0) } } -/** - * Models a dataflow wherein a source is either a implicit or explicit string - * literal that is assigned to a non modifiable type or wherein the string - * literal arises as a argument to a function that may modify its argument. - */ -module ImplicitOrExplicitStringLiteralModifiedConfig implements DataFlow::ConfigSig { +/** Provides dataflow from assigned string literals to writes. */ +private module StringLiteralConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node node) { - // usage through variables exists(Variable v | v.getAnAssignedValue() = node.asExpr() and - ( - node.asExpr() instanceof ImplicitStringLiteral or - node.asExpr() instanceof StringLiteralOrConstChar - ) and + mayBeStringLiteral(node.asExpr()) and v.getType().getUnderlyingType() instanceof CharPointerType ) - or - // direct usage of string literals as function parameters - exists(BufferWrite bw | - bw.getDest() = node.asExpr() and - ( - node.asExpr() instanceof ImplicitStringLiteral or - node.asExpr() instanceof StringLiteralOrConstChar - ) - ) } predicate isSink(DataFlow::Node node) { - // it's either a buffer write of some kind that we - // know about - exists(BufferWrite bw | bw.getDest() = node.asExpr()) + node.asExpr() = any(BufferWrite bw).getDest() or - // or it is a direct assignment of some kind - including reassignment of the pointer - exists(AssignExpr aexp | aexp.getLValue().(ArrayExpr).getArrayBase() = node.asExpr()) + node.asExpr() = any(AssignExpr a).getLValue().(ArrayExpr).getArrayBase() or - exists(AssignExpr aexp | aexp.getLValue().(PointerDereferenceExpr).getOperand() = node.asExpr()) + node.asExpr() = any(AssignExpr a).getLValue().(PointerDereferenceExpr).getOperand() } } -module ImplicitOrExplicitStringLiteralModifiedFlow = - DataFlow::Global; +/** Provides dataflow from possible string literals to writes. */ +private module StringLiteralFlow { + private module Global = DataFlow::Global; -class MaybeReturnsStringLiteralFunctionCall extends FunctionCall { - MaybeReturnsStringLiteralFunctionCall() { - getTarget().getName() in [ - "strpbrk", "strchr", "strrchr", "strstr", "wcspbrk", "wcschr", "wcsrchr", "wcsstr", - "memchr", "wmemchr" - ] + /** Holds if `source` may point to a string literal that is written at `sink`. */ + predicate flow(Expr source, Expr sink) { + // Report the pointer operand rather than a dereference represented by the same dataflow node. + not sink instanceof PointerDereferenceExpr and + ( + Global::flow(DataFlow::exprNode(source), DataFlow::exprNode(sink)) + or + source = sink and + mayBeStringLiteral(sink) and + sink = any(BufferWrite bw).getDest() + ) } } -class ImplicitStringLiteral extends Expr { +/** A call that may return a pointer into a possible string literal. */ +private class ImplicitStringLiteral extends FunctionCall { ImplicitStringLiteral() { - exists(MaybeReturnsStringLiteralFunctionCall fc, Variable e | - e.getAnAssignedValue() = fc and - this = fc and - // additionally, we require that the first argument is either an explicit - // or implicit string literal - ( - // directly a string literal - fc.getArgument(0) instanceof StringLiteralOrConstChar - or - // a string literal flows into it - exists(StringLiteralOrConstChar sl | - DataFlow::localFlow(DataFlow::exprNode(sl), DataFlow::exprNode(fc.getArgument(0))) - ) - or - // or a base flows into it - exists(ImplicitStringLiteralBase base | - DataFlow::localFlow(DataFlow::exprNode(base), DataFlow::exprNode(fc.getArgument(0))) - ) - ) + getTarget().getName() in [ + "strpbrk", "strchr", "strrchr", "strstr", "wcspbrk", "wcschr", "wcsrchr", "wcsstr", + "memchr", "wmemchr" + ] and + exists(Variable v | v.getAnAssignedValue() = this) and + exists(Expr source | + mayBeStringLiteral(source) and DataFlow::localExprFlow(source, getArgument(0)) ) } } -class StringLiteralOrConstChar extends Expr { - StringLiteralOrConstChar() { - this instanceof StringLiteral - or - getUnspecifiedType() instanceof CharPointerType and - getType().(PointerType).getBaseType().isConst() - } -} - -/** - * Since it is possible to produce an implicit literal by either - * an explicit literal being passed to one of these functions this - * class exists to establish the "base" type, that is an explicit - * string literal passed or flowing into the first argument. The other - * Implicit string literal class will then check to see if it is inductively - * an implicit string literal. - */ -class ImplicitStringLiteralBase extends Expr { - ImplicitStringLiteralBase() { - exists(MaybeReturnsStringLiteralFunctionCall fc, Variable e | - e.getAnAssignedValue() = fc and - this = fc and - // it either directly gets a string literal or one via flow - ( - fc.getArgument(0) instanceof StringLiteralOrConstChar or - exists(StringLiteralOrConstChar sl | - DataFlow::localFlow(DataFlow::exprNode(sl), DataFlow::exprNode(fc.getArgument(0))) - ) - ) - ) - } +/** Holds if `e` may point to a string literal. */ +private predicate mayBeStringLiteral(Expr e) { + e instanceof StringLiteral + or + e.getUnspecifiedType() instanceof CharPointerType and + e.getType().(PointerType).getBaseType().isConst() + or + e instanceof ImplicitStringLiteral } from Expr literal, Expr literalWrite where not isExcluded(literal, Strings1Package::doNotAttemptToModifyStringLiteralsQuery()) and not isExcluded(literalWrite, Strings1Package::doNotAttemptToModifyStringLiteralsQuery()) and - ImplicitOrExplicitStringLiteralModifiedFlow::flow(DataFlow::exprNode(literal), - DataFlow::exprNode(literalWrite)) + StringLiteralFlow::flow(literal, literalWrite) select literalWrite, "This operation may write to a string that may be a string literal that was $@.", literal, "created here" diff --git a/c/cert/test/rules/STR30-C/DoNotAttemptToModifyStringLiterals.expected b/c/cert/test/rules/STR30-C/DoNotAttemptToModifyStringLiterals.expected index d95b48e1c3..27ef66bc7a 100644 --- a/c/cert/test/rules/STR30-C/DoNotAttemptToModifyStringLiterals.expected +++ b/c/cert/test/rules/STR30-C/DoNotAttemptToModifyStringLiterals.expected @@ -1,18 +1,3 @@ -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:47,65-73) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:48,22-30) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:69,20-28) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:82,3-11) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:106,11-19) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:106,31-39) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:106,55-63) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:111,11-19) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:111,31-39) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:111,57-65) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:144,11-19) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:144,31-39) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:144,55-63) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:155,53-61) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (DoNotAttemptToModifyStringLiterals.ql:156,5-13) | test.c:7:3:7:3 | a | This operation may write to a string that may be a string literal that was $@. | test.c:6:13:6:20 | codeql | created here | | test.c:30:3:30:3 | a | This operation may write to a string that may be a string literal that was $@. | test.c:29:13:29:18 | call to strchr | created here | | test.c:36:3:36:3 | b | This operation may write to a string that may be a string literal that was $@. | test.c:35:13:35:18 | call to strchr | created here | From 91027c9048df3fb6fcdcfd3731094ada78c905f0 Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Thu, 24 Sep 2026 12:30:38 +0200 Subject: [PATCH 2/2] fix formatting --- .../src/rules/STR30-C/DoNotAttemptToModifyStringLiterals.ql | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/c/cert/src/rules/STR30-C/DoNotAttemptToModifyStringLiterals.ql b/c/cert/src/rules/STR30-C/DoNotAttemptToModifyStringLiterals.ql index b4ef7820aa..48fd38bf79 100644 --- a/c/cert/src/rules/STR30-C/DoNotAttemptToModifyStringLiterals.ql +++ b/c/cert/src/rules/STR30-C/DoNotAttemptToModifyStringLiterals.ql @@ -23,9 +23,7 @@ import semmle.code.cpp.dataflow.new.DataFlow /** A modeled buffer write through the first argument of a library call. */ private class ModifiesFirstArgFunction extends BufferWrite, FunctionCall { - ModifiesFirstArgFunction() { - getTarget().getName() = ["mkstemp", "memset", "memcpy", "memmove"] - } + ModifiesFirstArgFunction() { getTarget().getName() = ["mkstemp", "memset", "memcpy", "memmove"] } override Type getBufferType() { none() }