#1513 PR `merged`: Avoid leaking unprotected SSH keys for root user into rescue medium and introduce ssh-agent¶

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

schlomo opened issue at 2017-09-24 16:19:¶

Fixes #1511 and continues the work from #1500

Thanks a lot to @OliverO2 for the initial work. I removed the code that encrypts SSH keys so that we now simply remove unprotected SSH keys from the rescue media (unless explicit configuration tells us to keep unprotected SSH keys).

This change improves the security of ReaR but constitutes a breaking in change. I find improving the security a good reason for breaking changes, we just have to explain it in the release notes.

jsmeix commented at 2017-09-26 08:21:¶

I fully agree with all you - which means from my point of view
there is actually no conflict - everybody is right - only the
current implementation is insufficient - according to how
I understand the above comments.

The current implementation has the same shortcoming
as much (probaby even most) other code in ReaR also has:
The general shortcoming in ReaR is that it too often
tries to do "things right only fully automated".
The "only" is the actual problem.

Nothing is wrong with "try to do things right fully automated".
This should be the usual default behaviour in ReaR.

But things fail when "fully automated" is the only way.
It is against the idea that the user must have final power.
To provide ready-made final power to our ReaR users
there must be powerful config variable settings.

Currently the boolean SSH_UNPROTECTED_PRIVATE_KEYS
cannot provide much power to the user because all what the
user can do is to set this to a 'true' value and then all power
is left to ReaR to do "things right fully automated".

But any automatism will fail in this or that cases.
In this case the automated unprotected SSH keys detection
fails in this or that cases.

Therefore the user must get the final power to specify
what exactly he wants and ReaR must just obey.

Note the crucial difference:
Ideally the user should not need to specify something.
Ideally ReaR does "things right fully automated"
when the user has not specified what to do.
But when needed the user must be able to explicitly
specify anything exactly as he he wants it to be done.

The solution is to implement a config variable that specifies
what SSH keys get included in the recovery system
as a string of words (i.e. each value is a word)
or preferably as an array (to be future-proof so that
a single value can be also a strings of several words).

A setting like
SSH_KEYS=( 'no_autodetected_unprotected_keys' )
has the same meaning as now, i.e. a best effort attempt
to autodetect unprotected SSH keys and exclude them.

A setting like
SSH_KEYS=( 'no' )
will not copy any SSH keys (i.e. all scripts and other
code pieces that copy SSH keys are completely skipped).
Nevertheless the user can still use COPY_AS_IS
to copy exactly only those SSH keys that he wants.

A setting like
SSH_KEYS=( 'yes' )
will copy all SSH keys in a predefined (i.e. hardcoded) list
of directories (those directories need to be documented).
I think this is the current behaviour (without this pull request)
so that it provides the backward compatible behaviour
for those users who need that.
It needs to be documented that this behaviour is insecure.
This behaviour should no longer be the default.

A setting like
SSH_KEYS=( ' ' )
will do whatever we think the best default is.

A setting like
SSH_KEYS=( '/etc/ssh*' '/etc/openssh*' '/etc/centrifydc/ssh*' '/root/.ssh' '/root/.shosts' )
could be a list of bash globbing patterns or probably even
better a list of filename globbing patterns that are used
as '-iname' arguments for 'find' calls to find SSH keys.

Cf. how I implemented FIRMWARE_FILES
UEFI_BOOTLOADER and MODULES.

jsmeix commented at 2017-09-26 08:40:¶

@schlomo
only a side note regarding
"How should users restore their backup if the keys are required for that?"
see RECOVERY_UPDATE_URL my grand big hammer
for anything an unlucky admin might need when it is already
too late to recreate his recovery system :-)

jsmeix commented at 2017-09-26 08:46:¶

@OliverO2
only a side note regarding
"inconvenience of entering a password once or twice during restore":
When the new UserInput function is used to get the password
from the user one can even predefine the input in a (hopefully)
sufficiently secure way in the running recovery system before
running "rear recover" via

# export USER_INPUT_matching_ID="mypassword"

OliverO2 commented at 2017-09-26 20:11:¶

@jsmeix Understood. For me it would be sufficient to ask for the password once in the restore script. This is where all SSH access takes place.

