[PATCH 4/4] appletlib: Fix file-system race in parse_config_file()
Denys Vlasenko
vda.linux at googlemail.com
Fri Jan 10 17:11:22 UTC 2014
On Thu, Jan 2, 2014 at 11:13 PM, Ryan Mallon <rmallon at gmail.com> wrote:
> There is a small filesystem race window between the stat checks for
> the Busybox config file and opening of the file. Although this window
> is very small, and the file must be called /etc/busybox, the code is
> written to be paranoid.
Paranoid of *root* replacing the file under our nose?
That's a bit too much paranoia. If attacker can do that
(replace a root-owned file), we are toast already.
It is a "pedantically more correct" code, yes, but...
>
> - if ((stat(config_file, &st) != 0) /* No config file? */
> + f = fopen_for_read(config_file);
> + if (!f)
> + return;
> +
> + if ((fstat(fileno(f), &st) != 0) /* Cannot stat? */
> || !S_ISREG(st.st_mode) /* Not a regular file? */
> || (st.st_uid != 0) /* Not owned by root? */
> || (st.st_mode & (S_IWGRP | S_IWOTH)) /* Writable by non-root? */
> - || !(f = fopen_for_read(config_file)) /* Cannot open? */
> ) {
> + fclose(f);
> return;
> }
...adds extra code (fclose call). IOW: it is not zero cost.
More information about the busybox
mailing list