GNOME Bugzilla – Bug 699959
[PATCH] g_file_copy(): Ensure we create private files by default
Last modified: 2013-06-05 18:06:13 UTC
If an application calls g_file_copy() on a file whose mode is e.g. 0600, we'll end up creating a temporary copy of it with mode 0644, and then doing a chown after. This is spectacularly bad for apps like ostree which use g_file_copy on /etc/shadow. This is not a complete fix; we also should set up the SELinux label for example too properly. But it's better than what we have now. --- gio/gfile.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
Created attachment 243630 [details] [review] 0001-g_file_copy-Ensure-we-create-private-files-by-defaul.patch
Created attachment 243648 [details] [review] 0001-g_file_copy-Clean-up-logic-for-info-query.patch
Next, what we need is a new API: g_output_stream_set_info() to apply the info *before* we do the rename. I'll get to that soon. The final step is to ensure we use setfscreatecon() before even calling open() - that way the resulting file will have the proper SELinux security context.
Comment on attachment 243630 [details] [review] 0001-g_file_copy-Ensure-we-create-private-files-by-defaul.patch seems right, but i'm not that familiar with gfile stuff
Review of attachment 243630 [details] [review]: Ok I did a quick self-review on this one before pushing, and I realized there's one case that's conceptually a regression. When the application explicitly specifies G_FILE_COPY_TARGET_DEFAULT_PERMS, we will end up leaving the file as mode 0600, whereas (dependent on umask) we'd end up with 0644 before. That could potentially break things. So the question is - take this simple patch which fixes a bad security issue (besides ostree, imagine a user backup program like deja-dup that might transiently expose ~/.ssh keys), or wait until I do the full stack of fixes?
Created attachment 243985 [details] [review] 0001-g_file_copy-Clean-up-logic-for-info-query.patch
Created attachment 243986 [details] [review] 0002-GLocalFileOutputStream-Deduplicate-stream-creation-c.patch
Created attachment 243987 [details] [review] 0003-GLocalFileOutputStream-Further-deduplicate-error-cod.patch
Created attachment 243988 [details] [review] 0004-Ensure-g_file_copy-does-not-temporarily-expose-priva.patch This is a much better version: * We also match executable mode of the original file atomically * Doesn't create any new bugs * Paves the way for even more improvements like matching SELinux context
See also https://bugzilla.gnome.org/show_bug.cgi?id=696014
Comment on attachment 243985 [details] [review] 0001-g_file_copy-Clean-up-logic-for-info-query.patch >+ as_move = flags & G_FILE_COPY_ALL_METADATA; while reading gfile.c to try to figure out what that is supposed to mean, I noticed that you had copied most of g_file_copy_attributes() into file_copy_fallback(). You could move the code that's duplicated between the two into build_attribute_list_for_copy() instead. (And "as_move" really ought to be renamed "copy_all_attributes"...)
Comment on attachment 243986 [details] [review] 0002-GLocalFileOutputStream-Deduplicate-stream-creation-c.patch ugh. yes
Comment on attachment 243988 [details] [review] 0004-Ensure-g_file_copy-does-not-temporarily-expose-priva.patch This looks right, I think. >+ /* In the local file path, we pass down the source info which >+ * includes things like unix::mode "In the local file *case*...". (Or "codepath".) Otherwise it sounds like you encoded the file's mode into its pathname... > return _g_local_file_output_stream_create (G_LOCAL_FILE (file)->filename, >- FALSE, >- flags, cancellable, error); >+ FALSE, flags, NULL, >+ cancellable, error); in several places you end up mixing tabbed lines with spaced lines
Created attachment 244065 [details] [review] 0001-g_file_copy-Clean-up-logic-for-info-query.patch Updated for comments.
(In reply to comment #13) > in several places you end up mixing tabbed lines with spaced lines It's the opposite from what I see; the old file had mixed, and due to my addition of .dir-locals.el in 6b6bef753, Emacs is now converting to spaces where I reindented, but I didn't reindent every call site. Options: 1) Leave the patch as is, and keep incrementally untabifying over time as people patch the source more 2) I can untabify: 2a) Each call site 2b) Each file I modified 3) Untabify all .c files in GLib
(In reply to comment #15) > (In reply to comment #13) > > > in several places you end up mixing tabbed lines with spaced lines > > It's the opposite from what I see yeah, what I meant was, your changes are introducing lines that are not consistent with the lines around them. > Options: I don't actually care much, I just figured I'd point it out. If you don't care either, then feel free to go with #1.
(In reply to comment #16) > I don't actually care much, I just figured I'd point it out. If you don't care > either, then feel free to go with #1. It's a fair point, it probably looks quite ugly to anyone not using a tab width of 4, like all vi users. I think I'll compromise here and reindent call sites only.
Created attachment 245203 [details] [review] Ensure g_file_copy() does not temporarily expose private files Just whitespace reindentation in affected callsites.
Comment on attachment 244065 [details] [review] 0001-g_file_copy-Clean-up-logic-for-info-query.patch good to go except for a bit of cruft in file_copy_fallback(): >+ GFileAttributeInfoList *attributes_to_copy = NULL, *namespaces_to_copy = NULL; >+ if (attributes_to_copy) >+ g_file_attribute_info_list_unref (attributes_to_copy); >+ if (namespaces_to_copy) >+ g_file_attribute_info_list_unref (namespaces_to_copy);
Comment on attachment 245203 [details] [review] Ensure g_file_copy() does not temporarily expose private files ok
(In reply to comment #19) > (From update of attachment 244065 [details] [review]) > good to go except for a bit of cruft in file_copy_fallback(): Good catch, thanks!
https://git.gnome.org/browse/glib/commit/?id=9f1a0b57cdca9eb2f9d8a8ecd414369df739fb8d