Skip to content

C++: Take into account the delta at the final sink in cpp/invalid-pointer-deref #13320

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.

Sign up for GitHub

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

Merged
MathiasVP merged 5 commits into from
May 31, 2023
Merged

C++: Take into account the delta at the final sink in cpp/invalid-pointer-deref #13320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a8c7638
C++: Fix configuration names in comments in `cpp/invalid-pointer-deref`
jketema May 30, 2023
de974cc
C++: Add `cpp/invalid-pointer-deref` test case that shows some duplic…
jketema May 30, 2023
f5ed02a
C++: Take into account the delta at the final sink in `cpp/invalid-po…
jketema May 30, 2023
dd30acf
C++: Add nodes query predicate to `cpp/invalid-pointer-deref`
jketema May 30, 2023
ace7b6b
C++: Add `cpp/invalid-pointer-deref` FP test case
jketema May 31, 2023
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
56 changes: 32 additions & 24 deletions cpp/ql/src/experimental/Security/CWE/CWE-193/InvalidPointerDeref.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ predicate hasSize(HeuristicAllocationExpr alloc, DataFlow::Node n, int state) {
* ```
*
* We do this by splitting the task up into two configurations:
* 1. `AllocToInvalidPointerConf` find flow from `malloc(size)` to `begin + size`, and
* 2. `InvalidPointerToDerefConf` finds flow from `begin + size` to an `end` (on line 3).
* 1. `AllocToInvalidPointerConfig` find flow from `malloc(size)` to `begin + size`, and
* 2. `InvalidPointerToDerefConfig` finds flow from `begin + size` to an `end` (on line 3).
*
* Finally, the range-analysis library will find a load from (or store to) an address that
* is non-strictly upper-bounded by `end` (which in this case is `*p`).
Expand Down Expand Up @@ -180,13 +180,13 @@ predicate isSinkImpl(
}

