Confidential values leaked into log file in debug mode

[jsmeix]

• ReaR version ("/usr/sbin/rear -V"):

current GitHub master code

• ReaR configuration files ("cat /etc/rear/site.conf" and/or "cat /etc/rear/local.conf"):

excerpt:

SSH_ROOT_PASSWORD="rear"

BACKUP_PROG_CRYPT_ENABLED="yes"
{ BACKUP_PROG_CRYPT_KEY='my_secret_passphrase' ; } 2>/dev/null

• Description of the issue (ideally so that others can reproduce it):

Since
b83059a
there is - by the way - a new
init/default/998_dump_variables.sh
which is unrelated to what the commit message describes.

The new script does currently only

Debug "Runtime Configuration:$LF$(declare -p)"

which outputs in debug modes also confidential
variable values into the ReaR log file like

# usr/sbin/rear -d help
Running 'init' stage ======================
Running workflow help on the normal/original system
...

# egrep -i 'password|key' var/log/rear/rear-linux-h9wr.log.lockless
...
declare -- BACKUP_PROG_CRYPT_KEY="my_secret_passphrase"
declare -- GALAXY11_PASSWORD=""
declare -- OPAL_PBA_DEBUG_PASSWORD=""
declare -- SSH_ROOT_PASSWORD="rear"
declare -- TTY_ROOT_PASSWORD=""
declare -- YUM_ROOT_PASSWORD="root"
declare -- ZYPPER_ROOT_PASSWORD="root"

In particular BACKUP_PROG_CRYPT_KEY is critical
because that new script foils my security/privacy
code enhancements that I did for BACKUP_PROG_CRYPT_KEY
see #2155
and #2156

[jsmeix]

95f3082
is a quick initial attempt to mitigate this issue.

[schlomo]

I have two conflicting thoughts about this:

1. in debug mode we should dump all variables and also give a hint that the dump can contain secrets - this helps with debugging
2. in debug mode we should mask/hide secrets (as your suggestion does) and give a hint that secrets are skipped - and debug mode won't help with secrets related problems

Personally, I'd prefer option 1) (full dump) and expect users to check the output they send us. We could also add another warning to our GitHub issue template to remind users to scrape secrets from the debug log, if they set any.

For reference, other tools like for example the aws CLI also dump all secrets in debug mode and expect the user to be circumspect.

@rear/contributors other opinions?

[jsmeix]

maintain a list of variables that contain confidential values
(e.g. a new array in default.conf that contains such variable names)
and hide/skip/suppress the values of those variables in the output
(we at ReaR usptream should never need secret values from a user
to debug an issue).

[jsmeix]

Sleeping on it made it even more critical at least for me:

Now I have legal liability worries about it, at least
as far as I understand general principles of German law:

I think in general when someone does something
one has generally some reasonable amount of
legal liability for what one does.

I think in particular when making software this means
that one's program must not knowingly do something bad
except the user had explicity requested to do it.

In this case init/default/998_dump_variables.sh
leaks user's confidential values into the log file
without the user's explicit request to do exactly this.
Running a program in debug mode is no explicit request
to get (arbitrary) confidential values shown.

Furthermore - and this is the crucial point for me
why I have now legal liability worries:
It is now know to all of us ReaR upstream developers
that init/default/998_dump_variables.sh
does bad things for the user without explicit request.

I think - at least as far as I understand general principles
of German law - when one knows about something bad,
then one becomes to some reasonable extent responsible
to (try to) do something against that bad
as far as possible within reason.

[jsmeix]

With
65f0ad5
I feel better now (so I can have a relaxed Easter public holiday)
because now I did something against that (possibly) bad
as far as possible for me right now within reason.

[jsmeix]

@rear/contributors
I think there is no longer urgency / time pressure here
so we can take our time to develop step by step
how to solve the generic problem properly.

The generic problem is that debug information may in general
reveal data that is confidential for the user, not only
obviously confidential data like secrets (e.g. passwords or keys)
but also non-obvious confidential data that could be misused
by others e.g. for spear phishing attacks or things like that.

[schlomo]

@jsmeix I strongly disagree with this approach.

At some level ReaR debugging must be "complete", that is normal for every software. DEBUGSCRIPTS is actually our "strongest" debug mode so for me this is where full variable disclosure belongs. I'm happy to add extra warnings to it and also to the help message or manpage, but in the end full debug-level variable disclosure must be possible.

In any case, we won't be able to find a 100% "safe" solution here, as users can add additional variables with potential secrets.

