openssh bugs - Apr 2023 - [Bug 3558] New: Spelling "yes" as "Yes" in sshd_config has a fatal result

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

            Bug ID: 3558
           Summary: Spelling "yes" as "Yes" in sshd_config
has a fatal
                    result
           Product: Portable OpenSSH
           Version: 7.2p2
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: Ulrich.Windl at rz.uni-regensburg.de

Being logged in via SSH I changed an option in sshd_config from "no"
to
"Yes". Then I used "sytemctl reload sshd.service", and it ws
the last
command I could execute; specifically I was no longer able to connect
via SSH.
As it turned out, sshd died on reload because of the misspelled option
like this:
/etc/ssh/sshd_config line 77: Bad yes/no argument: Yes

And as a consequence sshd died. Any attempt to restart it failed again.

I'm not discussing whether "yes" should be allowed for
"yes", but
whether it's OK for sshd to terminate on reload if one single option
was found invalid.
Wouldn't it make much more sense to ignore that option (i.e. ignore the
change) while continuing to run?  I think bind did that for ages.

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

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at dtucker.net

--- Comment #1 from Darren Tucker <dtucker at dtucker.net> ---
Erroring out on an invalid config is pretty standard behaviour for most
if not all Unix daemons.  I've never seen Bind start with an invalid
config (I have heard of it ignoring zone files it didn't have
permission to read, but that's not quite the same thing).

You can check the config file with sshd test mode (sshd -t) and testing
a new connection before logging out of the one you restated from is a
worthwhile precaution.

-- 
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=3558

Dmitry Belyavskiy <dbelyavs at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dbelyavs at redhat.com

--- Comment #2 from Dmitry Belyavskiy <dbelyavs at redhat.com> ---
I agree that the behavior not to start on erroneous config is standard
but having some more robustness with yes/no parsing may be worth doing.

-- 
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=3558

--- Comment #3 from Darren Tucker <dtucker at dtucker.net> ---
The config is either valid or isn't.  Even if we did make it accept
increasingly vague directives there will still be invalid configs.

Many SysV-style init scripts checked the config before committing to a
restart (the ones in our contrib/ directory do) but it seems like
systemd doesn't have this capability.

-- 
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=3558

--- Comment #4 from Ulrich Windl <u20230201 at gmail.com> ---
May the issue is a systemd one: From the past I know you can restart
sshd without loosing your connection. Maybe (I didn't verify) systemd
kills all sshd processes if the main process died.
If so, the solution would be to check the configuration in the unit
file before attempting a restart.
On "terminate on config error": I can agree when starting the daemon,
but when reloading it, it's rather unexpected that the result of
reloading it is not having a daemon at all.

-- 
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=3558

--- Comment #5 from Darren Tucker <dtucker at dtucker.net> ---
Left to its own devices, sshd will leave existing connections up when
sshd is restarted, including in the case where the restart fails.  If
your active connection was terminated on a failed restart that's
probably something systemd did.

-- 
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=3558

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
note that sshd doesn't do graceful configuration reload - on SIGHUP it
re-executes itself to pick up the new configuration, so "reload" is
almost exactly equivalent to restarting from scratch, maybe with a
little less latency.

We do provide a number of hooks to validate a config. IMO the systemd
unit should use those _and_ not kill existing sessions on
reload/restart. I can't imagine why anyone would want that latter
behaviour...

-- 
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=3558

Jim Knoble <jmknoble at pobox.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmknoble at pobox.com

--- Comment #7 from Jim Knoble <jmknoble at pobox.com> ---
All the prior comments notwithstanding, when modifying sshd's
configuration, it is best practice for the person making the
modification to test the configuration *before* restarting. Not sure
that this practice is documented anywhere, however. Perhaps it should
be (in the manual page, for example)?

-- 
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=3558

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3689|                            |ok?(djm at mindrot.org)
              Flags|                            |

--- Comment #8 from Darren Tucker <dtucker at dtucker.net> ---
Created attachment 3689
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3689&action=edit
Validate config on SIGHUP.

(In reply to Damien Miller from comment #6)
> note that sshd doesn't do graceful configuration reload - on SIGHUP
> it re-executes itself to pick up the new configuration
Given we have sshd -t and the subprocess interface, it would not be
hard to validate the config before committing to the re-exec, like so.

-- 
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=3558

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
I don't like this because the message doesn't (can't) go to the user
sending the SIGHUP, only the system log. So if the intended config
change was really important (e.g. one with security consequences) but
contained a typo, then sshd could silently fail to reload on SIGHUP and
leave the user in a bad state.

-- 
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=3558

--- Comment #10 from Darren Tucker <dtucker at dtucker.net> ---
The message doesn't go to the user sending the SIGHUP now (unless
you're arguing "when it stops working they'll eventually get the
message").

-- 
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=3558

--- Comment #11 from Damien Miller <djm at mindrot.org> ---
Sure, but the current behaviour has the benefit of being obvious and we
do provide hooks to verify the config before reloading. 

Maybe we could provide a "visudo" like tool to wrap config editing in
sshd -T?

-- 
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=3558

--- Comment #12 from Darren Tucker <dtucker at dtucker.net> ---
(In reply to Damien Miller from comment #11)
> Maybe we could provide a "visudo" like tool to wrap config
editing
> in sshd -T?
I like that even less than leaving it as is.

-- 
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.