Skip to content

Commit 6f7f86c

Browse files
dschodscho
committed
Merge branch 'codeql'
This patch series has been long in the making, ever since Johannes Nicolai and myself spiked this in November/December 2020. Signed-off-by: Johannes Schindelin <[email protected]>
2 parents 61f757b + e55ca89 commit 6f7f86c

22 files changed

+260
-60
lines changed

‎.github/codeql/codeql-config.yml

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
name: "CodeQL config"
2+
3+
queries:
4+
- uses: security-extended
5+
6+
paths-ignore:
7+
- gitweb/**/*.js # GitWeb is not distributed
8+
9+
query-filters:
10+
- exclude:
11+
# yes, this extra indentation is intentional
12+
# too common in Git's source code
13+
id: cpp/trivial-switch
14+
- exclude:
15+
id: cpp/loop-variable-changed
16+
- exclude:
17+
# we override this locally with a modified version
18+
id: cpp/non-constant-format
19+
- exclude:
20+
# Git does not consider this a problem
21+
id: cpp/irregular-enum-init
22+
- exclude:
23+
# Git has many long functions, this alert would match too many
24+
id: cpp/poorly-documented-function
25+
- exclude:
26+
# In Git, there is a lot of commented-out code
27+
id: cpp/commented-out-code
28+
- exclude:
29+
# While it is true that long switch cases are hard to read and
30+
# validate, Git has way too many for us to allow this query to
31+
# churn out alerts left and right
32+
id: cpp/long-switch
33+
- exclude:
34+
# CodeQL does not expect Git to heed the umask(), but it does
35+
id: cpp/world-writable-file-creation
36+
- exclude:
37+
# Git uses the construct `if (<not this>) ; else ...` often, to
38+
# avoid an extra indentation level. CodeQL does not like that.
39+
id: cpp/empty-block
40+
- exclude:
41+
# This rule unfortunately triggers some false positives, e.g.
42+
# where Git tries to redact URLs or where Git specifically
43+
# asks for a password upon GIT_SSL_CERT_PASSWORD_PROTECTED.
44+
id: cpp/user-controlled-bypass
45+
- exclude:
46+
# This rule fails to recognize that xmallocz() _specifically_
47+
# makes room for a trailing NUL, and instead assumes that this
48+
# function behaves like malloc(), which does not.
49+
id: cpp/invalid-pointer-deref
50+
- exclude:
51+
# CodeQL fails to recognize that xmallocz() accounts for the NUL,
52+
# instead assuming malloc() semantics.
53+
id: cpp/no-space-for-terminator
54+
- exclude:
55+
# Git does exchange plain-text passwords via stdin/stdout e.g.
56+
# with helpers in the credential protocol, or in credential-cache.
57+
# This rule, though, assumes that writing to _any_ file descriptor
58+
# is unsafe.
59+
id: cpp/cleartext-storage-file
60+
- exclude:
61+
# When storing the value of the environment variable `PWD` as the
62+
# current directory in absolute_pathdup(), or when allocating memory
63+
# for a binary patch where the size is specified in the patch itself,
64+
# CodeQL assumes that this can lead to a denial of service because
65+
# of an unbounded size, but Git's code works as designed here.
66+
id: cpp/uncontrolled-allocation-size
67+
- exclude:
68+
# lock_repo_for_gc() has admittedly obtuse logic to parse the
69+
# process ID out of the `gc.pid` file, which is correct, but
70+
# due to its construction throws a false positive here.
71+
id: cpp/missing-check-scanf
72+
- exclude:
73+
# discard_cache_entry() overwrites the name in a FLEX_ARRAY struct
74+
# if GIT_TEST_VALIDATE_INDEX_CACHE_ENTRIES is set, which CodeQL fails
75+
# to recognize as valid.
76+
id: cpp/overrun-write
77+
- exclude:
78+
# Since `time_t` can be signed or unsigned, there is unfortunately
79+
# no way to avoid letting this rule report a potential
80+
id: cpp/integer-multiplication-cast-to-long
81+
- exclude:
82+
# There are many, many legitimate code paths in Git where a path is
83+
# constructed from an environment variable, e.g. GIT_DIR. Let's suppress
84+
# this slightly overzealous query.
85+
id: cpp/path-injection
86+
- exclude:
87+
# Git has 99 instances of this at the time of writing :-(
88+
id: cpp/declaration-hides-variable
89+
- exclude:
90+
id: cpp/declaration-hides-parameter
91+
- exclude:
92+
id: cpp/local-variable-hides-global-variable
93+
- exclude:
94+
id: cpp/complex-condition
95+
- exclude:
96+
# Nested, long-winded switch statements are hard to read and hard
97+
# to reason about. Looking at you, `format_commit_one()`.
98+
id: cpp/complex-block
99+
- exclude:
100+
# There are four instances of this at time of writing, all intentional.
101+
# However, it is very easy to introduce unintentional re-use of loop
102+
# variable names, therefore we will most likely want to either change these
103+
# instances or add suppressions.
104+
id: cpp/nested-loops-with-same-variable
105+
- exclude:
106+
# zOMG so many FIXMEs
107+
id: cpp/fixme-comment
108+
- exclude:
109+
# Git assumes quite a bit about the user's control of the current worktree
110+
# Therefore, it kind of assumes that TOCTOU issues are not a thing when
111+
# it comes to files.
112+
id: cpp/toctou-race-condition
113+
- exclude:
114+
# Too many results in Git where the code was, however, intentionally written
115+
# the way it is.
116+
id: cpp/stack-address-escape
117+
- exclude:
118+
id: cpp/inconsistent-null-check
119+
- exclude:
120+
# This would trigger alerts in the functions in `help.c` that want to open
121+
# external programs to show manual pages.
122+
id: cpp/uncontrolled-process-operation
123+
- exclude:
124+
# The code in t/unit-tests/u-ctype.c implicitly exercises the `sane_istest()`
125+
# macro extensively, and CodeQL seems to miss the cast to `(unsigned char)`,
126+
# thereby mistaking the accesses for being past the end of the array (which
127+
# is incorrect).
128+
#
129+
# Ideally, we would exclude test programs from CodeQL anyways, but
130+
# unfortunately there is no Makefile rule in Git's code base to build only
131+
# the production code, and CodeQL's `paths-ignore` directive described at
132+
# https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/customizing-your-advanced-setup-for-code-scanning#specifying-directories-to-scan
133+
# unfortunately is _ignored_ for compiled languages.
134+
id: cpp/overflow-buffer

