ci: Drop codeql workflow
After analyzing all 218 CodeQL alerts across the project's history, the
workflow has not justified its CI cost:
- The most impactful query (PotentiallyDangerousFunction) was a custom
systemd-specific query that has already been replaced by clang-tidy's
bugprone-unsafe-functions check (6fb5ec3dd1).
- Of the remaining C++ queries, 6 never triggered at all
(bad-strncpy-size, unsafe-strcat, unsafe-strncat,
suspicious-pointer-scaling, suspicious-pointer-scaling-void,
inconsistent-null-check).
- Several high-value-sounding queries had extreme false positive rates:
toctou-race-condition (95% FP), use-after-free (88% FP),
cleartext-transmission (100% FP).
- Many queries that did trigger are already covered by compiler warnings
(-Wshadow, -Wformat, -Wunused-variable, -Wreturn-type,
-Wtautological-compare) or existing clang-tidy checks
(bugprone-sizeof-expression).
- Across all alerts, only 3 genuinely useful C++ fixes can be
attributed to CodeQL: 1 tainted-format-string, 2
incorrectly-checked-scanf. The rest were either false positives or
incidental fixes during refactoring that weren't prompted by CodeQL.
- The Python queries are largely superseded by ruff (already in CI) and
had an 89% false positive rate on the security-focused checks.
The workflow consumed significant CI resources (40+ minutes per run) and
the ongoing maintenance burden of triaging false positives outweighs the
marginal value of the 2-3 real findings it produced across its entire
lifetime.
This commit is contained in:
Daan De Meyer
committed by
Zbigniew Jędrzejewski-Szmek
Zbigniew Jędrzejewski-Szmek
parent
f498a5652f
commit
53d5f5c02f
@@ -1,12 +0,0 @@
|
||||
---
|
||||
# vi: ts=2 sw=2 et:
|
||||
# SPDX-License-Identifier: LGPL-2.1-or-later
|
||||
name: "CodeQL config"
|
||||
|
||||
disable-default-queries: false
|
||||
|
||||
queries:
|
||||
- name: Enable possibly useful queries which are disabled by default
|
||||
uses: ./.github/codeql-custom.qls
|
||||
- name: systemd-specific CodeQL queries
|
||||
uses: ./.github/codeql-queries/
|
||||
@@ -1,44 +0,0 @@
|
||||
---
|
||||
# vi: ts=2 sw=2 et syntax=yaml:
|
||||
# SPDX-License-Identifier: LGPL-2.1-or-later
|
||||
#
|
||||
# Note: it is not recommended to directly reference the respective queries from
|
||||
# the github/codeql repository, so we have to "dance" around it using
|
||||
# a custom QL suite
|
||||
# See:
|
||||
# - https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning#running-additional-queries
|
||||
# - https://github.com/github/codeql-action/issues/430#issuecomment-806092120
|
||||
# - https://codeql.github.com/docs/codeql-cli/creating-codeql-query-suites/
|
||||
|
||||
# Note: the codeql/<lang>-queries pack name can be found in the CodeQL repo[0]
|
||||
# in <lang>/ql/src/qlpack.yml. The respective codeql-suites are then
|
||||
# under <lang>/ql/src/codeql-suites/.
|
||||
#
|
||||
# [0] https://github.com/github/codeql
|
||||
- import: codeql-suites/cpp-lgtm.qls
|
||||
from: codeql/cpp-queries
|
||||
- import: codeql-suites/python-lgtm.qls
|
||||
from: codeql/python-queries
|
||||
- include:
|
||||
id:
|
||||
- cpp/bad-strncpy-size
|
||||
- cpp/declaration-hides-variable
|
||||
- cpp/include-non-header
|
||||
- cpp/inconsistent-null-check
|
||||
- cpp/mistyped-function-arguments
|
||||
- cpp/nested-loops-with-same-variable
|
||||
- cpp/sizeof-side-effect
|
||||
- cpp/suspicious-pointer-scaling
|
||||
- cpp/suspicious-pointer-scaling-void
|
||||
- cpp/suspicious-sizeof
|
||||
- cpp/unsafe-strcat
|
||||
- cpp/unsafe-strncat
|
||||
- cpp/unsigned-difference-expression-compared-zero
|
||||
- cpp/unused-local-variable
|
||||
tags:
|
||||
- "security"
|
||||
- "correctness"
|
||||
severity: "error"
|
||||
- exclude:
|
||||
id:
|
||||
- cpp/fixme-comment
|
||||
@@ -1,110 +0,0 @@
|
||||
/**
|
||||
* vi: sw=2 ts=2 et syntax=ql:
|
||||
*
|
||||
* Based on cpp/uninitialized-local.
|
||||
*
|
||||
* @name Potentially uninitialized local variable using the cleanup attribute
|
||||
* @description Running the cleanup handler on a possibly uninitialized variable
|
||||
* is generally a bad idea.
|
||||
* @id cpp/uninitialized-local-with-cleanup
|
||||
* @kind problem
|
||||
* @problem.severity error
|
||||
* @precision high
|
||||
* @tags security
|
||||
*/
|
||||
|
||||
import cpp
|
||||
import semmle.code.cpp.controlflow.StackVariableReachability
|
||||
|
||||
/** Auxiliary predicate: List cleanup functions we want to explicitly ignore
|
||||
* since they don't do anything illegal even when the variable is uninitialized
|
||||
*/
|
||||
predicate cleanupFunctionDenyList(string fun) {
|
||||
fun = "erase_char"
|
||||
}
|
||||
|
||||
/**
|
||||
* A declaration of a local variable using __attribute__((__cleanup__(x)))
|
||||
* that leaves the variable uninitialized.
|
||||
*/
|
||||
DeclStmt declWithNoInit(LocalVariable v) {
|
||||
result.getADeclaration() = v and
|
||||
not v.hasInitializer() and
|
||||
/* The variable has __attribute__((__cleanup__(...))) set */
|
||||
v.getAnAttribute().hasName("cleanup") and
|
||||
/* Check if the cleanup function is not on a deny list */
|
||||
not cleanupFunctionDenyList(v.getAnAttribute().getAnArgument().getValueText())
|
||||
}
|
||||
|
||||
class UninitialisedLocalReachability extends StackVariableReachability {
|
||||
UninitialisedLocalReachability() { this = "UninitialisedLocal" }
|
||||
|
||||
override predicate isSource(ControlFlowNode node, StackVariable v) { node = declWithNoInit(v) }
|
||||
|
||||
/* Note: _don't_ use the `useOfVarActual()` predicate here (and a couple of lines
|
||||
* below), as it assumes that the callee always modifies the variable if
|
||||
* it's passed to the function.
|
||||
*
|
||||
* i.e.:
|
||||
* _cleanup_free char *x;
|
||||
* fun(&x);
|
||||
* puts(x);
|
||||
*
|
||||
* `useOfVarActual()` won't treat this as an uninitialized read even if the callee
|
||||
* doesn't modify the argument, however, `useOfVar()` will
|
||||
*/
|
||||
override predicate isSink(ControlFlowNode node, StackVariable v) { useOfVar(v, node) }
|
||||
|
||||
override predicate isBarrier(ControlFlowNode node, StackVariable v) {
|
||||
/* only report the _first_ possibly uninitialized use */
|
||||
useOfVar(v, node) or
|
||||
(
|
||||
/* If there's a return statement somewhere between the variable declaration
|
||||
* and a possible definition, don't accept is as a valid initialization.
|
||||
*
|
||||
* E.g.:
|
||||
* _cleanup_free_ char *x;
|
||||
* ...
|
||||
* if (...)
|
||||
* return;
|
||||
* ...
|
||||
* x = malloc(...);
|
||||
*
|
||||
* is not a valid initialization, since we might return from the function
|
||||
* _before_ the actual initialization (emphasis on _might_, since we
|
||||
* don't know if the return statement might ever evaluate to true).
|
||||
*/
|
||||
definitionBarrier(v, node) and
|
||||
not exists(ReturnStmt rs |
|
||||
/* The attribute check is "just" a complexity optimization */
|
||||
v.getFunction() = rs.getEnclosingFunction() and v.getAnAttribute().hasName("cleanup") |
|
||||
rs.getLocation().isBefore(node.getLocation())
|
||||
)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
pragma[noinline]
|
||||
predicate containsInlineAssembly(Function f) { exists(AsmStmt s | s.getEnclosingFunction() = f) }
|
||||
|
||||
/**
|
||||
* Auxiliary predicate: List common exceptions or false positives
|
||||
* for this check to exclude them.
|
||||
*/
|
||||
VariableAccess commonException() {
|
||||
/* If the uninitialized use we've found is in a macro expansion, it's
|
||||
* typically something like va_start(), and we don't want to complain. */
|
||||
result.getParent().isInMacroExpansion()
|
||||
or
|
||||
result.getParent() instanceof BuiltInOperation
|
||||
or
|
||||
/* Finally, exclude functions that contain assembly blocks. It's
|
||||
* anyone's guess what happens in those. */
|
||||
containsInlineAssembly(result.getEnclosingFunction())
|
||||
}
|
||||
|
||||
from UninitialisedLocalReachability r, LocalVariable v, VariableAccess va
|
||||
where
|
||||
r.reaches(_, v, va) and
|
||||
not va = commonException()
|
||||
select va, "The variable $@ may not be initialized here, but has a cleanup handler.", v, v.getName()
|
||||
@@ -1,11 +0,0 @@
|
||||
---
|
||||
# vi: ts=2 sw=2 et syntax=yaml:
|
||||
# SPDX-License-Identifier: LGPL-2.1-or-later
|
||||
|
||||
library: false
|
||||
name: systemd/cpp-queries
|
||||
version: 0.0.1
|
||||
dependencies:
|
||||
codeql/cpp-all: "*"
|
||||
codeql/suite-helpers: "*"
|
||||
extractor: cpp
|
||||
@@ -1,68 +0,0 @@
|
||||
---
|
||||
# vi: ts=2 sw=2 et:
|
||||
# SPDX-License-Identifier: LGPL-2.1-or-later
|
||||
#
|
||||
name: "CodeQL"
|
||||
|
||||
on:
|
||||
pull_request:
|
||||
branches:
|
||||
- main
|
||||
- v[0-9]+-stable
|
||||
paths:
|
||||
- '**/meson.build'
|
||||
- '.github/**/codeql*'
|
||||
- 'src/**'
|
||||
- 'test/**'
|
||||
- 'tools/**'
|
||||
push:
|
||||
branches:
|
||||
- main
|
||||
- v[0-9]+-stable
|
||||
|
||||
permissions:
|
||||
contents: read
|
||||
|
||||
jobs:
|
||||
analyze:
|
||||
name: Analyze
|
||||
if: github.repository != 'systemd/systemd-security'
|
||||
runs-on: ubuntu-24.04
|
||||
concurrency:
|
||||
group: ${{ github.workflow }}-${{ matrix.language }}-${{ github.ref }}
|
||||
cancel-in-progress: true
|
||||
permissions:
|
||||
actions: read
|
||||
security-events: write
|
||||
|
||||
strategy:
|
||||
fail-fast: false
|
||||
matrix:
|
||||
language: ['cpp', 'python']
|
||||
|
||||
steps:
|
||||
- name: Checkout repository
|
||||
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd
|
||||
with:
|
||||
persist-credentials: false
|
||||
|
||||
- name: Initialize CodeQL
|
||||
uses: github/codeql-action/init@89a39a4e59826350b863aa6b6252a07ad50cf83e
|
||||
with:
|
||||
languages: ${{ matrix.language }}
|
||||
config-file: ./.github/codeql-config.yml
|
||||
|
||||
- run: |
|
||||
sudo -E .github/workflows/unit-tests.sh SETUP
|
||||
# TODO: drop after we switch to ubuntu 26.04
|
||||
bpftool_binary=$(find /usr/lib/linux-tools/ /usr/lib/linux-tools-* -name 'bpftool' -perm /u=x 2>/dev/null | sort -r | head -n1)
|
||||
if [ -n "$bpftool_binary" ]; then
|
||||
sudo rm -f /usr/{bin,sbin}/bpftool
|
||||
sudo ln -s "$bpftool_binary" /usr/bin/
|
||||
fi
|
||||
|
||||
- name: Autobuild
|
||||
uses: github/codeql-action/autobuild@89a39a4e59826350b863aa6b6252a07ad50cf83e
|
||||
|
||||
- name: Perform CodeQL Analysis
|
||||
uses: github/codeql-action/analyze@89a39a4e59826350b863aa6b6252a07ad50cf83e
|
||||
@@ -70,10 +70,6 @@ available functionality:
|
||||
13. When building systemd from a git checkout the build scripts will
|
||||
automatically enable a git commit hook that ensures whitespace cleanliness.
|
||||
|
||||
14. [CodeQL](https://codeql.github.com/) analyzes each PR and every commit
|
||||
pushed to `main`. The list of active alerts can be found
|
||||
[here](https://github.com/systemd/systemd/security/code-scanning).
|
||||
|
||||
15. Each PR is automatically tested with [Address Sanitizer](https://clang.llvm.org/docs/AddressSanitizer.html)
|
||||
and [Undefined Behavior Sanitizer](https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html).
|
||||
See [Testing systemd using sanitizers](/TESTING_WITH_SANITIZERS)
|
||||
|
||||
@@ -339,71 +339,6 @@ where `--test-name=` is the name of the test you want to run/debug. The
|
||||
`--shell-fail` option will pause the execution in case the test fails and shows
|
||||
you the information how to connect to the testbed for further debugging.
|
||||
|
||||
## Manually running CodeQL analysis
|
||||
|
||||
This is mostly useful for debugging various CodeQL quirks.
|
||||
|
||||
Download the CodeQL Bundle from https://github.com/github/codeql-action/releases
|
||||
and unpack it somewhere. From now the 'tutorial' assumes you have the `codeql`
|
||||
binary from the unpacked archive in $PATH for brevity.
|
||||
|
||||
Switch to the systemd repository if not already:
|
||||
|
||||
```shell
|
||||
$ cd <systemd-repo>
|
||||
```
|
||||
|
||||
Create an initial CodeQL database:
|
||||
|
||||
```shell
|
||||
$ CCACHE_DISABLE=1 codeql database create codeqldb --language=cpp -vvv
|
||||
```
|
||||
|
||||
Disabling ccache is important, otherwise you might see CodeQL complaining:
|
||||
|
||||
No source code was seen and extracted to
|
||||
/home/mrc0mmand/repos/@ci-incubator/systemd/codeqldb. This can occur if the
|
||||
specified build commands failed to compile or process any code.
|
||||
- Confirm that there is some source code for the specified language in the
|
||||
project.
|
||||
- For codebases written in Go, JavaScript, TypeScript, and Python, do not
|
||||
specify an explicit --command.
|
||||
- For other languages, the --command must specify a "clean" build which
|
||||
compiles all the source code files without reusing existing build artefacts.
|
||||
|
||||
If you want to run all queries systemd uses in CodeQL, run:
|
||||
|
||||
```shell
|
||||
$ codeql database analyze codeqldb/ --format csv --output results.csv .github/codeql-custom.qls .github/codeql-queries/*.ql -vvv
|
||||
```
|
||||
|
||||
Note: this will take a while.
|
||||
|
||||
If you're interested in a specific check, the easiest way (without hunting down
|
||||
the specific CodeQL query file) is to create a custom query suite. For example:
|
||||
|
||||
```shell
|
||||
$ cat >test.qls <<EOF
|
||||
- queries: .
|
||||
from: codeql/cpp-queries
|
||||
- include:
|
||||
id:
|
||||
- cpp/missing-return
|
||||
EOF
|
||||
```
|
||||
|
||||
And then execute it in the same way as above:
|
||||
|
||||
```shell
|
||||
$ codeql database analyze codeqldb/ --format csv --output results.csv test.qls -vvv
|
||||
```
|
||||
|
||||
More about query suites here: https://codeql.github.com/docs/codeql-cli/creating-codeql-query-suites/
|
||||
|
||||
The results are then located in the `results.csv` file as a comma separated
|
||||
values list (obviously), which is the most human-friendly output format the
|
||||
CodeQL utility provides (so far).
|
||||
|
||||
## Running Coverity locally
|
||||
|
||||
Note: this requires a Coverity license, as the public tool
|
||||
|
||||
Reference in New Issue
Block a user