Skip to content

Adds another rule for null deref #16524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions cpp/ql/src/experimental/Likely Bugs/DerefNullResult.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
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);
}
26 changes: 26 additions & 0 deletions cpp/ql/src/experimental/Likely Bugs/DerefNullResult.qhelp
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>

<overview>
<p>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.</p>
</overview>

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

<example>
<sample src="DerefNullResult.cpp" />
</example>

<references>
<li>
<a href="https://www.owasp.org/index.php/Null_Dereference">
Null Dereference
</a>
</li>
</references>

</qhelp>
34 changes: 34 additions & 0 deletions cpp/ql/src/experimental/Likely Bugs/DerefNullResult.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* @name Null dereference from a function result
* @description A function parameter is dereferenced,
* while it comes from a function that may return NULL,
* and is not checked for nullness by the caller.
* @kind problem
* @id cpp/deref-null-result
* @problem.severity recommendation
* @tags reliability
* security
* external/cwe/cwe-476
*/

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

from Function nuller, Parameter pd, FunctionCall fc, Variable v
where
mayReturnNull(nuller) and
functionDereferences(pd.getFunction(), pd.getIndex()) and
// 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()),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

DataFlow::exprNode(fc.getArgument(pd.getIndex()))) and
// this variable v was assigned by a call to the nuller function
unique( | | v.getAnAssignedValue()) = nuller.getACallToThisFunction() and
// this variable v is not accessed for an operation (check for NULLness)
not exists(VariableAccess vc |
vc.getTarget() = v and
(vc.getParent() instanceof Operation or vc.getParent() instanceof IfStmt)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor

@MathiasVP MathiasVP May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

select fc, "This function call may deref $@ when it can be NULL from $@", v, v.getName(), nuller,
nuller.getName()
Loading