#1530 PR `merged`: Empower the user to specify what ssh files get included in his recovery system…¶

Labels: enhancement, cleanup, fixed / solved / done, critical / security / legal

jsmeix opened issue at 2017-10-10 13:47:¶

This is my very first non-expert attempt to deal with
https://github.com/rear/rear/issues/1512

I tried to implement what @OliverO2 suggested in
https://github.com/rear/rear/issues/1512#issuecomment-331638066

The new config variable SSH_FILES is intentionally
not yet documented in default.conf because currently
anything can change here.

FWIW:
At least for my minimal use case - I only use
SSH_ROOT_PASSWORD="rear"
things still work for me.

Without SSH_FILES set I get now by default in the
recovery system (from a SLES12 original system):

# find /tmp/rear.gCCTXDuhnUTeiFD/rootfs/ | grep 'ssh/'       
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/ssh_host_rsa_key
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/ssh_host_dsa_key.pub
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/moduli
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/ssh_host_ecdsa_key
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/ssh_config
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/ssh_host_dsa_key
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/ssh_host_ecdsa_key.pub
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/sshd_config
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/ssh_host_ed25519_key.pub
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/ssh_host_rsa_key.pub
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/etc/ssh/ssh_host_ed25519_key
/tmp/rear.gCCTXDuhnUTeiFD/rootfs/root/.ssh/known_hosts

and this is what I have on the original SLES12 system:

# find /etc/ssh /root/.ssh
/etc/ssh
/etc/ssh/ssh_host_rsa_key
/etc/ssh/ssh_host_dsa_key.pub
/etc/ssh/moduli
/etc/ssh/ssh_host_ecdsa_key
/etc/ssh/ssh_host_key
/etc/ssh/ldap.conf
/etc/ssh/ssh_config
/etc/ssh/ssh_host_dsa_key
/etc/ssh/ssh_host_key.pub
/etc/ssh/ssh_host_ecdsa_key.pub
/etc/ssh/sshd_config
/etc/ssh/ssh_host_ed25519_key.pub
/etc/ssh/ssh_host_rsa_key.pub
/etc/ssh/ssh_host_ed25519_key
/root/.ssh
/root/.ssh/known_hosts

The crucial point is that with empty SSH_FILES
the SSH key files in the recovery system
were re-generated anew (i.e. they are
not the ones from the original system).
In contrast with SSH_FILES="yes" the SSH key files
in the recovery system are the ones from the original system.

jsmeix commented at 2017-10-11 12:04:¶

@OliverO2
could you have again a look if it is better now.

For me plain
SSH_ROOT_PASSWORD="rear"
without any additional SSH_* config variable settings still works.
Now with the more secure defaults I get those

WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED!
...
Offending ECDSA key in /home/user/.ssh/known_hosts:123

and

Are you sure you want to continue connecting (yes/no)?

as expected when I do 'ssh' to the recovery system.

Using
SSH_ROOT_PASSWORD="rear"
SSH_FILES="yes"
results for me the backward compatible behaviour
where 'ssh' to the recovery system "just works".

jsmeix commented at 2017-10-11 12:15:¶

@gdha
now I made clear in default.conf that with SSH_FILES=no
there is no SSH setup in particular no sshd in the recovery system.

But I don't know in what file I could make clear right now
that the default behavior will change with these new variables.
Of course I will emphasize this clearly in the release notes
of ReaR v2.3 when we make those release notes.

jsmeix commented at 2017-10-11 13:33:¶

@OliverO2
a general security question when generating SSH host keys:
Currently I run as usual commands with $v

ssh-keygen $v ...

which results in the log file things like

Generating public/private rsa key pair.
Your identification has been saved in /tmp/rear.YI2aUkgvEOG9pUj/rootfs/etc/ssh/ssh_host_rsa_key.
Your public key has been saved in /tmp/rear.YI2aUkgvEOG9pUj/rootfs/etc/ssh/ssh_host_rsa_key.pub.
The key fingerprint is:
SHA256:7XXBiLFmOrg/L39G0SvabGA+7xL2t7mEGAPnaLZKqZw root@e205
The key's randomart image is:
+---[RSA 2048]----+
|          .      |
|           + o   |
|        . * ..o  |
|       . X  . .. |
|      . S = .... |
|       = +==oo.  |
|      + .+oO...  |
|   . + oo = B... |
|    E . .=oOo.+o |
+----[SHA256]-----+

