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.

Sign up for GitHub

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

improve ssh filter btrbk.sh #511

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

improve ssh filter btrbk.sh #511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
35a0fd3
ssh_filter_btrbk.sh: use single quotes where possible
calestyo Nov 15, 2022
5d1abf8
ssh_filter_btrbk.sh: double quote variables expansions
calestyo Nov 15, 2022
53b3290
ssh_filter_btrbk.sh: remove unnecessary bashishms
calestyo Nov 21, 2022
2d3e8e2
ssh_filter_btrbk.sh: convert to POSIX sh
calestyo Nov 21, 2022
47fb294
ssh_filter_btrbk.sh: use printf instead of echo
calestyo Nov 21, 2022
9050329
ssh_filter_btrbk.sh: set only needed directories in PATH
calestyo Nov 21, 2022
e510594
ssh_filter_btrbk.sh: use more common exit statuses
calestyo Nov 21, 2022
4888cc5
ssh_filter_btrbk.sh: replace OpenSSH’s deprecated SSH_CLIENT
calestyo Nov 21, 2022
7db20c9
ssh_filter_btrbk.sh: minor improvements
calestyo Nov 21, 2022
0d34d67
ssh_filter_btrbk.sh: further harden the shell execution environment
calestyo Nov 21, 2022
a0237fe
ssh_filter_btrbk.sh: properly normalise pathnames
calestyo Nov 22, 2022
5702978
ssh_filter_btrbk.sh: forbid non-absolute pathnames to --restrict-path
calestyo Nov 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
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

