Removing False Positives in CodeQL
Introduction
When people ask me about buffer overflows, my mind usually drifts towards vulnerable memcpy writes to an unchecked destination variable. I mean…it is one of the most common examples to depict overflow situations.
Unfortunately, despite being one of the most generic depictions of buffer overflows, this did not preclude the possibility of coming across one of these bad boys in a live production server. That was the case with the Accel-PPP VPN server.
Earlier this year, we filed three Common Vulnerabilities and Exposures advisories (CVEs) found in this server, both involving the notorious memcpy overflow. It was a grueling process of reviewing and scouring through code and by the end of it, my pitiful eyes had been scorched by the glaring blue light emitted from my 24-inch monitor.
However, struck by the holy rays of advancing technology, we found a savior that made the journey a tad bit easier — CodeQL. Being a semantic code analysis engine, CodeQL gave us the ability to “filter” specific code scenarios using its proprietary query language, which greatly reminded me of writing SQL queries (though if only it were that easy!).
Like a newborn prodigy straight from the womb, CodeQL had great potential for streamlining the monotonous procedure of manual code analysis, though much had yet to be explored to reach the heights of its full capabilities.
My fellow intern Chloe has written an in-depth blog post on hunting the Accel-PPP bugs with CodeQL.
In this blog, I will focus on filtering out false positives in CodeQL results to sharpen your bug-hunting javelins and hit the jackpot of those sweet, sweet CVEs.
Do note that the examples portrayed in this blog are based on the accel-ppp CodeQL database.
Memcpy Overflows
void * memcpy ( void * destination, const void * source, size_t num );The above depicts the syntax of a memcpy function call in C code. A buffer overflow occurs when the size written to the destination buffer exceeds it.
To filter out all occasions of a source to a memcpy sink in its size argument, we can use the following CodeQL query.
import cpp
import semmle.code.cpp.dataflow.TaintTracking
import DataFlow::PathGraph
class Config extends TaintTracking::Configuration {
Config() { this = “MemcpySink” }
override predicate isSource(DataFlow::Node source) {
exists(Expr E |
source.asExpr() = E
)
}
override predicate isSink(DataFlow::Node sink) {
exists(FunctionCall fc | fc.getTarget().hasName(“memcpy”) and sink.asExpr() = fc.getArgument(2))
and not sink.asExpr().isConstant()
}
}
from Config cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink, source, sink, “All source expressions to memcpy “ + source
This query returned a whopping 1149 results which will probably take an ironclad resolve and a generous portion of your sanity to thoroughly vet each one of them.
A quick scan of the results showed many instances of “guarded” memcpy function calls that made it invulnerable to overflow attacks.
For example, the above snippet depicts a memcpy write to a mschap_error variable that had earlier been allocated memory through malloc using the ra->len variable which was also used in memcpy.
This ensures that the size of the dst argument will always be equal to or bigger than the size argument used in a memcpy write.
Another instance can be seen above where the length of the dst argument is validated against the attr->len variable and reallocated memory with realloc if it doesn’t match up.
These “guarded” memcpys abound, bloating the results as if it just returned from a company-sponsored mookata buffet.
An Attempt to Rule out False Positives
To sieve out those instances from our query results, we must find code paths where a source had not been allocated memory with a function like malloc using the size argument in a memcpy size field.
void* malloc (size_t size);
An example would be like the above, where the attr->length variable had previously been used to allocate memory into the attr->val.octets variable which was used as a destination argument in a memcpy call. This makes it immune to overflow exploits.
To do that, we can write a new class MallocSize that will represent the first argument (size) of malloc function calls.
…
class MallocSize extends Expr {
MallocSize () {
exists(
FunctionCall fc |
fc.getTarget().hasName(“malloc”) and
this = fc.getArgument(0)
)
}
}
…The above query snippet finds all function calls in the CodeQL database that has a name of malloc and extracts its first argument which is its size.
We can then incorporate that into our isSink predicate to find instances where the memcpy size argument had been used in a malloc call.
…
override predicate isSink(DataFlow::Node sink) {
exists(FunctionCall fc, MallocSize mc |
fc.getTarget().hasName(“memcpy”) and
fc.getArgument(2).(VariableAccess).getTarget() = mc.(VariableAccess).getTarget() and
sink.asExpr() = fc.getArgument(2))
and not sink.asExpr().isConstant()
}
…The final query would look something like this.
import cpp
import semmle.code.cpp.dataflow.TaintTracking
import DataFlow::PathGraph
// Finds all non-constant operands of arithmetic operations
class MallocSize extends Expr {
MallocSize () {
exists(
FunctionCall fc |
fc.getTarget().hasName(“malloc”) and
this = fc.getArgument(0)
)
}
}
class Config extends TaintTracking::Configuration {
Config() { this = “MallocToMemcpySink” }
override predicate isSource(DataFlow::Node source) {
exists(Expr E |
source.asExpr() = E
and not E.isConstant()
)
}
override predicate isSink(DataFlow::Node sink) {
exists(FunctionCall fc, MallocSize mc |
fc.getTarget().hasName(“memcpy”) and
fc.getArgument(2).(VariableAccess).getTarget() = mc.(VariableAccess).getTarget() and
sink.asExpr() = fc.getArgument(2))
and not sink.asExpr().isConstant()
}
}
from Config cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink, source, sink, “any source flows to memcpy where size is operand in malloc”
Lo and behold, it had found 78 results of such instances in our CodeQL database.
Expanding the Filter’s Scope
However, like an 18-year-old learning how to file taxes for the first time, things are not always so simple.
What if our size attribute was a rojak mix between other variables? In the example above, the size argument in the memcpy call (size — n) is an arithmetic operation that was also previously used in another arithmetic operation (sizeof(*b) + size — n) within a malloc call.
Since our previous query simply matched one argument to another, it had failed to match against instances where the size argument was merely a part of the argument in a malloc call instead.
After some research, I found that CodeQL had a built-in class BinaryArithmeticOperation that captured arithmetic operations like a + b and could return operands (a or b) with the getAnOperand predicate.
We can create a new class RecursiveBinaryArithmeticOperation with our custom predicate recursiveGetAnOperand that will recursively capture and match a sink with operands in an arithmetic operation.
class RecursiveBinaryArithmeticOperation extends BinaryArithmeticOperation {
Expr recursiveGetAnOperand() {
(result = this.getAnOperand() or
(
this.getAnOperand() instanceof BinaryArithmeticOperation and
result = this.getAnOperand().(RecursiveBinaryArithmeticOperation).recursiveGetAnOperand()
)) and not
result instanceof BinaryArithmeticOperation and not
result.isConstant()
}
}This class can then be incorporated into our MallocSize class.
class MallocSize extends Expr {
MallocSize () {
exists(
FunctionCall fc |
fc.getTarget().hasName(“malloc”) and
fc.getArgument(0) instanceof RecursiveBinaryArithmeticOperation and
this = fc.getArgument(0).(RecursiveBinaryArithmeticOperation).recursiveGetAnOperand()
)
}
}Our final query should look something like this:
import cpp
import semmle.code.cpp.dataflow.TaintTracking
import DataFlow::PathGraph
// Adds recursive getAnOperand
class RecursiveBinaryArithmeticOperation extends BinaryArithmeticOperation {
Expr recursiveGetAnOperand() {
(result = this.getAnOperand() or
(
this.getAnOperand() instanceof BinaryArithmeticOperation and
result = this.getAnOperand().(RecursiveBinaryArithmeticOperation).recursiveGetAnOperand()
)) and not
result instanceof BinaryArithmeticOperation and not
result.isConstant()
}
}
// Finds all non-constant operands of arithmetic operations
class MallocSize extends Expr {
MallocSize () {
exists(
FunctionCall fc |
fc.getTarget().hasName(“malloc”) and
fc.getArgument(0) instanceof RecursiveBinaryArithmeticOperation and
this = fc.getArgument(0).(RecursiveBinaryArithmeticOperation).recursiveGetAnOperand()
)
}
}
class Config extends TaintTracking::Configuration {
Config() { this = “RecvUserInputToSink” }
override predicate isSource(DataFlow::Node source) {
exists(Expr E |
source.asExpr() = E
and not E.isConstant()
)
}
override predicate isSink(DataFlow::Node sink) {
exists(FunctionCall fc, MallocSize mc |
fc.getTarget().hasName(“memcpy”) and not
fc.getArgument(2).(VariableAccess).getTarget() = mc.(VariableAccess).getTarget() and
sink.asExpr() = fc.getArgument(2))
and not sink.asExpr().isConstant()
}
}
from Config cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink, source, sink, “any source flows to memcpy where size is operand in malloc”
This yielded 822 results of instances where the memcpy function is “guarded” by a previous malloc allocation to the destination variable.
It works! We can then use this query to filter out such occurrences from the results which captured all the memcpy sinks.
Time to celebrate!!
…or not?
Improvements
Even though the previous query worked well, there were still some loopholes that it failed to identify.
Take this for example, even though there wasn’t a direct malloc call with cln->cmdline_len in this code block, it was still flagged by our query.
This was because that same variable had been used in a malloc within an entirely different code block. Even though it was not a direct “guard” against our target memcpy function, our query was not able to discern which malloc call corresponds to which memcpy call but rather just checks if any malloc call had been used with that variable in any location.
We can implement another filter-check in our CodeQL query using classes like BasicBlock to only capture nearby malloc calls…but that’s work for another day.
Closing Off
Together with the support of a local Cybersecurity expert Eugene (spaceraccoon) and intern Chloe, we eventually filed 3 CVEs — CVE-2022–24704, CVE-2022–24705, and CVE-2022–0982 — within the Accel-PPP VPN server!
Regardless, there’s certainly much to improve on the query though I still had a blast challenging myself and exploring the fresh raw lands of a new code analysis engine CodeQL.
If you would like to try out writing CodeQL queries on your own, you can visit https://lgtm.com/ for a cool browser query console and a huge list of pre-built CodeQL-ready databases.
If you’re interested in CodeQL and the visualisation of query results in a graph format, you can also check out C0deVari4nt, a variant analysis and visualisation tool built on CodeQL and the vis.js browser-based visualisation library. We will also be presenting this tool at Black Hat Asia Arsenal 2022 happening this May 2022!
I hope this blog gave you some insights on writing CodeQL queries! Ciao!