Does that somehow contain confidential information
which appears now in a possibly not very securely
stored ReaR log file?

E.g. in build/default/501_check_ssh_keys.sh there is

... ssh-keygen -q ...

but without a comment why that '-q' is there.

jsmeix commented at 2017-10-12 11:01:¶

Now things seem to work reasonably well for me
so that I like to merge it if there are no strict objections.
I.e. I like to merge it when it is at least a reasonable
first step in the right direction so that (if needed) it could be
further adapted and enhanced (without a complete rewrite).

OliverO2 commented at 2017-10-12 11:44:¶

@jsmeix I'll look into it and respond to your questions later today or tomorrow.

jsmeix commented at 2017-10-12 12:29:¶

@OliverO2
many thanks in advance for having a look!

I have another question regarding our current
checking for unprotected SSH key files:

By default /root/.ssh/authorized_key gets included in the
recovery system but build/default/501_check_ssh_keys.sh
does not check if /root/.ssh/authorized_key is protected
nor does it check any keys in /etc/ssh - it only checks
some identity files in /root/.ssh/ - is that o.k.?

OliverO2 commented at 2017-10-13 09:47:¶

@jsmeix

Regarding https://github.com/rear/rear/pull/1530#issuecomment-335810846:

@OliverO2
a general security question when generating SSH host keys:
Currently I run as usual commands with $v

ssh-keygen $v ...
which results in the log file things like
[...]
Does that somehow contain confidential information
which appears now in a possibly not very securely
stored ReaR log file?

No, this is all public information. ssh-keygen generates a private key (which must be kept secret) and a public key (which can be, well, widely published). The information in the log file (fingerprint, ASCII art) is all about the public host key. It could be published on a website, where it would allow ssh users to convince themselves that the host they are connecting to via ssh is actually the right one (and not a fake).

E.g. in build/default/501_check_ssh_keys.sh there is

... ssh-keygen -q ...
but without a comment why that '-q' is there.

The -q is there because this ssh-keygen invocation is actually a no-op, just used to check if the key is unprotected. So it is used like a shell test expression [ -f /path/to/file ]. See the adjacent comment:

# We therefore try to change the passphrase from empty to empty and if that succeeds then it is unprotected

As the output of ssh-keygen -q ... is suppressed anyway the -q could even be removed, I guess (but check first in case it uses /dev/tty directly).

OliverO2 commented at 2017-10-13 09:56:¶

@jsmeix
Regarding https://github.com/rear/rear/pull/1530#issuecomment-336094769:

By default /root/.ssh/authorized_key gets included in the
recovery system but build/default/501_check_ssh_keys.sh
does not check if /root/.ssh/authorized_key is protected
nor does it check any keys in /etc/ssh - it only checks
some identity files in /root/.ssh/ - is that o.k.?

An authorized_keys file contains public keys only. Public keys cannot be protected.

authorized_keys files copied to a rescue system would allow access to the rescue system in the same way that they allow access to the original system. So if the original system trusts certain accounts on certain hosts, the rescue system would do the same. If the original system's trust relationships were safe, so will be the rescue system's. It's all OK.

OliverO2 commented at 2017-10-13 17:17:¶

After performing some tests, I have these observations and suggestions:

The COPY_AS_IS_EXCLUDE example from default.conf seems incorrect (quoted globbing patterns, keys in /root/.ssh/ are named id_*, not *key*)
- Correction: COPY_AS_IS_EXCLUDE=( "${COPY_AS_IS_EXCLUDE[@]}" /etc/ssh/ssh_host_* /root/.ssh/id_* )
COPY_AS_IS_EXCLUDE does not prevent keys from being re-generated. For details, see below.
SSH_UNPROTECTED_PRIVATE_KEYS="no" does not prevent unprotected host keys from being copied. If SSH_FILES=( /etc/ssh /root/.ssh ) is used, build/default/501_check_ssh_keys.sh will still check for unprotected user keys in /root/.ssh, but not the (always unprotected) host keys in /etc/ssh.

Regarding observation number 2:

What doesn't work currently is

having a rescue medium completely free of SSH keys, and
having the RSA host key pair /etc/ssh/ssh_host_rsa_key* generated during rescue system boot.

The reason is that build/default/500_ssh_setup.sh

always re-regenerates host keys existing on the original system (rendering COPY_AS_IS_EXCLUDE ineffective here), and
will generate an RSA host key even if no keys existed on the original system.

