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.

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

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

Closed
wants to merge 6 commits into from

Conversation

bendlas
Copy link

@bendlas bendlas commented Aug 24, 2024

#586 rebased onto v0.10.0

opening this PR for easy patch consumption

follow-up after #641

aaruni96 and others added 6 commits August 19, 2024 19:20
[bubblewrap] Propagate SIGTERM and SIGINT to child

Co-authored-by: Earl Chew <[email protected]>
Signed-off-by: Aaruni Kaushik <[email protected]>
Signed-off-by: Aaruni Kaushik <[email protected]>
@smcv smcv self-requested a review September 30, 2024 18:42
@smcv
Copy link
Collaborator

smcv commented Oct 18, 2024

@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 #586 (review) and #586 (comment) 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?

Copy link
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

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

Marking as "changes requested", see previous comment for details

@q3cpma
Copy link

q3cpma commented Oct 19, 2024

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
Copy link

q3cpma commented Oct 19, 2024

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
Copy link
Author

bendlas commented Oct 22, 2024

This PR was meant strictly as an updated rebase of #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.

@bendlas bendlas closed this Oct 22, 2024
@smcv
Copy link
Collaborator

smcv commented Oct 22, 2024

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants