[PATCH 1/2] ssh-keygen: close TOCTOU window when writing host keys in -A mode

[Muhammad Bilal]

do_gen_all_hostkeys() (used by 'ssh-keygen -A') used mkstemp() only
to reserve a unique filename, closing the returned fd immediately:

    fd = mkstemp(prv_tmp);
    (void)close(fd); /* just using mkstemp() to reserve a name */
    ... sshkey_generate() ...        /* can take a while, e.g. RSA */
    sshkey_save_private(private, prv_tmp, ...);   /* re-opens by path */

and identically for the public key via sshkey_save_public(). Both
sshkey_save_private() and sshkey_save_public() end up calling
open(path, O_WRONLY|O_CREAT|O_TRUNC, ...) with no O_EXCL/O_NOFOLLOW.
Between the close() and that re-open, an attacker able to write to
the destination directory could unlink the reserved file and replace
it with a symlink, causing the re-open to follow it and truncate an
arbitrary path the process can write to.

This keeps each mkstemp() fd open and writes the key straight
through it -- sshkey_private_to_fileblob() + atomicio() for the
private key (mirroring what sshkey_save_private()/sshbuf_write_file()
do internally, since mkstemp() already creates the file with the same
0600 mode sshkey_save_private() achieved via umask), and fdopen() +
sshkey_write() for the public key (mirroring sshkey_save_public()).
No second path-based open() of either temp file occurs, so there is
no window to exploit. On a write failure, prv_tmp is now unlinked to
match sshbuf_write_file()'s existing cleanup-on-failure behaviour,
which the initial version of this fix had dropped. This only touches
do_gen_all_hostkeys(); the shared sshkey_save_private()/
sshkey_save_public() helpers are untouched, since they're also used
for paths a user may legitimately have symlinked on purpose (e.g. an
identity file pointed at another volume), where following the link
is expected behaviour.

Verified with strace -e trace=openat,rename on 'ssh-keygen -A -f
<dir>': the unpatched binary shows, per key, an O_CREAT|O_EXCL open
(mkstemp) immediately followed by a *second* open of the same path
with O_CREAT|O_TRUNC and no O_EXCL -- the vulnerable re-open -- before
the rename. The patched binary shows only the single O_CREAT|O_EXCL
open per path before its rename; no second open occurs. All four
generated host key types (RSA, ECDSA, ED25519, MLDSA44-ED25519) still
verify correctly with 'ssh-keygen -l' after the change, with the
expected 0600/0644 modes on the private/public files.

Note on severity: in the default deployment (-A writing to a
root-owned, non-world-writable /etc/ssh) this requires the attacker
to already have write access to the destination directory, which is
not the case out of the box -- it is not exploitable by an arbitrary
unprivileged local user against a stock install. It is a real
TOCTOU/CWE-367 pattern worth closing as defense-in-depth regardless,
and matters more directly for non-default invocations (e.g. -f
pointing at a shared/staging directory during image or package
builds).
---
 ssh-keygen.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/ssh-keygen.c b/ssh-keygen.c
index 6667a5c1b..fab1612af 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -1015,6 +1015,8 @@ do_gen_all_hostkeys(struct passwd *pw)
 	struct sshkey *private, *public;
 	char comment[1024], *prv_tmp, *pub_tmp, *prv_file, *pub_file;
 	int i, type, fd, r;
+	struct sshbuf *keyblob;
+	FILE *pub_f;
 
 	for (i = 0; key_types[i].key_type; i++) {
 		public = private = NULL;
@@ -1056,21 +1058,49 @@ do_gen_all_hostkeys(struct passwd *pw)
 			    prv_tmp, strerror(errno));
 			goto failnext;
 		}
-		(void)close(fd); /* just using mkstemp() to reserve a name */
+		/*
+		 * Keep the fd mkstemp() gave us open and write the key
+		 * through it directly, rather than closing it and having
+		 * sshkey_save_private() re-open prv_tmp by name below: an
+		 * attacker who can replace prv_tmp with a symlink in the
+		 * window between the close and the re-open could redirect
+		 * the write to an arbitrary path.
+		 */
 		bits = 0;
 		type_bits_valid(type, NULL, &bits);
 		if ((r = sshkey_generate(type, bits, &private)) != 0) {
 			error_r(r, "sshkey_generate failed");
+			close(fd);
 			goto failnext;
 		}
 		if ((r = sshkey_from_private(private, &public)) != 0)
 			fatal_fr(r, "sshkey_from_private");
 		snprintf(comment, sizeof comment, "%s@%s", pw->pw_name,
 		    hostname);
-		if ((r = sshkey_save_private(private, prv_tmp, "",
+		if ((keyblob = sshbuf_new()) == NULL)
+			fatal_f("sshbuf_new failed");
+		if ((r = sshkey_private_to_fileblob(private, keyblob, "",
 		    comment, private_key_format, openssh_format_cipher,
 		    rounds)) != 0) {
 			error_r(r, "Saving key \"%s\" failed", prv_tmp);
+			sshbuf_free(keyblob);
+			close(fd);
+			goto failnext;
+		}
+		if (atomicio(vwrite, fd, sshbuf_mutable_ptr(keyblob),
+		    sshbuf_len(keyblob)) != sshbuf_len(keyblob)) {
+			error("Could not save your private key in %s: %s",
+			    prv_tmp, strerror(errno));
+			sshbuf_free(keyblob);
+			close(fd);
+			unlink(prv_tmp);
+			goto failnext;
+		}
+		sshbuf_free(keyblob);
+		if (close(fd) != 0) {
+			error("Could not save your private key in %s: %s",
+			    prv_tmp, strerror(errno));
+			unlink(prv_tmp);
 			goto failnext;
 		}
 		if ((fd = mkstemp(pub_tmp)) == -1) {
@@ -1079,10 +1109,23 @@ do_gen_all_hostkeys(struct passwd *pw)
 			goto failnext;
 		}
 		(void)fchmod(fd, 0644);
-		(void)close(fd);
-		if ((r = sshkey_save_public(public, pub_tmp, comment)) != 0) {
+		/* As above: write through the held-open fd, don't re-open. */
+		if ((pub_f = fdopen(fd, "w")) == NULL) {
+			error("fdopen %s failed: %s", pub_tmp,
+			    strerror(errno));
+			close(fd);
+			goto failnext;
+		}
+		if ((r = sshkey_write(public, pub_f)) != 0) {
 			error_r(r, "Unable to save public key to %s",
 			    identity_file);
+			fclose(pub_f);
+			goto failnext;
+		}
+		fprintf(pub_f, " %s\n", comment);
+		if (fclose(pub_f) != 0) {
+			error("Unable to save public key to %s: %s",
+			    identity_file, strerror(errno));
 			goto failnext;
 		}
 
-- 
2.55.0