To solve this problem, I added a variable to the configuration:

SSH_GENERATE_PRIVATE_KEYS="no"

and this line in build/default/500_ssh_setup.sh:

is_false "$SSH_GENERATE_PRIVATE_KEYS" && return

just before this comment:

# Generate new SSH host key in the recovery system when no SSH host key file
# had been copied into the the recovery system in rescue/default/500_ssh.sh

With this correction, everything worked as expected. The missing RSA key was generated during the rescue system boot and an SSH login was successful.

OliverO2 commented at 2017-10-13 17:35:¶

@jsmeix Last not least: Thank you for all the hard work and effort you are putting into these issues.

jsmeix commented at 2017-10-14 10:14:¶

@OliverO2
many thanks for your careful and helpful review.
I do appreciate that very much.

I already know about issue number 2 in
https://github.com/rear/rear/pull/1530#issuecomment-336513903

My reasoning was that whenever SSH_FILES is not a 'false' value
then sshd is meant to be run and then the needed keys must be generated
because without the needed keys a running sshd does not make sense
and then also SSH_FILES not 'false' would not make sense.

Since
https://github.com/rear/rear/pull/1530#issuecomment-336513903
I know a use-case where SSH_FILES is not 'false' even without
any of the needed sshd keys in the recovery system.

Regarding issue number 3 in
https://github.com/rear/rear/pull/1530#issuecomment-336513903

This is what I had also asked about (a bit hidden) in
https://github.com/rear/rear/pull/1530#issuecomment-336116891
"nor does it check any keys in /etc/ssh"

I am thinking about to enhance the SSH_UNPROTECTED_PRIVATE_KEYS
functionality from a plain boolean setting into a possible array where
the user can specify where ReaR will search for unproteded keys
(similar how SSH_FILES already works).

OliverO2 commented at 2017-10-14 14:06:¶

@jsmeix

I already know about issue number 2 in
#1530 (comment)

My reasoning was that whenever SSH_FILES is not a 'false' value
then sshd is meant to be run and then the needed keys must be generated
because without the needed keys a running sshd does not make sense
and then also SSH_FILES not 'false' would not make sense.

True, but since that time you have added the feature to generate an RSA host key at boot time. Now we don't need any host keys on the recsue medium and sshd still works.

I am thinking about to enhance the SSH_UNPROTECTED_PRIVATE_KEYS
functionality from a plain boolean setting into a possible array where
the user can specify where ReaR will search for unproteded keys
(similar how SSH_FILES already works).

I'd advise against further complicating configuration options:

Since private host keys are never protected and their names are standardized, the best option in my view would be to remove them if SSH_UNPROTECTED_PRIVATE_KEYS is false.
I wouldn't even generate new host keys for the rescue medium as this can be done more securely at boot time.
I'd give the user basically three simple options, which are
1. have an entirely secret-free rescue medium (no user keys, no host keys, not even newly generated keys), or
2. have a rescue medium which contains only protected secrets (so no unprotected user keys, no host keys here), or
3. have a rescue medium which contains secrets.

Having a rescue medium with (some) unprotected keys actually means having an insecure rescue medium. The difference will just be the type of possible attacks. The question is: Can we really expect users to acquire all the knowledge needed to decide whether it is safe to allow certain types of attacks?

Hope this helps to clarify things. Have a great weekend!

jsmeix commented at 2017-10-16 12:58:¶

With
https://github.com/rear/rear/pull/1530/commits/93f17aedb8da8ab82b08c8c0e9f3428dfe8a353b
I tried to implement corrections according to
https://github.com/rear/rear/pull/1530#issuecomment-336636983
but I need to test them a bit more...

jsmeix commented at 2017-10-17 12:32:¶

@OliverO2
regardless that you "advise against further complicating"
I found a use case where I needed to enhance the
SSH_UNPROTECTED_PRIVATE_KEYS
functionality from a plain boolean setting into a string
or an array where the user can specify where ReaR
will search for unprotected keys.

For my testing I had done

# cp -a /etc/ssh /etc/ssh.save

and I got all keys in /etc/ssh.save in the recovery system
regardless of
SSH_UNPROTECTED_PRIVATE_KEYS='no'.

Now with an array like

SSH_UNPROTECTED_PRIVATE_KEYS=( '/etc/ssh*/ssh_host_*' '/root/.ssh/id_*' )

and also simpler with a string