/**
* Holds if `sink` is a sink for `InvalidPointerToDerefConf` and `i` is a `StoreInstruction` that
* Holds if `sink` is a sink for `InvalidPointerToDerefConfig` and `i` is a `StoreInstruction` that
* writes to an address that non-strictly upper-bounds `sink`, or `i` is a `LoadInstruction` that
* reads from an address that non-strictly upper-bounds `sink`.
*/
pragma[inline]
predicate isInvalidPointerDerefSink(DataFlow::Node sink, Instruction i, string operation) {
exists(AddressOperand addr, int delta |
predicate isInvalidPointerDerefSink(DataFlow::Node sink, Instruction i, string operation, int delta) {
exists(AddressOperand addr |
bounded1(addr.getDef(), sink.asInstruction(), delta) and
delta >= 0 and
i.getAnOperand() = addr
Expand All @@ -201,13 +201,13 @@ predicate isInvalidPointerDerefSink(DataFlow::Node sink, Instruction i, string o

/**
* A configuration to track flow from a pointer-arithmetic operation found
* by `AllocToInvalidPointerConf` to a dereference of the pointer.
* by `AllocToInvalidPointerConfig` to a dereference of the pointer.
*/
module InvalidPointerToDerefConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { invalidPointerToDerefSource(_, source, _) }

pragma[inline]
predicate isSink(DataFlow::Node sink) { isInvalidPointerDerefSink(sink, _, _) }
predicate isSink(DataFlow::Node sink) { isInvalidPointerDerefSink(sink, _, _, _) }

predicate isBarrier(DataFlow::Node node) {
node = any(DataFlow::SsaPhiNode phi | not phi.isPhiRead()).getAnInput(true)
Expand Down Expand Up @@ -237,17 +237,17 @@ predicate invalidPointerToDerefSource(
}

newtype TMergedPathNode =
// The path nodes computed by the first projection of `AllocToInvalidPointerConf`
// The path nodes computed by the first projection of `AllocToInvalidPointerConfig`
TPathNode1(AllocToInvalidPointerFlow::PathNode1 p) or
// The path nodes computed by `InvalidPointerToDerefConf`
// The path nodes computed by `InvalidPointerToDerefConfig`
TPathNode3(InvalidPointerToDerefFlow::PathNode p) or
// The read/write that uses the invalid pointer identified by `InvalidPointerToDerefConf`.
// This one is needed because the sink identified by `InvalidPointerToDerefConf` is the
// The read/write that uses the invalid pointer identified by `InvalidPointerToDerefConfig`.
// This one is needed because the sink identified by `InvalidPointerToDerefConfig` is the
// pointer, but we want to raise an alert at the dereference.
TPathNodeSink(Instruction i) {
exists(DataFlow::Node n |
InvalidPointerToDerefFlow::flowTo(n) and
isInvalidPointerDerefSink(n, i, _)
isInvalidPointerDerefSink(n, i, _, _)
)
}

Expand Down Expand Up @@ -321,7 +321,15 @@ query predicate edges(MergedPathNode node1, MergedPathNode node2) {
or
node1.asPathNode3().getASuccessor() = node2.asPathNode3()
or
joinOn2(node1.asPathNode3(), node2.asSinkNode(), _)
joinOn2(node1.asPathNode3(), node2.asSinkNode(), _, _)
}

query predicate nodes(MergedPathNode n, string key, string val) {
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW nodes is implicitly generated by the CLI in a path-problem query based on the edges predicate. So strictly speaking this query predicate is redundant (since we're not using it to modify the string representation of the nodes). However, I do like having the nodes predicate in here since it can be useful to append concat(n.getAQlClass(), ", ") for debugging purposes.

Copy link
Copy Markdown
Contributor Author

@jketema jketema May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, learnt something new. Thanks.

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't blame you. I don't think this is documented anywhere 😅

AllocToInvalidPointerFlow::PathGraph1::nodes(n.asPathNode1(), key, val)
or
InvalidPointerToDerefFlow::PathGraph::nodes(n.asPathNode3(), key, val)
or
key = "semmle.label" and val = n.asSinkNode().toString()
}

query predicate subpaths(
Expand All @@ -335,8 +343,8 @@ query predicate subpaths(
}

/**
* Holds if `p1` is a sink of `AllocToInvalidPointerConf` and `p2` is a source
* of `InvalidPointerToDerefConf`, and they are connected through `pai`.
* Holds if `p1` is a sink of `AllocToInvalidPointerConfig` and `p2` is a source
* of `InvalidPointerToDerefConfig`, and they are connected through `pai`.
*/
predicate joinOn1(
PointerArithmeticInstruction pai, AllocToInvalidPointerFlow::PathNode1 p1,
Expand All @@ -347,37 +355,37 @@ predicate joinOn1(
}

/**
* Holds if `p1` is a sink of `InvalidPointerToDerefConf` and `i` is the instruction
* Holds if `p1` is a sink of `InvalidPointerToDerefConfig` and `i` is the instruction
* that dereferences `p1`. The string `operation` describes whether the `i` is
* a `StoreInstruction` or `LoadInstruction`.
*/
pragma[inline]
predicate joinOn2(InvalidPointerToDerefFlow::PathNode p1, Instruction i, string operation) {
isInvalidPointerDerefSink(p1.getNode(), i, operation)
predicate joinOn2(InvalidPointerToDerefFlow::PathNode p1, Instruction i, string operation, int delta) {
isInvalidPointerDerefSink(p1.getNode(), i, operation, delta)
}

predicate hasFlowPath(
MergedPathNode source1, MergedPathNode sink, InvalidPointerToDerefFlow::PathNode source3,
PointerArithmeticInstruction pai, string operation
PointerArithmeticInstruction pai, string operation, int delta
) {
exists(InvalidPointerToDerefFlow::PathNode sink3, AllocToInvalidPointerFlow::PathNode1 sink1 |
AllocToInvalidPointerFlow::flowPath(source1.asPathNode1(), _, sink1, _) and
joinOn1(pai, sink1, source3) and
InvalidPointerToDerefFlow::flowPath(source3, sink3) and
joinOn2(sink3, sink.asSinkNode(), operation)
joinOn2(sink3, sink.asSinkNode(), operation, delta)
)
}

from
MergedPathNode source, MergedPathNode sink, int k, string kstr,
MergedPathNode source, MergedPathNode sink, int k2, int k3, string kstr,
InvalidPointerToDerefFlow::PathNode source3, PointerArithmeticInstruction pai, string operation,
Expr offset, DataFlow::Node n
where
hasFlowPath(source, sink, source3, pai, operation) and
invalidPointerToDerefSource(pai, source3.getNode(), k) and
hasFlowPath(source, sink, source3, pai, operation, k3) and
invalidPointerToDerefSource(pai, source3.getNode(), k2) and
offset = pai.getRight().getUnconvertedResultExpression() and
n = source.asPathNode1().getNode() and
if k = 0 then kstr = "" else kstr = " + " + k
if (k2 + k3) = 0 then kstr = "" else kstr = " + " + (k2 + k3)
select sink, source, sink,
"This " + operation + " might be out of bounds, as the pointer might be equal to $@ + $@" + kstr +
".", n, n.toString(), offset, offset.toString()
Loading