Do not log BACKUP_PROG_CRYPT_KEY value (issue 2155)

usr/sbin/rear

@@ -28,7 +28,7 @@ function get_bash_flags_and_options_commands () {
     # Output bash commands that set the currently set flags
     # in reverse ordering to have "set ... xtrace" (i.e. "set +/- x") first
     # so that this flag is set first via apply_bash_flags_and_options_commands
-    # which results better matching logging output in debugscripts mode:
+    # which results better matching logging output in debugscript mode:
     set +o | tac | tr '\n' ';'
     # Output bash commands that set the currently set options:
     shopt -p | tr '\n' ';'

usr/share/rear/backup/NETFS/default/500_make_backup.sh

@@ -1,3 +1,4 @@
+#
 # 500_make_backup.sh
 #
 
@@ -37,16 +38,16 @@ if [[ "$opath" ]]; then
     mkdir -p $v "${opath}" >&2
 fi
 
-# Disable BACKUP_PROG_CRYPT_OPTIONS by replacing the default with 'cat' when encryption is disabled
-# (by default encryption is disabled but the default BACKUP_PROG_CRYPT_OPTIONS is not 'cat'):
 if is_true "$BACKUP_PROG_CRYPT_ENABLED" ; then
     # Backup archive encryption is only supported with 'tar':
     test "tar" = "$BACKUP_PROG" || Error "Backup archive encryption is only supported with BACKUP_PROG=tar"
-    LogPrint "Encrypting backup archive with key defined in variable \$BACKUP_PROG_CRYPT_KEY"
-else
-    Log "Encrypting backup archive is disabled"
-    BACKUP_PROG_CRYPT_OPTIONS="cat"
-    BACKUP_PROG_CRYPT_KEY=""
+    # Backup archive encryption is impossible without a BACKUP_PROG_CRYPT_KEY value.
+    # Avoid that the BACKUP_PROG_CRYPT_KEY value is shown in debugscript mode
+    # cf. 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):
+    { test "$BACKUP_PROG_CRYPT_KEY" || Error "BACKUP_PROG_CRYPT_KEY must be set for backup archive encryption" ; } 2>/dev/null
+    LogPrint "Encrypting backup archive with key defined in BACKUP_PROG_CRYPT_KEY"
 fi
 
 # Check if the backup needs to be splitted or not (on multiple ISOs)
@@ -71,20 +72,42 @@ fi
 FAILING_BACKUP_PROG_FILE="$TMP_DIR/failing_backup_prog"
 FAILING_BACKUP_PROG_RC_FILE="$TMP_DIR/failing_backup_prog_rc"
 
+# Do not show the BACKUP_PROG_CRYPT_KEY value in a log file
+# where BACKUP_PROG_CRYPT_KEY is only used if BACKUP_PROG_CRYPT_ENABLED is true
+# therefore 'Log ... BACKUP_PROG_CRYPT_KEY ...' is used (and not '$BACKUP_PROG_CRYPT_KEY')
+# but '$BACKUP_PROG_CRYPT_KEY' must be used in the actual command call which means
+# the BACKUP_PROG_CRYPT_KEY value would appear in the log when rear is run in debugscript mode
+# so that stderr of the whole actual command is redirected to /dev/null
+# cf. the comment of the UserInput function in lib/_input-output-functions.sh
+# how to keep things confidential when rear is run in debugscript mode
+# because it is more important to not leak out user secrets into a log file
+# than having stderr error messages when a confidential command fails
+# cf. https://github.com/rear/rear/issues/2155
 LogPrint "Creating $BACKUP_PROG archive '$backuparchive'"
 ProgressStart "Preparing archive operation"
 (
 case "$(basename ${BACKUP_PROG})" in
     # tar compatible programs here
     (tar)
         set_tar_features
-        Log $BACKUP_PROG $TAR_OPTIONS --sparse --block-number --totals --verbose \
-            --no-wildcards-match-slash --one-file-system \
-            --ignore-failed-read "${BACKUP_PROG_OPTIONS[@]}" \
-            $BACKUP_PROG_CREATE_NEWER_OPTIONS \
-            ${BACKUP_PROG_BLOCKS:+-b $BACKUP_PROG_BLOCKS} "${BACKUP_PROG_COMPRESS_OPTIONS[@]}" \
-            -X $TMP_DIR/backup-exclude.txt -C / -c -f - \
-            $(cat $TMP_DIR/backup-include.txt) $RUNTIME_LOGFILE \| $BACKUP_PROG_CRYPT_OPTIONS BACKUP_PROG_CRYPT_KEY \| $SPLIT_COMMAND
+
+        if is_true "$BACKUP_PROG_CRYPT_ENABLED" ; then
+            Log $BACKUP_PROG $TAR_OPTIONS --sparse --block-number --totals --verbose \
+                --no-wildcards-match-slash --one-file-system \
+                --ignore-failed-read "${BACKUP_PROG_OPTIONS[@]}" \
+                $BACKUP_PROG_CREATE_NEWER_OPTIONS \
+                ${BACKUP_PROG_BLOCKS:+-b $BACKUP_PROG_BLOCKS} "${BACKUP_PROG_COMPRESS_OPTIONS[@]}" \
+                -X $TMP_DIR/backup-exclude.txt -C / -c -f - \
+                $(cat $TMP_DIR/backup-include.txt) $RUNTIME_LOGFILE \| $BACKUP_PROG_CRYPT_OPTIONS BACKUP_PROG_CRYPT_KEY \| $SPLIT_COMMAND
+        else
+            Log $BACKUP_PROG $TAR_OPTIONS --sparse --block-number --totals --verbose \
+                --no-wildcards-match-slash --one-file-system \
+                --ignore-failed-read "${BACKUP_PROG_OPTIONS[@]}" \
+                $BACKUP_PROG_CREATE_NEWER_OPTIONS \
+                ${BACKUP_PROG_BLOCKS:+-b $BACKUP_PROG_BLOCKS} "${BACKUP_PROG_COMPRESS_OPTIONS[@]}" \
+                -X $TMP_DIR/backup-exclude.txt -C / -c -f - \
+                $(cat $TMP_DIR/backup-include.txt) $RUNTIME_LOGFILE \| $SPLIT_COMMAND
+        fi
 
         # Variable used to record the short name of piped commands in case of
         # error, e.g. ( "tar" "cat" "dd" ) in case of unencrypted and unsplit backup.
@@ -97,19 +120,30 @@ case "$(basename ${BACKUP_PROG})" in
             [ -n "${backup_prog_shortnames[$index]}" ] || BugError "No computed shortname for pipe component $index"
         done
 
-        $BACKUP_PROG $TAR_OPTIONS --sparse --block-number --totals --verbose    \
-            --no-wildcards-match-slash --one-file-system                        \
-            --ignore-failed-read "${BACKUP_PROG_OPTIONS[@]}"                    \
-            $BACKUP_PROG_CREATE_NEWER_OPTIONS                                   \
-            ${BACKUP_PROG_BLOCKS:+-b $BACKUP_PROG_BLOCKS}                       \
-            "${BACKUP_PROG_COMPRESS_OPTIONS[@]}"                                \
-            -X $TMP_DIR/backup-exclude.txt -C / -c -f -                         \
-            $(cat $TMP_DIR/backup-include.txt) $RUNTIME_LOGFILE |   \
-                                                                    \
-        $BACKUP_PROG_CRYPT_OPTIONS $BACKUP_PROG_CRYPT_KEY |         \
-                                                                    \
-        $SPLIT_COMMAND
-        pipes_rc=( ${PIPESTATUS[@]} )
+        if is_true "$BACKUP_PROG_CRYPT_ENABLED" ; then
+            { $BACKUP_PROG $TAR_OPTIONS --sparse --block-number --totals --verbose    \
+                  --no-wildcards-match-slash --one-file-system                        \
+                  --ignore-failed-read "${BACKUP_PROG_OPTIONS[@]}"                    \
+                  $BACKUP_PROG_CREATE_NEWER_OPTIONS                                   \
+                  ${BACKUP_PROG_BLOCKS:+-b $BACKUP_PROG_BLOCKS}                       \
+                  "${BACKUP_PROG_COMPRESS_OPTIONS[@]}"                                \
+                  -X $TMP_DIR/backup-exclude.txt -C / -c -f -                         \
+                  $(cat $TMP_DIR/backup-include.txt) $RUNTIME_LOGFILE |   \
+              $BACKUP_PROG_CRYPT_OPTIONS $BACKUP_PROG_CRYPT_KEY |         \
+              $SPLIT_COMMAND ; } 2>/dev/null
+            pipes_rc=( ${PIPESTATUS[@]} )
+        else
+            $BACKUP_PROG $TAR_OPTIONS --sparse --block-number --totals --verbose    \
+                --no-wildcards-match-slash --one-file-system                        \
+                --ignore-failed-read "${BACKUP_PROG_OPTIONS[@]}"                    \
+                $BACKUP_PROG_CREATE_NEWER_OPTIONS                                   \
+                ${BACKUP_PROG_BLOCKS:+-b $BACKUP_PROG_BLOCKS}                       \
+                "${BACKUP_PROG_COMPRESS_OPTIONS[@]}"                                \
+                -X $TMP_DIR/backup-exclude.txt -C / -c -f -                         \
+                $(cat $TMP_DIR/backup-include.txt) $RUNTIME_LOGFILE |   \
+            $SPLIT_COMMAND
+            pipes_rc=( ${PIPESTATUS[@]} )
+        fi
 
         # Exit code logic:
         # - never return rc=1 (this is reserved for "tar" warning about modified files)

usr/share/rear/build/default/960_remove_encryption_keys.sh

@@ -1,12 +1,28 @@
-# Remove all encryption Keys from initrd
+#
+# build/default/960_remove_encryption_keys.sh
+#
+# Remove the BACKUP_PROG_CRYPT_KEY value from ReaR's initrd
+# because the ReaR recovery system must be free of secrets
+# cf. the reasoning about SSH_UNPROTECTED_PRIVATE_KEYS in default.conf
+# and see https://github.com/rear/rear/issues/2155
 
-if is_false "$BACKUP_PROG_CRYPT_ENABLED" ; then
-  return
-fi
+# Nothing to do when there is no BACKUP_PROG_CRYPT_KEY value.
+# Avoid that the BACKUP_PROG_CRYPT_KEY value is shown in debugscript mode
+# cf. 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):
+{ test "$BACKUP_PROG_CRYPT_KEY" || return 0 ; } 2>/dev/null
 
-LogPrint "Removing all encryption Keys from initrd"
-for configfile in $(find ${ROOTFS_DIR}/etc/rear ${ROOTFS_DIR}/usr/share/rear/conf -name "*.conf" -type f -print 2>&1)
+# BACKUP_PROG_CRYPT_KEY must be removed regardless if BACKUP_PROG_CRYPT_ENABLED is true or false
+# because when the user has in his etc/rear/local.conf BACKUP_PROG_CRYPT_KEY=my_secret_key
+# and BACKUP_PROG_CRYPT_ENABLED=false the BACKUP_PROG_CRYPT_KEY value is still there.
+
+LogPrint "Removing BACKUP_PROG_CRYPT_KEY value from config files in the recovery system"
+for configfile in $( find ${ROOTFS_DIR}/etc/rear ${ROOTFS_DIR}/usr/share/rear/conf -name "*.conf" -type f )
 do
-  sed -i -e 's/BACKUP_PROG_CRYPT_KEY=.*/BACKUP_PROG_CRYPT_KEY=""/' $configfile
+    # Without '-q' grep would output the BACKUP_PROG_CRYPT_KEY value on stdout which is redirected to the log:
+    grep -q 'BACKUP_PROG_CRYPT_KEY=' $configfile || continue
+    sed -i -e 's/BACKUP_PROG_CRYPT_KEY=.*/BACKUP_PROG_CRYPT_KEY=""/' $configfile && continue
+    LogPrintError "Failed to remove BACKUP_PROG_CRYPT_KEY value from $configfile"
 done
 

usr/share/rear/conf/default.conf

@@ -918,9 +918,27 @@ BACKUP_PROG_SUFFIX=".tar"
 # http://git.savannah.gnu.org/cgit/tar.git/plain/NEWS?id=release_1_27
 BACKUP_PROG_COMPRESS_OPTIONS=( --gzip )
 BACKUP_PROG_COMPRESS_SUFFIX=".gz"
-# Addons for encryption of the backup (currently only tar is supported)
-BACKUP_PROG_CRYPT_ENABLED=0
-BACKUP_PROG_CRYPT_KEY=""
+# Addons for encryption and decryption of the backup (currently only tar is supported):
+BACKUP_PROG_CRYPT_ENABLED="false"
+# When BACKUP_PROG_CRYPT_ENABLED is set to a true value, BACKUP_PROG_CRYPT_KEY must be also set.
+# There is no BACKUP_PROG_CRYPT_KEY value in etc/rear/local.conf in the ReaR recovery system.
+# It gets removed by build/default/960_remove_encryption_keys.sh
+# because the ReaR recovery system must be free of secrets
+# cf. the reasoning about SSH_UNPROTECTED_PRIVATE_KEYS below
+# and see https://github.com/rear/rear/issues/2155
+# Therefore BACKUP_PROG_CRYPT_KEY must be manually set before running "rear recover".
+# BACKUP_PROG_CRYPT_KEY is set to a default value here only
+# if not already set so that the user can set it also like
+#   export BACKUP_PROG_CRYPT_KEY="my_secret_key"
+# directly before he calls "rear ..." so that there is no need to store it in a config file.
+# Avoid that the BACKUP_PROG_CRYPT_KEY 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):
+{ test "$BACKUP_PROG_CRYPT_KEY" || BACKUP_PROG_CRYPT_KEY="" ; } 2>/dev/null
 BACKUP_PROG_CRYPT_OPTIONS="/usr/bin/openssl des3 -salt -k "
 BACKUP_PROG_DECRYPT_OPTIONS="/usr/bin/openssl des3 -d -k "
 # One might even create a dynamic name like BACKUP_PROG_ARCHIVE="backup_$( date -Iseconds )"

usr/share/rear/lib/_input-output-functions.sh

@@ -365,7 +365,7 @@ function trap () {
 # but only when the user launched 'rear -v' in verbose mode:
 function Print () {
     # It is crucial to append to /dev/$DISPENSABLE_OUTPUT_DEV when $DISPENSABLE_OUTPUT_DEV is not 'null'.
-    # In debugscripts mode $DISPENSABLE_OUTPUT_DEV is 'stderr' (see usr/sbin/rear)
+    # In debugscript mode $DISPENSABLE_OUTPUT_DEV is 'stderr' (see usr/sbin/rear)
     # and /dev/stderr is fd2 which is redirected to append to RUNTIME_LOGFILE (see usr/sbin/rear)
     # so that 2>/dev/stderr would truncate RUNTIME_LOGFILE to zero size (see 'REDIRECTION' in "man bash")
     # but 2>>/dev/stderr does not change things so that fd2 output is still appended to RUNTIME_LOGFILE:
@@ -545,7 +545,7 @@ function Error () {
     # (but do not stop at lines that are logged like '++ StopIfError ...' or '++ PrintError ...')
     # if such a '+ Error' or '+ BugError' line exists, otherwise sed proceeds to the end
     # (the sed pattern '[Bug]*Error' is fuzzy because it would also match things like 'uuggError').
-    # The reason to stop at a line that contains '+ [Bug]*Error ' is that in debugscripts mode '-D'
+    # The reason to stop at a line that contains '+ [Bug]*Error ' is that in debugscript mode '-D'
     # a BugError or Error function call with a multi line error message (e.g. BugError does that)
     # results 'set -x' debug output of that function call in the log file that looks like:
     #   ++ [Bug]Error 'first error message line
@@ -756,9 +756,9 @@ function LogPrintIfError () {
 #       the prompt, the actual input, the default value, and the choices are still shown on the user's terminal.
 #       In confidential user input mode the actual input coming from the user's terminal is still echoed
 #       on the user's terminal unless also the -s option is specified.
-#       When usr/sbin/rear is run in debugscripts mode (which runs the scripts with 'set -x') arbitrary values
-#       appear in the log file so that the confidential user input mode does not help in debugscripts mode.
-#       If confidential user input is needed also in debugscripts mode the caller of the UserInput function
+#       When usr/sbin/rear is run in debugscript mode (which runs the scripts with 'set -x') arbitrary values
+#       appear in the log file so that the confidential user input mode does not help in debugscript mode.
+#       If confidential user input is needed also in debugscript mode the caller of the UserInput function
 #       must call it in an appropriate (temporary) environment e.g. with STDERR redirected to /dev/null like:
 #           { password="$( UserInput -I PASSWORD -C -r -s -p 'Enter the pasword' )" ; } 2>/dev/null
 # Result:

usr/share/rear/lib/framework-functions.sh

@@ -42,7 +42,7 @@ function Source () {
     Log "Including $relname"
     # DEBUGSCRIPTS mode settings:
     if test "$DEBUGSCRIPTS" ; then
-        Debug "Entering debugscripts mode via 'set -$DEBUGSCRIPTS_ARGUMENT'."
+        Debug "Entering debugscript mode via 'set -$DEBUGSCRIPTS_ARGUMENT'."
         local saved_bash_flags_and_options_commands="$( get_bash_flags_and_options_commands )"
         set -$DEBUGSCRIPTS_ARGUMENT
     fi
@@ -58,7 +58,7 @@ function Source () {
     test "0" -eq "$source_return_code" || Debug "Source function: 'source $source_file' returns $source_return_code"
     # Undo DEBUGSCRIPTS mode settings:
     if test "$DEBUGSCRIPTS" ; then
-        Debug "Leaving debugscripts mode (back to previous bash flags and options settings)."
+        Debug "Leaving debugscript mode (back to previous bash flags and options settings)."
         # The only known way how to do 'set +x' after 'set -x' without 'set -x' output for the 'set +x' call
         # is a current shell environment where stderr is redirected to /dev/null before 'set +x' is run via
         #   { set +x ; } 2>/dev/null

usr/share/rear/restore/NETFS/default/400_restore_backup.sh

@@ -1,3 +1,4 @@
+#
 # 400_restore_backup.sh
 #
 
@@ -22,18 +23,6 @@ local restore_input_basename=""
 
 mkdir -p $BUILD_DIR/outputfs/$NETFS_PREFIX
 
-# Disable BACKUP_PROG_DECRYPT_OPTIONS by replacing the default with 'cat' when encryption is disabled
-# (by default encryption is disabled but the default BACKUP_PROG_DECRYPT_OPTIONS is not 'cat'):
-if is_true "$BACKUP_PROG_CRYPT_ENABLED" ; then
-    # Backup archive decryption is only supported with 'tar':
-    test "tar" = "$BACKUP_PROG" || Error "Backup archive decryption is only supported with BACKUP_PROG=tar"
-    LogPrint "Decrypting backup archive with key defined in variable \$BACKUP_PROG_CRYPT_KEY"
-else
-    Log "Decrypting backup archive is disabled"
-    BACKUP_PROG_DECRYPT_OPTIONS="cat"
-    BACKUP_PROG_CRYPT_KEY=""
-fi
-
 # The RESTORE_ARCHIVES array contains the restore input files.
 # If it is not set, RESTORE_ARCHIVES is only one element which is the backup archive:
 test "$RESTORE_ARCHIVES" || RESTORE_ARCHIVES=( "$backuparchive" )
@@ -137,13 +126,31 @@ for restore_input in "${RESTORE_ARCHIVES[@]}" ; do
     # output of a possibly simultaneously running process that likes to append to the log file
     # (e.g. when background processes run that also uses the log file for logging)
     # cf. https://github.com/rear/rear/issues/885#issuecomment-310308763
+    # Do not show the BACKUP_PROG_CRYPT_KEY value in a log file
+    # where BACKUP_PROG_CRYPT_KEY is only used if BACKUP_PROG_CRYPT_ENABLED is true
+    # therefore 'Log ... BACKUP_PROG_CRYPT_KEY ...' is used (and not '$BACKUP_PROG_CRYPT_KEY')
+    # but '$BACKUP_PROG_CRYPT_KEY' must be used in the actual command call which means
+    # the BACKUP_PROG_CRYPT_KEY value would appear in the log when rear is run in debugscript mode
+    # so that stderr of the whole actual command is redirected to /dev/null
+    # cf. the comment of the UserInput function in lib/_input-output-functions.sh
+    # how to keep things confidential when rear is run in debugscript mode
+    # because it is more important to not leak out user secrets into a log file
+    # than having stderr error messages when a confidential command fails
+    # cf. https://github.com/rear/rear/issues/2155
     (   case "$BACKUP_PROG" in
             (tar)
                 if [ -s $TMP_DIR/restore-exclude-list.txt ] ; then
                     BACKUP_PROG_OPTIONS=( "${BACKUP_PROG_OPTIONS[@]}" "--exclude-from=$TMP_DIR/restore-exclude-list.txt" )
                 fi
-                Log dd if=$restore_input \| $BACKUP_PROG_DECRYPT_OPTIONS $BACKUP_PROG_CRYPT_KEY \| $BACKUP_PROG --block-number --totals --verbose "${BACKUP_PROG_OPTIONS[@]}" "${BACKUP_PROG_COMPRESS_OPTIONS[@]}" -C $TARGET_FS_ROOT/ -x -f -
-                dd if=$restore_input | $BACKUP_PROG_DECRYPT_OPTIONS $BACKUP_PROG_CRYPT_KEY | $BACKUP_PROG --block-number --totals --verbose "${BACKUP_PROG_OPTIONS[@]}" "${BACKUP_PROG_COMPRESS_OPTIONS[@]}" -C $TARGET_FS_ROOT/ -x -f -
+                if is_true "$BACKUP_PROG_CRYPT_ENABLED" ; then 
+                    Log "dd if=$restore_input | $BACKUP_PROG_DECRYPT_OPTIONS BACKUP_PROG_CRYPT_KEY | $BACKUP_PROG --block-number --totals --verbose ${BACKUP_PROG_OPTIONS[@]} ${BACKUP_PROG_COMPRESS_OPTIONS[@]} -C $TARGET_FS_ROOT/ -x -f -"
+                    { dd if=$restore_input | $BACKUP_PROG_DECRYPT_OPTIONS $BACKUP_PROG_CRYPT_KEY | $BACKUP_PROG --block-number --totals --verbose "${BACKUP_PROG_OPTIONS[@]}" "${BACKUP_PROG_COMPRESS_OPTIONS[@]}" -C $TARGET_FS_ROOT/ -x -f - ; } 2>/dev/null
+
+                else
+                    Log "dd if=$restore_input | $BACKUP_PROG --block-number --totals --verbose ${BACKUP_PROG_OPTIONS[@]} ${BACKUP_PROG_COMPRESS_OPTIONS[@]} -C $TARGET_FS_ROOT/ -x -f -"
+
+                    dd if=$restore_input | $BACKUP_PROG --block-number --totals --verbose "${BACKUP_PROG_OPTIONS[@]}" "${BACKUP_PROG_COMPRESS_OPTIONS[@]}" -C $TARGET_FS_ROOT/ -x -f -
+                fi
                 ;;
             (rsync)
                 if [ -s $TMP_DIR/restore-exclude-list.txt ] ; then