Adds another rule for null deref #16524
Conversation
|
QHelp previews: cpp/ql/src/experimental/Likely Bugs/DerefNullResult.qhelpNull dereference from a function resultThis rule finds a dereference of a function parameter, whose value comes from another function call that may return NULL, without checks in the meantime. RecommendationA check should be added between the return of the function which may return NULL, and its use by the function dereferencing ths pointer. Examplechar * 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
|
There was a problem hiding this comment.
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.
| */ | ||
|
|
||
| import cpp | ||
| import semmle.code.cpp.dataflow.DataFlow |
There was a problem hiding this comment.
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 🤞
| import semmle.code.cpp.dataflow.DataFlow | |
| import semmle.code.cpp.dataflow.new.DataFlow |
There was a problem hiding this comment.
Just pushed that
Thanks @MathiasVP
Just changed that in the latest commit. |
|
Some comments on the results on the top 100 projects. Most seem to come from ignoring that 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 ;-) |
| // there is a function call which will deref parameter pd | ||
| fc.getTarget() = pd.getFunction() and | ||
| // the parameter pd comes from a variable v | ||
| DataFlow::localFlow(DataFlow::exprNode(v.getAnAccess()), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| not exists(VariableAccess vc | | ||
| vc.getTarget() = v and | ||
| (vc.getParent() instanceof Operation or vc.getParent() instanceof IfStmt) | ||
| ) |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
Just did that as the other solution does not work out so good ;-)
|
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 |
|
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/:
|
Should I not ?
Ok, will do |
That's fine. The SecurityLab team will take it from here 🤞 |
Inspired by OISF/suricata#11098