Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add terminal password check via 'TTY_ROOT_PASSWORD' #2539

Merged
merged 2 commits into from Jan 27, 2021

Conversation

OliverO2
Copy link
Contributor

Pull Request Details:
  • Type: New Feature

  • Impact: Normal

  • Reference to related issue (URL):

  • How was this pull request tested? On Ubuntu 20.04 LTS Server

  • Brief description of the changes in this pull request:

Currently, ReaR allows a password check on SSH sessions only (via SSH_ROOT_PASSWORD).

This PR enables the rescue/recovery system to enforce a password check also on terminal connections (the system console and/or serial terminals).

This prevents unauthenticated local root access

  • while the rescue/recovery system is being run via a remote SSH session, or
  • when a system reboot has inadvertently started the recovery system installed on a local disk partition (e.g. where a buggy firmware has inadvertently reshuffled the boot order).
Usage
  1. Configure TTY_ROOT_PASSWORD in the same way as SSH_ROOT_PASSWORD.
  2. Run rear mkrescue.
  3. Start the rescue/recovery system.
  4. Log in locally, providing the correct password.
Example Configuration

Configure the password test:

TTY_ROOT_PASSWORD='$6$SixRXJl0b37m4rl3$alfGpFd3dp1tBk5/EDquyxqiviB2oQQq3z7VULx9qiBHweFlBquivq28.I4YNLknhKjDax1uM/4c2xuVzdNcy1'
SSH_ROOT_PASSWORD="$TTY_ROOT_PASSWORD"

@jsmeix jsmeix self-requested a review December 11, 2020 08:34
@jsmeix jsmeix self-assigned this Dec 11, 2020
@jsmeix jsmeix added the enhancement Adaptions and new features label Dec 11, 2020
@jsmeix jsmeix added this to the ReaR v2.7 milestone Dec 11, 2020
@@ -0,0 +1,3 @@
# Store TTY_ROOT_PASSWORD for terminal logins

[[ -n "$TTY_ROOT_PASSWORD" ]] && echo "$TTY_ROOT_PASSWORD" > "$ROOTFS_DIR/.TTY_ROOT_PASSWORD"
Copy link
Member

@jsmeix jsmeix Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh!
I see now that also the SSH_ROOT_PASSWORD value
would appear in the log file when the code is run with set -x.
So we need to do for SSH_ROOT_PASSWORD and TTY_ROOT_PASSWORD
the same as I did for BACKUP_PROG_CRYPT_KEY by running commands
that would expose those *_ROOT_PASSWORD values
with stderr redirected to /dev/null like

{ COMMAND ; } 2>/dev/null

Redirecting 2>/dev/null outside of a group command { ... ; } is mandatory
also for a single command inside the group command, cf. the comment
for the UserInput function in lib/_input-output-functions.sh that reads (excerpt):

The redirection must be done via a compound group command
{ confidential_command ; } 2>/dev/null
even for a single confidential command to ensure
STDERR is redirected to /dev/null also for 'set -x'
otherwise the confidential command and its arguments
would be shown in the log file, for example
{ openssl des3 -salt -k secret_passphrase ; } 2>/dev/null
where the secret passphrase must not appear in the log,
cf. https://github.com/rear/rear/issues/2155

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted. Addressed via 8bc2b1c.

# { TTY_ROOT_PASSWORD=''; } 2>/dev/null
# NOTE: stderr is redirected in the above line to avoid exposing the password hash
# in the log file when ReaR runs in debugscript mode.
TTY_ROOT_PASSWORD=''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to not expose *_ROOT_PASSWORD values
see what I did in #2541 via
5292851

Set SSH_ROOT_PASSWORD to a default value
only if not already set so that the user can set it e.g. with
export SSH_ROOT_PASSWORD=my_recovery_system_root_password
before he calls "rear ..." so there is no need
to have SSH_ROOT_PASSWORD in local.conf.

for usr/share/rear/conf/default.conf

# Avoid that the SSH_ROOT_PASSWORD value is shown when usr/sbin/rear was called with 'set -x'
# for debugging usr/sbin/rear cf. https://github.com/rear/rear/issues/2144#issuecomment-493908133
# In debugscript mode only scripts sourced by the Source function in lib/framework-functions.sh
# are run with 'set -x' but default.conf is sourced by usr/sbin/rear directly.
# See the comment of the UserInput function in lib/_input-output-functions.sh
# how to keep things confidential when usr/sbin/rear is run in debugscript mode
# ('2>/dev/null' should be sufficient here because 'test' does not output on stdout).
# SSH_ROOT_PASSWORD is set to a default value here only
# if not already set so that the user can set it also like
#   export SSH_ROOT_PASSWORD='my_recovery_system_root_password'
# directly before he calls "rear ...":
{ test "$SSH_ROOT_PASSWORD" ; } 2>/dev/null || SSH_ROOT_PASSWORD=''

@jsmeix
Copy link
Member

jsmeix commented Dec 11, 2020

@OliverO2
again many thanks for your continuous enhancements!

Only an offhanded idea:

I didn't had sufficient free mind-space to have a closer look
so I could misunderstand things but I think what this is about is
to also have a password dialog for the "normal" login on the
ReaR recovery system console (where currently no password is needed).
If I am right I appreciate this option to make things more secure very much!

Do you think an automatism to have TTY_ROOT_PASSWORD and
SSH_ROOT_PASSWORD somehow synchronized would be good?

Offhandedly I think about something like:
If one is set and the other one is empty
automatically set the empty one to the one that is set, e.g.

