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

Back-port actions and fuzzer to 0.27-maintenance #1854

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>
47 changes: 47 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,47 @@
/**
* @name NULL iterator deref
* @description Dereferencing an iterator without checking that it's valid could cause a crash.
* @kind problem
* @problem.severity warning
* @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
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<!DOCTYPE qhelp SYSTEM "qhelp.dtd">
<qhelp>
<overview>
<p>
The <a href="https://en.cppreference.com/w/cpp/container/vector/operator_at"><tt>operator[]</tt></a> method of <a href="https://en.cppreference.com/w/cpp/container/vector"><tt>std::vector</tt></a> does not do any bounds checking on the index. It is safer to use the <a href="https://en.cppreference.com/w/cpp/container/vector/at"><tt>at()</tt></a> method, which does do bounds checking.
</p>
</overview>
<recommendation>
<p>
Use the <a href="https://en.cppreference.com/w/cpp/container/vector/at"><tt>at()</tt></a> method, rather than <a href="https://en.cppreference.com/w/cpp/container/vector/operator_at"><tt>operator[]</tt></a>.
</p>
<p>
Some uses of <a href="https://en.cppreference.com/w/cpp/container/vector/operator_at"><tt>operator[]</tt></a> are safe because they are protected by a bounds check. The query recognises the following safe coding patterns:
</p>
<ul>
<li><tt>if (!x.empty()) { ...x[0]... }</tt></li>
<li><tt>if (x.length()) { ...x[0]... }</tt></li>
<li><tt>if (x.size() > 2) { ...x[2]... }</tt></li>
<li><tt>if (x.size() == 2) { ...x[1]... }</tt></li>
<li><tt>if (x.size() != 0) { ...x[0]... }</tt></li>
<li><tt>if (i < x.size()) { ... x[i] ... }</tt></li>
<li><tt>if (!x.empty()) { ... x[x.size() - 1] ... }</tt></li>
</ul>
</recommendation>
<example>
<p>
<a href="https://github.com/Exiv2/exiv2/issues/1706">#1706</a> was caused by a lack of bounds-checking on <a href="https://github.com/Exiv2/exiv2/blob/15098f4ef50cc721ad0018218acab2ff06e60beb/include/exiv2/value.hpp#L1639">this array access</a>. The bug was <a href="https://github.com/Exiv2/exiv2/pull/1735">fixed</a> calling the <a href="https://en.cppreference.com/w/cpp/container/vector/at"><tt>at()</tt></a> method instead.
</p>
</example>
</qhelp>
181 changes: 181 additions & 0 deletions .github/codeql-queries/exiv2-cpp-queries/unsafe_vector_access.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
/**
* @name Unsafe vector access
* @description std::vector::operator[] does not do any runtime
* bounds-checking, so it is safer to use std::vector::at()
* @kind problem
* @problem.severity warning
* @id cpp/unsafe-vector-access
* @tags security
* external/cwe/cwe-125
*/

import cpp
import semmle.code.cpp.controlflow.Guards
import semmle.code.cpp.dataflow.DataFlow
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
import semmle.code.cpp.valuenumbering.HashCons

// A call to `operator[]`.
class ArrayIndexCall extends FunctionCall {
ClassTemplateInstantiation ti;
TemplateClass tc;

ArrayIndexCall() {
this.getTarget().getName() = "operator[]" and
ti = this.getQualifier().getType().getUnspecifiedType() and
tc = ti.getTemplate() and
tc.getSimpleName() != "map" and
tc.getSimpleName() != "match_results"
}

ClassTemplateInstantiation getClassTemplateInstantiation() { result = ti }

TemplateClass getTemplateClass() { result = tc }
}

// A call to `size` or `length`.
class SizeCall extends FunctionCall {
string fname;

SizeCall() {
fname = this.getTarget().getName() and
(fname = "size" or fname = "length")
}
}

// `x[i]` is safe if `x` is a `std::array` (fixed-size array)
// and `i` within the bounds of the array.
predicate indexK_with_fixedarray(ClassTemplateInstantiation t, ArrayIndexCall call) {
t = call.getClassTemplateInstantiation() and
exists(Expr idx |
t.getSimpleName() = "array" and
idx = call.getArgument(0) and
lowerBound(idx) >= 0 and
upperBound(idx) < t.getTemplateArgument(1).(Literal).getValue().toInt()
)
}

// Holds if `cond` is a Boolean condition that checks the size of
// the array. It handles the following code patterns:
//
// 1. `if (!x.empty()) { ... }`
// 2. `if (x.length()) { ... }`
// 3. `if (x.size() > 2) { ... }`
// 4. `if (x.size() == 2) { ... }`
// 5. `if (x.size() != 0) { ... }`
//
// If it safe to assume that `x.size() >= minsize` on the exit `branch`.
predicate minimum_size_cond(Expr cond, Expr arrayExpr, int minsize, boolean branch) {
// `if (!x.empty()) { ...x[0]... }`
exists(FunctionCall emptyCall |
cond = emptyCall and
arrayExpr = emptyCall.getQualifier() and
emptyCall.getTarget().getName() = "empty" and
minsize = 1 and
branch = false
)
or
// `if (x.length()) { ...x[0]... }`
exists(SizeCall sizeCall |
cond = sizeCall and
arrayExpr = sizeCall.getQualifier() and
minsize = 1 and
branch = true
)
or
// `if (x.size() > 2) { ...x[2]... }`
exists(SizeCall sizeCall, Expr k, RelationStrictness strict |
arrayExpr = sizeCall.getQualifier() and
relOpWithSwapAndNegate(cond, sizeCall, k, Greater(), strict, branch)
|
strict = Strict() and minsize = 1 + k.getValue().toInt()
or
strict = Nonstrict() and minsize = k.getValue().toInt()
)
or
// `if (x.size() == 2) { ...x[1]... }`
exists(SizeCall sizeCall, Expr k |
arrayExpr = sizeCall.getQualifier() and
eqOpWithSwapAndNegate(cond, sizeCall, k, true, branch) and
minsize = k.getValue().toInt()
)
or
// `if (x.size() != 0) { ...x[0]... }`
exists(SizeCall sizeCall, Expr k |
arrayExpr = sizeCall.getQualifier() and
eqOpWithSwapAndNegate(cond, sizeCall, k, false, branch) and
k.getValue().toInt() = 0 and
minsize = 1
)
}