About your worries under German law: That is why ReaR is shipped under the GPL where we explicitly shed responsiilities for what ReaR does after a user installs and uses it, like any other Open Source software. As long as there is no obvious malicious intent we are safe. Malicious intent could be a piece of code that uploads all users' secrets to a website while obfuscating what it does.

If it helps I'm happy to expand the variable dump feature to mask out all KEY|SECRET|PASSWORD type variables unless the user explicitly disables this behaviour.

BTW, your check for variable naming should probably be expanded to only match on the variable names and not content.

[pcahyna]

For inspiration, I checked what Ansible does - there are similar concerns, because it is easy to show all Ansible module call parameters (including values) during playbook execution (merely by requesting verbose output), and secrets are passed to modules using parameters, so they would leak quite often. Ansible allows marking some module parameters as no_log and they are then omitted from the log output. If a parameter name seems to indicate that it is a secret and is not marked as no_log, Ansible shows the value, but emits a warning. This allows the module developer to catch the problem before the module gets to production.
I think we could reuse the heuristic that determines whether the parameter is a secret (apply it to shell variables). The regular expression used in Ansible is

^(.+[-_\s])?pass([-_\s]?(word|phrase|wrd|wd)?)([-_\s].+)?

We would use upper case of course, and we should add KEY to the possibilities (not sure about SECRET). I already see one false positive : BACKUP_DUPLICITY_ASK_PASSPHRASE.
The regex would be used to mask variables in variable dump (not just for warning, unlike in Ansible).

The question about debugscript/-D/set -x is a more difficult one: I see @jsmeix changed code to protect against that in #2156, but doing it for all such variables will be difficult.
By the way, is assigning such variables in local.conf dangerous ? I see the comment in default.conf:

# ... insert it between the single quotes of the following line:
#        { 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.

but there is also this comment

# 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

which seem contradictory.

Regarding

At some level ReaR debugging must be "complete", that is normal for every software. DEBUGSCRIPTS is actually our "strongest" debug mode so for me this is where full variable disclosure belongs.

this makes some sense, as an example Ansible also shows all values when you debug it itself by setting ANSIBLE_DEBUG=1 (even the no_log variables). But note that our issue template asks the user to submit logs generated using -D. So it would actually encourage users who report issues to leak confidential data, if this logging is "complete" in this sense, and I see why this is not desirable. Not sure what to do here - perhaps another debug level that would show really everything, but we would normally not request its output?

[schlomo]

OK, how about we add a new option --debug-complete or --expose-secrets that enables the full variable dump?

[jsmeix]

In general individual agreements (like GPL)
do not take precedence over general law.

In a specific case one needs to get a legal judgement
to find out how general law and individual agreements
apply (i.e. one has to actually take a court action).

Without a court decision about this particular case
there is the risk of legal issues.

[jsmeix]

@schlomo
I strongly disagree with your approach.

In the past I never needed all variables values
to find the root cause of an issue in ReaR.

In the past the 'set -x' debugscript mode output
was sufficient for me to find the root cause.

If I needed even more details I could easily add
a script like yours or add commands for debugging
to existing scripts as needed in the specific case.
I vaguely remember there was one single case where
the latter was needed (i.e. I told the user to add some
specific commands for debugging into an existing script).

[schlomo]

If I'm the only one who would like to see all variables in debug logs then I'm fine with removing that script altogether and relying on on-demand hacks if needed.

[pcahyna]

I have felt a need for such a script myself some time ago (and not for debugging, so I would even introduce a separate workflow), so I would keep it, if the secrets issue can be addressed sufficiently.

[schlomo]

@jsmeix @pcahyna how about we extract the script from the current place into a dedicated show-all-variables workflow that will then, as advertised, dump all variables, including secrets - of course with a suitable warning?

[jsmeix]

I asked colleagues about their general thoughts regarding
"show also secrets in debug mode":

A colleague told me about this

# wpa_supplicant -h
...
 -K = include keys (passwords, etc.) in debug output

which matches what @schlomo suggests in
#2967 (comment)

Currently I think a command line option is the best way
to solve the conflict that @schlomo described in
#2967 (comment)

My reasoning:

My reasoning is primarily about legal things and usability:

A command line option must be explicitly typed in by the user.
When the command line option name tells what it means for the user
typing in that command line option name is an explicit request
by the user to let ReaR do exactly this.
Because running a program in whatever debug mode
is no explicit request to (also) get secret values shown
(if at all debug mode might be an implicit request),
the command line option name '--debug-complete'
is no explicit request to get secret values shown.
But the command line option name '--expose-secrets'
is an explicit request to get secret values shown.

A command line option provides better usability
(compared to a separated new workflow)
because it works for any workflow so that the user
can easily increase debug info step by step
for the workflow that fails for him like

