diff options
author | Frantisek Sumsal <frantisek@sumsal.cz> | 2022-09-13 21:28:00 +0200 |
---|---|---|
committer | Frantisek Sumsal <frantisek@sumsal.cz> | 2022-09-13 21:32:15 +0200 |
commit | 774cf0d8fdc735f71f835987aaf153a809b53403 (patch) | |
tree | 682ff10fdfd43f11d6f609fd6cb33ea43512b15d /.github/codeql-queries | |
parent | ci: run CodeQL on push to main/stable branches as well (diff) | |
download | systemd-774cf0d8fdc735f71f835987aaf153a809b53403.tar.xz systemd-774cf0d8fdc735f71f835987aaf153a809b53403.zip |
ci: drop LGTM stuff and move remaining bits into a new location
Diffstat (limited to '.github/codeql-queries')
-rw-r--r-- | .github/codeql-queries/PotentiallyDangerousFunction.ql | 59 | ||||
-rw-r--r-- | .github/codeql-queries/UninitializedVariableWithCleanup.ql | 110 | ||||
-rw-r--r-- | .github/codeql-queries/qlpack.yml | 11 |
3 files changed, 180 insertions, 0 deletions
diff --git a/.github/codeql-queries/PotentiallyDangerousFunction.ql b/.github/codeql-queries/PotentiallyDangerousFunction.ql new file mode 100644 index 0000000000..63fd14e75f --- /dev/null +++ b/.github/codeql-queries/PotentiallyDangerousFunction.ql @@ -0,0 +1,59 @@ +/** + * vi: sw=2 ts=2 et syntax=ql: + * + * Borrowed from + * https://github.com/Semmle/ql/blob/master/cpp/ql/src/Security/CWE/CWE-676/PotentiallyDangerousFunction.ql + * + * @name Use of potentially dangerous function + * @description Certain standard library functions are dangerous to call. + * @id cpp/potentially-dangerous-function + * @kind problem + * @problem.severity error + * @precision high + * @tags reliability + * security + */ +import cpp + +predicate potentiallyDangerousFunction(Function f, string message) { + ( + f.getQualifiedName() = "fgets" and + message = "Call to fgets() is potentially dangerous. Use read_line() instead." + ) or ( + f.getQualifiedName() = "strtok" and + message = "Call to strtok() is potentially dangerous. Use extract_first_word() instead." + ) or ( + f.getQualifiedName() = "strsep" and + message = "Call to strsep() is potentially dangerous. Use extract_first_word() instead." + ) or ( + f.getQualifiedName() = "dup" and + message = "Call to dup() is potentially dangerous. Use fcntl(fd, FD_DUPFD_CLOEXEC, 3) instead." + ) or ( + f.getQualifiedName() = "htonl" and + message = "Call to htonl() is confusing. Use htobe32() instead." + ) or ( + f.getQualifiedName() = "htons" and + message = "Call to htons() is confusing. Use htobe16() instead." + ) or ( + f.getQualifiedName() = "ntohl" and + message = "Call to ntohl() is confusing. Use be32toh() instead." + ) or ( + f.getQualifiedName() = "ntohs" and + message = "Call to ntohs() is confusing. Use be16toh() instead." + ) or ( + f.getQualifiedName() = "strerror" and + message = "Call to strerror() is not thread-safe. Use strerror_r() or printf()'s %m format string instead." + ) or ( + f.getQualifiedName() = "accept" and + message = "Call to accept() is not O_CLOEXEC-safe. Use accept4() instead." + ) or ( + f.getQualifiedName() = "dirname" and + message = "Call dirname() is icky. Use path_extract_directory() instead." + ) +} + +from FunctionCall call, Function target, string message +where + call.getTarget() = target and + potentiallyDangerousFunction(target, message) +select call, message diff --git a/.github/codeql-queries/UninitializedVariableWithCleanup.ql b/.github/codeql-queries/UninitializedVariableWithCleanup.ql new file mode 100644 index 0000000000..6b3b62f8bc --- /dev/null +++ b/.github/codeql-queries/UninitializedVariableWithCleanup.ql @@ -0,0 +1,110 @@ +/** + * 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 an 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 an 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 iniitialization (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() diff --git a/.github/codeql-queries/qlpack.yml b/.github/codeql-queries/qlpack.yml new file mode 100644 index 0000000000..a1a2dec6d6 --- /dev/null +++ b/.github/codeql-queries/qlpack.yml @@ -0,0 +1,11 @@ +--- +# 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 |