krb5 commit: Remove last uses of "possibly-insecure" mktemp(3)
Benjamin Kaduk
kaduk at MIT.EDU
Mon Nov 4 14:22:22 EST 2013
https://github.com/krb5/krb5/commit/0415740bb569bad53b18f4483837e7e037f88544
commit 0415740bb569bad53b18f4483837e7e037f88544
Author: Ben Kaduk <kaduk at mit.edu>
Date: Tue Jul 3 10:27:20 2012 -0400
Remove last uses of "possibly-insecure" mktemp(3)
Many libc implementations include notations to the linker to generate
warnings upon references to mktemp(3), due to its potential for
insecure operation. This has been the case for quite some time,
as was noted in RT #6199. Our usage of the function has decreased
with time, but has not yet disappeared entirely. This commit
removes the last few instances from our tree.
kprop's credentials never need to hit the disk, so a MEMORY ccache
is sufficient (and does not need randomization).
store_master_key_list is explicitly putting keys on disk so as to
do an atomic rename of the stash file, but since the stash file
should be in a root-only directory, we can just use a fixed name
for the temporary file. When using this fixed name, we must detect
(and error out) if the temporary file already exists; add a test to
confirm that we do so.
ticket: 1794
src/lib/kdb/kdb_default.c | 37 +++++++++++++++++++++++--------------
src/slave/kprop.c | 16 +++++++---------
src/tests/t_mkey.py | 9 +++++++++
3 files changed, 39 insertions(+), 23 deletions(-)
diff --git a/src/lib/kdb/kdb_default.c b/src/lib/kdb/kdb_default.c
index 82fa5c4..b7a2f24 100644
--- a/src/lib/kdb/kdb_default.c
+++ b/src/lib/kdb/kdb_default.c
@@ -156,10 +156,6 @@ krb5_def_store_mkey_list(krb5_context context,
keyfile = defkeyfile;
}
- /*
- * XXX making the assumption that the keyfile is in a dir that requires root
- * privilege to write to thus making timing attacks unlikely.
- */
if ((statrc = stat(keyfile, &stb)) >= 0) {
/* if keyfile exists it better be a regular file */
if (!S_ISREG(stb.st_mode)) {
@@ -171,27 +167,40 @@ krb5_def_store_mkey_list(krb5_context context,
}
}
- /* Use temp keytab file name in case creation of keytab fails */
-
- /* create temp file template for use by mktemp() */
- if ((retval = asprintf(&tmp_ktname, "WRFILE:%s_XXXXXX", keyfile)) < 0) {
+ /*
+ * We assume the stash file is in a directory writable only by root.
+ * As such, don't worry about collisions, just do an atomic rename.
+ */
+ retval = asprintf(&tmp_ktname, "FILE:%s_tmp", keyfile);
+ if (retval < 0) {
krb5_set_error_message(context, retval,
_("Could not create temp keytab file name."));
goto out;
}
/*
- * Set tmp_ktpath to point to the keyfile path (skip WRFILE:). Subtracting
+ * Set tmp_ktpath to point to the keyfile path (skip FILE:). Subtracting
* 1 to account for NULL terminator in sizeof calculation of a string
* constant. Used further down.
*/
- tmp_ktpath = tmp_ktname + (sizeof("WRFILE:") - 1);
+ tmp_ktpath = tmp_ktname + (sizeof("FILE:") - 1);
- if (mktemp(tmp_ktpath) == NULL) {
+ /*
+ * This time-of-check-to-time-of-access race is fine; we care only
+ * about an administrator running the command twice, not an attacker
+ * trying to beat us to creating the file. Per the above comment, we
+ * assume the stash file is in a directory writable only by root.
+ */
+ statrc = stat(tmp_ktpath, &stb);
+ if (statrc == -1 && errno != ENOENT) {
+ /* ENOENT is the expected case */
retval = errno;
+ goto out;
+ } else if (statrc == 0) {
+ retval = EEXIST;
krb5_set_error_message(context, retval,
- _("Could not create temp stash file: %s"),
- error_message(errno));
+ _("Temporary stash file already exists: %s."),
+ tmp_ktpath);
goto out;
}
@@ -215,7 +224,7 @@ krb5_def_store_mkey_list(krb5_context context,
/* Clean up by deleting the tmp keyfile if it exists. */
(void)unlink(tmp_ktpath);
} else {
- /* rename original keyfile to original filename */
+ /* Atomically rename temp keyfile to original filename. */
if (rename(tmp_ktpath, keyfile) < 0) {
retval = errno;
krb5_set_error_message(context, retval,
diff --git a/src/slave/kprop.c b/src/slave/kprop.c
index acdca0a..b668147 100644
--- a/src/slave/kprop.c
+++ b/src/slave/kprop.c
@@ -187,9 +187,9 @@ void PRS(argc, argv)
void get_tickets(context)
krb5_context context;
{
- char buf[BUFSIZ], *def_realm;
+ char const ccname[] = "MEMORY:kpropcc";
+ char *def_realm;
krb5_error_code retval;
- static char tkstring[] = "/tmp/kproptktXXXXXX";
krb5_keytab keytab = NULL;
/*
@@ -230,20 +230,18 @@ void get_tickets(context)
#endif
/*
- * Initialize cache file which we're going to be using
+ * Use a memory cache to avoid possible filesystem conflicts.
*/
- (void) mktemp(tkstring);
- snprintf(buf, sizeof(buf), "FILE:%s", tkstring);
-
- retval = krb5_cc_resolve(context, buf, &ccache);
+ retval = krb5_cc_resolve(context, ccname, &ccache);
if (retval) {
- com_err(progname, retval, _("while opening credential cache %s"), buf);
+ com_err(progname, retval, _("while opening credential cache %s"),
+ ccname);
exit(1);
}
retval = krb5_cc_initialize(context, ccache, my_principal);
if (retval) {
- com_err(progname, retval, _("when initializing cache %s"), buf);
+ com_err(progname, retval, _("when initializing cache %s"), ccname);
exit(1);
}
diff --git a/src/tests/t_mkey.py b/src/tests/t_mkey.py
index 35d14f7..3cecabf 100644
--- a/src/tests/t_mkey.py
+++ b/src/tests/t_mkey.py
@@ -155,6 +155,15 @@ check_master_dbent(1, (1, defetype))
check_stash((1, defetype))
check_mkvno(realm.user_princ, 1)
+# Check that stash will fail if a temp stash file is already present.
+collisionfile = os.path.join(realm.testdir, 'stash_tmp')
+f = open(collisionfile, 'w')
+f.close()
+output = realm.run([kdb5_util, 'stash'], expected_code=1)
+if 'Temporary stash file already exists' not in output:
+ fail('Did not detect temp stash file collision')
+os.unlink(collisionfile)
+
# Add a new master key with no options. Verify that:
# 1. The new key appears in list_mkeys but has no activation time and
# is not active.
More information about the cvs-krb5
mailing list