Help: how to ensure a secure PointerFieldAccess chain

[mindcrunch4u]

Example source code:

#include <stdio.h>
#include <stdlib.h>

typedef struct {
    int name;
} type4;

typedef struct {
    int name;
    type4 *p4;
} type3;

typedef struct {
    int name;
    type3 *p3;
} type2;

typedef struct {
    int name;
    type2* p2;
} type1;

int main(void){

    type1 *p1       = malloc(sizeof(type1));
    p1->p2          = malloc(sizeof(type2));
    p1->p2->p3      = malloc(sizeof(type3));

    /* p4 is not initialized! So accessing its field will crash the program. */
    /* p1->p2->p3->p4  = malloc(sizeof(type4)); */

    if( p1 && p1->p2 && p1->p2->p3 ){
        /* the checks here are sufficient, accessing fields in p3 is safe */
        printf("p3: %d\n", p1->p2->p3->name);
    }

    if( p1 && p1->p2 ){
        /* missing checks for p1->p2->p3, and p1->p2->p3->p4! */
        printf("p4: %d\n", p1->p2->p3->p4->name);
    }

    /* pointers' nullness not checked before free! */
    free(p1->p2->p3->p4);
    free(p1->p2->p3);
    free(p1->p2);
    free(p1);

    return 0;
}

About this C source:

• in production, we are writing a packet processing/filtering/inspection app,
• after receiving a raw packet, we cast the buffer to a type, so that we can parse it further (people do this a lot),
• the thing is, after casting a pointer, its fields are sometimes not checked before use.

Programmers here used to make assumptions about a raw packet buffer, i.e., trusting the cast, but this gives the attacker a chance to construct malformed packet to crash our application, explained below:

In this example, p2, p3, p4 are similar to what we have after casting a raw buffer:

• if the packet is well-formatted, then each pointer field points to a tidy memory space,
• but if the packet is malformed, then it is possible that we get a NULL somewhere in the PointerFieldAccess chain.

And this indeed caused a few crashes in our app, so I wanted to model the issue:

• First I narrow down the scope to locally created pointers (in production, these can also be input parameters to a function call).
• And then I try to make sure that each time a pointer's field is accessed, each of the qualifiers must be previousely checked. "checked" refers to nullness check.

For example:

(this is good)

if(p1){ p1->p2; }

if(p1){
	if(...){
		if(p2){
			p1->p2->p3;
		}
	}
}

(this is bad)

if(p1){ p1->p2->p3; }

And here is my attempt:

/**
* @id localflow-test
* @kind problem
* @problem.severity warning
*/

import cpp

import semmle.code.cpp.dataflow.new.DataFlow
import semmle.code.cpp.dataflow.new.TaintTracking
import semmle.code.cpp.controlflow.Guards

class LocalPointerVariableNode extends DataFlow::Node {
    LocalPointerVariableNode(){
        /* creating the pointer itself */
        exists( Initializer i|
            i.
                getDeclaration().
                getDefinition().
                getType() instanceof PointerType
            and this.asExpr() = i.getExpr()
        )
        or
        /* assigning values to a pointer's field */
        exists( AssignExpr ae|
            ae.getLValue() instanceof PointerFieldAccess
            and this.asExpr() = ae.getLValue()
        )
    }
}

class PointerFieldAccessExprNode extends DataFlow::ExprNode {
    PointerFieldAccessExprNode(){
        /* covers accesses like p1->p2->, accessing the p2 field */
        exists(PointerFieldAccess pfa|
            this.asExpr() = pfa
        )
        or
        /* covers p1->, i.e., accessing the p1 pointer variable */
        exists(VariableAccess va|
            va.getTarget().getType() instanceof PointerType
            and this.asExpr() = va
        )
    }
}

module TestFlowCfg  implements DataFlow::ConfigSig {

    predicate isSource(DataFlow::Node source) {
      exists(LocalPointerVariableNode lpvn|
        source = lpvn
      )
    }
  
    predicate isSink(DataFlow::Node sink) {
        exists(PointerFieldAccessExprNode pfaen|
            sink = pfaen)
    }
  
    predicate isBarrier(DataFlow::Node node) {
      exists(GuardCondition gc, Variable v |
        gc.getAChild*() = v.getAnAccess() and
        node.asExpr() = v.getAnAccess() and
        gc.controls(node.asExpr().getBasicBlock(), _)
      )
    }
  }
  
  module TestFlow = TaintTracking::Global<TestFlowCfg>;
  
  from DataFlow::Node source, DataFlow::Node sink
  where TestFlow::flow(source, sink)
  select sink, "Insecure Sink"

• LocalPointerVariableNode covers the initialization and assignment of p1, p1->p2 and p1->p2->p3
• PointerFieldAccessExprNode covers pointer accesses and field accesses, such as p1->p2, p1->p2->p3.

