Help with isAdditionalFlowStep for Taint Flow Involving Pointers

[sei-yhindka]

Hello CodeQL Team,

We are trying to track taint flow of a pointer and its fields through a program. Specifically, we are trying to track the taint of the function parameter named state here: https://github.com/ggerganov/whisper.cpp/blob/master/whisper.cpp#L7159. Here is the query we are using:

/**
 * @name Data Flow
 * @kind path-problem
 * @severity warning
 * @id cpp/data-flow
 */

import cpp
import semmle.code.cpp.dataflow.new.TaintTracking

module StructDataFlowConfiguration implements DataFlow::ConfigSig 
{
    predicate isSource(DataFlow::Node source) 
    {
        exists(Function fxn, Parameter p |

            // scope to function
            fxn.getName() = "whisper_exp_compute_token_level_timestamps_dtw"

            // scope to parameter within function
            and p.getFunction() = fxn
            and p.getName() = "state"
            and source.asParameter() = p

        )
    }

    predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) 
    {
        exists(PointerFieldAccess pfa |
            
            // check node2 is the FieldAccess
            node2.asExpr() = pfa

            // check node1 is an indirect parent
            and node1.asIndirectExpr() = pfa.getQualifier()
            
            // prevent false positives
            and pfa.getQualifier().getType().(UserType).hasName("whisper_state")

        )
    }
    
    predicate isSink(DataFlow::Node sink)
    {
        not isSource(sink) // Eliminate zero-step paths
    }
}

module StructDataFlow = TaintTracking::Global<StructDataFlowConfiguration>;
import StructDataFlow::PathGraph

from StructDataFlow::PathNode source, StructDataFlow::PathNode sink
// provenance is related to features from CodeQL for C# that have not been implemented for C++
where StructDataFlow::PathGraph::edges(source, sink, "provenance", "")
select 
    sink.getNode(), source, sink, "Found edge"

Currently, this query does track taint through all uses of the state parameter. However, it does not consider the fields of the state parameter as tainted, which is something we need for our use case. We have a feeling our issue is with the isAdditionalFlowStep predicate. How can we modify this so that fields of a pointer are considered tainted? Thanks in advance for your help!

[aibaars]

If I understand correctly you would like to add additional taint steps for field accesses that are performed on a tainted qualifier.

I see you used an or in your definition of isAdditionalTaintStep that should probably be an and, otherwise you end up with very many false flow steps. I guess you added the additional restriction on the type to reduce the number of false flows.

To debug things like this I'd recommend using the "quick evaluation" feature from QL for VSCode. This feature allows you to select a predicate or expression, right click, to evaluate and inspect the results. This makes it easier to figure out where you are unexpectedly losing or gaining results.

[jketema]

If you want to taint the struct and not the pointer to the struct, then for the source you can use:

source.asParameter(1) = p

instead of

source.asParameter() = p

The documentation of asParameter explains the meaning of the 1 value:

codeql/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

Lines 384 to 397 in 61593ae

	/**
	* Gets the positional parameter corresponding to the node that represents
	* the value of the parameter after `index` number of loads, if any. For
	* example, in:
	* ```cpp
	* void f(int** x) { ... }
	* ```
	* - The node `n` such that `n.asParameter(0)` is the parameter `x` represents
	* the value of `x`.
	* - The node `n` such that `n.asParameter(1)` is the parameter `x` represents
	* the value of `*x`.
	* - The node `n` such that `n.asParameter(2)` is the parameter `x` represents
	* the value of `**x`.
	*/

[sei-yhindka]

Thanks so much to both of you, @aibaars and @jketema! We tried out your solution, @jketema, but it produced many false positives. We don't want to taint the struct itself, because then all instances of the struct are tainted, whereas we are only interested in the specific parameter (state) of the function. The query below is very close to achieving the results we want:

exists(FieldAccess fa |
            
            // check node2 is the FieldAccess
            node2.asExpr() = fa

            // check node1 is an indirect parent
            and node1.asIndirectExpr() = fa.getQualifier()
            
            // prevent false positives
            and fa.getQualifier().getType().(PointerType).getBaseType().hasName("whisper_state")

        )

When we run the quick eval on the isSource predicate, CodeQL correctly identifies the state parameter as the only source. In addition, when we run the quick eval on the isAdditionalFlowStep predicate, CodeQL correctly identifies the field accesses of the the state parameter as tainted (see the screenshot below). However, when we run the whole query, we do not get the results we expect: the field accesses of the state parameter are not part of the results. We have a feeling there is a disconnect between the source and the nodes selected by the isAdditionalFlowStep predicate. Do you have any ideas on how to proceed?

[jketema]

@sei-yhindka I'm not sure I understand what you're actually after. On the one hand it seems that you only want the state pointer to be tainted. On the other hand it seems you want the whole struct to be tainted, because it seems you're trying to add an addition step that taints all the field accesses. Those seem to be conflicting requirements.

[sei-yhindka]

@jketema thank you for your response and we apologize for any confusion. We are trying to follow the taint of the state parameter, including its fields. So, as you can see on the right side of the screenshot above, CodeQL identifies flow from the pointer to its fields (which is what we want). However, those results are from a quick evaluation of the isAdditionalFlowStep predicate. When we run the whole query (including isSource and isSink predicates), we do not see these same flows, leading us to believe that there is a disconnect between the results of the isSource predicate and the results of the isAdditionalFlowStep predicate. This may be easier to resolve over a video conference call, so please let us know if that is an option.

[jketema]

Thanks for the clarification. In that case I do not understand why the solution I proposed above does not work for you.