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.

Sign up for GitHub

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

Jump to bottom

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

Open
rusty-snake opened this issue Sep 3, 2021 · 9 comments
Open

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

rusty-snake opened this issue Sep 3, 2021 · 9 comments
Labels
security Security issues and discussions
Milestone
0.9.68

Comments

@rusty-snake
Copy link
Collaborator

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 kmk3 changed the title Code scanning alerts Code scanning alerts (CodeQL CWE-367/TOCTOU warnings) Sep 22, 2021
@kmk3
Copy link
Collaborator

kmk3 commented Sep 22, 2021

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

@reinerh
Copy link
Collaborator

reinerh commented Sep 22, 2021

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

@rusty-snake rusty-snake added this to the 0.9.68 milestone Oct 13, 2021
@reinerh
Copy link
Collaborator

reinerh commented Oct 18, 2021

@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
Copy link
Collaborator

kmk3 commented Oct 19, 2021

@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 added a commit to kmk3/firejail that referenced this issue Oct 29, 2021
@kmk3
Fix TOCTOU/CodeQL CWE-367 warnings (easy ones)
ca9a8fa 
This should fix all such warnings on the following files:

* src/fids/main.c
* src/firejail/fs.c
* src/firejail/seccomp.c

Misc: Besides the above reason, these are some of the more
straightforward TOCTOU warning fixes and they are done without any
additional refactor commits, so that's the reason for "easy ones".

See https://cwe.mitre.org/data/definitions/367.html

Relates to netblue30#4503.
@kmk3 kmk3 mentioned this issue Oct 29, 2021
Merged
kmk3 added a commit to kmk3/firejail that referenced this issue Oct 29, 2021
@kmk3
Fix TOCTOU/CodeQL CWE-367 warnings (easy ones)
8e1bac2 
This should fix all such warnings on the following files:

* src/fids/main.c
* src/firejail/fs.c
* src/firejail/seccomp.c

Misc: Besides the above reason, these are some of the more
straightforward TOCTOU warning fixes and they are done without any
additional refactor commits, so that's the reason for "easy ones".

List of TOCTOU warnings:
https://github.com/netblue30/firejail/security/code-scanning?query=id%3Acpp%2Ftoctou-race-condition

See https://cwe.mitre.org/data/definitions/367.html

Relates to netblue30#4503.
@kmk3
Copy link
Collaborator

kmk3 commented Oct 29, 2021

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
Copy link
Collaborator Author

rusty-snake commented Oct 29, 2021 •
edited
Loading

Insteresting, ./configure --enable-analyzer && en_US-locale make finds only #4592 (comment) 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

firejail/configure.ac

Line 45 in d04f63c

EXTRA_CFLAGS="$EXTRA_CFLAGS -fanalyzer -Wno-analyzer-malloc-leak"

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

@smitsohu
Copy link
Collaborator

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

@rusty-snake
Copy link
Collaborator Author

rusty-snake commented Oct 29, 2021 •
edited
Loading

[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
Copy link
Collaborator

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

kmk3 added a commit to kmk3/firejail that referenced this issue Oct 30, 2021
@kmk3
Fix TOCTOU/CodeQL CWE-367 warnings (easy ones)
908f8ad 
This should fix all such warnings on the following files:

* src/fids/main.c
* src/firejail/seccomp.c

Misc: Besides the above reason, these are some of the more
straightforward TOCTOU warning fixes and they are done without any
additional refactor commits, so that's the reason for "easy ones".

List of TOCTOU warnings:
https://github.com/netblue30/firejail/security/code-scanning?query=id%3Acpp%2Ftoctou-race-condition

See https://cwe.mitre.org/data/definitions/367.html

Relates to netblue30#4503.
kmk3 added a commit to kmk3/firejail that referenced this issue Oct 30, 2021
@kmk3
fs.c: Fix TOCTOU/CodeQL CWE-367 warning
ddb828b 
Relates to netblue30#4503.
@kmk3 kmk3 mentioned this issue Dec 27, 2021
Open
@kmk3 kmk3 added the security Security issues and discussions label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
security Security issues and discussions
Projects
None yet
Milestone
0.9.68
Development

No branches or pull requests
4 participants
@reinerh @smitsohu @kmk3 @rusty-snake