ifcfg,keyfile: fix temporary file races (CVE-2016-0764)

Two of these raised Coverity's eyebrows.

CID 59389 (#1 of 1): Insecure temporary file (SECURE_TEMP)
5.  secure_temp: Calling mkstemp without securely setting umask first.

CID 59388 (#1 of 1): Insecure temporary file (SECURE_TEMP)
1.  secure_temp: Calling mkstemp without securely setting umask first.

Last one raised mine.

When a connection is edited and saved, there's a small window during which and
unprivileged authenticated local user can read out connection secrets (e.g. a
VPN or Wi-Fi password). The security impact is perhaps of low severity as
there's no way to force another user to save their connection.

diff --git a/src/settings/plugins/ifcfg-rh/writer.c b/src/settings/plugins/ifcfg-rh/writer.c
index ec174a5..c16d7ab 100644 (file)
--- a/src/settings/plugins/ifcfg-rh/writer.c
+++ b/src/settings/plugins/ifcfg-rh/writer.c
@@ -144,11 +144,15 @@ write_secret_file (const char *path,
        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) {
@@ -158,17 +162,6 @@ write_secret_file (const char *path,
                goto out;
        }
 
-       /* Only readable by root */
-       errno = 0;
-       if (fchmod (fd, S_IRUSR | S_IWUSR)) {
-               close (fd);
-               unlink (tmppath);
-               g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED,
-                            "Could not set permissions for temporary file '%s': %d",
-                            path, errno);
-               goto out;
-       }
-
        errno = 0;
        written = write (fd, data, len);
        if (written != len) {
@@ -193,6 +186,7 @@ write_secret_file (const char *path,
        success = TRUE;
 
 out:
+       umask (saved_umask);
        g_free (tmppath);
        return success;
 }

diff --git a/src/settings/plugins/keyfile/writer.c b/src/settings/plugins/keyfile/writer.c
index db00b06..d03aba8 100644 (file)
--- a/src/settings/plugins/keyfile/writer.c
+++ b/src/settings/plugins/keyfile/writer.c
@@ -46,12 +46,16 @@ write_cert_key_file (const char *path,
        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) {
@@ -61,17 +65,6 @@ write_cert_key_file (const char *path,
                goto out;
        }
 
-       /* Only readable by root */
-       errno = 0;
-       if (fchmod (fd, S_IRUSR | S_IWUSR) != 0) {
-               close (fd);
-               unlink (tmppath);
-               g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED,
-                            "Could not set permissions for temporary file '%s': %d",
-                            path, errno);
-               goto out;
-       }
-
        errno = 0;
        written = write (fd, data, data_len);
        if (written != data_len) {
@@ -96,6 +89,7 @@ write_cert_key_file (const char *path,
        }
 
 out:
+       umask (saved_umask);
        g_free (tmppath);
        return success;
 }
@@ -241,6 +235,8 @@ _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,13 +320,15 @@ _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);
        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);
-               return FALSE;
+               goto out;
        }
 
        if (chown (path, owner_uid, owner_grp) < 0) {
@@ -339,23 +337,18 @@ _internal_write_connection (NMConnection *connection,
                             "error chowning '%s': %s (%d)",
                             path, g_strerror (errsv), errsv);
                unlink (path);
-               return FALSE;
-       }
-
-       if (chmod (path, S_IRUSR | S_IWUSR) < 0) {
-               errsv = errno;
-               g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED,
-                            "error setting permissions on '%s': %s (%d)",
-                            path, g_strerror (errsv), errsv);
-               unlink (path);
-               return FALSE;
+               goto out;
        }
 
        if (out_path && g_strcmp0 (existing_path, path)) {
                *out_path = path;  /* pass path out to caller */
                path = NULL;
        }
-       return TRUE;
+
+       success = TRUE;
+out:
+       umask (saved_umask);
+       return success;
 }
 
 gboolean