# rear WORKFLOW
[fails]

# rear -d WORKFLOW
[insufficient info why it fails]

# rear -d --expose-secrets WORKFLOW
[indicates a possible reason of the failure]

# rear -D WORKFLOW
[not clear how exactly it fails]

# rear -D --expose-secrets WORKFLOW
[now it is clear how exactly it fails]

Finally with a generic command line option
we can implement what is right in each specific code place.
For example there might be very specific use cases where

# rear --expose-secrets WORKFLOW

could make sense even in normal (non-debug) node
e.g. to show some specific secrets on the user's terminal.

[jsmeix]

@schlomo @pcahyna @gdha
if you agree with my above reasoning in my
#2967 (comment)
I would try to implement what @schlomo suggests in his
#2967 (comment)
i.e. implement a new command line option '--expose-secrets'.

[codefritzel]

Only an offhanded idea:

Alternatively we may introduce in usr/sbin/rear by default

SECRET_OUTPUT_DEV="null"

and set it to

SECRET_OUTPUT_DEV="stderr"

only if '--expose-secrets' is set in a similar way as we currently use DEBUG_OUTPUT_DEV and DISPENSABLE_OUTPUT_DEV

For example like

if { COMMAND $SECRET_ARGUMENT ; } 2>>/dev/$SECRET_OUTPUT_DEV ; then
    Log "COMMAND succeeded"
else
    { Log "'COMMAND $SECRET_ARGUMENT' failed with exit code $?" ; } 2>>/dev/$SECRET_OUTPUT_DEV
    Error "COMMAND failed"
fi

No special LogSecret function is needed and now also the command call itself can appear in the log file in debuscript mode via 'set -x' when additionally --expose-secrets is set.

Additionally the

{ ... ; } 2>>/dev/$SECRET_OUTPUT_DEV

syntax makes it clear which redirections are explicitly meant to hide secrets to distinguish them from usual unwanted output discard via '2>/dev/null'

By the way: The

Log "COMMAND succeeded"

is needed to have something in the log because the command call itself is normally invisible.

I think the idea is very good. For debugging purposes, the user or developer can see the executed command, if activated.

@jsmeix
Do you want to implement this and create a WIP RP?

[jsmeix]

@codefritzel
I will try it out - likely next week (as time permits)
and if it works OK for me I will make a pull request.

[jsmeix]

Ouch!
One must be super careful to not leak secrets by accident:

if { COMMAND $SECRET_ARGUMENT ; } 2>>/dev/$SECRET_OUTPUT_DEV ; then
    Log "COMMAND succeeded"
else
    { Log "'COMMAND $SECRET_ARGUMENT' failed with exit code $?" ; } 2>>/dev/$SECRET_OUTPUT_DEV
    Error "COMMAND failed"
fi

leaks $SCRET_ARGUMENT when COMMAND failed because

{ Log "'COMMAND $SECRET_ARGUMENT' ... " ; } 2>>/dev/null

outputs into the log file in any case because
in lib/_input-output-functions.sh there is

function Log () {
    ...
    echo "$log_message" >>"$RUNTIME_LOGFILE" || true
}

So we also need the 'LogSecret' function.

[jsmeix]

In #3006
I implemented the basics of the new

• -e / --expose-secrets option for sbin/rear
• SECRET_OUTPUT_DEV
• LogSecret function

Nothing is documented yet in "rear help" or "man rear"
until I did some more tests and until I got some feedback
what others think about it and how it behaves for them.

[jsmeix]

With #3006 merged
I will now (as time permits) replace our current

{ SECRET STUFF ; } 2>/dev/null

with

{ SECRET STUFF ; } 2>>/dev/$SECRET_OUTPUT_DEV

[jsmeix]

With #3034 merged also
#2967 (comment)
is done.

[jsmeix]

Now I will think about how to properly re-enable
the intended functionality of
usr/share/rear/init/default/998_dump_variables.sh
that I had crippled and finally disabled (see above) via
95f3082
0093828
65f0ad5

Perhaps the new LogSecret function could be useful.

[jsmeix]

As far as I see
#3036
should be the last step to get this issue solved.

[jsmeix]

#3036
is not the last step because documentation is still missing
(in particular about the new 'expose-secrets' option), see
#3039

[jsmeix]

With #3036
and #3039 merged
this issue should (hopefully) finally be done.

[schlomo]

@jsmeix I just noticed that rear help doesn't mention the -e option, is that on purpose?

The manpage actually does mention it, so I was wondering.

[jsmeix]

The help workflow was simply forgotten. I fixed it via
93a17c1