I thought this is what I did:

• if the source (a pointer or its fields) can flow to the sink (pointer access or field access)
• and the barrier doesn't block the flow (in other words: no checks)
• then raise a warning

The script indeed detects those unsafe calls to free, but it doesn't detect exactly what I wanted, such as this part in the source code:

    if( p1 && p1->p2 ){
        /* missing checks for p1->p2->p3, and p1->p2->p3->p4! */
        printf("p4: %d\n", p1->p2->p3->p4->name);
    }

Yes, there is an if guard, but the check isn't sufficient though.

And then there is the uninitialized p4 field, running codeql-suites/cpp-security-extended.qls also shows nothing.

What I am missing here? Any help will be appreciated!

[MathiasVP]

Hi @mindcrunch4u,

I'm not sure if you really need a taint-tracking configuration for your query. The following snippet (which just uses a GuardCondition) detects all the problematic cases in your example:

/**
 * @kind problem
 */

import semmle.code.cpp.controlflow.Guards
import semmle.code.cpp.ir.ValueNumbering

/** Holds if `instr` is guarded to be non-null */
predicate isNullGuarded(Instruction instr) {
  any(IRGuardCondition gc).ensuresEq(valueNumber(instr).getAUse(), 0, instr.getBlock(), false)
}

from LoadInstruction load, Instruction qualifier, PointerFieldAccess pfa
where
  // The load is from a load of a pointer field access
  load.getUnconvertedResultExpression() = pfa and
  qualifier.getUnconvertedResultExpression() = pfa.getQualifier() and
  not isNullGuarded(qualifier)
select pfa, "This dereference is not guarded."

Perhaps you can use this as inspiration? Note that I didn't limit the query to be locally created pointers, but I'm sure you can adjust the query to only look at those.

Note that I used the C++ intermediate representation (IR) for the above query. This is because the basic blocks we have at the AST level don't properly support guards like:

if(s1 && s1->p) {
  ...
}

When working at the IR level we can see that s1 is non-null when we reach the right-hand side of &&, but this doesn't really work at the AST level.

I hope this helps! If I've misunderstood anything about your problem feel free to let me know and I'm sure we can work something out :)

[mindcrunch4u]

Hello @MathiasVP !

Your script looks very promising! But after fixing the import to remove errors, I get this:

ensuresEq(Operand, int, IRBlock, boolean) cannot be resolved for type IRGuards::IRGuardCondition

But I believe IRGuardCondition supports .ensureEq predicate, what am I doing wrong here?

Another question if you don't mind: I feel like the official guide isn't introducing some parts of CodeQL, such as the IR you are using, which looks powerful and crucial. Where can I read about these extra functionalities and learn to use them? Most of the guides cover at most taint tracking, but somehow I have a feeling that there is more to it. I really hope I can master CodeQL.

Wish you a good day!

[mindcrunch4u]

Oh, and using the original script (pasted from your comment):

• Instruction: Could not resolve type Instruction
• IRGuardCondition: Could not resolve type IRGuardCondition

So I added import semmle.code.cpp.ir.IR, with one more error left:

• IRGuardCondition: Could not resolve type IRGuardCondition

And then I added import semmle.code.cpp.controlflow.IRGuards:

• now I have both import semmle.code.cpp.controlflow.Guards and import semmle.code.cpp.controlflow.IRGuards, which are conflicting:
• The class Guards::GuardCondition, which was brought into scope by this import, conflicts with existing declarations: class IRGuards::GuardCondition from IRGuards.qll:26

So I removed import semmle.code.cpp.controlflow.Guards, and then I got the script shown in my screenshot.

[MathiasVP]

But I believe IRGuardCondition supports .ensureEq predicate, what am I doing wrong here?

Yeah, the predicate should be available for you to use. Which version of CodeQL are you using? The predicate has been available since version 2.17.0.

[MathiasVP]

Another question if you don't mind: I feel like the official guide isn't introducing some parts of CodeQL, such as the IR you are using, which looks powerful and crucial. Where can I read about these extra functionalities and learn to use them? Most of the guides cover at most taint tracking, but somehow I have a feeling that there is more to it. I really hope I can master CodeQL.

That's a very fair point. We've been wanting to write more documentation for stuff like the IR for a long time, but we've never really got around to it, unfortunately. The code itself is fairly well documented (see for example IR.qll), but I totally agree that there should be an easier starting point than diving into the library files.

I'll discuss this with the team at our next meeting. Thanks for raising this point!

[mindcrunch4u]

YOU ARE AMAZING!

This is exactly what I needed.

I am running CodeQL command-line toolchain release 2.17.5, executing the script from terminal works, while on VSCode it somehow shows the error. I will try to tweak my setup, could be something wrong with my VSCode configuration.

Thank you!!!