containers / bubblewrap

Low-level unprivileged sandboxing tool used by Flatpak and similar projects
Other
3.99k stars 239 forks source link

Add CLI option to pass common signals to sandboxed process (v0.10.0) #652

Closed bendlas closed 1 month ago

bendlas commented 3 months ago

https://github.com/containers/bubblewrap/pull/586 rebased onto v0.10.0

opening this PR for easy patch consumption

follow-up after https://github.com/containers/bubblewrap/pull/641

smcv commented 1 month ago

@bendlas, if you are intending to take over development of this change from @aaruni96, please see the review comments that I left on #586.

A summary:

I think whichever version of this ends up ready to apply (#586, #652, I don't really mind which one) should be squashed into a single commit, with the primary author as the commit author, the other contributors in Co-authored-by, and a commit message that explains what is being done and why. If it isn't clear who is the primary author, then "the person who is taking responsibility for it" is as good a guess as any.

Please see https://github.com/containers/bubblewrap/pull/586#pullrequestreview-2375682552 and https://github.com/containers/bubblewrap/pull/586#issuecomment-2419854727 for more details of what I would like to see in the commit message.

And, like #586, this is missing automated test coverage, which means nobody will notice when it regresses. Adding a test for it shouldn't be particularly difficult: I think it's all doable in a few lines of shell script.

If @aaruni96 and @bendlas are both interested in working on this feature, perhaps the two of you could work together on writing an automated test and an explanatory commit message, and then come back for another review round when it's ready?

q3cpma commented 1 month ago

Well, here's a simple test (race-y, hence the sleep 0.1) which sadly fails here (no USR1 printed in the second case):

#!/bin/sh
set -u

out=$(
    "$1" --ro-bind /bin/sh /bin/sh \
        /bin/sh -c 'trap "echo USR1; exit" USR1; sleep 1' &
    sleep 0.1
    kill -USR1 $!
   )
[ -z "$out" ] && echo PASS || echo "FAIL; out: $out"

out=$(
    "$1" --forward-signals --ro-bind /bin/sh /bin/sh \
        /bin/sh -c 'trap "echo USR1; exit" USR1; sleep 1' &
    sleep 0.1
    kill -USR1 $!
   )
[ "$out" = USR1 ] && echo PASS || echo "FAIL; out: $out"

Call with the bwrap executable path as $1. Obviously only works with a statically linked /bin/sh.

q3cpma commented 1 month ago

Here's a more robust version (should work on most distros, don't want to go down the rabbit hole of parsing ldd output):

#!/bin/sh
set -u

sh_path=/bin/sh
sleep_path=$("$sh_path" -c 'command -v sleep')
[ "$sleep_path" = sleep ] && unset sleep_path # builtin

bwrap_common()
{
    "$bwrap_path" \
        $(
        for i in /lib /lib64 /lib32 /libx32 /libexec /usr/lib /usr/lib64 /usr/lib32 /usr/libx32 \
            /usr/libexec /usr/local/lib /usr/local/lib64 /usr/local/lib32 /usr/local/libx32 \
            /usr/local/libexec /etc/ld.so.preload /etc/ld.so.cache /etc/ld.so.conf /etc/ld.so.conf.d
        do
            [ -e "$i" ] && echo --ro-bind "$i" "$i"
        done
        ) \
            --ro-bind "$sh_path" "$sh_path" \
            ${sleep_path+--ro-bind "$sleep_path" "$sleep_path"} \
            "$@"
}

bwrap_path=$1
out=$(
    bwrap_common \
        "$sh_path" -c 'trap "echo USR1; exit" USR1; sleep 1' &
    sleep 0.1
    kill -USR1 $!
   )
[ -z "$out" ] && echo PASS || echo "FAIL; out: $out"

out=$(
    bwrap_common --forward-signals \
        "$sh_path" -c 'trap "echo USR1; exit" USR1; sleep 1' &
    sleep 0.1
    kill -USR1 $!
   )
[ "$out" = USR1 ] && echo PASS || echo "FAIL; out: $out"
bendlas commented 1 month ago

This PR was meant strictly as an updated rebase of https://github.com/containers/bubblewrap/pull/586

Since that's been updated, I'm going to close this PR now. I've switched to #586, applied to current main. I might open another PR, in case the original PR stalls again ...

@q3cpma thanks for figuring out a test. I encourage you to work on contributing that as a PR.

smcv commented 1 month ago

As a side note on that test script, if you're not aiming to create a security boundary anyway (which, in a test, you are usually not), a major simplification is that you can use --bind / / to have the entire host filesystem appear read/write in the sandbox. That means you no longer need to know implementation details like the locations where the host OS stores necessary libraries.

(Look for RUN= in tests/libtest.sh, then look at the various commands that invoke $RUN in tests/test-run.sh.)