[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