// Array accesses like these are safe:
// `if (!x.empty()) { ... x[0] ... }`
// `if (x.size() > 2) { ... x[2] ... }`
predicate indexK_with_check(GuardCondition guard, ArrayIndexCall call) {
exists(Expr arrayExpr, BasicBlock block, int i, int minsize, boolean branch |
minimum_size_cond(guard, arrayExpr, minsize, branch) and
(
globalValueNumber(arrayExpr) = globalValueNumber(call.getQualifier()) or
hashCons(arrayExpr) = hashCons(call.getQualifier())
) and
guard.controls(block, branch) and
block.contains(call) and
i = call.getArgument(0).getValue().toInt() and
0 <= i and
i < minsize
)
}

// Array accesses like this are safe:
// `if (i < x.size()) { ... x[i] ... }`
predicate indexI_with_check(GuardCondition guard, ArrayIndexCall call) {
exists(Expr idx, SizeCall sizeCall, BasicBlock block, boolean branch |
relOpWithSwapAndNegate(guard, idx, sizeCall, Lesser(), Strict(), branch) and
(
globalValueNumber(sizeCall.getQualifier()) = globalValueNumber(call.getQualifier()) and
globalValueNumber(idx) = globalValueNumber(call.getArgument(0))
or
hashCons(sizeCall.getQualifier()) = hashCons(call.getQualifier()) and
hashCons(idx) = hashCons(call.getArgument(0))
) and
guard.controls(block, branch) and
block.contains(call)
)
}

// Array accesses like this are safe:
// `if (!x.empty()) { ... x[x.size() - 1] ... }`
predicate index_last_with_check(GuardCondition guard, ArrayIndexCall call) {
exists(Expr arrayExpr, SubExpr minusExpr, SizeCall sizeCall, BasicBlock block, boolean branch |
minimum_size_cond(guard, arrayExpr, _, branch) and
(
globalValueNumber(arrayExpr) = globalValueNumber(call.getQualifier()) or
hashCons(arrayExpr) = hashCons(call.getQualifier())
) and
guard.controls(block, branch) and
block.contains(call) and
minusExpr = call.getArgument(0) and
minusExpr.getRightOperand().getValue().toInt() = 1 and
DataFlow::localExprFlow(sizeCall, minusExpr.getLeftOperand()) and
(
globalValueNumber(sizeCall.getQualifier()) = globalValueNumber(call.getQualifier()) or
hashCons(sizeCall.getQualifier()) = hashCons(call.getQualifier())
)
)
}

from ArrayIndexCall call
where
not indexK_with_fixedarray(_, call) and
not indexK_with_check(_, call) and
not indexI_with_check(_, call) and
not index_last_with_check(_, call) and
// Ignore accesses like this: `vsnprintf(&buffer[0], buffer.size(), format, args)`
// That's pointer arithmetic, not a deref, so it's usually a false positive.
not exists(AddressOfExpr addrExpr | addrExpr.getOperand() = call) and
// Ignore results in the xmpsdk directory.
not call.getLocation().getFile().getRelativePath().matches("xmpsdk/%")
select call, "Unsafe use of operator[]. Use the at() method instead."
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
61 changes: 61 additions & 0 deletions .github/workflows/codeql-analysis.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# For most projects, this workflow file will not need changing; you simply need
# to commit it to your repository.
#
# You may wish to alter this file to override the set of languages analyzed,
# or to provide custom queries or build logic.
name: "CodeQL"

on:
push:
branches: [0.27-maintenance, main]
pull_request:
# The branches below must be a subset of the branches above
branches: [0.27-maintenance, main]
workflow_dispatch:

jobs:
analyze:
name: Analyze
runs-on: ubuntu-latest

strategy:
fail-fast: false
matrix:
language: [ 'cpp' ]
# CodeQL supports [ 'cpp', 'csharp', 'go', 'java', 'javascript', 'python' ]
# Learn more...
# https://docs.github.com/en/github/finding-security-vulnerabilities-and-errors-in-your-code/configuring-code-scanning#overriding-automatic-language-detection

steps:
- name: Checkout repository
uses: actions/checkout@v2

# Initializes the CodeQL tools for scanning.
- name: Initialize CodeQL
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.
# queries: ./path/to/local/query, your-org/your-repo/queries@main

# Autobuild attempts to build any compiled languages (C/C++, C#, or Java).
# If this step fails, then you should remove it and run the build manually (see below)
- name: Autobuild
uses: github/codeql-action/autobuild@v1

# ℹ️ Command-line programs to run using the OS shell.
# 📚 https://git.io/JvXDl

# ✏️ If the Autobuild fails above, remove it and uncomment the following three lines
# and modify them (or add more) to build your code if your project
# uses a compiled language

#- run: |
# make bootstrap
# make release

- name: Perform CodeQL Analysis
uses: github/codeql-action/analyze@v1
Loading