C++: Adds another redundant null check rule

[catenacyber]

CVE ID(s)

List the CVE ID(s) associated with this vulnerability. GitHub will automatically link CVE IDs to the GitHub Advisory Database.

• CVE-2020-10957

Report

A potential NULL dereference (or redundant NULL check) may happen if a pointer is checked once against NULL in a function but not every time.

This is a new rule similar to RedundantNullCheckSimple, in addition to finding the NULL dereference from CVE-2020-10957 also finds the redundant null checks from :

• dovecot/core@cab3449
• https://hackerone.com/bugs?report_id=883606&subject=user
• https://hackerone.com/bugs?report_id=908894&subject=user

This follows #3576 cc @jbj

I used Critical/MissingNullTest.ql as a start for finding the dereference (which does not use the Intermediate Representation)
Performance seems much better now, on a project like dovecot where I could find 2 redundant/missing null checks with this rule.

• Are you planning to discuss this vulnerability submission publicly? (Blog Post, social networks, etc). We would love to have you spread the word about the good work you are doing

[jbj]

Thank you for the contribution. I look forward to seeing what results it has on a larger set of projects.

Please go through the steps in CONTRIBUTING.md -- in particular the steps where the query is moved to experimental and auto-formatted.

[jbj]

Suggested change

	blockDominates(checked.getEnclosingBlock(), unchecked.getEnclosingBlock())
	bbDominates(checked.getBasicBlock(), unchecked.getBasicBlock())

I'm guessing you're interested in BasicBlock (from the control-flow graph) rather than Block (from the tree of statements). A Block is a { ...; ...; } statement.

If so, you can delete the location-based blockDominates predicate.

[catenacyber]

indeed, thanks

[catenacyber]

In facts, it seems the basic block logic leads to false positives with condition like if (X && ptr == NULL)

[jbj]

We'll have to investigate that if we ever want to promote this query out of experimental, but we can leave it as is for now.

[jbj]

Suggested change

	if (arg2 != NULL) {
	if (arg2 != NULL) { // redundant

[catenacyber]

ok thanks

[jbj]

I'll be on holiday for the rest of July, so I'm assigning this PR over to @rdmarsh2.

[catenacyber]

I look forward to seeing what results it has on a larger set of projects.

Is there an easy way to test on all my Github repos ?

How can I get the ql file to be auto-formatted without Visual Code ?

[geoffw0]

Is there an easy way to test on all my Github repos ?

If your projects are on LGTM and favourited you should be select multiple projects at the LGTM query console.

Does this link take you to an LGTM console set to run on 12 projects?

[catenacyber]

Does this link take you to an LGTM console set to run on 12 projects?

Yes :-D thank you very much for this pointer

[catenacyber]

So, running on my 12 projects found one more alert in ndpi
Not sure what to think of it :

There is indeed a NULL check here
https://github.com/ntop/nDPI/blob/dev/src/lib/ndpi_classify.c#L514
preceded by a call to a function using the pointer as an argument
https://github.com/ntop/nDPI/blob/dev/src/lib/ndpi_classify.c#L489
Function which does not check for this pointer NULLness explicitly
https://github.com/ntop/nDPI/blob/dev/src/lib/ndpi_classify.c#L263
And dereferences it
https://github.com/ntop/nDPI/blob/dev/src/lib/ndpi_classify.c#L298

I am not sure why dereferenced(unchecked) gets this case

[geoffw0]

I am not sure why dereferenced(unchecked) gets this case

The definition of dereferenced appears to look inside function definitions to see if the corresponding parameter is dereferenced anywhere inside the function. Inside the call to ndpi_merge_splt_arrays the third parameter is dereferenced with pkt_len_twin[r].

[catenacyber]

The definition of dereferenced appears to look inside function definitions to see if the corresponding parameter is dereferenced anywhere inside the function

So, this is another true alert as far as I can tell :-)

[hubwriter]

Editorial check of the qhelp: 👍
LGTM

[jbj]

The query now complies with everything in https://github.com/github/codeql/blob/master/CONTRIBUTING.md except the auto-formatting requirement:

The queries and libraries must be autoformatted, for example using the "Format Document" command in CodeQL for Visual Studio Code.

[catenacyber]

Thanks @jbj Did I get it right now ?

[jbj]

I've started the CI tests and will merge if they're clean.