Code scanning alerts (CodeQL CWE-367/TOCTOU warnings)

[rusty-snake]

GitHub seems to have updated CodeQL.

https://github.com/netblue30/firejail/security/code-scanning

Can someone have a look whether this are false-positives or unproblematic alerts. @netblue30 @smitsohu @reinerh

[kmk3]

Is anyone already working on this? I think I managed to fix some of them; might submit a PR eventually.

[reinerh]

I wanted to have a look at it, but didn't find the time so far. Please go ahead. :)

[reinerh]

@kmk3 Feel free to open PRs for the ones you already worked on. No need to fix all of them at once. Then others could already have a look at the remaining ones.

[kmk3]

@reinerh commented on Oct 18:

@kmk3 Feel free to open PRs for the ones you already worked on. No need to fix all of them at once. Then others could already have a look at the remaining ones.

Alright, I'll try to wrap up the branch and open a PR. I have indeed not fully fixed all of them; I'll just send the ones I'm more sure about in the PR.

[kmk3]

Sorry for the delay; I've opened #4652 with just the more trivial fixes to get some basic feedback. I have a few more fixes mostly ready.

[rusty-snake]

Insteresting, ./configure --enable-analyzer && en_US-locale make finds only https://github.com/netblue30/firejail/issues/4592#issuecomment-937507639 while CFLAGS=-fanalyzer ./configure && en_US-locale CFLAGS=-fanalyzer make finds a few [CWE-401] [-Wanalyzer-malloc-leak] in addition. Any my experimental meson setup finds even [CWE-415] [-Wanalyzer-double-free].

edit: this explains it

https://github.com/netblue30/firejail/blob/d04f63cba2ebfcfc4c3b99ac24e39c2b0ce37e1f/configure.ac#L45

gcc --version: gcc (GCC) 11.2.1 20210728 (Red Hat 11.2.1-1)

[smitsohu]

@rusty-snake Could you please share [CWE-415] [-Wanalyzer-double-free] ?

[rusty-snake]

[CWE-415] [-Wanalyzer-double-free]