‎.github/workflows/codeql.yml

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
name: "CodeQL"
2+
3+
on:
4+
push:
5+
pull_request:
6+
workflow_dispatch:
7+
8+
jobs:
9+
analyze:
10+
name: Analyze
11+
runs-on: ubuntu-latest
12+
permissions:
13+
actions: read
14+
contents: read
15+
security-events: write
16+
17+
strategy:
18+
fail-fast: false
19+
matrix:
20+
language: ["cpp", "javascript"]
21+
22+
steps:
23+
- name: Checkout repository
24+
uses: actions/checkout@v4
25+
26+
- name: Install dependencies
27+
run: ci/install-dependencies.sh
28+
if: matrix.language == 'cpp'
29+
env:
30+
jobname: codeql
31+
32+
# Initializes the CodeQL tools for scanning.
33+
- name: Initialize CodeQL
34+
uses: github/codeql-action/init@v3
35+
with:
36+
languages: ${{ matrix.language }}
37+
config-file: ./.github/codeql/codeql-config.yml
38+
39+
- name: Build
40+
if: matrix.language == 'cpp'
41+
run: |
42+
cat /proc/cpuinfo
43+
make -j$(nproc)
44+
45+
- name: Perform CodeQL Analysis
46+
uses: github/codeql-action/analyze@v3
47+
with:
48+
upload: False
49+
output: sarif-results
50+
51+
- name: debug
52+
shell: bash
53+
run: ls -la sarif-results
54+
55+
- name: publish sarif for debugging
56+
uses: actions/upload-artifact@v4
57+
with:
58+
name: sarif-results-${{ matrix.language }}
59+
path: sarif-results
60+
61+
- name: Upload SARIF
62+
uses: github/codeql-action/upload-sarif@v3
63+
with:
64+
sarif_file: sarif-results/${{ matrix.language }}.sarif

