Bug 2561

Summary:

ssh-keygen -A does not recreate broken zero-sized host keys

Product:

Portable OpenSSH

Reporter:

Krzysztof Cieplucha <krzysztof.cieplucha>

Component:

ssh-keygen

Assignee:

Damien Miller <djm>

Status:

CLOSED FIXED

Severity:

normal

CC:

djm, dtucker, krzysztof.cieplucha

Priority:

Version:

7.2p1

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

2698

Attachments:

Description	Flags
check that key files are loadable in ssh-keygen -A	dtucker: ok-
overwrite zero-length key files, move keys into place atomically	none
update to current	dtucker: ok-
revised diff	none
fix double-fclose	dtucker: ok+

Description Krzysztof Cieplucha 2016-04-03 22:50:45 AEST

In case something goes wrong during host keys generation and there are zero-sized files which should contain keys left in the filesystem, ssh-keygen run with -A option is not trying to re-generate those keys. As a consequence sshd daemon is unable to start because of corrupted keys and users cannot access the machine remotely through ssh.

We have observed lots of such situations during large-scale deployments. The root cause for corrupting keys is yet to be determined, but the ssh-keygen -A should take care of re-generating evidently broken keys anyway.

Simple fix would be to check not only for key file existence, but also for it's size, and re-generate the key if it does not exist or it exists but the file size is equal to zero.

The best approach would be to not only detect existence and size, but also verifying if the key is not corrupted.

Comment 1 Damien Miller 2016-04-08 14:21:46 AEST

Created attachment 2804 [details]
check that key files are loadable in ssh-keygen -A

This patch checks that public and private key files can be loaded and will re-generate keys if they can't.

Changes to authfile.c/sshkey.c required to allow running sshkey_load_public()/sshkey_load_private_type() with a NULL key argument.

Comment 2 Darren Tucker 2016-04-08 14:48:52 AEST

Comment on attachment 2804 [details]
check that key files are loadable in ssh-keygen -A

I am not sure this is a good idea.  It has the possibility of overwriting the private key if it can't be loaded due to any of: file system problem, ssh-keygen bugs, encrypted private keys, bugs in libcraypto.

Comment 3 Damien Miller 2016-04-08 15:37:05 AEST

Created attachment 2805 [details]
overwrite zero-length key files, move keys into place atomically

Darren correctly points out that the previous diff could clobber valid keys under some circumstances.

This diff is much less aggressive: it only overwrites zero-length private keys, and uses rename() to atomically move finished private keys into position so there should be fewer circumstances under which it leaves partial keys at valid keys path names.

Comment 4 Damien Miller 2016-07-22 14:10:49 AEST

retarget unfinished bugs to next release

Comment 5 Damien Miller 2016-07-22 14:14:41 AEST

retarget unfinished bugs to next release

Comment 6 Damien Miller 2016-07-22 14:15:43 AEST

retarget unfinished bugs to next release

Comment 7 Damien Miller 2016-07-22 14:17:19 AEST

retarget unfinished bugs to next release

Comment 8 Damien Miller 2016-12-16 14:31:10 AEDT

OpenSSH 7.4 release is closing; punt the bugs to 7.5

Comment 9 Damien Miller 2017-06-30 13:43:20 AEST

Move incomplete bugs to openssh-7.6 target since 7.5 shipped a while back.

To calibrate expectations, there's little chance all of these are going to make 7.6.

Comment 10 Damien Miller 2017-06-30 13:44:30 AEST

remove 7.5 target

Comment 11 Damien Miller 2017-06-30 13:59:21 AEST

Created attachment 3003 [details]
update to current

Update to HEAD, remove appending '/' to path since the paths we later append start with '/' anyway.

Comment 12 Darren Tucker 2017-06-30 14:29:33 AEST

Comment on attachment 3003 [details]
update to current

>+		public = private = NULL;
>+		xasprintf(&prv_tmp, "%s%s.XXXXXXXXXX",
>+		    identity_file, key_types[i].path);
>+		xasprintf(&pub_tmp, "%s%s.pub.XXXXXXXXXX",
>+		    identity_file, key_types[i].path);
>+		xasprintf(&prv_file, "%s%s",
>+		    identity_file, key_types[i].path);
>+		xasprintf(&pub_file, "%s%s.pub",
>+		    identity_file, key_types[i].path);
>+
>+		if (stat(prv_file, &st) == 0) {
>+			if (st.st_size != 0)
>+				continue;

This will leak prv_tmp and friends in the case where the key already exists.
The next iteration of the loop will allocate them again, leaking the first.

Comment 13 Damien Miller 2017-06-30 15:27:44 AEST

Created attachment 3004 [details]
revised diff

incorporating feedback: skip good keys early, NULL filenames to avoid risk of double-free, check fwrite/fclose errors

Comment 14 Damien Miller 2017-07-07 13:35:49 AEST

Created attachment 3009 [details]
fix double-fclose

Comment 15 Damien Miller 2017-07-07 13:55:30 AEST

Patch applied - this will be in OpenSSH 7.6

Comment 16 Damien Miller 2018-04-06 12:26:47 AEST

Close all resolved bugs after release of OpenSSH 7.7.