Re: [libvirt] [PATCH 2/4] Introduce virFileRewrite for safe file rewrite
On 10/26/2011 03:16 PM, Jiri Denemark wrote:
On Mon, Oct 24, 2011 at 16:48:58 -0600, Eric Blake wrote:
> On 10/19/2011 11:26 AM, Jiri Denemark wrote:
>> When saving config files we just overwrite old content of the file. In
>> case something fails during that process (e.g. disk gets full) we lose
>> both old and new content. This patch makes the process more robust by
>> writing the new content into a separate file and only if that succeeds
>> the original file is atomically replaced with the new one.
>> ---
>> + if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC,
mode))< 0) {
>
> Should we be using O_EXCL instead of O_TRUNC, and insisting that no .new
> file already exists, so as to protect ourselves from symlink attacks
> where someone installs a symlink and tricks us into truncating a random
> file?
I think this would need to be configurable and can be safely deferred to the
future when there is a need to use this API for other purposes than writing
libvirt's XML files. In this case we don't care about existing .new files and
it's much easier for the user running libvirtd to replace a random file than
trick libvirtd to do it.
On the other hand, while the lack of O_EXCL isn't as robust as it could
be, if someone already has enough write permissions to inject a symlink
attack into our libvirt-internal directory with .new files, they already
have enough permissions to do a lot of other damage, too. I'm okay with
pushing this as-is for now, and worrying about further improvements with
O_EXCL at a later date, if we ever use this function in more than just
libvirt-internal directories.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org