linux-kernel - Re: [PATCH] utimensat() non-conformances and fixes [v3]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 30 May 2008 18:37:25 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	mtk.manpages@...glemail.com
CC:	miklos@...redi.hu, drepper@...hat.com, viro@...iv.linux.org.uk,
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	linux-man@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] utimensat() non-conformances and fixes [v3]

> Here's a further version of the patch, against 2.6.26rc2, with the
> 2008-05-19 git changes you sent me applied.  This patch is based on
> the draft patch you sent me.  I've tested this version of the patch,
> and it works as for all cases except the one mentioned below.  But
> note the following points:
> 
> 1) I didn't make use of the code in notify_change() that checks
> IS_IMMUTABLE() and IS_APPEND() (i.e., I did not add
> ATTR_OWNER_CHECK to the mask in the controlling if statement).
> Doing this can't easily be made to work for the
> do_futimes() case without reworking the arguments passed to
> notify_change().  Actually, I'm inclined to doubt whether it
> is a good idea to try to roll that check into notify_change() --
> at least for utimensat() it seems simpler to not do so.

Ugh...  Could we just omit this part (the if !times and write error
then check owner)?  I know it was my idea, but

 a) my ideas are often stupid
 b) one patch should ideally do just one thing

After we fixed the original issue, we can still think about this other
thing :)

> 2) I've found yet another divergence from the spec -- but this
> was in the original implementation, rather than being
> something that has been introduced.  In do_futimes() there is
> 
>         if (!times && !(file->f_mode & FMODE_WRITE))
>                 write_error = -EACCES;
> 
> However, the check here should not be against the f_mode (file access
> mode), but the against actual permission of the file referred to by
> the underlying descriptor.  This means that for the do_futimes() +
> times==NULL case, a set-user-ID root program could open a file
> descriptor O_RDWR/O_WRONLY for which the real UID does not have write
> access, and then even after reverting the the effective UID, the real
> user could still update file.

Sure, but so could a write(2), so that doesn't seem such a big
problem.

I think we should leave it this way, since changing it would affect
not just utimensat() and futimesat() but utime() and utimes() as well,
which are well established, old interfaces.  Shanging them could in
theory break userspace, which we try to avoid if possible.

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/