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

3 files changed, 39 insertions, 23 deletions

diff --git a/src/lib/kdb/kdb_default.c b/src/lib/kdb/kdb_default.c
index 82fa5c4a0..b7a2f2427 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 acdca0a5a..b668147dc 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 35d14f71c..3cecabffa 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.