SSH_UNPROTECTED_PRIVATE_KEYS='/etc/ssh*/ssh_host_* /root/.ssh/id_*'

I can get all those unprotected keys removed.

Currently this enhanced SSH_UNPROTECTED_PRIVATE_KEYS
functionality is not officially documented in default.conf.

jsmeix commented at 2017-10-17 13:47:¶

I will merge it so that others can test it in the
ReaR master code.
If something is still missing or
when there are bugs I will fix it.

OliverO2 commented at 2017-10-17 20:31:¶

@jsmeix
Regarding https://github.com/rear/rear/pull/1530#issuecomment-337216386:

For my testing I had done

# cp -a /etc/ssh /etc/ssh.save
and I got all keys in /etc/ssh.save in the recovery system
regardless of

I wonder why ReaR would copy /etc/ssh.save at all? I haven't yet looked into this but I was under the impression that ReaR is normally pretty selective when deciding what to copy onto the rescue medium. Otherwise ReaR could in theory copy lots of secrets from /etc directories belonging to software it doesn't even know about.

Anyway, I'll take a look at your latest commits and I'll test things as soon as I can. Might take until the end of this week, though, but hopefully I'm quicker than that.

Thanks again for tackling the issues!

jsmeix commented at 2017-10-18 08:02:¶

ReaR does tons of inexplicable stuff simply because
it is not explained by meaningful comments in the code.

In such cases all I can do is to keep inexplicable code as is
because I have no chance to understand it with reasonable effort
where "reasonable effort" primarily means "within a reasonable
amount of time" (I will not reverse-engineer inexplicable code).

Often all I can do to move forward is to delete inexplicable code
and replace it with my own code according to how I understand
what is meant that the code should do so that I can maintain it
in the future.
Of course there is the risk that I might destroy whatever
sophisticated support for whatever obscure special cases
but then I could easily re-add such support to code that
I can maintain (if bugs ever appear because of that because
usually such special cases are long obsolete nowadays).

In this particular case run

git log -p --follow usr/share/rear/rescue/default/500_ssh.sh | less

and look for '/etc/ssh*' which is there since ever.
In my current code it is

