Thanks. Pushed.
commit cc2e4db36f25c32a118e2c4b06c719d636c3196f
Author: Jakub Filak <[email protected]>
Date: Mon Feb 4 09:13:43 2013 +0100
dd: always sanitize mode of saved file
- we use O_CREAT as the argument for open() and man says:
'The permissions of the created file are (mode & ~umask)'
- we want to set the required mode without mangling it by umask, thus we
have to
change file's mode by chmod
- related to trac#927
Signed-off-by: Jakub Filak <[email protected]>
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c
index 37606dd..213236c 100644
--- a/src/lib/dump_dir.c
+++ b/src/lib/dump_dir.c
@@ -893,6 +893,15 @@ static bool save_binary_file(const char *path, const char*
data, unsigned size,
}
}
+ /* O_CREATE in the open() call above causes that the permissions of the
+ * created file are (mode & ~umask) */
+ if (fchmod(fd, mode) == -1)
+ {
+ perror_msg("Can't change mode of '%s'", path);
+ close(fd);
+ return false;
+ }
+
unsigned r = full_write(fd, data, size);
close(fd);
if (r != size)
On Friday, February 01, 2013 05:29:15 PM Denys Vlasenko wrote:
> 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.
