From 66e8b2d7e51d7c3f323e4c46aa7b4fcc0dbe41f2 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 12 Mar 2025 17:04:06 +0000 Subject: [PATCH 1/8] C++: Add an 'asDefinition' overload to check if a definition is certain or not. --- .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 72 ++++++++++++++++++- 1 file changed, 69 insertions(+), 3 deletions(-) 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 c9e2a7136216..38a389f00e52 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 @@ -313,13 +313,79 @@ class Node extends TIRDataFlowNode { * `n.asExpr() instanceof IncrementOperation` since the result of evaluating * the expression `x++` is passed to `sink`. */ - Expr asDefinition() { - exists(StoreInstruction store | + Expr asDefinition() { result = this.asDefinition(_) } + + /** + * Gets the definition associated with this node, if any. + * + * For example, consider the following example + * ```cpp + * int x = 42; // 1 + * x = 34; // 2 + * ++x; // 3 + * x++; // 4 + * x += 1; // 5 + * int y = x += 2; // 6 + * ``` + * - For (1) the result is `42`. + * - For (2) the result is `x = 34`. + * - For (3) the result is `++x`. + * - For (4) the result is `x++`. + * - For (5) the result is `x += 1`. + * - For (6) there are two results: + * - For the definition generated by `x += 2` the result is `x += 2` + * - For the definition generated by `int y = ...` the result is + * also `x += 2`. + * + * For assignments, `node.asDefinition()` and `node.asExpr()` will both exist + * for the same dataflow node. However, for expression such as `x++` that + * both write to `x` and read the current value of `x`, `node.asDefinition()` + * will give the node corresponding to the value after the increment, and + * `node.asExpr()` will give the node corresponding to the value before the + * increment. For an example of this, consider the following: + * + * ```cpp + * sink(x++); + * ``` + * in the above program, there will not be flow from a node `n` such that + * `n.asDefinition() instanceof IncrementOperation` to the argument of `sink` + * since the value passed to `sink` is the value before to the increment. + * However, there will be dataflow from a node `n` such that + * `n.asExpr() instanceof IncrementOperation` since the result of evaluating + * the expression `x++` is passed to `sink`. + * + * If `uncertain = false` then the definition is guaranteed to overwrite + * the entire buffer pointed to by the destination address of the definition. + * Otherwise, `uncertain = true`. + * + * For example, the write `int x; x = 42;` is guaranteed to overwrite all the + * bytes allocated to `x`, while the assignment `int p[10]; p[3] = 42;` has + * `uncertain = true` since the write will not overwrite the entire buffer + * pointed to by `p`. + */ + Expr asDefinition(boolean uncertain) { + exists(StoreInstruction store, Ssa::DefinitionExt def | store = this.asInstruction() and - result = asDefinitionImpl(store) + result = asDefinitionImpl(store) and + Ssa::defToNode(this, def, _, _, _, _) and + if def.isCertain() then uncertain = false else uncertain = true ) } + /** + * Gets the definition associated with this node, if this node is a certain definition. + * + * See `Node.asDefinition` for a description of certain and uncertain definitions. + */ + Expr asCertainDefinition() { result = this.asDefinition(false) } + + /** + * Gets the definition associated with this node, if this node is an uncertain definition. + * + * See `Node.asDefinition` for a description of certain and uncertain definitions. + */ + Expr asUncertainDefinition() { result = this.asDefinition(true) } + /** * Gets the indirect definition at a given indirection corresponding to this * node, if any. From aeb1acba97bb5e1c7691233d022b8714febf16d6 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Wed, 12 Mar 2025 17:09:05 +0000 Subject: [PATCH 2/8] C++: Use the new API in queries. --- cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql | 2 +- .../Security/CWE/CWE-114/UncontrolledProcessOperation.ql | 2 +- cpp/ql/src/Security/CWE/CWE-134/UncontrolledFormatString.ql | 2 +- .../Security/CWE/CWE-170/ImproperNullTerminationTainted.ql | 2 +- cpp/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql | 6 ++++-- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql b/cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql index 812fe236f764..9b27e95fd658 100644 --- a/cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql +++ b/cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql @@ -37,7 +37,7 @@ module Config implements DataFlow::ConfigSig { predicate isBarrier(DataFlow::Node node) { isSink(node) and node.asExpr().getUnspecifiedType() instanceof ArithmeticType or - node.asInstruction().(StoreInstruction).getResultType() instanceof ArithmeticType + node.asCertainDefinition().getUnspecifiedType() instanceof ArithmeticType } } diff --git a/cpp/ql/src/Security/CWE/CWE-114/UncontrolledProcessOperation.ql b/cpp/ql/src/Security/CWE/CWE-114/UncontrolledProcessOperation.ql index 9672a830ce06..e5fd1a94f181 100644 --- a/cpp/ql/src/Security/CWE/CWE-114/UncontrolledProcessOperation.ql +++ b/cpp/ql/src/Security/CWE/CWE-114/UncontrolledProcessOperation.ql @@ -37,7 +37,7 @@ module Config implements DataFlow::ConfigSig { predicate isBarrier(DataFlow::Node node) { isSink(node) and node.asExpr().getUnspecifiedType() instanceof ArithmeticType or - node.asInstruction().(StoreInstruction).getResultType() instanceof ArithmeticType + node.asCertainDefinition().getUnspecifiedType() instanceof ArithmeticType } } diff --git a/cpp/ql/src/Security/CWE/CWE-134/UncontrolledFormatString.ql b/cpp/ql/src/Security/CWE/CWE-134/UncontrolledFormatString.ql index f4a716765b8a..04b4fe45fcfa 100644 --- a/cpp/ql/src/Security/CWE/CWE-134/UncontrolledFormatString.ql +++ b/cpp/ql/src/Security/CWE/CWE-134/UncontrolledFormatString.ql @@ -42,7 +42,7 @@ module Config implements DataFlow::ConfigSig { predicate isBarrier(DataFlow::Node node) { isSink(node) and isArithmeticNonCharType(node.asExpr().getUnspecifiedType()) or - isArithmeticNonCharType(node.asInstruction().(StoreInstruction).getResultType()) + isArithmeticNonCharType(node.asCertainDefinition().getUnspecifiedType()) } } diff --git a/cpp/ql/src/Security/CWE/CWE-170/ImproperNullTerminationTainted.ql b/cpp/ql/src/Security/CWE/CWE-170/ImproperNullTerminationTainted.ql index e1e459c259f1..9b595657fce9 100644 --- a/cpp/ql/src/Security/CWE/CWE-170/ImproperNullTerminationTainted.ql +++ b/cpp/ql/src/Security/CWE/CWE-170/ImproperNullTerminationTainted.ql @@ -37,7 +37,7 @@ private module Config implements DataFlow::ConfigSig { predicate isBarrier(DataFlow::Node node) { isSink(node) and node.asExpr().getUnspecifiedType() instanceof ArithmeticType or - node.asInstruction().(StoreInstruction).getResultType() instanceof ArithmeticType + node.asCertainDefinition().getUnspecifiedType() instanceof ArithmeticType or mayAddNullTerminator(_, node.asIndirectExpr()) } diff --git a/cpp/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql b/cpp/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql index 810039f4e1a4..c9efaf9f6952 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql @@ -75,9 +75,11 @@ module Config implements DataFlow::ConfigSig { predicate isSink(DataFlow::Node sink) { isSink(sink, _, _) } predicate isBarrier(DataFlow::Node node) { - exists(StoreInstruction store | store = node.asInstruction() | + exists(StoreInstruction store, Expr e | + store = node.asInstruction() and e = node.asCertainDefinition() + | // Block flow to "likely small expressions" - bounded(store.getSourceValue().getUnconvertedResultExpression()) + bounded(e) or // Block flow to "small types" store.getResultType().getUnspecifiedType().(IntegralType).getSize() <= 1 From 0fe77154e1c549bb605d495b356e1a91c8203458 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 13 Mar 2025 14:29:34 +0000 Subject: [PATCH 3/8] C++: Add library change note. --- cpp/ql/lib/change-notes/2025-03-13-ascertaindef.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/lib/change-notes/2025-03-13-ascertaindef.md diff --git a/cpp/ql/lib/change-notes/2025-03-13-ascertaindef.md b/cpp/ql/lib/change-notes/2025-03-13-ascertaindef.md new file mode 100644 index 000000000000..6a55fc7bdd08 --- /dev/null +++ b/cpp/ql/lib/change-notes/2025-03-13-ascertaindef.md @@ -0,0 +1,4 @@ +--- +category: feature +--- +* Added `Node.asUncertainDefinition` and `Node.asCertainDefinition` to the `DataFlow::Node` class for querying whether a definition overwrites the entire destination buffer. \ No newline at end of file From 68b414d169d9ad54b922dd92802c3a4a79c8b33f Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 13 Mar 2025 15:59:48 +0000 Subject: [PATCH 4/8] Update cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com> --- .../lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 38a389f00e52..7855b8d337c7 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 @@ -337,7 +337,7 @@ class Node extends TIRDataFlowNode { * - For the definition generated by `int y = ...` the result is * also `x += 2`. * - * For assignments, `node.asDefinition()` and `node.asExpr()` will both exist + * For assignments, `node.asDefinition(_)` and `node.asExpr()` will both exist * for the same dataflow node. However, for expression such as `x++` that * both write to `x` and read the current value of `x`, `node.asDefinition()` * will give the node corresponding to the value after the increment, and From 9cde2bb94d1f933b193930b7ea968eb2c64b2f34 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 13 Mar 2025 15:59:57 +0000 Subject: [PATCH 5/8] Update cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com> --- .../lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 7855b8d337c7..1bf02dbe7a77 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 @@ -339,7 +339,7 @@ class Node extends TIRDataFlowNode { * * For assignments, `node.asDefinition(_)` and `node.asExpr()` will both exist * for the same dataflow node. However, for expression such as `x++` that - * both write to `x` and read the current value of `x`, `node.asDefinition()` + * both write to `x` and read the current value of `x`, `node.asDefinition(_)` * will give the node corresponding to the value after the increment, and * `node.asExpr()` will give the node corresponding to the value before the * increment. For an example of this, consider the following: From 470321e8b6e582ced27084b05bc9b6f559479e05 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 13 Mar 2025 16:00:15 +0000 Subject: [PATCH 6/8] Update cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com> --- .../lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 1bf02dbe7a77..5b0ed2aeb093 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 @@ -348,7 +348,7 @@ class Node extends TIRDataFlowNode { * sink(x++); * ``` * in the above program, there will not be flow from a node `n` such that - * `n.asDefinition() instanceof IncrementOperation` to the argument of `sink` + * `n.asDefinition(_) instanceof IncrementOperation` to the argument of `sink` * since the value passed to `sink` is the value before to the increment. * However, there will be dataflow from a node `n` such that * `n.asExpr() instanceof IncrementOperation` since the result of evaluating From 0e5fa1b5eb24314247e201b0dd660096133a7b9f Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 13 Mar 2025 16:00:23 +0000 Subject: [PATCH 7/8] Update cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com> --- .../lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 5b0ed2aeb093..20a66b72738d 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 @@ -375,7 +375,7 @@ class Node extends TIRDataFlowNode { /** * Gets the definition associated with this node, if this node is a certain definition. * - * See `Node.asDefinition` for a description of certain and uncertain definitions. + * See `Node.asDefinition/1` for a description of certain and uncertain definitions. */ Expr asCertainDefinition() { result = this.asDefinition(false) } From 6f4e9ed1366362264d06bc47b63a57929be7822d Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 13 Mar 2025 16:00:36 +0000 Subject: [PATCH 8/8] Update cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com> --- .../lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 20a66b72738d..a6018535824a 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 @@ -382,7 +382,7 @@ class Node extends TIRDataFlowNode { /** * Gets the definition associated with this node, if this node is an uncertain definition. * - * See `Node.asDefinition` for a description of certain and uncertain definitions. + * See `Node.asDefinition/1` for a description of certain and uncertain definitions. */ Expr asUncertainDefinition() { result = this.asDefinition(true) }