The difficulty lies in transmitting the password to ssh. As it wants to gather the password via direct tty access, transferring it via stdin is not an option. You'd have to resort to using sshpass for that. The sshpass manual page explains:

SECURITY CONSIDERATIONS
       First  and  foremost,  users  of  sshpass  should  realize  that  ssh's
       insistance on only getting the password interactively  is  not  without
       reason.  It  is close to impossible to securely store the password, [...]

Also note that sshpass might not be installed by default. On Ubuntu 16.04 LTS, it is not.

So yes, it can be done to have the user enter an SSH password once and use it multiple times afterwards. It's not as risky on a (single-user, run-once) rescue system as it would be on a multi-user production system. It's also not risk free (e.g., future modifications to general-purpose ReaR code might introduce accidental logging/storing).

schlomo commented at 2017-09-27 05:32:¶

About the general direction: I think that we should keep in mind that ReaR is just a tool and not a magic solution. For me that means that it should strive to be as secure by default as we can make it without completely loosing our main functionality. It also means that we should let the admin decide everything else. And we should try to automate the common cases while enabling custom cases through configuration.

I'd also like to keep it simple. As the ssh handling only uses the COPY_AS_IS array the admin can easily use COPY_AS_IS_EXCLUDE to exclude further files if he wants to keep them off the rescue media. This is actually a real advantage of using existing ReaR infrastructure for the copy instead of coding something special. I there at this moment would prefer not to introduce a complex SSH_KEYS variable as described by @jsmeix above (sorry and thanks for the illustration) in an attempt to keep the complexity of this topic low.

With regard to passwords I also think we should keep things sufficiently simple. If we want users to be able to enter SSH passwords during recovery then we should let the ssh tool prompt for them. Why? Because ReaR is meant to be a wrapper that automates the existing standard tools.

I actually don't remember anybody who explicitly wanted to use SSH passwords before, to the best of my knowledge everybody seems to use SSH keys. I would therefore consider SSH password a custom use case that we should make efforts not to obstruct but I wouldn't optimize for it.

jsmeix commented at 2017-09-27 11:52:¶

Only FYI regarding "let the ssh tool prompt for [passwords]", cf.
https://github.com/rear/rear/pull/1493#issuecomment-332141858

OliverO2 commented at 2017-09-27 15:01:¶

To be sure, I've just checked the ssh client (OpenSSH 7.2): It does not use file descriptors 0, 1, 2. It opens /dev/tty in read/write mode and uses it to issue the password prompt and to read the password. So it just ignores I/O redirections. AFAIK it's been this way almost from the beginning of time ;-).

jsmeix commented at 2017-09-27 16:09:¶

@schlomo
ultimately it this pull request is your code
so that the final decision is of course yours.

Nevertheless an addedum regarding my proposal in
https://github.com/rear/rear/pull/1513#issuecomment-332123766

I did not mean that all those fancy SSH_KEYS functionality
is needed right now - perhaps none of the fancy functionality is
ever needed.

But I meant that you may not introduce now a rather limited
new boolean SSH_UNPROTECTED_PRIVATE_KEYS
but use a more versatile usable variable from the beginning
so that if needed we could more easily enhance its functionality.

For now I think only support for something like
SSH_KEYS=( 'no_autodetected_unprotected_keys' )
plus
SSH_KEYS=( 'no' )
to provide also a secure setting is probably sufficient.

For that a plain string variable is also sufficient like
SSH_KEYS='no_autodetected_unprotected_keys'
SSH_KEYS='no'
because one can easily enhance string variables into arrays.

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

@schlomo
I would like to suggest that you "just merge" whatever
seems to be "the right thing from your current point of view"
at least as a first step to get the whole issue moved forward.

There are still three weeks left until ReaR 2.3 is planned
to be released (currently "Due by October 31, 2017")
where others could implement further adaptions and
enhancements if needed.

jsmeix commented at 2017-10-09 10:31:¶

@gdha @gozora do you have an opinion
how we could proceed here?

I would just merge it as is
so that others (if needed) may further adapt and enhance it
so that in the end we could release something that is
considered to be " reasonably o.k." in ReaR 2.3.

Simply put:
Should I just merge it as is soon?