* Correct handling of zero-size raw info file (close #491).
Expand Down
130 changes: 92 additions & 38 deletions ssh_filter_btrbk.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
#!/bin/bash
#!/bin/sh

set -e
set -u

export PATH=/sbin:/bin:/usr/sbin:/usr/bin
# initialise and sanitise the shell execution environment
unset -v IFS
export LC_ALL=C
export PATH='/usr/bin:/bin'
Copy link
Owner

Choose a reason for hiding this comment

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

Nope, some distros still install btrfs-progs to /{usr,}/sbin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm that leads me to some more general question:

Right now ssh_filter_btrbk.sh tries to be compatible to all kinds of versions.

I think this makes things unnecessarily complex and lax (since we need to allow more patters/styles) that are actually used.

Would I get your blessing, if we changed the script, so that it effectively only accepts exactly those commands, from the current version (respectively git commit)?

In many scenarios, people will anyway use the same version of btrbk all over their systems.

And even if not, they could simply copy the specific one per case and use that.

Since that one can be selected on a per-SSH-key basis, full control would be possible, whilst greatly simplifying the script.

If you'd agree with that, we could make the PATH configurable via Makefile, or simply leave it the duty of the distribution maintainer, to override if still using old sbin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh and also:

I guess most distros will switch to /usr/bin, simply because btrfs can be run as a user, too.

And we don't really need to care too much on ancient versions of some distros:

Those would anyway still ship the old ssh_filter_btrbk.sh, so people could simply use that.

Copy link
Owner

Choose a reason for hiding this comment

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

I guess most distros will switch to /usr/bin, simply because btrfs can be run as a user, too.

I hope so too, but these things usually take quite a while to propagate through all distros.

Probably the "correct" way would be not to restrict PATH at all, as this should be done in a layer below (i.e. sshd_config). Note that as some exotic distros like NixOS install executables outside /usr/bin.

We need to find a good balance between security and convenience here. As long as there are distros out there installing btrfs to /sbin/ (notably Gentoo), we need to keep both.

If you'd agree with that, we could make the PATH configurable via Makefile, or simply leave it the duty of the distribution maintainer, to override if still using old sbin.

This is asking for trouble. Are you proposing something like "try to find out where third-party software (btrfs, ...) was installed, and patch a variable according to it" ?

Copy link

Choose a reason for hiding this comment

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

@calestyo Fedora 41, which is one of the biggest distros out there and defaults to btrfs as its FS of choice (thus a high percent of users use btrfs) uses /usr/sbin for btrfs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm seems a pretty strange choice (I'd call it a bug, TBH), as quite some parts of btrfs are usable as normal user.


set -e -u

enable_log=
restrict_path_list=
Expand All @@ -12,48 +15,86 @@ 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
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

is_pathname_absolute()
Copy link
Owner

Choose a reason for hiding this comment

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

See previous comment, I don't think we should contstrain too much here. This just makes the code more complex, and implies that // has some special meaning for btrbk, which it does not.
A simple check for leading slash might make some sense for crazy users trying to specify relative paths for whatever reason. Even then, the error is just shifted to here, without that check it will fail saying that the path does not match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may not have in btrbk, but it may in principle have in the OS... so a OS could choose to handle // not like / but e.g. like and object identifier.

Then all the constraints we set up to make things secure, wouldn't work anymore. Thus I think it's better to simply forbid // at all. In practise no one should anyway ever set that.

Copy link
Owner

Choose a reason for hiding this comment

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

Sadly btrbk does exactly this in some cases (which are not uncommon), see fix: 142fa6c

This means we should keep supporting leading //, at least before next major release.

Unless it really imposes a security risk, but: I don't know of any linux (!) OS where // is treated in a special way, but maybe you know better. OS other that linux will not install ssh_filter_btrbk.sh, as btrfs is only available on linux as far as I know.

{
# 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:
# * 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%/+$%%'
Copy link
Owner

Choose a reason for hiding this comment

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

This might break things. This function should normalize the paths exactly the same way as btrbk does: I believe in some versions btrbk printed // in some weird situations, this might match your implementation but also might not.
I don't think we should sanitize the paths too much, after all it's the user specifying it, I find it convenient to let the user specify whatever he wants here (especially regex).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it be better to fix those places in btrbk instead? btrbk should never issue leading //, cause that's dangerous depending on the OS.

Especially the regexp thingy I find dangerous... it's not documented as that, and easily breaks things. The tool should rather not support all possible weird scenarios, but therefore restrict for 100% sure in all cases for any normal scenario.

}

log_cmd()
{
if [[ -n "$enable_log" ]]; then
logger -p $1 -t ssh_filter_btrbk.sh "$2 (Name: ${LOGNAME:-<unknown>}; Remote: ${SSH_CLIENT:-<unknown>})${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:-<unknown>}; Connection: ${SSH_CONNECTION:-<unknown>})${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"
echo "ERROR: ssh_filter_btrbk.sh: ssh command rejected: $reason: $SSH_ORIGINAL_COMMAND" 1>&2
exit 255
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}" >&2
exit 1
Copy link
Owner

Choose a reason for hiding this comment

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

This is intentionally set to 255, so that btrbk can distinguish between ssh errors and exit codes from the command called by ssh:
https://github.com/digint/btrbk/blob/v0.32.5/btrbk#L994

see ssh(1):

EXIT STATUS
     ssh exits with the exit status of the remote command or with 255 if an
     error occurred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm? I don't quite understand that. You say you want it to differ between ssh and ssh_filter_btrbk.sh errors, but with 255 that’s exactly what wouldn’t work, cause then the program returns that on an errror... as would ssh.

E.g. on an config file error:

$ ssh -o foobar example.org true
command-line line 0: no argument after keyword "foobar"
$ echo $?
255
$

Copy link
Owner

Choose a reason for hiding this comment

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

No, I want to differ between ssh errors and remote command errors (while regarding errors from ssh_filter_btrbk.sh as ssh errors).
I need to know that if e.g. btrfs receive fails, this is not due to some failure of ssh/ssh_filter_btrbk.sh, but is a failure of the btrfs receive command.

}

run_cmd()
{
log_cmd "auth.info" "btrbk ACCEPT"
eval " $SSH_ORIGINAL_COMMAND"
log_cmd 'auth.info' 'btrbk ACCEPT'
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})?'"
Expand All @@ -66,7 +107,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
Expand All @@ -76,8 +117,8 @@ 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]?)?"
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=
fi
Expand All @@ -87,31 +128,39 @@ 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}\")}"
}


# 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
if [ "${key}" = '--sudo' ]; then
sudo_prefix='sudo -n '
fi
if [ "${key}" = '--doas' ]; then
sudo_prefix='doas -n '
fi
done

while [[ "$#" -ge 1 ]]; do
while [ "$#" -ge 1 ]; do
key="$1"

case $key in
case "${key}" in
-l|--log)
enable_log=1
;;
Expand All @@ -121,7 +170,12 @@ while [[ "$#" -ge 1 ]]; do
;;

-p|--restrict-path)
restrict_path_list="${restrict_path_list}|${2%/}" # add to list while removing trailing slash
# 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
;;

Expand Down Expand Up @@ -161,8 +215,8 @@ while [[ "$#" -ge 1 ]]; do
;;

*)
echo "ERROR: ssh_filter_btrbk.sh: failed to parse command line option: $key" 1>&2
exit 255
printf 'ERROR: ssh_filter_btrbk.sh: failed to parse command line option: %s\n' "${key}" >&2
exit 2
;;
esac
shift
Expand All @@ -173,15 +227,15 @@ 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#\|}
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
case "${SSH_ORIGINAL_COMMAND}" in
*\.\./*) reject_and_die 'directory traversal' ;;
*\$*) reject_and_die 'unsafe character "$"' ;;
*\&*) reject_and_die 'unsafe character "&"' ;;
Expand All @@ -191,7 +245,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
Expand Down