``` ../src/fcopy/main.c: In Funktion »duplicate_dir«: ../src/fcopy/main.c:370:9: Warnung: double-»free« of »rdest« [CWE-415] [-Wanalyzer-double-free] 370 | free(rdest); | ^~~~~~~~~~~ »duplicate_dir«: events 1-2 | | 356 | static void duplicate_dir(const char *src, const char *dest, struct stat *s) { | | ^~~~~~~~~~~~~ | | | | | (1) entry to »duplicate_dir« | 357 | (void) s; | 358 | char *rsrc = check(src); | | ~~~~~~~~~~ | | | | | (2) calling »check« from »duplicate_dir« | +--> »check«: events 3-6 | | 322 | static char *check(const char *src) { | | ^~~~~ | | | | | (3) entry to »check« |...... | 325 | if (!rsrc || stat(rsrc, &s) == -1) | | ~ | | | | | (4) following »false« branch... |...... | 331 | uid_t user = getuid(); | | ~~~~~~~~ | | | | | (5) ...to here |...... | 341 | if (s.st_uid != user) | | ~ | | | | | (6) following »false« branch... | »check«: event 7 | | 346 | if (S_ISDIR(s.st_mode) || S_ISREG(s.st_mode) || S_ISLNK(s.st_mode)) | | ^ | | | | | (7) ...to here | <------+ | »duplicate_dir«: events 8-9 | | 358 | char *rsrc = check(src); | | ^~~~~~~~~~ | | | | | (8) returning to »duplicate_dir« from »check« | 359 | char *rdest = check(dest); | | ~~~~~~~~~~~ | | | | | (9) calling »check« from »duplicate_dir« | +--> »check«: events 10-13 | | 322 | static char *check(const char *src) { | | ^~~~~ | | | | | (10) entry to »check« |...... | 325 | if (!rsrc || stat(rsrc, &s) == -1) | | ~ | | | | | (11) following »false« branch... |...... | 331 | uid_t user = getuid(); | | ~~~~~~~~ | | | | | (12) ...to here |...... | 341 | if (s.st_uid != user) | | ~ | | | | | (13) following »false« branch... | »check«: event 14 | | 346 | if (S_ISDIR(s.st_mode) || S_ISREG(s.st_mode) || S_ISLNK(s.st_mode)) | | ^ | | | | | (14) ...to here | <------+ | »duplicate_dir«: events 15-19 | | 359 | char *rdest = check(dest); | | ^~~~~~~~~~~ | | | | | (15) returning to »duplicate_dir« from »check« |...... | 364 | if(nftw(rsrc, fs_copydir, 1, FTW_PHYS) != 0) { | | ~ | | | | | (16) following »false« branch... |...... | 369 | free(rsrc); | | ~~~~~~~~~~ | | | | | (17) ...to here | | (18) first »free« here | 370 | free(rdest); | | ~~~~~~~~~~~ | | | | | (19) second »free« here; first »free« was at (18) | ../src/fcopy/main.c: In Funktion »duplicate_file«: ../src/fcopy/main.c:393:9: Warnung: double-»free« of »rdest« [CWE-415] [-Wanalyzer-double-free] 393 | free(rdest); | ^~~~~~~~~~~ »duplicate_file«: events 1-2 | | 374 | static void duplicate_file(const char *src, const char *dest, struct stat *s) { | | ^~~~~~~~~~~~~~ | | | | | (1) entry to »duplicate_file« | 375 | char *rsrc = check(src); | | ~~~~~~~~~~ | | | | | (2) calling »check« from »duplicate_file« | +--> »check«: events 3-6 | | 322 | static char *check(const char *src) { | | ^~~~~ | | | | | (3) entry to »check« |...... | 325 | if (!rsrc || stat(rsrc, &s) == -1) | | ~ | | | | | (4) following »false« branch... |...... | 331 | uid_t user = getuid(); | | ~~~~~~~~ | | | | | (5) ...to here |...... | 341 | if (s.st_uid != user) | | ~ | | | | | (6) following »false« branch... | »check«: event 7 | | 346 | if (S_ISDIR(s.st_mode) || S_ISREG(s.st_mode) || S_ISLNK(s.st_mode)) | | ^ | | | | | (7) ...to here | <------+ | »duplicate_file«: events 8-9 | | 375 | char *rsrc = check(src); | | ^~~~~~~~~~ | | | | | (8) returning to »duplicate_file« from »check« | 376 | char *rdest = check(dest); | | ~~~~~~~~~~~ | | | | | (9) calling »check« from »duplicate_file« | +--> »check«: events 10-13 | | 322 | static char *check(const char *src) { | | ^~~~~ | | | | | (10) entry to »check« |...... | 325 | if (!rsrc || stat(rsrc, &s) == -1) | | ~ | | | | | (11) following »false« branch... |...... | 331 | uid_t user = getuid(); | | ~~~~~~~~ | | | | | (12) ...to here |...... | 341 | if (s.st_uid != user) | | ~ | | | | | (13) following »false« branch... | »check«: event 14 | | 346 | if (S_ISDIR(s.st_mode) || S_ISREG(s.st_mode) || S_ISLNK(s.st_mode)) | | ^ | | | | | (14) ...to here | <------+ | »duplicate_file«: events 15-18 | | 376 | char *rdest = check(dest); | | ^~~~~~~~~~~ | | | | | (15) returning to »duplicate_file« from »check« |...... | 385 | if (asprintf(&name, "%s/%s", rdest, ptr) == -1) | | ~ | | | | | (16) following »false« branch... |...... | 389 | copy_file(rsrc, name, mode, uid, gid); | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (17) ...to here | | (18) calling »copy_file« from »duplicate_file« | +--> »copy_file«: event 19 | | 104 | static void copy_file(const char *srcname, const char *destname, mode_t mode, uid_t uid, gid_t gid) { | | ^~~~~~~~~ | | | | | (19) entry to »copy_file« | »copy_file«: event 20 | | 105 | assert(srcname); | | ^~~~~~ | | | | | (20) following »true« branch (when »srcname« is non-NULL)... | »copy_file«: event 21 | | 106 | assert(destname); | | ^~~~~~ | | | | | (21) ...to here | »copy_file«: event 22 | | 106 | assert(destname); | | ^~~~~~ | | | | | (22) following »true« branch (when »destname« is non-NULL)... | »copy_file«: event 23 | | 107 | mode &= 07777; | | ~~~~~^~~~~~~~ | | | | | (23) ...to here | <------+ | »duplicate_file«: events 24-26 | | 389 | copy_file(rsrc, name, mode, uid, gid); | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (24) returning to »duplicate_file« from »copy_file« |...... | 392 | free(rsrc); | | ~~~~~~~~~~ | | | | | (25) first »free« here | 393 | free(rdest); | | ~~~~~~~~~~~ | | | | | (26) second »free« here; first »free« was at (25) | ../src/fcopy/main.c: In Funktion »duplicate_link«: ../src/fcopy/main.c:417:9: Warnung: double-»free« of »rdest« [CWE-415] [-Wanalyzer-double-free] 417 | free(rdest); | ^~~~~~~~~~~ »duplicate_link«: events 1-2 | | 397 | static void duplicate_link(const char *src, const char *dest, struct stat *s) { | | ^~~~~~~~~~~~~~ | | | | | (1) entry to »duplicate_link« | 398 | char *rsrc = check(src); // we drop the result and use the original name | | ~~~~~~~~~~ | | | | | (2) calling »check« from »duplicate_link« | +--> »check«: events 3-6 | | 322 | static char *check(const char *src) { | | ^~~~~ | | | | | (3) entry to »check« |...... | 325 | if (!rsrc || stat(rsrc, &s) == -1) | | ~ | | | | | (4) following »false« branch... |...... | 331 | uid_t user = getuid(); | | ~~~~~~~~ | | | | | (5) ...to here |...... | 341 | if (s.st_uid != user) | | ~ | | | | | (6) following »false« branch... | »check«: event 7 | | 346 | if (S_ISDIR(s.st_mode) || S_ISREG(s.st_mode) || S_ISLNK(s.st_mode)) | | ^ | | | | | (7) ...to here | <------+ | »duplicate_link«: events 8-9 | | 398 | char *rsrc = check(src); // we drop the result and use the original name | | ^~~~~~~~~~ | | | | | (8) returning to »duplicate_link« from »check« | 399 | char *rdest = check(dest); | | ~~~~~~~~~~~ | | | | | (9) calling »check« from »duplicate_link« | +--> »check«: events 10-13 | | 322 | static char *check(const char *src) { | | ^~~~~ | | | | | (10) entry to »check« |...... | 325 | if (!rsrc || stat(rsrc, &s) == -1) | | ~ | | | | | (11) following »false« branch... |...... | 331 | uid_t user = getuid(); | | ~~~~~~~~ | | | | | (12) ...to here |...... | 341 | if (s.st_uid != user) | | ~ | | | | | (13) following »false« branch... | »check«: event 14 | | 346 | if (S_ISDIR(s.st_mode) || S_ISREG(s.st_mode) || S_ISLNK(s.st_mode)) | | ^ | | | | | (14) ...to here | <------+ | »duplicate_link«: events 15-19 | | 399 | char *rdest = check(dest); | | ^~~~~~~~~~~ | | | | | (15) returning to »duplicate_link« from »check« |...... | 409 | if (asprintf(&name, "%s/%s", rdest, ptr) == -1) | | ~ | | | | | (16) following »false« branch... |...... | 413 | copy_link(rsrc, name, mode, uid, gid); | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (17) ...to here |...... | 416 | free(rsrc); | | ~~~~~~~~~~ | | | | | (18) first »free« here | 417 | free(rdest); | | ~~~~~~~~~~~ | | | | | (19) second »free« here; first »free« was at (18) | ```

[smitsohu]

@rusty-snake Thank you. I think it is a false positive, too, but maybe someone else wants to confirm.