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.

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;
 }

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;
+		goto out;
-	}
-
-	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;
 	}
 
 	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