openssh bugs - Apr 2016 - [Bug 2561] New: ssh-keygen -A does not recreate broken zero-sized host keys

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

            Bug ID: 2561
           Summary: ssh-keygen -A does not recreate broken zero-sized host
                    keys
           Product: Portable OpenSSH
           Version: 7.2p1
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh-keygen
          Assignee: unassigned-bugs at mindrot.org
          Reporter: krzysztof.cieplucha at intel.com

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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
                 CC|                            |djm at mindrot.org,
                   |                            |dtucker at zip.com.au
   Attachment #2804|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Created attachment 2804
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2804&action=edit
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2543


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2543
[Bug 2543] Tracking bug for OpenSSH 7.3 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2804|ok?(dtucker at zip.com.au)     |ok-
              Flags|                            |

--- Comment #2 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2804
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2804
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2804|0                           |1
        is obsolete|                            |
   Attachment #2805|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Created attachment 2805
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2805&action=edit
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Krzysztof Cieplucha <krzysztof.cieplucha at intel.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |krzysztof.cieplucha at intel.c
                   |                            |om

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next release

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2594

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next release


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next release

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next release

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2543                        |


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2543
[Bug 2543] Tracking bug for OpenSSH 7.3 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2647

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
OpenSSH 7.4 release is closing; punt the bugs to 7.5


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2647
[Bug 2647] Tracking bug for OpenSSH 7.5 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2594                        |


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2698

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
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.


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2698
[Bug 2698] Tracking bug for OpenSSH 7.6 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
remove 7.5 target

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2647                        |


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2647
[Bug 2647] Tracking bug for OpenSSH 7.5 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2805|ok?(dtucker at zip.com.au)     |
              Flags|                            |
   Attachment #2805|0                           |1
        is obsolete|                            |
   Attachment #3003|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #11 from Damien Miller <djm at mindrot.org> ---
Created attachment 3003
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3003&action=edit
update to current

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

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3003|ok?(dtucker at zip.com.au)     |ok-
              Flags|                            |

--- Comment #12 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 3003
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3003
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3003|0                           |1
        is obsolete|                            |
   Attachment #3004|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #13 from Damien Miller <djm at mindrot.org> ---
Created attachment 3004
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3004&action=edit
revised diff

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

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3004|0                           |1
        is obsolete|                            |
   Attachment #3004|ok?(dtucker at zip.com.au)     |
              Flags|                            |
   Attachment #3009|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #14 from Damien Miller <djm at mindrot.org> ---
Created attachment 3009
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3009&action=edit
fix double-fclose

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3009|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #15 from Damien Miller <djm at mindrot.org> ---
Patch applied - this will be in OpenSSH 7.6

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #16 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after release of OpenSSH 7.7.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.