On Wed, Oct 15, 2014 at 06:12:42PM -0400, Simo Sorce wrote:
On Wed, 15 Oct 2014 22:24:04 +0200
Jakub Hrozek <jhrozek(a)redhat.com> wrote:
> - ret = check_file(filename,
> - 0, 0, S_IFSOCK|S_IRUSR|S_IWUSR, 0, NULL, true);
> + /* check_file() uses -1 as a special value that means "no
> checks" */
> + check_uid = (geteuid() == 0) ? (uid_t) -1 : geteuid();
> + check_gid = (getegid() == 0) ? (gid_t) -1 : getegid();
> +
> + ret = check_file(filename, check_uid, check_gid,
> + S_IFSOCK|S_IRUSR|S_IWUSR, 0, NULL, true);
It is difficult to read inlined ifs like this and in a critical
security case I would rather use some more real estate but make the
check crystal clear.
OK, I'll change the patch.
It would also be nice if the comment wouldn't state the obvious fact
Not sure I agree it's obvious that -1 means 'no checks', but fine..
that check_file() accepts -1 as a special value and rather explained
why you are using it to avoid checks.
I would do something like:
/* when running as root ignore file permission checks because .... */
check_uid = geteuid();
check_gid = getegid();
if (check_uid == 0) check_uid = -1;
if (check_gid == 0) check_gid = -1;
Thanks for the review!