diff options
author | Lubomir Rintel <lkundrak@v3.sk> | 2016-01-29 10:27:10 +0100 |
---|---|---|
committer | Lubomir Rintel <lkundrak@v3.sk> | 2016-01-29 20:18:28 +0100 |
commit | 60b7ed3bdc3941a3b7c56824fba4b7291e79041f (patch) | |
tree | 55828c826487465033990e17438387b8b8250e77 | |
parent | 503b714f15dd7c9369ed13dfb324739a386ceb03 (diff) | |
download | NetworkManager-60b7ed3bdc3941a3b7c56824fba4b7291e79041f.tar.gz |
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.
-rw-r--r-- | src/settings/plugins/ifcfg-rh/writer.c | 16 | ||||
-rw-r--r-- | src/settings/plugins/keyfile/writer.c | 39 |
2 files changed, 21 insertions, 34 deletions
diff --git a/src/settings/plugins/ifcfg-rh/writer.c b/src/settings/plugins/ifcfg-rh/writer.c index ec174a5987..c16d7ab291 100644 --- 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 db00b06104..d03aba8ae6 100644 --- 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 |