Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

C++: Fix conflation in barrier guards #21171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MathiasVP wants to merge 6 commits into github:main
base: main
Choose a base branch
Loading
from MathiasVP:fix-conflation-in-guards
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: fix
---
* Fixed a bug in the `DataFlow::BarrierGuard<...>::getABarrierNode` predicate which caused the predicate to return `DataFlow::Node`s with incorrect indirections. If you use `getABarrierNode` to implement barriers in a dataflow/taint-tracking query it may result in more query results. You can use `DataFlow::BarrierGuard<...>::getAnIndirectBarrierNode` to remove those query results.
32 changes: 15 additions & 17 deletions cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ class Node extends TIRDataFlowNode {
* If `isGLValue()` holds, then the type of this node
* should be thought of as "pointer to `getType()`".
*/
DataFlowType getType() { none() } // overridden in subclasses
Type getType() { none() } // overridden in subclasses

/** Gets the instruction corresponding to this node, if any. */
Instruction asInstruction() { result = this.(InstructionNode).getInstruction() }
Expand Down Expand Up @@ -541,7 +541,7 @@ class Node extends TIRDataFlowNode {
/**
* Gets an upper bound on the type of this node.
*/
DataFlowType getTypeBound() { result = this.getType() }
Type getTypeBound() { result = this.getType() }

/** Gets the location of this element. */
cached
Expand Down Expand Up @@ -585,7 +585,7 @@ private class Node0 extends Node, TNode0 {

override string toStringImpl() { result = node.toString() }

override DataFlowType getType() { result = node.getType() }
override Type getType() { result = node.getType() }

override predicate isGLValue() { node.isGLValue() }
}
Expand Down Expand Up @@ -704,7 +704,7 @@ class SsaSynthNode extends Node, TSsaSynthNode {

override Declaration getFunction() { result = node.getBasicBlock().getEnclosingFunction() }

override DataFlowType getType() { result = node.getSourceVariable().getType() }
override Type getType() { result = node.getSourceVariable().getType() }

override predicate isGLValue() { node.getSourceVariable().isGLValue() }

Expand Down Expand Up @@ -732,7 +732,7 @@ class SsaIteratorNode extends Node, TSsaIteratorNode {

override Declaration getFunction() { result = node.getFunction() }

override DataFlowType getType() { result = node.getType() }
override Type getType() { result = node.getType() }

final override Location getLocationImpl() { result = node.getLocation() }

Expand Down Expand Up @@ -792,7 +792,7 @@ class FinalGlobalValue extends Node, TFinalGlobalValue {

override Declaration getFunction() { result = globalUse.getIRFunction().getFunction() }

override DataFlowType getType() {
override Type getType() {
exists(int indirectionIndex |
indirectionIndex = globalUse.getIndirectionIndex() and
result = getTypeImpl(globalUse.getUnderlyingType(), indirectionIndex)
Expand Down Expand Up @@ -826,7 +826,7 @@ class InitialGlobalValue extends Node, TInitialGlobalValue {

final override predicate isGLValue() { globalDef.getIndirectionIndex() = 0 }

override DataFlowType getType() { result = globalDef.getUnderlyingType() }
override Type getType() { result = globalDef.getUnderlyingType() }

final override Location getLocationImpl() { result = globalDef.getLocation() }

Expand All @@ -853,7 +853,7 @@ class BodyLessParameterNodeImpl extends Node, TBodyLessParameterNodeImpl {
/** Gets the indirection index of this node. */
int getIndirectionIndex() { result = indirectionIndex }

override DataFlowType getType() {
override Type getType() {
result = getTypeImpl(p.getUnderlyingType(), this.getIndirectionIndex())
}

Expand Down Expand Up @@ -1117,8 +1117,8 @@ private module RawIndirectNodes {

override predicate isGLValue() { this.getOperand().isGLValue() }

override DataFlowType getType() {
exists(int sub, DataFlowType type, boolean isGLValue |
override Type getType() {
exists(int sub, Type type, boolean isGLValue |
type = getOperandType(this.getOperand(), isGLValue) and
if isGLValue = true then sub = 1 else sub = 0
|
Expand Down Expand Up @@ -1163,8 +1163,8 @@ private module RawIndirectNodes {

override predicate isGLValue() { this.getInstruction().isGLValue() }

override DataFlowType getType() {
exists(int sub, DataFlowType type, boolean isGLValue |
override Type getType() {
exists(int sub, Type type, boolean isGLValue |
type = getInstructionType(this.getInstruction(), isGLValue) and
if isGLValue = true then sub = 1 else sub = 0
|
Expand Down Expand Up @@ -1263,7 +1263,7 @@ class FinalParameterNode extends Node, TFinalParameterNode {
result.asSourceCallable() = this.getFunction()
}

override DataFlowType getType() { result = getTypeImpl(p.getUnderlyingType(), indirectionIndex) }
override Type getType() { result = getTypeImpl(p.getUnderlyingType(), indirectionIndex) }

final override Location getLocationImpl() {
// Parameters can have multiple locations. When there's a unique location we use
Expand Down Expand Up @@ -1539,7 +1539,7 @@ abstract class PostUpdateNode extends Node {
*/
abstract Node getPreUpdateNode();

final override DataFlowType getType() { result = this.getPreUpdateNode().getType() }
final override Type getType() { result = this.getPreUpdateNode().getType() }
}

/**
Expand Down Expand Up @@ -1632,9 +1632,7 @@ class VariableNode extends Node, TGlobalLikeVariableNode {
result.asSourceCallable() = v
}

override DataFlowType getType() {
result = getTypeImpl(v.getUnderlyingType(), indirectionIndex - 1)
}
override Type getType() { result = getTypeImpl(v.getUnderlyingType(), indirectionIndex - 1) }

final override Location getLocationImpl() {
// Certain variables (such as parameters) can have multiple locations.
Expand Down
13 changes: 10 additions & 3 deletions cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ private module SourceVariables {
* the type of this source variable should be thought of as "pointer
* to `getType()`".
*/
DataFlowType getType() {
Type getType() {
if this.isGLValue()
then result = base.getType()
else result = getTypeImpl(base.getType(), ind - 1)
Expand Down Expand Up @@ -1064,8 +1064,15 @@ module BarrierGuardWithIntParam<guardChecksNodeSig/4 guardChecksNode> {
DataFlowIntegrationInput::Guard g, SsaImpl::Definition def, IRGuards::GuardValue val,
int indirectionIndex
) {
IRGuards::Guards_v1::ParameterizedValidationWrapper<int, guardChecksInstr/4>::guardChecksDef(g,
def, val, indirectionIndex)
exists(Instruction e |
IRGuards::Guards_v1::ParameterizedValidationWrapper<int, guardChecksInstr/4>::guardChecks(g,
e, val, indirectionIndex)
|
indirectionIndex = 0 and
def.(Definition).getAUse().getDef() = e
or
def.(Definition).getAnIndirectUse(indirectionIndex).getDef() = e
)
}

Node getABarrierNode(int indirectionIndex) {
Expand Down
3 changes: 2 additions & 1 deletion cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,8 @@ module Config implements DataFlow::ConfigSig {

predicate isBarrier(DataFlow::Node node) {
// Block flow if the node is guarded by any <, <= or = operations.
node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getABarrierNode()
node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getABarrierNode() or
node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getAnIndirectBarrierNode()
}

predicate observeDiffInformedIncrementalMode() { any() }
Expand Down
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ module ModelGeneratorCommonInput implements ModelGeneratorCommonInputSig<Cpp::Lo
{
private module DataFlow = Df::DataFlow;

class Type = DataFlowPrivate::DataFlowType;
class Type = Cpp::Type;

// Note: This also includes `this`
class Parameter = DataFlow::ParameterNode;
Expand Down
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ void sink(int);

void testCheckArgument(int* p) {
if (checkArgument(p)) {
sink(*p); // $ barrier barrier=1
sink(*p); // $ indirect_barrier=int barrier=int*
}
}

void testCheckArgument(int p) {
if (checkArgument(&p)) {
sink(p); // $ barrier=glval<int> indirect_barrier=int
}
}
31 changes: 19 additions & 12 deletions cpp/ql/test/library-tests/dataflow/ir-barrier-guards/test.ql
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,33 @@ predicate instructionGuardChecks(IRGuardCondition gc, Instruction checked, boole

module BarrierGuard = DataFlow::InstructionBarrierGuard<instructionGuardChecks/3>;

predicate indirectBarrierGuard(DataFlow::Node node, int indirectionIndex) {
node = BarrierGuard::getAnIndirectBarrierNode(indirectionIndex)
predicate indirectBarrierGuard(DataFlow::Node node, string s) {
node = BarrierGuard::getAnIndirectBarrierNode(_) and
if node.isGLValue()
then s = "glval<" + node.getType().toString().replaceAll(" ", "") + ">"
else s = node.getType().toString().replaceAll(" ", "")
}

predicate barrierGuard(DataFlow::Node node) { node = BarrierGuard::getABarrierNode() }
predicate barrierGuard(DataFlow::Node node, string s) {
node = BarrierGuard::getABarrierNode() and
if node.isGLValue()
then s = "glval<" + node.getType().toString().replaceAll(" ", "") + ">"
else s = node.getType().toString().replaceAll(" ", "")
}

module Test implements TestSig {
string getARelevantTag() { result = "barrier" }
string getARelevantTag() { result = ["barrier", "indirect_barrier"] }

predicate hasActualResult(Location location, string element, string tag, string value) {
exists(DataFlow::Node node |
barrierGuard(node) and
value = ""
exists(DataFlow::Node node, string s |
indirectBarrierGuard(node, s) and
value = s and
tag = "indirect_barrier"
or
exists(int indirectionIndex |
indirectBarrierGuard(node, indirectionIndex) and
value = indirectionIndex.toString()
)
barrierGuard(node, s) and
value = s and
tag = "barrier"
|
tag = "barrier" and
element = node.toString() and
location = node.getLocation()
)
Expand Down
2 changes: 1 addition & 1 deletion shared/controlflow/codeql/controlflow/Guards.qll
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1364,7 +1364,7 @@ module Make<
/**
* Holds if the guard `g` validates the expression `e` upon evaluating to `val`.
*/
private predicate guardChecks(Guard g, Expr e, GuardValue val, P par) {
predicate guardChecks(Guard g, Expr e, GuardValue val, P par) {
guardChecks0(g, e, val, par)
or
exists(NonOverridableMethodCall call, ParameterPosition ppos, ArgumentPosition apos |
Expand Down
Loading

AltStyle によって変換されたページ (->オリジナル) /