Skip to content
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

Add custom CodeQL query for null iterator deref #1768

Merged
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
3 changes: 3 additions & 0 deletions .github/codeql-queries/exiv2-code-scanning.qls
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Reusing existing QL Pack
- import: codeql-suites/cpp-code-scanning.qls
from: codeql-cpp
23 changes: 23 additions & 0 deletions .github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.qhelp
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<!DOCTYPE qhelp SYSTEM "qhelp.dtd">
<qhelp>
<overview>
<p>
A C++ iterator is a lot like a C pointer: if you dereference it without
first checking that it's valid then it can cause a crash.
</p>
</overview>
<recommendation>
<p>
Always check that the iterator is valid before derefencing it.
</p>
</recommendation>
<example>
<p>
<a href="https://github.com/Exiv2/exiv2/issues/1763">Issue 1763</a> was caused by
<a href="https://github.com/Exiv2/exiv2/blob/9b3ed3f9564b4ea51b43c78671435bde6b862e08/src/canonmn_int.cpp#L2755">this
dereference</a> of the iterator <tt>pos</tt>.
The bug was <a href="https://github.com/Exiv2/exiv2/pull/1767">fixed</a> by not dereferencing
<tt>pos</tt> if <tt>pos == metadata->end()</tt>.
</p>
</example>
</qhelp>
46 changes: 46 additions & 0 deletions .github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* @name NULL iterator deref
* @description Dereferencing an iterator without checking that it's valid could cause a crash.
* @kind problem
* @id cpp/null-iterator-deref
* @tags security
* external/cwe/cwe-476
*/

import cpp
import semmle.code.cpp.controlflow.Guards
import semmle.code.cpp.dataflow.DataFlow

// Holds if `cond` is a condition like `use == table.end()`.
// `eq_is_true` is `true` for `==`, `false` for '!=`.
// Note: the `==` is actually an overloaded `operator==`.
predicate end_condition(GuardCondition cond, Expr use, FunctionCall endCall, boolean eq_is_true) {
exists(FunctionCall eq |
exists(string opName | eq.getTarget().getName() = opName |
opName = "operator==" and eq_is_true = true
or
opName = "operator!=" and eq_is_true = false
) and
DataFlow::localExprFlow(use, eq.getAnArgument()) and
DataFlow::localExprFlow(endCall, eq.getAnArgument()) and
endCall.getTarget().getName() = "end" and
DataFlow::localExprFlow(eq, cond)
)
}

from FunctionCall call, Expr use
where
call.getTarget().getName() = "findKey" and
DataFlow::localExprFlow(call, use) and
use != call and
not use.(AssignExpr).getRValue() = call and
not end_condition(_, use, _, _) and
not exists(
Expr cond_use, FunctionCall endCall, GuardCondition cond, BasicBlock block, boolean branch
|
end_condition(cond, cond_use, endCall, branch) and
DataFlow::localExprFlow(call, cond_use) and
cond.controls(block, branch.booleanNot()) and
block.contains(use)
)
select call, "Iterator returned by findKey might cause a null deref $@.", use, "here"
4 changes: 4 additions & 0 deletions .github/codeql-queries/exiv2-cpp-queries/qlpack.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
name: exiv2-cpp-queries
version: 0.0.0
libraryPathDependencies: codeql-cpp
suites: exiv2-cpp-suite
5 changes: 5 additions & 0 deletions .github/codeql/codeql-config.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
name: "Exiv2 CodeQL config"

queries:
- uses: ./.github/codeql-queries/exiv2-code-scanning.qls
- uses: ./.github/codeql-queries/exiv2-cpp-queries
1 change: 1 addition & 0 deletions .github/workflows/codeql-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ jobs:
uses: github/codeql-action/init@v1
with:
languages: ${{ matrix.language }}
config-file: .github/codeql/codeql-config.yml
# If you wish to specify custom queries, you can do so here or in a config file.
# By default, queries listed here will override any specified in a config file.
# Prefix the list here with "+" to use these queries and those in the config file.
Expand Down