#2156 PR merged: Do not log BACKUP_PROG_CRYPT_KEY value (issue 2155)

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

jsmeix opened issue at 2019-06-06 10:35:

  • Type: Bug Fix

  • Impact: Critical
    This is a critical security issue only if BACKUP_PROG_CRYPT_ENABLED
    with a BACKUP_PROG_CRYPT_KEY is used.

  • Reference to related issue (URL):
    https://github.com/rear/rear/issues/2155
    therein in particular
    https://github.com/rear/rear/issues/2155#issuecomment-499376750

  • How was this pull request tested?
    This is an early submit so that you can have an early look
    that is not yet tested at all by me because I never used
    BACKUP_PROG_CRYPT_ENABLED myself.

  • Brief description of the changes in this pull request:

Separate the special case when BACKUP_PROG_CRYPT_ENABLED
with a BACKUP_PROG_CRYPT_KEY is used
from the usual case when it is not used.

Only when BACKUP_PROG_CRYPT_ENABLED with
a BACKUP_PROG_CRYPT_KEY is used do special stuff
(in particular redirect stderr to /dev/null for certain commands)
to avoid that the BACKUP_PROG_CRYPT_KEY value appears
in a log file in particular when set -x is set.

I think it is more important to not leak out user secrets into a log file
than having stderr error messages when a confidential command fails.

That separation caused "mostly duplicated" code parts, i.e. code parts
that are almost same - except the differences that are related to
the "BACKUP_PROG_CRYPT_ENABLED" case but I have no better idea
how to redirect stderr to /dev/null versus not redirect stderr to /dev/null
for the two cases in a clean way without making the code even more
complicated and oversophisticated than it already is.

By the way I also corrected the spelling typo
from debugscripts mode to debugscript mode
to match the spelling in "man rear" that reads

       -D
           debugscript mode ...

jsmeix commented at 2019-06-06 13:46:

My very first usage of BACKUP_PROG_CRYPT_ENABLED
with a BACKUP_PROG_CRYPT_KEY "just worked" for me.

Because in this case the whole backup command pipe
has stderr redirected to /dev/null the backup.log file
is rather empty in this case.

Here my whole backup.log for "rear -D mkbackup":

++ case "$(basename ${BACKUP_PROG})" in
+++ basename tar
++ set_tar_features
++ TAR_OPTIONS=
++ FEATURE_TAR_WARNINGS=
+++ get_version tar --version
+++ sed -rn 's/^[^0-9\.]*([0-9]+\.[-0-9a-z\.]+).*$/\1/p'
+++ head -1
+++ TERM=dumb
+++ tar --version
++ local tar_version=1.30
++ version_newer 1.30 1.23
++ v1list=(${1//[-.]/ })
++ local v1list
++ v2list=(${2//[-.]/ })
++ local v2list
++ local max=2
++ ((  2 < 2  ))
++ local pos
+++ seq 0 1
++ for pos in $(seq 0 $(( max -1 )))
++ ((  10#01 < 10#01  ))
++ ((  10#01 > 10#01  ))
++ for pos in $(seq 0 $(( max -1 )))
++ ((  10#030 < 10#023  ))
++ ((  10#030 > 10#023  ))
++ return 0
++ FEATURE_TAR_WARNINGS=y
++ TAR_OPTIONS=' --warning=no-xdev'
++ FEATURE_TAR_IS_SET=1
++ is_true yes
++ case "$1" in
++ return 0
+++ cat /tmp/rear.F2EnBRBZNh8dU9N/tmp/backup-include.txt
++ Log tar --warning=no-xdev --sparse --block-number --totals --verbose --no-wildcards-match-slash --one-file-system --ignore-failed-read --anchored --xattrs --xattrs-include=security.capability --xattrs-include=security.selinux --acls --gzip -X /tmp/rear.F2EnBRBZNh8dU9N/tmp/backup-exclude.txt -C / -c -f - / /root/rear.jsmeix/var/log/rear/rear-linux-44ml.log '|' /usr/bin/openssl des3 -salt -k BACKUP_PROG_CRYPT_KEY '|' dd of=/tmp/rear.F2EnBRBZNh8dU9N/outputfs/linux-44ml/backup.tar.gz
++ echo '2019-06-06 12:52:10.168472467 tar --warning=no-xdev --sparse --block-number --totals --verbose --no-wildcards-match-slash --one-file-system --ignore-failed-read --anchored --xattrs --xattrs-include=security.capability --xattrs-include=security.selinux --acls --gzip -X /tmp/rear.F2EnBRBZNh8dU9N/tmp/backup-exclude.txt -C / -c -f - / /root/rear.jsmeix/var/log/rear/rear-linux-44ml.log | /usr/bin/openssl des3 -salt -k BACKUP_PROG_CRYPT_KEY | dd of=/tmp/rear.F2EnBRBZNh8dU9N/outputfs/linux-44ml/backup.tar.gz'
2019-06-06 12:52:10.168472467 tar --warning=no-xdev --sparse --block-number --totals --verbose --no-wildcards-match-slash --one-file-system --ignore-failed-read --anchored --xattrs --xattrs-include=security.capability --xattrs-include=security.selinux --acls --gzip -X /tmp/rear.F2EnBRBZNh8dU9N/tmp/backup-exclude.txt -C / -c -f - / /root/rear.jsmeix/var/log/rear/rear-linux-44ml.log | /usr/bin/openssl des3 -salt -k BACKUP_PROG_CRYPT_KEY | dd of=/tmp/rear.F2EnBRBZNh8dU9N/outputfs/linux-44ml/backup.tar.gz
++ backup_prog_shortnames=("$(basename $(echo "$BACKUP_PROG" | awk '{ print $1 }'))" "$(basename $(echo "$BACKUP_PROG_CRYPT_OPTIONS" | awk '{ print $1 }'))" "$(basename $(echo "$SPLIT_COMMAND" | awk '{ print $1 }'))")
++++ awk '{ print $1 }'
++++ echo tar
+++ basename tar
++++ awk '{ print $1 }'
++++ echo '/usr/bin/openssl des3 -salt -k '
+++ basename /usr/bin/openssl
++++ awk '{ print $1 }'
++++ echo 'dd of=/tmp/rear.F2EnBRBZNh8dU9N/outputfs/linux-44ml/backup.tar.gz'
+++ basename dd
++ for index in ${!backup_prog_shortnames[@]}
++ '[' -n tar ']'
++ for index in ${!backup_prog_shortnames[@]}
++ '[' -n openssl ']'
++ for index in ${!backup_prog_shortnames[@]}
++ '[' -n dd ']'
++ is_true yes
++ case "$1" in
++ return 0
++ pipes_rc=(${PIPESTATUS[@]})
++ let index=3-1
++ '[' 2 -ge 0 ']'
++ rc=0
++ '[' 0 -ne 0 ']'
++ let index--
++ '[' 1 -ge 0 ']'
++ rc=0
++ '[' 0 -ne 0 ']'
++ let index--
++ '[' 0 -ge 0 ']'
++ rc=1
++ '[' 1 -ne 0 ']'
++ echo tar
++ echo 1
++ '[' 1 -eq 1 ']'
++ '[' tar '!=' tar ']'
++ exit 1

The only line that is not caused by set -x is

2019-06-06 12:52:10.168472467 tar --warning=no-xdev --sparse --block-number --totals --verbose --no-wildcards-match-slash --one-file-system --ignore-failed-read --anchored --xattrs --xattrs-include=security.capability --xattrs-include=security.selinux --acls --gzip -X /tmp/rear.F2EnBRBZNh8dU9N/tmp/backup-exclude.txt -C / -c -f - / /root/rear.jsmeix/var/log/rear/rear-linux-44ml.log | /usr/bin/openssl des3 -salt -k BACKUP_PROG_CRYPT_KEY | dd of=/tmp/rear.F2EnBRBZNh8dU9N/outputfs/linux-44ml/backup.tar.gz

i.e. the Log command output in
usr/share/rear/backup/NETFS/default/500_make_backup.sh

The recover.backup.tar.gz.*.restore.log looks a bit better
(with "rear -D recover")

++ case "$BACKUP_PROG" in
++ '[' -s /tmp/rear.jRWmBt9cjV4SAnM/tmp/restore-exclude-list.txt ']'
++ is_true yes
++ case "$1" in
++ return 0
++ Log 'dd if=/tmp/rear.jRWmBt9cjV4SAnM/outputfs/linux-44ml/backup.tar.gz | /usr/bin/openssl des3 -d -k  BACKUP_PROG_CRYPT_KEY | tar --block-number --totals --verbose --anchored' --xattrs --xattrs-include=security.capability --xattrs-include=security.selinux '--acls --gzip -C /mnt/local/ -x -f -'
++ echo '2019-06-06 14:26:20.803458550 dd if=/tmp/rear.jRWmBt9cjV4SAnM/outputfs/linux-44ml/backup.tar.gz | /usr/bin/openssl des3 -d -k  BACKUP_PROG_CRYPT_KEY | tar --block-number --totals --verbose --anchored --xattrs --xattrs-include=security.capability --xattrs-include=security.selinux --acls --gzip -C /mnt/local/ -x -f -'
2019-06-06 14:26:20.803458550 dd if=/tmp/rear.jRWmBt9cjV4SAnM/outputfs/linux-44ml/backup.tar.gz | /usr/bin/openssl des3 -d -k  BACKUP_PROG_CRYPT_KEY | tar --block-number --totals --verbose --anchored --xattrs --xattrs-include=security.capability --xattrs-include=security.selinux --acls --gzip -C /mnt/local/ -x -f -
block 3: ./
block 6: etc/
block 9: etc/fstab
block 13: etc/mtab
...

i.e. tons of tar output lines.

jsmeix commented at 2019-06-06 14:18:

I think I can improve it so that only the actually confidential command
in the pipe has stderr redirected to /dev/null because redirection
in a pipe can be done individually for each command in the pipe.

E.g. on my SLES11 system

# ( set -x ; cat /etc/issue 1>&2 | { cat qqq ; } 2>/dev/null | cat /etc/os-release 1>&2 | { cat QQQ ; } 2>/dev/null )
+ cat /etc/issue

Welcome to SUSE Linux Enterprise Desktop 11 SP4  (i586) - Kernel \r (\l).


+ cat /etc/os-release
NAME="SLED"
VERSION="11.4"
VERSION_ID="11.4"
PRETTY_NAME="SUSE Linux Enterprise Desktop 11 SP4"
ID="sled"
ANSI_COLOR="0;32"
CPE_NAME="cpe:/o:suse:sled:11:4"

and on my openSUSE Leap 15.0 system

# ( set -x ; cat /etc/issue 1>&2 | { cat qqq ; } 2>/dev/null | cat /etc/os-release 1>&2 | { cat QQQ ; } 2>/dev/null )
+ cat /etc/issue
+ cat /etc/os-release
Welcome to \S - Kernel \r (\l).


NAME="openSUSE Leap"
VERSION="15.0"
ID="opensuse-leap"
ID_LIKE="suse opensuse"
VERSION_ID="15.0"
PRETTY_NAME="openSUSE Leap 15.0"
ANSI_COLOR="0;32"
CPE_NAME="cpe:/o:opensuse:leap:15.0"
BUG_REPORT_URL="https://bugs.opensuse.org"
HOME_URL="https://www.opensuse.org/"

I use the subshell here only to encapsulate the set -x effect.

jsmeix commented at 2019-06-07 12:38:

Now all works well for me
so that I like to merge it right now
unless there is an immediate objection.

[Export of Github issue for rear/rear.]