copy_as_is_ssh_files=( /etc/ssh* ...

which I used as is from its former form

COPY_AS_IS=( "${COPY_AS_IS[@]}" /etc/ssh* ...

that is there since the beginning of the Git history.
The first entry in Git history is

# assume that we have openssh with configs in /etc/ssh
COPY_AS_IS=( "${COPY_AS_IS[@]}" /etc/ssh* ...

where the comment may or may not contradict the implementation.
If the comment means "assume that we have openssh with configs in /etc/ssh/" the implementation contradicts the comment.
If the comment means "assume that we have openssh with configs in
/etc/ssh*" the implementation matches the comment.
Because I cannot prove "with reasonable effort" (cf. above)
that the latter case is false, I must assume there could be systems
where directories like /etc/ssh.conf or /etc/ssh.d are used
so that /etc/ssh* would be the right thing.

schlomo commented at 2017-10-18 08:06:¶

About the /etc/ssh* I can only suspect that back then we had old systems that put all the SSH configs and keys in /etc/ instead of /etc/ssh/ so that this catches all possibilities. However, I don't remember the details of those past days and I can imagine that all systems where ReaR is now supported use a sub directory.

jsmeix commented at 2017-10-18 08:10:¶

FYI
how it looks on my SLES10 SP4 test system where I keep
the SLES10 SP4 default installation unchanged:

# find /etc | grep ssh
/etc/init.d/rc3.d/S11sshd
/etc/init.d/rc3.d/K11sshd
/etc/init.d/rc5.d/S11sshd
/etc/init.d/rc5.d/K11sshd
/etc/init.d/sshd
/etc/profile.d/csh.ssh
/etc/profile.d/sh.ssh
/etc/sysconfig/ssh
/etc/pam.d/sshd
/etc/slp.reg.d/ssh.reg
/etc/ssh
/etc/ssh/ssh_config
/etc/ssh/moduli
/etc/ssh/ssh_host_key
/etc/ssh/sshd_config
/etc/ssh/ssh_host_key.pub
/etc/ssh/ssh_host_dsa_key
/etc/ssh/ssh_host_dsa_key.pub
/etc/ssh/ssh_host_rsa_key
/etc/ssh/ssh_host_rsa_key.pub

# rpm -qa | grep -i ssh
openssh-5.1p1-41.8.20
openssh-askpass-5.1p1-41.8.20

OliverO2 commented at 2017-10-18 09:39:¶

The OpenSSH Release Notes state for version 3.1/3.1p1 (2004-04-09):

/etc/ssh/ now default directory for keys and configuration files

So maybe it's reasonable to drop support for OpenSSH < 3.1 and refer the user to use COPY_AS_IS for files outside of /etc/ssh?

jsmeix commented at 2017-10-18 11:31:¶

The following change seems to fix it for me:

# git diff usr/share/rear/rescue/default/500_ssh.sh | cat 
diff --git a/usr/share/rear/rescue/default/500_ssh.sh b/usr/share/rear/rescue/default/500_ssh.sh
index a7831b6..a0d6332 100644
--- a/usr/share/rear/rescue/default/500_ssh.sh
+++ b/usr/share/rear/rescue/default/500_ssh.sh
@@ -13,14 +13,16 @@ if is_false "$SSH_FILES" ; then
     return
 fi

-# Assume that we have openssh with configs in /etc/ssh
+# Assume that we have OpenSSH >= 3.1 where /etc/ssh/ is the default directory for keys and configuration files
+# according to the OpenSSH release notes for version 3.1/3.1p1 at https://www.openssh.com/releasenotes.html
+# cf. https://github.com/rear/rear/pull/1530#issuecomment-337526810
 local copy_as_is_ssh_files=()
 # The funny [] around a letter makes 'shopt -s nullglob' remove this file from the list if it does not exist.
 if is_true "$SSH_FILES" ; then
     # Copy all the "usual SSH files" (including SSH private host keys) to make things "just work"
     # (provided SSH_UNPROTECTED_PRIVATE_KEYS is not false - otherwise unprotected keys get removed)
     # into the recovery system, cf. https://github.com/rear/rear/issues/1512
-    copy_as_is_ssh_files=( /etc/ssh* /etc/openssh* /etc/centrifydc/ssh* /root/.s[s]h /root/.shos[t]s )
+    copy_as_is_ssh_files=( /etc/ssh /etc/openssh* /etc/centrifydc/ssh* /root/.s[s]h /root/.shos[t]s )
 else
     # Use a reasonably secure fallback if SSH_FILES is not set or empty:
     test "$SSH_FILES" || SSH_FILES="avoid_sensitive_files"

I still wonder about

/etc/openssh* /etc/centrifydc/ssh*

in copy_as_is_ssh_files
On none of my machines I have either of them
so that I cannot say anything about them.

jsmeix commented at 2017-10-18 12:26:¶

/etc/openssh* and /etc/centrifydc/ssh* were added recently via
https://github.com/rear/rear/commit/5b2aca92c69a4f38556d2c6bc51c920e54cbd0ec
but currently I cannot see a reason why.

schlomo commented at 2017-10-18 12:55:¶

The original change for centrify was #836 on request of a user, about the openssh path I also don't remember.

jsmeix commented at 2017-10-18 14:27:¶

@schlomo
thanks for the information about the reason
why /etc/centrifydc/ssh* was added.

I think in our new "sufficiently secure by default" implementation
both /etc/openssh* and /etc/centrifydc/ssh* should be removed
again to ensure we are really "sufficiently secure by default".

If the user needs something in addition to our default
it is described in default.conf at the SSH_* variables
how he can do that i.e. via COPY_AS_IS and if needed
additionally via COPY_AS_IS_EXCLUDE.

(Right now I noticed another inconsistency in that
description in default.conf ...)

jsmeix commented at 2017-10-19 10:56:¶

https://github.com/rear/rear/pull/1538
intends to implement what is described in the above
https://github.com/rear/rear/pull/1530#issuecomment-337526810
and subsequent comments.

jsmeix commented at 2017-10-19 14:31:¶

With https://github.com/rear/rear/pull/1538 merged there is now
only support for OpenSSH 3.1 and later with its default directory
/etc/ssh/ for keys and config files and its default sshd config file
/etc/ssh/sshd_config for the SSH setup of the recovery system.
I removed support for non-standard directories
like /etc/openssh or non-standard sshd config files
like /etc/sshd_config or /etc/openssh/sshd_config.
I documented in the SSH_* section in default.conf
how to manually set up ReaR with a secure shell
software other than OpenSSH >= 3.1.

[Export of Github issue for rear/rear.]

#1530 PR merged: Empower the user to specify what ssh files get included in his recovery system…¶