SSH_ROOT_PASSWORD='mypw'
TTY_ROOT_PASSWORD=''

results that TTY_ROOT_PASSWORD is also set to be 'mypw'
and vice versa.
The current default that both are empty would not change the behaviour.
Only with SSH_ROOT_PASSWORD='mypw' one would get automatically
also that same password to login at the recovery system console.
Probably that could get in the way for automated recovery.
So it must be possible for the user to have SSH_ROOT_PASSWORD set
but tell ReaR to not set TTY_ROOT_PASSWORD.
This could be done with TTY_ROOT_PASSWORD='no' and
is_false $TTY_ROOT_PASSWORD ... to enforce no TTY_ROOT_PASSWORD
and vice versa for SSH_ROOT_PASSWORD.
This would make the password 'no' impossible but such a password is nonsense anyway.

It seems my automated sync idea gets too complicated.

@OliverO2
Copy link
Contributor Author

@jsmeix
I had a similar idea: Having separate passwords depending on the terminal connection used does not make much sense. However, that's the way ReaR grew up and some people might currently depend on it. I did not want to change too much in one step, so basically the next logical thing for me would be:

  • Add a variable LOGIN_ROOT_PASSWORD (initially empty).
  • Use the value of LOGIN_ROOT_PASSWORD as a fallback for TTY_ROOT_PASSWORD and SSH_ROOT_PASSWORD if these are unset/empty.

This way one can have all combinations with LOGIN_ROOT_PASSWORD being the preferred one.

@jsmeix jsmeix requested a review from schlomo December 11, 2020 14:39
@jsmeix jsmeix assigned schlomo and unassigned jsmeix Dec 11, 2020
@gdha
Copy link
Member

gdha commented Dec 14, 2020

@OliverO2 @jsmeix IMHO I would go for 1 variable REAR_ROOT_PASSWORD to distinguish between the real root password and the one we define for the ReaR image. For backwards compatibility I would keep SSH_ROOT_PASSWORD for the time being, but in de default.conf file we should mention it will become deprecated in favour of REAR_ROOT_PASSWORD which should be used in the same way foor SSH connections or TTY login sessions. Why do you think about thei proposal?

@OliverO2
Copy link
Contributor Author

@gdha The motivation for having a name REAR_ROOT_PASSWORD sounds reasonable. There is one caveat, though: As @schlomo described in #2541 (comment), a prefix of REAR_ might suggest that this is an environment variable. If ReaR would allow that, user's might be tempted to use something like REAR_ROOT_PASSWORD='...' sudo --preserve-env rear mkrescue, which would make the password end up in their shell command history and in ps output for others to see.

So maybe go for RECOVERY_ROOT_PASSWORD instead? Additionally, we should probably follow best practices and stop supporting plain text passwords (those are currently allowed for SSH_ROOT_PASSWORD).

@schlomo
Copy link
Member

schlomo commented Dec 14, 2020

About the env names in my opinion 2 things are important:

  1. all officially recognized env variables start from REAR_ to make it visible for which program they are used, similar to ls expecting LS_COLOR or ssh expecting SSH_* variables
  2. env variables that should be read by default (if they exist) should have the same name (plus the REAR_ prefix) as used internally

Why? To make it simple for end-users to understand what is going on.

Furthermore, if we ever want to have a generic env variable based configuration mechanism then we can just iterate over all REAR_* variables and import them (or do we already have this feature?)

What do you think?

@OliverO2
Copy link
Contributor Author

So if I understand correctly, there would be

  • SOME_FUNNY_OPTION used throughout ReaR with a standard initialization in default.conf, and
  • REAR_SOME_FUNNY_OPTION as an optional environment variable, which would initialize SOME_FUNNY_OPTION and have priority over possible settings of SOME_FUNNY_OPTION in *.conf files.

If so, that sounds good to me.

Any other opinions on environment variables containing secrets? And the final naming of the password variable in this case?

@schlomo
Copy link
Member

schlomo commented Dec 14, 2020

Yes, like that.

However I am not sure about the priority question: Should env variables always override configuration settings? Or should it only override the built-in defaults?

Probably good to look at other software and follow that.

@OliverO2
Copy link
Contributor Author

I have no good precedent at hand, but if I did a test invocation like this

REAR_OUTPUT_URL=file:///my/test/destination rear mkrescue

and found out that this would not override local.conf or site.conf, I'd be pretty surprised.

So I'd say an environment variable is like a command line parameter: An explicit setting for a single invocation that must always be honored. Configuration files must not overturn such explicit user decisions.

@gdha
Copy link
Member

gdha commented Jan 26, 2021

@OliverO2 Are we good to merge this PR?
@jsmeix Any objections against it?

@OliverO2
Copy link
Contributor Author

@gdha Yes, this is ready to merge as is. Once we agree on some common name unifying TTY_ROOT_PASSWORD and SSH_ROOT_PASSWORD and on whether to stop using plaintext passwords (#2539 (comment)), we could do that in a subsequent PR.

@jsmeix
Copy link
Member

jsmeix commented Jan 27, 2021

I do fully agree with
#2539 (comment)
I prefer small steps that are doable and get them done.

@gdha gdha self-requested a review January 27, 2021 09:06
@gdha gdha assigned gdha and unassigned schlomo Jan 27, 2021
@gdha gdha merged commit 4cf3f19 into rear:master Jan 27, 2021
@OliverO2 OliverO2 deleted the feature_terminal_password branch June 3, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants