Adds another rule for null deref

[catenacyber]

Inspired by OISF/suricata#11098

[github-actions]

QHelp previews:

cpp/ql/src/experimental/Likely Bugs/DerefNullResult.qhelp

Null dereference from a function result

This rule finds a dereference of a function parameter, whose value comes from another function call that may return NULL, without checks in the meantime.

Recommendation

A check should be added between the return of the function which may return NULL, and its use by the function dereferencing ths pointer.

Example

char * create (int arg) {
    if (arg > 42) {
        // this function may return NULL
        return NULL;
    }
    char * r = malloc(arg);
    snprintf(r, arg -1, "Hello");
    return r;
}

void process(char *str) {
    // str is dereferenced
    if (str[0] == 'H') {
        printf("Hello H\n");
    }
}

void test(int arg) {
    // first function returns a pointer that may be NULL
    char *str = create(arg);
    // str is not checked for nullness before being passed to process function
    process(str);
}

References

• Null Dereference
• Common Weakness Enumeration: CWE-476.

[MathiasVP]

Hi @catenacyber,

Thanks for your contribution. It looks like a really sensible query!

I've got one comment, but otherwise I think this looks good to me.

CI is currently failing because the query isn't formatted according to our style guide. See the Formatting section in https://github.com/github/codeql/blob/main/CONTRIBUTING.md for how to automatically format the file to fit our style guide.

[MathiasVP]

semmle.code.cpp.dataflow.DataFlow has been deprecated in favor of semmle.code.cpp.dataflow.new.DataFlow. Would it be possible to use that library instead? It should just be a matter of changing the import since the new library should be strictly better than the old one 🤞

Suggested change

	import semmle.code.cpp.dataflow.DataFlow
	import semmle.code.cpp.dataflow.new.DataFlow

[catenacyber]

Just pushed that

[catenacyber]

Thanks for your contribution. It looks like a really sensible query!

Thanks @MathiasVP

I've got one comment, but otherwise I think this looks good to me.

CI is currently failing because the query isn't formatted according to our style guide. See the Formatting section in https://github.com/github/codeql/blob/main/CONTRIBUTING.md for how to automatically format the file to fit our style guide.

Just changed that in the latest commit.
Did not manage to do the formatting with vscode, but ran codeql query format --in-place manually

[catenacyber]

Some comments on the results on the top 100 projects.

Most seem to come from ignoring that malloc or such can return NULL.

I found at least one false positive, where besides the buffer being returned, there is a size, and the size is tested instead of the pointer for the NULL/0 case...

And there also other true positives ;-)

[catenacyber]

Is this the right way to express it ?
I would also like that process(create(arg)); from upper example is found even if there is no intermediary variable

[MathiasVP]

You could do local dataflow from the expression node corresponding to a call to nuller and to the argument of fc. This would remove the need for v completely.

[catenacyber]

If I do
DataFlow::localFlow(DataFlow::exprNode(nuller.getACallToThisFunction()), DataFlow::exprNode(fc.getArgument(pd.getIndex())))

I get a lot of false positives from the right use cases where the nuller return value is checked before calling fc

[catenacyber]

Could we test that the variable is assigned only once ?
So as not to have FP with

    char *str = create_unsafe(arg);
    process_safe(str);
    str = create_safe(arg);
    process_unsafe(str);

[MathiasVP]

You could do wrap v.getAnAssignedValue() in a unique such as:

unique(| | v.getAnAssignedValue()) = nuller.getACallToThisFunction()

Note, however, that it may be better to simply remove the need for v completely as I suggested here 🙂

[catenacyber]

Just did that as the other solution does not work out so good ;-)

[MathiasVP]

Looks good to me now!

[catenacyber]

Thanks Mathias, what is next ? (for https://securitylab.github.com/bounties/)

For instance, I see multiple projects use https://github.com/nothings/stb and https://github.com/nothings/stb/blob/master/stb_image_write.h#L767 malloc return is unchecked when other malloc returns are checked

I think the google/re2 finding is a FP due to functionDereferences stating that https://github.com/google/re2/blob/b7e96b34c0945fccb8b5282404f82c7ab0843717/re2/prefilter_tree.cc#L39 dereferences its only argument...

[MathiasVP]

Ah! I was not aware that you intended to have this be part of the bounty program 🙂

I guess you want to continue to point 4 in https://securitylab.github.com/bounties/:

Create an issue using the all for one template and a detailed report on the class of vulnerabilities your query is intended to find.

[catenacyber]

Ah! I was not aware that you intended to have this be part of the bounty program 🙂

Should I not ?

I guess you want to continue to point 4 in https://securitylab.github.com/bounties/:

Create an issue using the all for one template and a detailed report on the class of vulnerabilities your query is intended to find.

Ok, will do

[catenacyber]

github/securitylab#826

[MathiasVP]

Should I not ?

That's fine. The SecurityLab team will take it from here 🤞