On 02/01/2013 09:17 AM, Jakub Filak wrote:
> +void dd_sanitize_items_mode_and_owner(struct dump_dir *dd, const char *name)
I see that you use this function only in one place,
in "[ABRT PATCH 2/6] dbus: sanitize touched element":
dd_save_text(dd, element, value);
+ dd_sanitize_items_mode_and_owner(dd, element);
Maybe we should do it inside dd_save_text()?
For one, we will have a fd and will be able to use fchmod/fchown,
which aren't vulnerable to symlink attacks.
dd_save_text() ends up doing this:
static bool save_binary_file(const char *path, const char* data, unsigned size,
uid_t uid, gid_t gid, mode_t mode)
{
/* the mode is set by the caller, see dd_create() for security analysis */
unlink(path);
int fd = open(path, O_WRONLY | O_TRUNC | O_CREAT | O_NOFOLLOW, mode);
if (fd < 0)
{
perror_msg("Can't open file '%s'", path);
return false;
}
if (uid != (uid_t)-1L)
{
if (fchown(fd, uid, gid) == -1)
{
perror_msg("Can't change '%s' ownership to %lu:%lu", path,
(long)uid, (long)gid);
}
}
unsigned r = full_write(fd, data, size);
close(fd);
if (r != size)
{
error_msg("Can't save file '%s'", path);
return false;
}
return true;
}
It already does fchown().
If we'll replace O_TRUNC with O_EXCL, we'd ensure that mode is set correctly too
(but... umask?); or we can bite the bullet and add fchmod() here.
--
vda
