From 35a0fd3975f4080af6bc4cce525a82ea0f4edcc2 Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Tue, 15 Nov 2022 19:49:30 +0100 Subject: [PATCH 01/12] ssh_filter_btrbk.sh: use single quotes where possible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In strings that don’t contain `'` nor do any expansions, use single quotes to avoid any future unintended expansions or escapes. Signed-off-by: Christoph Anton Mitterer --- ssh_filter_btrbk.sh | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index 5c57409..f552ef1 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -12,7 +12,7 @@ allow_exact_list= allow_rate_limit=1 allow_stream_buffer=1 allow_compress=1 -compress_list="gzip|pigz|bzip2|pbzip2|bzip3|xz|lzop|lz4|zstd" +compress_list='gzip|pigz|bzip2|pbzip2|bzip3|xz|lzop|lz4|zstd' # note that the backslash is NOT a metacharacter in a POSIX bracket expression! option_match='-[a-zA-Z0-9=-]+' # matches short as well as long options @@ -40,14 +40,14 @@ allow_exact_cmd() reject_and_die() { local reason=$1 - log_cmd "auth.err" "btrbk REJECT" "$reason" + log_cmd 'auth.err' 'btrbk REJECT' "$reason" echo "ERROR: ssh_filter_btrbk.sh: ssh command rejected: $reason: $SSH_ORIGINAL_COMMAND" 1>&2 exit 255 } run_cmd() { - log_cmd "auth.info" "btrbk ACCEPT" + log_cmd 'auth.info' 'btrbk ACCEPT' eval " $SSH_ORIGINAL_COMMAND" } @@ -77,7 +77,7 @@ reject_filtered_cmd() # rate_limit_remote and stream_buffer_remote use combined # "mbuffer" as of btrbk-0.29.0 if [[ -n "$allow_stream_buffer" ]] || [[ -n "$allow_rate_limit" ]]; then - mbuffer_match="mbuffer -v 1 -q( -s [0-9]+[kmgKMG]?)?( -m [0-9]+[kmgKMG]?)?( -[rR] [0-9]+[kmgtKMGT]?)?" + mbuffer_match='mbuffer -v 1 -q( -s [0-9]+[kmgKMG]?)?( -m [0-9]+[kmgKMG]?)?( -[rR] [0-9]+[kmgtKMGT]?)?' else mbuffer_match= fi @@ -104,8 +104,8 @@ reject_filtered_cmd() # check for "--sudo" option before processing other options sudo_prefix= for key; do - [[ "$key" == "--sudo" ]] && sudo_prefix="sudo -n " - [[ "$key" == "--doas" ]] && sudo_prefix="doas -n " + [[ "$key" == '--sudo' ]] && sudo_prefix='sudo -n ' + [[ "$key" == '--doas' ]] && sudo_prefix='doas -n ' done while [[ "$#" -ge 1 ]]; do @@ -173,8 +173,8 @@ done allow_exact_cmd "${sudo_prefix}btrfs subvolume (show|list)( ${option_match})* ${file_arg_match}"; allow_cmd "${sudo_prefix}readlink" # resolve symlink allow_exact_cmd "${sudo_prefix}test -d ${file_arg_match}" # check directory (only for compat=busybox) -allow_exact_cmd "cat /proc/self/mountinfo" # resolve mountpoints -allow_exact_cmd "cat /proc/self/mounts" # legacy, for btrbk < 0.27.0 +allow_exact_cmd 'cat /proc/self/mountinfo' # resolve mountpoints +allow_exact_cmd 'cat /proc/self/mounts' # legacy, for btrbk < 0.27.0 # remove leading "|" on alternation lists allow_list=${allow_list#\|} From 5d1abf8d33c84688741fba99691fc39f01f7762c Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Tue, 15 Nov 2022 19:55:23 +0100 Subject: [PATCH 02/12] ssh_filter_btrbk.sh: double quote variables expansions Double quote any variable expansions that might ever contain field separators. Signed-off-by: Christoph Anton Mitterer --- ssh_filter_btrbk.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index f552ef1..9a54671 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -23,7 +23,7 @@ file_arg_match="('${file_match}'|${file_match_sane})" # support btrbk < 0.32.0 log_cmd() { if [[ -n "$enable_log" ]]; then - logger -p $1 -t ssh_filter_btrbk.sh "$2 (Name: ${LOGNAME:-}; Remote: ${SSH_CLIENT:-})${3:+: $3}: $SSH_ORIGINAL_COMMAND" + logger -p "$1" -t ssh_filter_btrbk.sh "$2 (Name: ${LOGNAME:-}; Remote: ${SSH_CLIENT:-})${3:+: $3}: $SSH_ORIGINAL_COMMAND" fi } @@ -39,7 +39,7 @@ allow_exact_cmd() reject_and_die() { - local reason=$1 + local reason="$1" log_cmd 'auth.err' 'btrbk REJECT' "$reason" echo "ERROR: ssh_filter_btrbk.sh: ssh command rejected: $reason: $SSH_ORIGINAL_COMMAND" 1>&2 exit 255 @@ -111,7 +111,7 @@ done while [[ "$#" -ge 1 ]]; do key="$1" - case $key in + case "$key" in -l|--log) enable_log=1 ;; @@ -177,9 +177,9 @@ allow_exact_cmd 'cat /proc/self/mountinfo' # resolve mountpoints allow_exact_cmd 'cat /proc/self/mounts' # legacy, for btrbk < 0.27.0 # remove leading "|" on alternation lists -allow_list=${allow_list#\|} -allow_exact_list=${allow_exact_list#\|} -restrict_path_list=${restrict_path_list#\|} +allow_list="${allow_list#\|}" +allow_exact_list="${allow_exact_list#\|}" +restrict_path_list="${restrict_path_list#\|}" case "$SSH_ORIGINAL_COMMAND" in *\.\./*) reject_and_die 'directory traversal' ;; From 53b3290e14ea9c8df6288bec981d8b28655c8fa7 Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Mon, 21 Nov 2022 04:10:34 +0100 Subject: [PATCH 03/12] ssh_filter_btrbk.sh: remove unnecessary bashishms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ssh_filter_btrbk.sh is mainly intended for security purposes and should therefore itself be written with that in mind. It is written for bash, which greatly extends the POSIX Shell Command Language and is incompatible with it in some niche cases. For several reasons, it seems a good idea to convert the program to (mostly) pure POSIX Shell Command Language: • People may try to use the program with other shells (for example when bash is not available) and make errors. More pure `sh` implementations like dash … • … have far less code and also less dependencies, which possibly also reduces the chance for bugs or exploits, • … are less dynamic in development (and have thus possibly a lower chance of incompatible changes) … • … and may run faster. This commit replaces any unnecessary “bashishms” with purely POSIX compatible code, with the exception of the `local`-built-in, which is however supported by most POSIX compatible shells (including dash, klibc-utils’s `sh` and BusyBox’ `sh`) in some way. Signed-off-by: Christoph Anton Mitterer --- ssh_filter_btrbk.sh | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index 9a54671..c9c0406 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -22,7 +22,7 @@ file_arg_match="('${file_match}'|${file_match_sane})" # support btrbk < 0.32.0 log_cmd() { - if [[ -n "$enable_log" ]]; then + if [ -n "$enable_log" ]; then logger -p "$1" -t ssh_filter_btrbk.sh "$2 (Name: ${LOGNAME:-}; Remote: ${SSH_CLIENT:-})${3:+: $3}: $SSH_ORIGINAL_COMMAND" fi } @@ -53,7 +53,7 @@ run_cmd() reject_filtered_cmd() { - if [[ -n "$restrict_path_list" ]]; then + if [ -n "$restrict_path_list" ]; then # match any of restrict_path_list, # or any file/directory (matching file_match) below restrict_path path_match="'(${restrict_path_list})(${file_match})?'" @@ -66,7 +66,7 @@ reject_filtered_cmd() # btrbk >= 0.32.0 quotes files, allow both (legacy) path_match="(${path_match}|${path_match_legacy})" - if [[ -n "$allow_compress" ]]; then + if [ -n "$allow_compress" ]; then decompress_match="(${compress_list}) -d -c( -[pT][0-9]+)?" compress_match="(${compress_list}) -c( -[0-9])?( -[pT][0-9]+)?" else @@ -76,7 +76,7 @@ reject_filtered_cmd() # rate_limit_remote and stream_buffer_remote use combined # "mbuffer" as of btrbk-0.29.0 - if [[ -n "$allow_stream_buffer" ]] || [[ -n "$allow_rate_limit" ]]; then + if [ -n "$allow_stream_buffer" ] || [ -n "$allow_rate_limit" ]; then mbuffer_match='mbuffer -v 1 -q( -s [0-9]+[kmgKMG]?)?( -m [0-9]+[kmgKMG]?)?( -[rR] [0-9]+[kmgtKMGT]?)?' else mbuffer_match= @@ -103,12 +103,12 @@ reject_filtered_cmd() # check for "--sudo" option before processing other options sudo_prefix= -for key; do - [[ "$key" == '--sudo' ]] && sudo_prefix='sudo -n ' - [[ "$key" == '--doas' ]] && sudo_prefix='doas -n ' +for key in "$@"; do + [ "$key" = '--sudo' ] && sudo_prefix='sudo -n ' + [ "$key" = '--doas' ] && sudo_prefix='doas -n ' done -while [[ "$#" -ge 1 ]]; do +while [ "$#" -ge 1 ]; do key="$1" case "$key" in @@ -191,7 +191,7 @@ case "$SSH_ORIGINAL_COMMAND" in *\<*) reject_and_die 'unsafe character "<"' ;; *\>*) reject_and_die 'unsafe character ">"' ;; *\`*) reject_and_die 'unsafe character "`"' ;; - *\|*) [[ -n "$allow_compress" ]] || [[ -n "$allow_rate_limit" ]] || [[ -n "$allow_stream_buffer" ]] || reject_and_die 'unsafe character "|"' ;; + *\|*) [ -n "$allow_compress" ] || [ -n "$allow_rate_limit" ] || [ -n "$allow_stream_buffer" ] || reject_and_die 'unsafe character "|"' ;; esac reject_filtered_cmd From 2d3e8e26a795cf7d3db4eefaa41e63b7765efc00 Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Mon, 21 Nov 2022 04:33:10 +0100 Subject: [PATCH 04/12] ssh_filter_btrbk.sh: convert to POSIX sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit finishes the work from the previous one and converts ssh_filter_btrbk.sh to (mostly) pure POSIX Shell Command Language. Instead of bash’s `=~`-operator for its `[[ … ]]`-compound-command it uses `grep`. At the time of writing, bash has at least the `nocasematch`-shell-option which would have a negatve security impact for this program. While it’s not enabled per default single users could potentially change that, not realising the consequences. Thus, moving away from this may also provide some hardening. Unlike bash’s `=~`-operator, which matches against the whole string at once, `grep` matches the pattern against each line of input. This would allow for attacks by including a newline in the SSH command like in: SSH_ORIGINAL_COMMAND="readlink /dev/stdout cat /etc/shadow" but is prevented by the general exclusion of newlines in commit TODO. `grep` may return an exit status of `0` when used with its `-q`-option, even when an error occurred. Since this program is intended specifically for security purposes this shall be avoided, even if such case is unlikely, and therefore its standard output and standard error are redirected to `/dev/null` instead. Further, using just: local formatted_restrict_path_list="$(printf '%s' "$restrict_path_list" | sed 's/|/", "/g')" rather than: local formatted_restrict_path_list=""; formatted_restrict_path_list="$(printf '%s' "$restrict_path_list" | sed 's/|/", "/g')" prevent `set -e` to take effect if the pipeline within the command substitution fails, as the returned exit status of the whole command is the result of `local`, not that of the assignment. This is however no security problem here, as `formatted_restrict_path_list` is only used for informative pruposes. Signed-off-by: Christoph Anton Mitterer --- ssh_filter_btrbk.sh | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index c9c0406..3896e30 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -1,4 +1,4 @@ -#!/bin/bash +#!/bin/sh set -e set -u @@ -87,17 +87,21 @@ reject_filtered_cmd() stream_in_match="(${decompress_match} \| )?(${mbuffer_match} \| )?" stream_out_match="( \| ${mbuffer_match})?( \| ${compress_match}$)?" + # `grep`’s `-q`-option is not used as it may cause an exit status of `0` even + # when an error occurred. + allow_stream_match="^${stream_in_match}${allow_cmd_match}${stream_out_match}" - if [[ $SSH_ORIGINAL_COMMAND =~ $allow_stream_match ]] ; then + if printf '%s' "$SSH_ORIGINAL_COMMAND" | grep -E "$allow_stream_match" >/dev/null 2>/dev/null; then return 0 fi exact_cmd_match="^(${allow_exact_list})$"; - if [[ $SSH_ORIGINAL_COMMAND =~ $exact_cmd_match ]] ; then + if printf '%s' "$SSH_ORIGINAL_COMMAND" | grep -E "$exact_cmd_match" >/dev/null 2>/dev/null; then return 0 fi - reject_and_die "disallowed command${restrict_path_list:+ (restrict-path: \"${restrict_path_list//|/\", \"}\")}" + local formatted_restrict_path_list="$(printf '%s' "$restrict_path_list" | sed 's/|/", "/g')" + reject_and_die "disallowed command${restrict_path_list:+ (restrict-path: \"$formatted_restrict_path_list\")}" } From 47fb294717f30a532233b634a8a8ef972a0fd688 Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Mon, 21 Nov 2022 05:10:59 +0100 Subject: [PATCH 05/12] ssh_filter_btrbk.sh: use printf instead of echo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In spirit, POSIX considers `echo` rather obsolete (it was just kept because of its widespread use). It’s also not possible to use `echo` portably unless it’s `-n`-option (as the first argument) and escape sequences are omitted. While neither was the case here, it’s better style to just always use `printf` in order to avoid any future confusion when both are used. Signed-off-by: Christoph Anton Mitterer --- ssh_filter_btrbk.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index 3896e30..7975158 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -41,7 +41,7 @@ reject_and_die() { local reason="$1" log_cmd 'auth.err' 'btrbk REJECT' "$reason" - echo "ERROR: ssh_filter_btrbk.sh: ssh command rejected: $reason: $SSH_ORIGINAL_COMMAND" 1>&2 + printf 'ERROR: ssh_filter_btrbk.sh: ssh command rejected: %s: %s\n' "$reason" "$SSH_ORIGINAL_COMMAND" 1>&2 exit 255 } @@ -165,7 +165,7 @@ while [ "$#" -ge 1 ]; do ;; *) - echo "ERROR: ssh_filter_btrbk.sh: failed to parse command line option: $key" 1>&2 + printf 'ERROR: ssh_filter_btrbk.sh: failed to parse command line option: %s\n' "$key" 1>&2 exit 255 ;; esac From 90503298ffa0e4ed0a5d06e939ca75d6f1b72b48 Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Mon, 21 Nov 2022 17:12:47 +0100 Subject: [PATCH 06/12] ssh_filter_btrbk.sh: set only needed directories in PATH Signed-off-by: Christoph Anton Mitterer --- ssh_filter_btrbk.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index 7975158..20ef87c 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -3,7 +3,7 @@ set -e set -u -export PATH=/sbin:/bin:/usr/sbin:/usr/bin +export PATH='/usr/bin:/bin' enable_log= restrict_path_list= From e5105949d606f6e07035bd5d60f1b1d94d42abbe Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Mon, 21 Nov 2022 17:49:28 +0100 Subject: [PATCH 07/12] ssh_filter_btrbk.sh: use more common exit statuses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POSIX designates a number of exit statuses around `127` for special use and GNU a few further. Further, using values >127 is rather uncommon for normal use-cases. Use `1` when the SSH command was rejected and `2` when the program’s arguments could not be parsed). Since this might at least in principle be used by 3rd-party programs, added a specific note to the changelog. Signed-off-by: Christoph Anton Mitterer --- ChangeLog | 6 ++++++ ssh_filter_btrbk.sh | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 7b348b2..c9373c4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +btrbk-current + + * ssh_filter_btrbk.sh uses new exit statuses on failures (1 when the + SSH command was rejected, 2 when the program’s arguments could not + be parsed). + btrbk-0.32.5 * Correct handling of zero-size raw info file (close #491). diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index 20ef87c..94cb69c 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -42,7 +42,7 @@ reject_and_die() local reason="$1" log_cmd 'auth.err' 'btrbk REJECT' "$reason" printf 'ERROR: ssh_filter_btrbk.sh: ssh command rejected: %s: %s\n' "$reason" "$SSH_ORIGINAL_COMMAND" 1>&2 - exit 255 + exit 1 } run_cmd() @@ -166,7 +166,7 @@ while [ "$#" -ge 1 ]; do *) printf 'ERROR: ssh_filter_btrbk.sh: failed to parse command line option: %s\n' "$key" 1>&2 - exit 255 + exit 2 ;; esac shift From 4888cc51e5034881ec76930dc4305ac4558dfa8b Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Mon, 21 Nov 2022 21:51:04 +0100 Subject: [PATCH 08/12] =?UTF-8?q?ssh=5Ffilter=5Fbtrbk.sh:=20replace=20Open?= =?UTF-8?q?SSH=E2=80=99s=20deprecated=20SSH=5FCLIENT?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenSSH’s environment variable `SSH_CLIENT` has been deprecated in upstream commit f37e246f858cdd79be4f4e158b7b04778d1cb7e9 (2002-09-19) and replaced by `SSH_CONNECTION`. Both contain more than just the remote information, thus adapted the log message to reflect that. Since this might be used by 3rd-party programs (like fail2ban), added a specific note to the changelog. Signed-off-by: Christoph Anton Mitterer --- ChangeLog | 1 + ssh_filter_btrbk.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index c9373c4..9e22a9d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,7 @@ btrbk-current * ssh_filter_btrbk.sh uses new exit statuses on failures (1 when the SSH command was rejected, 2 when the program’s arguments could not be parsed). + * ssh_filter_btrbk.sh’s logging output format has changed slightly. btrbk-0.32.5 diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index 94cb69c..3356559 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -23,7 +23,7 @@ file_arg_match="('${file_match}'|${file_match_sane})" # support btrbk < 0.32.0 log_cmd() { if [ -n "$enable_log" ]; then - logger -p "$1" -t ssh_filter_btrbk.sh "$2 (Name: ${LOGNAME:-}; Remote: ${SSH_CLIENT:-})${3:+: $3}: $SSH_ORIGINAL_COMMAND" + logger -p "$1" -t ssh_filter_btrbk.sh "$2 (Name: ${LOGNAME:-}; Connection: ${SSH_CONNECTION:-})${3:+: $3}: $SSH_ORIGINAL_COMMAND" fi } From 7db20c9d166248e5f14fd724eb9c2cf15506b119 Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Mon, 21 Nov 2022 22:12:13 +0100 Subject: [PATCH 09/12] ssh_filter_btrbk.sh: minor improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Set shell options in one command. • Homogeneously use local variables for function positional parameters in all places. • In redirections, omit `1` for standard output. • Homogeneously use `if`-compount-commands instead of `[ … ] && …` in all places. • Homogeneously use curly brackets with parameter expansion. Signed-off-by: Christoph Anton Mitterer --- ssh_filter_btrbk.sh | 58 +++++++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index 3356559..18c1b3f 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -1,7 +1,6 @@ #!/bin/sh -set -e -set -u +set -e -u export PATH='/usr/bin:/bin' @@ -16,44 +15,53 @@ compress_list='gzip|pigz|bzip2|pbzip2|bzip3|xz|lzop|lz4|zstd' # note that the backslash is NOT a metacharacter in a POSIX bracket expression! option_match='-[a-zA-Z0-9=-]+' # matches short as well as long options -file_match_sane='/[0-9a-zA-Z_@+./-]*' # matches file path (equal to $file_match in btrbk < 0.32.0) +file_match_sane='/[0-9a-zA-Z_@+./-]*' # matches file path (equal to ${file_match} in btrbk < 0.32.0) file_match="/[^']*" # btrbk >= 0.32.0 quotes file arguments: match all but single quote file_arg_match="('${file_match}'|${file_match_sane})" # support btrbk < 0.32.0 log_cmd() { - if [ -n "$enable_log" ]; then - logger -p "$1" -t ssh_filter_btrbk.sh "$2 (Name: ${LOGNAME:-}; Connection: ${SSH_CONNECTION:-})${3:+: $3}: $SSH_ORIGINAL_COMMAND" + local priority="$1" + local authorisation_decision="$2" + local reason="${3-}" + + if [ -n "${enable_log}" ]; then + logger -p "${priority}" -t ssh_filter_btrbk.sh "${authorisation_decision} (Name: ${LOGNAME:-}; Connection: ${SSH_CONNECTION:-})${reason:+: ${reason}}: ${SSH_ORIGINAL_COMMAND}" fi } allow_cmd() { - allow_list="${allow_list}|$1" + local cmd="$1" + + allow_list="${allow_list}|${cmd}" } allow_exact_cmd() { - allow_exact_list="${allow_exact_list}|$1" + local cmd="$1" + + allow_exact_list="${allow_exact_list}|${cmd}" } reject_and_die() { local reason="$1" - log_cmd 'auth.err' 'btrbk REJECT' "$reason" - printf 'ERROR: ssh_filter_btrbk.sh: ssh command rejected: %s: %s\n' "$reason" "$SSH_ORIGINAL_COMMAND" 1>&2 + + log_cmd 'auth.err' 'btrbk REJECT' "${reason}" + printf 'ERROR: ssh_filter_btrbk.sh: ssh command rejected: %s: %s\n' "${reason}" "${SSH_ORIGINAL_COMMAND}" >&2 exit 1 } run_cmd() { log_cmd 'auth.info' 'btrbk ACCEPT' - eval " $SSH_ORIGINAL_COMMAND" + eval " ${SSH_ORIGINAL_COMMAND}" } reject_filtered_cmd() { - if [ -n "$restrict_path_list" ]; then + if [ -n "${restrict_path_list}" ]; then # match any of restrict_path_list, # or any file/directory (matching file_match) below restrict_path path_match="'(${restrict_path_list})(${file_match})?'" @@ -66,7 +74,7 @@ reject_filtered_cmd() # btrbk >= 0.32.0 quotes files, allow both (legacy) path_match="(${path_match}|${path_match_legacy})" - if [ -n "$allow_compress" ]; then + if [ -n "${allow_compress}" ]; then decompress_match="(${compress_list}) -d -c( -[pT][0-9]+)?" compress_match="(${compress_list}) -c( -[0-9])?( -[pT][0-9]+)?" else @@ -76,7 +84,7 @@ reject_filtered_cmd() # rate_limit_remote and stream_buffer_remote use combined # "mbuffer" as of btrbk-0.29.0 - if [ -n "$allow_stream_buffer" ] || [ -n "$allow_rate_limit" ]; then + if [ -n "${allow_stream_buffer}" ] || [ -n "${allow_rate_limit}" ]; then mbuffer_match='mbuffer -v 1 -q( -s [0-9]+[kmgKMG]?)?( -m [0-9]+[kmgKMG]?)?( -[rR] [0-9]+[kmgtKMGT]?)?' else mbuffer_match= @@ -91,31 +99,35 @@ reject_filtered_cmd() # when an error occurred. allow_stream_match="^${stream_in_match}${allow_cmd_match}${stream_out_match}" - if printf '%s' "$SSH_ORIGINAL_COMMAND" | grep -E "$allow_stream_match" >/dev/null 2>/dev/null; then + if printf '%s' "${SSH_ORIGINAL_COMMAND}" | grep -E "${allow_stream_match}" >/dev/null 2>/dev/null; then return 0 fi exact_cmd_match="^(${allow_exact_list})$"; - if printf '%s' "$SSH_ORIGINAL_COMMAND" | grep -E "$exact_cmd_match" >/dev/null 2>/dev/null; then + if printf '%s' "${SSH_ORIGINAL_COMMAND}" | grep -E "${exact_cmd_match}" >/dev/null 2>/dev/null; then return 0 fi - local formatted_restrict_path_list="$(printf '%s' "$restrict_path_list" | sed 's/|/", "/g')" - reject_and_die "disallowed command${restrict_path_list:+ (restrict-path: \"$formatted_restrict_path_list\")}" + local formatted_restrict_path_list="$(printf '%s' "${restrict_path_list}" | sed 's/|/", "/g')" + reject_and_die "disallowed command${restrict_path_list:+ (restrict-path: \"${formatted_restrict_path_list}\")}" } # check for "--sudo" option before processing other options sudo_prefix= for key in "$@"; do - [ "$key" = '--sudo' ] && sudo_prefix='sudo -n ' - [ "$key" = '--doas' ] && sudo_prefix='doas -n ' + if [ "${key}" = '--sudo' ]; then + sudo_prefix='sudo -n ' + fi + if [ "${key}" = '--doas' ]; then + sudo_prefix='doas -n ' + fi done while [ "$#" -ge 1 ]; do key="$1" - case "$key" in + case "${key}" in -l|--log) enable_log=1 ;; @@ -165,7 +177,7 @@ while [ "$#" -ge 1 ]; do ;; *) - printf 'ERROR: ssh_filter_btrbk.sh: failed to parse command line option: %s\n' "$key" 1>&2 + printf 'ERROR: ssh_filter_btrbk.sh: failed to parse command line option: %s\n' "${key}" >&2 exit 2 ;; esac @@ -185,7 +197,7 @@ allow_list="${allow_list#\|}" allow_exact_list="${allow_exact_list#\|}" restrict_path_list="${restrict_path_list#\|}" -case "$SSH_ORIGINAL_COMMAND" in +case "${SSH_ORIGINAL_COMMAND}" in *\.\./*) reject_and_die 'directory traversal' ;; *\$*) reject_and_die 'unsafe character "$"' ;; *\&*) reject_and_die 'unsafe character "&"' ;; @@ -195,7 +207,7 @@ case "$SSH_ORIGINAL_COMMAND" in *\<*) reject_and_die 'unsafe character "<"' ;; *\>*) reject_and_die 'unsafe character ">"' ;; *\`*) reject_and_die 'unsafe character "`"' ;; - *\|*) [ -n "$allow_compress" ] || [ -n "$allow_rate_limit" ] || [ -n "$allow_stream_buffer" ] || reject_and_die 'unsafe character "|"' ;; + *\|*) [ -n "${allow_compress}" ] || [ -n "${allow_rate_limit}" ] || [ -n "${allow_stream_buffer}" ] || reject_and_die 'unsafe character "|"' ;; esac reject_filtered_cmd From 0d34d67385d628c4568dd8e64ea01f43de5115d7 Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Tue, 22 Nov 2022 00:09:31 +0100 Subject: [PATCH 10/12] ssh_filter_btrbk.sh: further harden the shell execution environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • In principle the special `IFS`-variable could be set to some unexpected non- standard value. Unsetting it causes its default to be used. • Locales and in particular their characters sets are quite complex in POSIX and may have many subtle implications. For example, the pattern matching notation (used in `case`-compound-commands or some forms of parameter expansion) are in principle only defined for character strings. While some shells handle it gracefully, the behaviour is undefined if, for example, the character set is UTF-8 and a variable contains bytes that do not form valid caracters in that. Actually, there are quite some more implications. Also, pathnames, in POSIX, are strings of bytes excluding 0x0. For these reasons, the locale is set to the `C`/`POSIX`-locale. Signed-off-by: Christoph Anton Mitterer --- ssh_filter_btrbk.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index 18c1b3f..c1616e5 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -1,9 +1,13 @@ #!/bin/sh -set -e -u +# initialise and sanitise the shell execution environment +unset -v IFS +export LC_ALL=C export PATH='/usr/bin:/bin' +set -e -u + enable_log= restrict_path_list= allow_list= From a0237fe540594ef8426e9d7395fa9b12767901dc Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Tue, 22 Nov 2022 04:11:31 +0100 Subject: [PATCH 11/12] ssh_filter_btrbk.sh: properly normalise pathnames Previously, pathnames specified via the `--restrict-path`-option had only a single trailing `/`, if any, stripped. This commit adds (and utilises) a function which normalises pathnames as described in its comments. Signed-off-by: Christoph Anton Mitterer --- ssh_filter_btrbk.sh | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index c1616e5..b8ac75e 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -23,6 +23,20 @@ file_match_sane='/[0-9a-zA-Z_@+./-]*' # matches file path (equal to ${file_match file_match="/[^']*" # btrbk >= 0.32.0 quotes file arguments: match all but single quote file_arg_match="('${file_match}'|${file_match_sane})" # support btrbk < 0.32.0 +print_normalised_pathname() +{ + # Normalises a pathname given via the positional parameter #1 as follows: + # * Folds any >=3 leading `/` into 1. + # POSIX specifies that implementations may treat exactly 2 leading `//` + # specially and therefore such are not folded here. + # * Folds any >=2 non-leading `/` into 1. + # * Strips any trailing `/`. + + local pathname="$1" + + printf '%s' "${pathname}" | sed -E 's%^///+%/%; s%(.)//+%\1/%g; s%/+$%%' +} + log_cmd() { local priority="$1" @@ -141,7 +155,7 @@ while [ "$#" -ge 1 ]; do ;; -p|--restrict-path) - restrict_path_list="${restrict_path_list}|${2%/}" # add to list while removing trailing slash + restrict_path_list="${restrict_path_list}|$(print_normalised_pathname "$2")" shift # past argument ;; From 57029783f938a06223bae58abfdb5f7ddbc2b70c Mon Sep 17 00:00:00 2001 From: Christoph Anton Mitterer Date: Fri, 25 Nov 2022 02:22:09 +0100 Subject: [PATCH 12/12] ssh_filter_btrbk.sh: forbid non-absolute pathnames to --restrict-path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds a function which checks whether a pathname is absolute and rejects and values to the `--restrict-path`-option which are not. The idea here is mostly a safeguard for users to prevent accidentally specified non-absolute pathnames, which would be taken relative to the executing user’s home-directory. Signed-off-by: Christoph Anton Mitterer --- ssh_filter_btrbk.sh | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ssh_filter_btrbk.sh b/ssh_filter_btrbk.sh index b8ac75e..782f01f 100755 --- a/ssh_filter_btrbk.sh +++ b/ssh_filter_btrbk.sh @@ -23,6 +23,21 @@ file_match_sane='/[0-9a-zA-Z_@+./-]*' # matches file path (equal to ${file_match file_match="/[^']*" # btrbk >= 0.32.0 quotes file arguments: match all but single quote file_arg_match="('${file_match}'|${file_match_sane})" # support btrbk < 0.32.0 +is_pathname_absolute() +{ + # Checks whether a string is an absolute pathname (that is: one that is non- + # empty and starts with either exactly one or more than two `/`). + + local pathname="$1" + + [ "${pathname}" != '//' ] || return 1 + [ -n "${pathname##//[!/]*}" ] || return 1 + [ -z "${pathname##/*}" ] || return 1 + [ -n "${pathname}" ] || return 1 + + return 0 +} + print_normalised_pathname() { # Normalises a pathname given via the positional parameter #1 as follows: @@ -155,6 +170,11 @@ while [ "$#" -ge 1 ]; do ;; -p|--restrict-path) + # check whether the pathname is absolute + if ! is_pathname_absolute "$2"; then + reject_and_die "pathname \"$2\" given to the \"--restrict-path\"-option is not absolute" + fi + restrict_path_list="${restrict_path_list}|$(print_normalised_pathname "$2")" shift # past argument ;;