‎.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,3 +256,5 @@ Release/
256256
CMakeSettings.json
257257
/contrib/libgit-rs/target
258258
/contrib/libgit-sys/target
259+
/.github/codeql/.cache/
260+
/.github/codeql/codeql-pack.lock.yml

‎builtin/am.c

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -434,33 +434,33 @@ static void am_load(struct am_state *state)
434434
}
435435

436436
read_state_file(&sb, state, "keep", 1);
437-
if (!strcmp(sb.buf, "t"))
437+
if (!strcmp(sb.buf, "t")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
438438
state->keep = KEEP_TRUE;
439-
else if (!strcmp(sb.buf, "b"))
439+
else if (!strcmp(sb.buf, "b")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
440440
state->keep = KEEP_NON_PATCH;
441441
else
442442
state->keep = KEEP_FALSE;
443443

444444
read_state_file(&sb, state, "messageid", 1);
445-
state->message_id = !strcmp(sb.buf, "t");
445+
state->message_id = !strcmp(sb.buf, "t"); // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
446446

447447
read_state_file(&sb, state, "scissors", 1);
448-
if (!strcmp(sb.buf, "t"))
448+
if (!strcmp(sb.buf, "t")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
449449
state->scissors = SCISSORS_TRUE;
450-
else if (!strcmp(sb.buf, "f"))
450+
else if (!strcmp(sb.buf, "f")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
451451
state->scissors = SCISSORS_FALSE;
452452
else
453453
state->scissors = SCISSORS_UNSET;
454454

455455
read_state_file(&sb, state, "quoted-cr", 1);
456456
if (!*sb.buf)
457457
state->quoted_cr = quoted_cr_unset;
458-
else if (mailinfo_parse_quoted_cr_action(sb.buf, &state->quoted_cr) != 0)
458+
else if (mailinfo_parse_quoted_cr_action(sb.buf, &state->quoted_cr) != 0) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
459459
die(_("could not parse %s"), am_path(state, "quoted-cr"));
460460

461461
read_state_file(&sb, state, "apply-opt", 1);
462462
strvec_clear(&state->git_apply_opts);
463-
if (sq_dequote_to_strvec(sb.buf, &state->git_apply_opts) < 0)
463+
if (sq_dequote_to_strvec(sb.buf, &state->git_apply_opts) < 0) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
464464
die(_("could not parse %s"), am_path(state, "apply-opt"));
465465

466466
state->rebasing = !!file_exists(am_path(state, "rebasing"));

‎builtin/clone.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
120120
continue;
121121
len = read_in_full(fd, signature, 8);
122122
close(fd);
123-
if (len != 8 || strncmp(signature, "gitdir: ", 8))
123+
if (len != 8 || strncmp(signature, "gitdir: ", 8)) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
124124
continue;
125125
dst = read_gitfile(path->buf);
126126
if (dst) {

‎builtin/commit.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1868,7 +1868,7 @@ int cmd_commit(int argc,
18681868
if (!stat(git_path_merge_mode(the_repository), &statbuf)) {
18691869
if (strbuf_read_file(&sb, git_path_merge_mode(the_repository), 0) < 0)
18701870
die_errno(_("could not read MERGE_MODE"));
1871-
if (!strcmp(sb.buf, "no-ff"))
1871+
if (!strcmp(sb.buf, "no-ff")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
18721872
allow_fast_forward = 0;
18731873
}
18741874
if (allow_fast_forward)

‎builtin/help.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ static void exec_woman_emacs(const char *path, const char *page)
278278
if (!path)
279279
path = "emacsclient";
280280
strbuf_addf(&man_page, "(woman \"%s\")", page);
281-
execlp(path, "emacsclient", "-e", man_page.buf, (char *)NULL);
281+
execlp(path, "emacsclient", "-e", man_page.buf, (char *)NULL); // CodeQL [SM01925] justification: Git's help system safely consumes user-controlled environment variables and paths
282282
warning_errno(_("failed to exec '%s'"), path);
283283
strbuf_release(&man_page);
284284
}
@@ -300,7 +300,7 @@ static void exec_man_konqueror(const char *path, const char *page)
300300
} else
301301
path = "kfmclient";
302302
strbuf_addf(&man_page, "man:%s(1)", page);
303-
execlp(path, filename, "newTab", man_page.buf, (char *)NULL);
303+
execlp(path, filename, "newTab", man_page.buf, (char *)NULL); // CodeQL [SM01925] justification: Git's help system safely consumes user-controlled environment variables and paths
304304
warning_errno(_("failed to exec '%s'"), path);
305305
strbuf_release(&man_page);
306306
}
@@ -310,7 +310,7 @@ static void exec_man_man(const char *path, const char *page)
310310
{
311311
if (!path)
312312
path = "man";
313-
execlp(path, "man", page, (char *)NULL);
313+
execlp(path, "man", page, (char *)NULL); // CodeQL [SM01925] justification: Git's help system safely consumes user-controlled environment variables and paths
314314
warning_errno(_("failed to exec '%s'"), path);
315315
}
316316

‎builtin/rebase.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -483,9 +483,9 @@ static int read_basic_state(struct rebase_options *opts)
483483
if (!read_oneliner(&buf, state_dir_path("allow_rerere_autoupdate", opts),
484484
READ_ONELINER_WARN_MISSING))
485485
return -1;
486-
if (!strcmp(buf.buf, "--rerere-autoupdate"))
486+
if (!strcmp(buf.buf, "--rerere-autoupdate")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
487487
opts->allow_rerere_autoupdate = RERERE_AUTOUPDATE;
488-
else if (!strcmp(buf.buf, "--no-rerere-autoupdate"))
488+
else if (!strcmp(buf.buf, "--no-rerere-autoupdate")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
489489
opts->allow_rerere_autoupdate = RERERE_NOAUTOUPDATE;
490490
else
491491
warning(_("ignoring invalid allow_rerere_autoupdate: "

‎bundle.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ static int parse_bundle_signature(struct bundle_header *header, const char *line
6666
int i;
6767

6868
for (i = 0; i < ARRAY_SIZE(bundle_sigs); i++) {
69-
if (!strcmp(line, bundle_sigs[i].signature)) {
69+
if (!strcmp(line, bundle_sigs[i].signature)) { // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
7070
header->version = bundle_sigs[i].version;
7171
return 0;
7272
}
@@ -82,7 +82,7 @@ int read_bundle_header_fd(int fd, struct bundle_header *header,
8282

8383
/* The bundle header begins with the signature */
8484
if (strbuf_getwholeline_fd(&buf, fd, '\n') ||
85-
parse_bundle_signature(header, buf.buf)) {
85+
parse_bundle_signature(header, buf.buf)) { // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
8686
if (report_path)
8787
error(_("'%s' does not look like a v2 or v3 bundle file"),
8888
report_path);

‎ci/install-dependencies.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ ClangFormat)
119119
sudo apt-get -q update
120120
sudo apt-get -q -y install clang-format
121121
;;
122-
StaticAnalysis)
122+
StaticAnalysis|codeql)
123123
sudo apt-get -q update
124124
sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
125125
libexpat-dev gettext make

0 commit comments

Comments
 (0)