[Cpp] Dataflow analysis will not consider _execvp as a sink

[jiezhuzzz]

According to the the paper Listing 1,

cpp/command-line-injection cannot detect that _execvp as a sink, because the specification of the sink (isSinkImpl) only consider exec as a vulnerable sink.

I would like to investigate why this happened. So I read the source code of isSinkImpl and search the keywords exec under the cpp, but I didn't find any useful information. Does anyone know which part of the codebase I should look for?

[jketema]

The sinks are defined by means of the shellCommand predicate, which can be found here. If you look closely at that predicate you'll see it has a case for ArrayExecFunctionCall here, which includes _execvp. So, the part of the paper you quote is actually incorrect.

One of two things is potentially going on: (1) the code studied in the paper concatenates strings in a different way than expected by the query, or (2) the dataflow library used by the query misses the relevant path from the source to the sink. It's likely (1), because the concatenation is done directly by recv in the example.

[jiezhuzzz]

Thank you for your answer. I don't see any different way for the string to concatenate, the program is quite simple. And I can only find the error usage of _execvp here.

void func(){
    char dataBuffer[100] = "dir";
    char *data = dataBuffer; 
    size_t dataLen = strlen(data); 
    int recvResult; 
    SOCKET connectSocket = INVALID_SOCKET;
    do {
        // setup socket and receive user input 
        conn = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); 
        recvResult = recv(conn, (char *)(data + dataLen), 	sizeof(char) * (100 - dataLen - 1), 0); 
        // missing validation of recvResult 
        data[dataLen + recvResult / sizeof(char)] = '\0'; 
    } while (0)
    
    char *args[] = {"%WINDIR%\\system32\\cmd.exe", "/c", data, NULL};
    _execvp("cmd.exe", args);
}

Do you mind elaborating your second point? Here, because the source comes from a user input, can we regard this as a missing of relevant path? Thanks a lot for your help. And apologies in advance if I ask some weird questions.

[jketema]

It's definitely (1) that's the problem here. The query assumes some form of string concatenation happens between the source and the sink, which is not the case here, as the concatenation happens directly at the source, i.e., in the recv call.

To be precise, one of the following needs to happen on the path, which is not the case in this example:

codeql/cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql

Lines 33 to 56 in 35a309f

	predicate interestingConcatenation(DataFlow::Node incoming, DataFlow::Node outgoing) {
	exists(FormattingFunctionCall call, int index, FormatLiteral literal |
	incoming.asIndirectArgument() = call.getConversionArgument(index) and
	outgoing.asDefiningArgument() = call.getOutputArgument(false) and
	literal = call.getFormat() and
	not literal.getConvSpecOffset(index) = 0 and
	literal.getConversionChar(index) = ["s", "S"]
	)
	or
	// strcat and friends
	exists(StrcatFunction strcatFunc, Call call |
	call.getTarget() = strcatFunc and
	incoming.asIndirectArgument() = call.getArgument(strcatFunc.getParamSrc()) and
	outgoing.asDefiningArgument() = call.getArgument(strcatFunc.getParamDest())
	)
	or
	exists(Call call, Operator op |
	call.getTarget() = op and
	op.hasQualifiedName("std", "operator+") and
	op.getType().(UserType).hasQualifiedName("std", "basic_string") and
	incoming.asIndirectArgument() = call.getArgument(1) and // left operand
	call = outgoing.asInstruction().getUnconvertedResultExpression()
	)
	}

[jiezhuzzz]

Thanks! Now I see the problem. Can we regard this as a bug? Or this design is based on some reasons.

[jketema]

This is just a design decision that we made, but there's definitely room for some improvement here.