diff options
author | Kelvin M. Klann <kmk3.code@protonmail.com> | 2021-10-29 16:39:11 -0300 |
---|---|---|
committer | Kelvin M. Klann <kmk3.code@protonmail.com> | 2021-10-30 16:41:26 -0300 |
commit | ddb828be0716972bc98f3470811420ba60bb9666 (patch) | |
tree | 474b7d65155bc97cedd0750cf0783a29efed9748 | |
parent | Fix TOCTOU/CodeQL CWE-367 warnings (easy ones) (diff) | |
download | firejail-ddb828be0716972bc98f3470811420ba60bb9666.tar.gz firejail-ddb828be0716972bc98f3470811420ba60bb9666.tar.zst firejail-ddb828be0716972bc98f3470811420ba60bb9666.zip |
fs.c: Fix TOCTOU/CodeQL CWE-367 warning
Relates to #4503.
-rw-r--r-- | src/firejail/fs.c | 43 |
1 files changed, 22 insertions, 21 deletions
diff --git a/src/firejail/fs.c b/src/firejail/fs.c index 9c1b889ed..f62e6404e 100644 --- a/src/firejail/fs.c +++ b/src/firejail/fs.c | |||
@@ -94,16 +94,7 @@ static void disable_file(OPERATION op, const char *filename) { | |||
94 | return; | 94 | return; |
95 | } | 95 | } |
96 | 96 | ||
97 | // if the file is not present, do nothing | ||
98 | assert(fname); | 97 | assert(fname); |
99 | struct stat s; | ||
100 | if (stat(fname, &s) < 0) { | ||
101 | if (arg_debug) | ||
102 | printf("Warning (blacklisting): cannot access %s: %s\n", fname, strerror(errno)); | ||
103 | free(fname); | ||
104 | return; | ||
105 | } | ||
106 | |||
107 | // check for firejail executable | 98 | // check for firejail executable |
108 | // we might have a file found in ${PATH} pointing to /usr/bin/firejail | 99 | // we might have a file found in ${PATH} pointing to /usr/bin/firejail |
109 | // blacklisting it here will end up breaking situations like user clicks on a link in Thunderbird | 100 | // blacklisting it here will end up breaking situations like user clicks on a link in Thunderbird |
@@ -113,6 +104,24 @@ static void disable_file(OPERATION op, const char *filename) { | |||
113 | return; | 104 | return; |
114 | } | 105 | } |
115 | 106 | ||
107 | // if the file is not present, do nothing | ||
108 | int fd = open(fname, O_PATH|O_CLOEXEC); | ||
109 | if (fd < 0) { | ||
110 | if (arg_debug) | ||
111 | printf("Warning (blacklisting): cannot open %s: %s\n", fname, strerror(errno)); | ||
112 | free(fname); | ||
113 | return; | ||
114 | } | ||
115 | |||
116 | struct stat s; | ||
117 | if (fstat(fd, &s) < 0) { | ||
118 | if (arg_debug) | ||
119 | printf("Warning (blacklisting): cannot stat %s: %s\n", fname, strerror(errno)); | ||
120 | free(fname); | ||
121 | close(fd); | ||
122 | return; | ||
123 | } | ||
124 | |||
116 | // modify the file | 125 | // modify the file |
117 | if (op == BLACKLIST_FILE || op == BLACKLIST_NOLOG) { | 126 | if (op == BLACKLIST_FILE || op == BLACKLIST_NOLOG) { |
118 | // some distros put all executables under /usr/bin and make /bin a symbolic link | 127 | // some distros put all executables under /usr/bin and make /bin a symbolic link |
@@ -136,13 +145,6 @@ static void disable_file(OPERATION op, const char *filename) { | |||
136 | printf(" - no logging\n"); | 145 | printf(" - no logging\n"); |
137 | } | 146 | } |
138 | 147 | ||
139 | int fd = open(fname, O_PATH|O_CLOEXEC); | ||
140 | if (fd < 0) { | ||
141 | if (arg_debug) | ||
142 | printf("Warning (blacklisting): cannot open %s: %s\n", fname, strerror(errno)); | ||
143 | free(fname); | ||
144 | return; | ||
145 | } | ||
146 | EUID_ROOT(); | 148 | EUID_ROOT(); |
147 | if (S_ISDIR(s.st_mode)) { | 149 | if (S_ISDIR(s.st_mode)) { |
148 | if (bind_mount_path_to_fd(RUN_RO_DIR, fd) < 0) | 150 | if (bind_mount_path_to_fd(RUN_RO_DIR, fd) < 0) |
@@ -153,7 +155,6 @@ static void disable_file(OPERATION op, const char *filename) { | |||
153 | errExit("disable file"); | 155 | errExit("disable file"); |
154 | } | 156 | } |
155 | EUID_USER(); | 157 | EUID_USER(); |
156 | close(fd); | ||
157 | 158 | ||
158 | if (op == BLACKLIST_FILE) | 159 | if (op == BLACKLIST_FILE) |
159 | fs_logger2("blacklist", fname); | 160 | fs_logger2("blacklist", fname); |
@@ -180,8 +181,7 @@ static void disable_file(OPERATION op, const char *filename) { | |||
180 | else if (op == MOUNT_TMPFS) { | 181 | else if (op == MOUNT_TMPFS) { |
181 | if (!S_ISDIR(s.st_mode)) { | 182 | if (!S_ISDIR(s.st_mode)) { |
182 | fwarning("%s is not a directory; cannot mount a tmpfs on top of it.\n", fname); | 183 | fwarning("%s is not a directory; cannot mount a tmpfs on top of it.\n", fname); |
183 | free(fname); | 184 | goto out; |
184 | return; | ||
185 | } | 185 | } |
186 | 186 | ||
187 | uid_t uid = getuid(); | 187 | uid_t uid = getuid(); |
@@ -191,8 +191,7 @@ static void disable_file(OPERATION op, const char *filename) { | |||
191 | strncmp(cfg.homedir, fname, strlen(cfg.homedir)) != 0 || | 191 | strncmp(cfg.homedir, fname, strlen(cfg.homedir)) != 0 || |
192 | fname[strlen(cfg.homedir)] != '/') { | 192 | fname[strlen(cfg.homedir)] != '/') { |
193 | fwarning("you are not allowed to mount a tmpfs on %s\n", fname); | 193 | fwarning("you are not allowed to mount a tmpfs on %s\n", fname); |
194 | free(fname); | 194 | goto out; |
195 | return; | ||
196 | } | 195 | } |
197 | } | 196 | } |
198 | 197 | ||
@@ -202,6 +201,8 @@ static void disable_file(OPERATION op, const char *filename) { | |||
202 | else | 201 | else |
203 | assert(0); | 202 | assert(0); |
204 | 203 | ||
204 | out: | ||
205 | close(fd); | ||
205 | free(fname); | 206 | free(fname); |
206 | } | 207 | } |
207 | 208 | ||