# improved shell arguments parsing /scripting of a sudo-script

## toralf

I do have a bash script which runs under root and can be run by a non-privileged local user (sudo). I do wonder how to improve its coding to prevent the local user from doing harmful things with special crafting arguments. The script is here https://github.com/toralf/tinderbox/blob/master/bin/bwrap.sh

Any hints?

Edit:The current version of that script FWIW: https://github.com/toralf/tinderbox/blob/ff5247a6ea3786d445ef8d4d92aa1e6785d0160a/bin/bwrap.sh/Edit:Last edited by toralf on Sun May 03, 2020 9:28 am; edited 1 time in total

----------

## Hu

 Always quote your shell variable expansions, unless you know why you should not.  In some cases, you want word splitting, and so should not quote them.

 Your tinderbox cgroup seems to use only part of the path name.  A malicious user might cause strange effects by choosing a path that succeeds on the lock test, but collides with the cgroup of an existing wrapper elsewhere.

 When setting the memory limits, you could just have bc write directly to the magic files.  There is no need to capture the value into a shell variable first.

 You set -u, which is good, but not set -e.  Is there a reason you don't want set -e?

 A malicious user could create a file named LOCK in an arbitrary directory by creating a private area that contains var/tmp, with a symlink named tb pointing to the directory into which the LOCK should be written.

 You might find it easier to quote arguments properly if sandbox is defined as an array.

 Clever abuse of basename's option -a can allow for parameter smuggling, which might allow mischief if bwrap can be convinced to do something evil.  Demonstration:

```
$ b='-a c d'

$ a="/usr/bin/printf %s\n --hostname bwrap-$(basename $b | sed -e 's,[\.],_,g' | cut -c-57) x"

$ $a

--hostname

bwrap-c

d

x
```

Your tmpfs mounts are not specifically constrained.  A malicious user might exhaust system memory and trigger excessive swapping by starting several of these containers, then filling each of their tmpfs mounts.If the script dies unexpectedly, the lock file may not be removed.  A malicious user might be able to kill the wrapper by exhausting resources in the cgroup.

----------

## toralf

Thx Hu for this comprehensive answer.

----------

## krinn

i would

```
if [[ $# -gt 0 && $(id -u) -eq 0 ]]; then
```

to disallow sudoer to use extra param, it doesn't look safe to allow them to run ${@}

----------

## toralf

 *krinn wrote:*   

> i would
> 
> ```
> if [[ $# -gt 0 && $(id -u) -eq 0 ]]; then
> ```
> ...

 Indeed, actually I do now avoid ${@} to run directly: https://github.com/toralf/tinderbox/blob/59b7a5e9f786ac66b0ca1c425f36fe491409eeb2/bin/bwrap.sh

In general I'm currently in the mood to never allow other users than me to log into that system, wasn't aware about how many pitfalls are lurking around  :Wink: 

----------

## Hu

trap Exit KILL has no effect, since SIGKILL cannot be caught.  The other signals can be caught though, so at least part of the line should be kept.

It is good that you locked down $mnt, because if that is not restricted, I can still smuggle arguments to bwrap.  :Wink: 

```
$ b='-a c d'

$ a="/usr/bin/printf %s\n --hostname "bwrap-$(echo ${b##*/} | sed -e 's,[\.],_,g' | cut -c-57)" x"

$ $a

--hostname

bwrap--a

c

d

x

$ 
```

For completeness, in case the restrictions on $mnt are ever lifted (or someone finds a clever way to beat them), I suggest rewriting sandbox with an array:

```
sandbox=(env -i

    PATH=/usr/sbin:/usr/bin:/sbin:/bin

    HOME=/root

    SHELL=/bin/bash

    TERM=linux

    /usr/bin/bwrap

    --bind "$mnt"                         /

    --bind /home/tinderbox/tb/data      /mnt/tb/data

    --bind /home/tinderbox/distfiles    /var/cache/distfiles

    --ro-bind /home/tinderbox/tb/sdata  /mnt/tb/sdata

    --ro-bind /var/db/repos             /mnt/repos

    --tmpfs                             /var/tmp/portage

    --tmpfs /dev/shm

    --dev /dev --proc /proc

    --mqueue /dev/mqueue

    --unshare-ipc --unshare-pid --unshare-uts

    --hostname "BWRAP-$(echo "${mnt##*/}" | sed -e 's,[\.],_,g' | cut -c-57)"

    --chdir /

    --die-with-parent

     /bin/bash -l

)

"${sandbox[@]}"
```

