From 21358edc549123a754e508f2b317c811516e138a Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 26 Aug 2016 17:07:14 +0200 Subject: [PATCH] core: introduce and use nm_utils_file_set_contents() In some places we use g_file_set_contents() after a umask() to limit the permissions of the created file. Unfortunately if the containing directory has a default ACL the umask will be ignored and the new file will have a mode equal to the default ACL (since g_file_set_contents() opens the file with mode 0666). Calling a chmod() after the file gets created is insecure (see commit 60b7ed3bdc39) and so the only solution seems to be to reimplement g_file_set_contents() and accept a mode as parameter. We already had similar functions in the tree, consolidate them into a new generic utility function. https://bugzilla.gnome.org/show_bug.cgi?id=769702 --- src/nm-core-utils.c | 101 ++++++++++++++++++ src/nm-core-utils.h | 6 ++ .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 65 +---------- .../plugins/keyfile/nms-keyfile-writer.c | 77 ++----------- 4 files changed, 118 insertions(+), 131 deletions(-) diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 00ab2d3800..c2f6d1ea27 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -3657,3 +3657,104 @@ nm_utils_get_reverse_dns_domains_ip6 (const struct in6_addr *ip, guint8 plen, GP #undef N_SHIFT } + +/** + * Copied from GLib's g_file_set_contents() et al., but allows + * specifying a mode for the new file. + */ +gboolean +nm_utils_file_set_contents (const gchar *filename, + const gchar *contents, + gssize length, + mode_t mode, + GError **error) +{ + gs_free char *tmp_name = NULL; + struct stat statbuf; + int errsv; + gssize s; + int fd; + + g_return_val_if_fail (filename, FALSE); + g_return_val_if_fail (contents || !length, FALSE); + g_return_val_if_fail (!error || !*error, FALSE); + g_return_val_if_fail (length >= -1, FALSE); + + tmp_name = g_strdup_printf ("%s.XXXXXX", filename); + fd = g_mkstemp_full (tmp_name, O_RDWR, mode); + if (fd < 0) { + errsv = errno; + g_set_error (error, + G_FILE_ERROR, + g_file_error_from_errno (errsv), + "failed to create file %s: %s", + tmp_name, + g_strerror (errsv)); + return FALSE; + } + + while (length > 0) { + s = write (fd, contents, length); + if (s < 0) { + errsv = errno; + if (errsv == EINTR) + continue; + + close (fd); + unlink (tmp_name); + + g_set_error (error, + G_FILE_ERROR, + g_file_error_from_errno (errsv), + "failed to write to file %s: %s", + tmp_name, + g_strerror (errsv)); + return FALSE; + } + + g_assert (s <= length); + + contents += s; + length -= s; + } + + /* If the final destination exists and is > 0 bytes, we want to sync the + * newly written file to ensure the data is on disk when we rename over + * the destination. Otherwise if we get a system crash we can lose both + * the new and the old file on some filesystems. (I.E. those that don't + * guarantee the data is written to the disk before the metadata.) + */ + if ( lstat (filename, &statbuf) == 0 + && statbuf.st_size > 0 + && fsync (fd) != 0) { + errsv = errno; + + close (fd); + unlink (tmp_name); + + g_set_error (error, + G_FILE_ERROR, + g_file_error_from_errno (errsv), + "failed to fsync %s: %s", + tmp_name, + g_strerror (errsv)); + return FALSE; + } + + close (fd); + + if (rename (tmp_name, filename)) { + errsv = errno; + unlink (tmp_name); + g_set_error (error, + G_FILE_ERROR, + g_file_error_from_errno (errsv), + "failed to rename %s to %s: %s", + tmp_name, + filename, + g_strerror (errsv)); + return FALSE; + } + + return TRUE; +} diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index 3ef960bb6d..9e4f8f7895 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -428,4 +428,10 @@ const char *nm_utils_dnsmasq_status_to_string (int status, char *dest, gsize siz void nm_utils_get_reverse_dns_domains_ip4 (guint32 ip, guint8 plen, GPtrArray *domains); void nm_utils_get_reverse_dns_domains_ip6 (const struct in6_addr *ip, guint8 plen, GPtrArray *domains); +gboolean nm_utils_file_set_contents (const gchar *filename, + const gchar *contents, + gssize length, + mode_t mode, + GError **error); + #endif /* __NM_CORE_UTILS_H__ */ diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index d43d8e926f..e3e05e8782 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -146,62 +146,6 @@ error: svSetValue (ifcfg, key, value, FALSE); } -static gboolean -write_secret_file (const char *path, - const char *data, - gsize len, - GError **error) -{ - char *tmppath; - int fd = -1, written; - gboolean success = FALSE; - mode_t saved_umask; - - tmppath = g_malloc0 (strlen (path) + 10); - memcpy (tmppath, path, strlen (path)); - strcat (tmppath, ".XXXXXX"); - - /* Only readable by root */ - saved_umask = umask (S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH); - - errno = 0; - fd = mkstemp (tmppath); - if (fd < 0) { - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, - "Could not create temporary file for '%s': %d", - path, errno); - goto out; - } - - errno = 0; - written = write (fd, data, len); - if (written != len) { - close (fd); - unlink (tmppath); - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, - "Could not write temporary file for '%s': %d", - path, errno); - goto out; - } - close (fd); - - /* Try to rename */ - errno = 0; - if (rename (tmppath, path)) { - unlink (tmppath); - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, - "Could not rename temporary file to '%s': %d", - path, errno); - goto out; - } - success = TRUE; - -out: - umask (saved_umask); - g_free (tmppath); - return success; -} - typedef struct ObjectType { const char *setting_key; NMSetting8021xCKScheme (*scheme_func)(NMSetting8021x *setting); @@ -356,10 +300,11 @@ write_object (NMSetting8021x *s_8021x, * can use paths from now on instead of pushing around the certificate * data itself. */ - success = write_secret_file (new_file, - (const char *) g_bytes_get_data (blob, NULL), - g_bytes_get_size (blob), - &write_error); + success = nm_utils_file_set_contents (new_file, + (const char *) g_bytes_get_data (blob, NULL), + g_bytes_get_size (blob), + 0600, + &write_error); if (success) { svSetValue (ifcfg, objtype->ifcfg_key, new_file, FALSE); g_free (new_file); diff --git a/src/settings/plugins/keyfile/nms-keyfile-writer.c b/src/settings/plugins/keyfile/nms-keyfile-writer.c index a36a53b902..380d695b27 100644 --- a/src/settings/plugins/keyfile/nms-keyfile-writer.c +++ b/src/settings/plugins/keyfile/nms-keyfile-writer.c @@ -39,65 +39,6 @@ typedef struct { const char *keyfile_dir; } WriteInfo; -/*****************************************************************************/ - -static gboolean -write_cert_key_file (const char *path, - const guint8 *data, - gsize data_len, - GError **error) -{ - char *tmppath; - int fd = -1, written; - gboolean success = FALSE; - mode_t saved_umask; - - tmppath = g_malloc0 (strlen (path) + 10); - g_assert (tmppath); - memcpy (tmppath, path, strlen (path)); - strcat (tmppath, ".XXXXXX"); - - /* Only readable by root */ - saved_umask = umask (S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH); - - errno = 0; - fd = mkstemp (tmppath); - if (fd < 0) { - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, - "Could not create temporary file for '%s': %d", - path, errno); - goto out; - } - - errno = 0; - written = write (fd, data, data_len); - if (written != data_len) { - close (fd); - unlink (tmppath); - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, - "Could not write temporary file for '%s': %d", - path, errno); - goto out; - } - close (fd); - - /* Try to rename */ - errno = 0; - if (rename (tmppath, path) == 0) - success = TRUE; - else { - unlink (tmppath); - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, - "Could not rename temporary file to '%s': %d", - path, errno); - } - -out: - umask (saved_umask); - g_free (tmppath); - return success; -} - static void cert_writer (NMConnection *connection, GKeyFile *file, @@ -182,7 +123,8 @@ cert_writer (NMConnection *connection, new_path = g_strdup_printf ("%s/%s-%s.%s", info->keyfile_dir, nm_connection_get_uuid (connection), cert_data->suffix, ext); - success = write_cert_key_file (new_path, blob_data, blob_len, &local); + success = nm_utils_file_set_contents (new_path, (const gchar *) blob_data, + blob_len, 0600, &local); if (success) { /* Write the path value to the keyfile. * We know, that basename(new_path) starts with a UUID, hence no conflict with "data:;base64," */ @@ -239,8 +181,6 @@ _internal_write_connection (NMConnection *connection, WriteInfo info = { 0 }; GError *local_err = NULL; int errsv; - gboolean success = FALSE; - mode_t saved_umask; g_return_val_if_fail (!out_path || !*out_path, FALSE); g_return_val_if_fail (keyfile_dir && keyfile_dir[0] == '/', FALSE); @@ -324,15 +264,13 @@ _internal_write_connection (NMConnection *connection, if (existing_path != NULL && strcmp (path, existing_path) != 0) unlink (existing_path); - saved_umask = umask (S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH); - - g_file_set_contents (path, data, len, &local_err); + nm_utils_file_set_contents (path, data, len, 0600, &local_err); if (local_err) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, "error writing to file '%s': %s", path, local_err->message); g_error_free (local_err); - goto out; + return FALSE; } if (chown (path, owner_uid, owner_grp) < 0) { @@ -341,7 +279,7 @@ _internal_write_connection (NMConnection *connection, "error chowning '%s': %s (%d)", path, g_strerror (errsv), errsv); unlink (path); - goto out; + return FALSE; } if (out_path && g_strcmp0 (existing_path, path)) { @@ -349,10 +287,7 @@ _internal_write_connection (NMConnection *connection, path = NULL; } - success = TRUE; -out: - umask (saved_umask); - return success; + return TRUE; } gboolean