diff options
author | netblue30 <netblue30@protonmail.com> | 2021-11-11 02:38:21 +0000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-11-11 02:38:21 +0000 |
commit | 3c68903c6818767c7cfb5266d2411e362dbbe61a (patch) | |
tree | 74dc1b8b9d608160b5b307b36ce9f8698925cc12 /src | |
parent | Merge pull request #4669 from hlein/firecfg_location (diff) | |
parent | fs.c: Fix TOCTOU/CodeQL CWE-367 warning (diff) | |
download | firejail-3c68903c6818767c7cfb5266d2411e362dbbe61a.tar.gz firejail-3c68903c6818767c7cfb5266d2411e362dbbe61a.tar.zst firejail-3c68903c6818767c7cfb5266d2411e362dbbe61a.zip |
Merge pull request #4652 from kmk3/fix-toctou-easy
Fix TOCTOU/CodeQL CWE-367 warnings (easy ones + fs.c)
Diffstat (limited to 'src')
-rw-r--r-- | src/fids/main.c | 19 | ||||
-rw-r--r-- | src/firejail/fs.c | 43 | ||||
-rw-r--r-- | src/firejail/seccomp.c | 6 |
3 files changed, 38 insertions, 30 deletions
diff --git a/src/fids/main.c b/src/fids/main.c index c899b55e1..8f9bc1ea0 100644 --- a/src/fids/main.c +++ b/src/fids/main.c | |||
@@ -210,22 +210,29 @@ static void process_config(const char *fname) { | |||
210 | exit(1); | 210 | exit(1); |
211 | } | 211 | } |
212 | 212 | ||
213 | // make sure the file is owned by root | 213 | fprintf(stderr, "Opening config file %s\n", fname); |
214 | struct stat s; | 214 | int fd = open(fname, O_RDONLY|O_CLOEXEC); |
215 | if (stat(fname, &s)) { | 215 | if (fd < 0) { |
216 | if (include_level == 1) { | 216 | if (include_level == 1) { |
217 | fprintf(stderr, "Error ids: config file not found\n"); | 217 | fprintf(stderr, "Error ids: cannot open config file %s\n", fname); |
218 | exit(1); | 218 | exit(1); |
219 | } | 219 | } |
220 | return; | 220 | return; |
221 | } | 221 | } |
222 | |||
223 | // make sure the file is owned by root | ||
224 | struct stat s; | ||
225 | if (fstat(fd, &s)) { | ||
226 | fprintf(stderr, "Error ids: cannot stat config file %s\n", fname); | ||
227 | exit(1); | ||
228 | } | ||
222 | if (s.st_uid || s.st_gid) { | 229 | if (s.st_uid || s.st_gid) { |
223 | fprintf(stderr, "Error ids: config file not owned by root\n"); | 230 | fprintf(stderr, "Error ids: config file not owned by root\n"); |
224 | exit(1); | 231 | exit(1); |
225 | } | 232 | } |
226 | 233 | ||
227 | fprintf(stderr, "Loading %s config file\n", fname); | 234 | fprintf(stderr, "Loading config file %s\n", fname); |
228 | FILE *fp = fopen(fname, "r"); | 235 | FILE *fp = fdopen(fd, "r"); |
229 | if (!fp) { | 236 | if (!fp) { |
230 | fprintf(stderr, "Error fids: cannot open config file %s\n", fname); | 237 | fprintf(stderr, "Error fids: cannot open config file %s\n", fname); |
231 | exit(1); | 238 | exit(1); |
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 | ||
diff --git a/src/firejail/seccomp.c b/src/firejail/seccomp.c index 3d9bf9082..e02be29f1 100644 --- a/src/firejail/seccomp.c +++ b/src/firejail/seccomp.c | |||
@@ -435,11 +435,11 @@ void seccomp_print_filter(pid_t pid) { | |||
435 | if (asprintf(&fname, "/proc/%d/root%s", pid, RUN_SECCOMP_LIST) == -1) | 435 | if (asprintf(&fname, "/proc/%d/root%s", pid, RUN_SECCOMP_LIST) == -1) |
436 | errExit("asprintf"); | 436 | errExit("asprintf"); |
437 | 437 | ||
438 | struct stat s; | 438 | int fd = open(fname, O_RDONLY|O_CLOEXEC); |
439 | if (stat(fname, &s) == -1) | 439 | if (fd < 0) |
440 | goto errexit; | 440 | goto errexit; |
441 | 441 | ||
442 | FILE *fp = fopen(fname, "re"); | 442 | FILE *fp = fdopen(fd, "r"); |
443 | if (!fp) | 443 | if (!fp) |
444 | goto errexit; | 444 | goto errexit; |
445 | free(fname); | 445 | free(fname); |