The required changes are quoting $mnt, and using parentheses instead of quotes for the outermost layer of sandbox, but I reproduced the whole thing here for ease of copying.

Similarly, the script never cleans out old cgroup objects, so a malicious user could create huge numbers of them with a script like:

```
seq 1 1000000 | while read d; do

    mkdir "$d"

    cp /path/to/reference/chroot "$d"

    sh bwrap.sh "$d"

    rmdir "$d"

done
```

For now, the limitation on $mnt should protect against this.

I think there are still some quirks around passing $1 as an empty string or as .., both of which will survive your use of ${1##*/} unchanged, and then match the tinderbox img1/img2 directories.  Having both directories end up in the ls output complicates exploiting this, but if ever only one of img1 or img2 existed, then only the extant one would be printed, and mischief could follow.

Do you enforce a particular starting directory for this script?  If not, a malicious user could defeat your mnt lockdown through abuse of word splitting.

```
#!/bin/bash

mkdir test

cd test

mkdir foo

set -- 'nosuch foo'

a="$(ls -d /bin/${1##*/} 2>/dev/null || true)"

echo "a=$a"
```

The lack of quoting on $1 causes ls -d /bin/nosuch foo, which skips over /bin/nosuch, then prints foo because foo is in the current directory.  Since the name foo is attacker-controlled, it can be anything, and the attacker can create as many variant directories as he wants.

Another interesting minor exploit, which I have not determined how to abuse fully, is that by using a symlink named LOCK, the attacker can touch an arbitrary non-existant file.  (If the link points to a file which exists, the -f test returns true because the shell uses stat, not lstat.)

An attacker may be able to defeat the pgrep check by naming the mount such that using it in a grep expression fails to match.

```
a=$(echo abc | grep 'abc[') || true

grep: Invalid regular expression

$ echo "$a"

$ 
```

The locking tests are racy.  An attacker who artificially slowed progress through the script might be able to make two copies each perform the test before either executes the statements to acquire the lock.

The real fun comes if an attacker can find a way to make $mnt evaluate to /, so that the real root filesystem is mapped into the bubblewrapped container.  I think that could be done if the script starts in . and $1 is nosuch ., so that the word splitting discards the nosuch and leaves the ..

----------

## krinn

2nd barriere could be use to bypass 1st protection!

```
result=$(pgrep -a bwrap | grep "bwrap .* $mnt") || true
```

```
cat /usr/bin/pgrep

touch "$lock"

chmod 777 "$lock"

echo "somestring"

```

he could also create a symlink "$lock" pointing to a target directory or file and your script will change owner to tinderbox:tinderbox for him

edit: sorry i assume /usr/bin could be change at will  :Smile: 

anyway should better use /usr/bin/pgrep and /usr/bin/ls rather than just pgrep and ls

----------

## toralf

Well, so there're 3 main threat vectors so far:

1. parsing $1 to be a valid dir under ~tinderbox/img{1,2}

2. avoid abusing of the lock file

3. cgroup handling

I tried to address these issues here: https://github.com/toralf/tinderbox/blob/18861870d7b80d74258a800e0d4ebc329fa67852/bin/bwrap.sh

Again - I'm astonished how hard it is to harden such a script  :Wink: 

----------

## Hu

I think you should redesign how you validate the input.  Your use of ls and word-splitting still has a weakness.  Consider a variant of the example I posited above:

```
/bin$ ls -d /bin/nonesuch .

ls: cannot access '/bin/nonesuch': No such file or directory

./

/bin$ mnt="$(ls -d /bin/nonesuch . 2>/dev/null)"

/bin$ echo "mnt=$mnt"

mnt=./

/bin$ if [[ -z "$mnt" || ! -d "$mnt" || -L "$mnt" || $(stat -c '%u' "$mnt") -ne 0 ]]; then echo 'invalid'; fi

/bin$ if [[ "$mnt" =~ ".." || "$mnt" =~ "//" || "$mnt" =~ [[:space:]] || "$mnt" =~ '\' ]]; then echo illegal; fi

/bin$ 
```

None of your checks reject that, and now $mnt refers to the current working directory, whatever that happens to be.  The only limitation is it needs to be owned by root, but / is usually owned by root, and I doubt you want to allow mnt=/.

Your test for a lock symlink is technically racy.

```
$ strace -e trace=stat,lstat /bin/sh -c '[[ -f a || -L a ]]'

stat("a", 0x7ffce13278a0)               = -1 ENOENT (No such file or directory)

lstat("a", 0x7ffce13278a0)              = -1 ENOENT (No such file or directory)
```

Two system calls, so an attacker could theoretically swap out the contents between those calls.  In practice, this is an extremely tight race, and one an attacker is unlikely to be able to exploit.  You might be better off using [[ -e "$lock" ]], since any existence at all is likely to be unwanted.  Even with that, you still have a TOCTOU problem with the lock file, although your recent hardening of mnt assignment makes that more difficult to exploit.  An attacker would need to have one of the intermediate directories be a symlink that the attacker could redirect at will.  With the requirement that mnt point to a root-owned directory, an attacker will be stuck using a var/tmp/tb starting from a root-owned directory, which probably means only /var/tmp/tb would work.  If you already created that and it is not world writable, an attacker cannot subvert it.  Can an attacker start up a chroot "normally", then create a malicious /var/tmp/tb inside the chroot to confuse subsequent invocations of this script?  The answer depends on how you have a legitimate chroot laid out.

Although you mostly adopted my change to the sandbox assignment, you didn't wrap quotes around the expansion, so it's still not quite right.

```
$ a=(printf '%s\n' a "b c")

$ "${a[@]}"

a

b c

$ ${a[@]}

a

b

c
```

Without the quotes, you get another round of word splitting, which you want to avoid.

Yes, hardening a shell script to run with untrusted input is hard.

----------

## krinn

```
pgrep -af "/usr/bin/bwrap .*$(echo ${mnt##*/} | sed 's,+,.,g')" && exit 1
```

can be easy fool

```
>pgrep -af "/bin/firefox"

2762 /usr/bin/firefox http://forums.gentoo.org/viewtopic.php?p=8453432#8453432

```

what it mean? it mean pgrep answer to /bin/firefox check while it's /usr/bin/firefox that is running

you can then easy abuse the test

create a /tmp/usr/bin/wrap file that do nothing but loop

start /tmp/usr/bin/wrap with needed parameters to fool any parameters test you have set

pgrep test will pass

the user only need to do now is ln -s /usr/bin/wrap /tmp/lol and run /tmp/lol instead of /usr/bin/wrap to make sure his /tmp/usr/bin/wrap will be catch

----------

## toralf

Well, reworked both lock and cgroup logic, FWIW here's the result: https://github.com/toralf/tinderbox/blob/5dc9aae8adfb0c3bad5ba88cb686a3dda7a9fa4f/bin/bwrap.sh

----------

## krinn

```
# a basic lock mechanism

if [[ ! -d /run/tinderbox ]]; then

  mkdir /run/tinderbox

fi

```

i would create it first and then check if it's really a directory to bail out if it is not, avoiding /run/tinderbox exist and is a symlink

mkdir /run/tinderbox

if [[ ! -d /run/tinderbox ]]; then exitpoint

----------

## toralf

 *krinn wrote:*   

> 
> 
> ```
> # a basic lock mechanism
> 
> ...

 Doesn't mkdir would fail and "set -e" takes effect?

----------

## krinn

 *toralf wrote:*   

> Doesn't mkdir would fail and "set -e" takes effect?

 

ah yes sorry  :Smile: 

----------

