bug? --rmenv seems to fire after check for length of environment variables, which makes long variables impossible to remove from firejail side

[MahouShoujoMivutilde]

First of all, thank you for this amazing tool, really enjoyed playing with it recently!

As for issue, there is not much to describe aside from the title, so here is

how to reproduce

steps

Let's set 2 short environment variables:

~ ❯ export AAAA=bbbb

~ ❯ export AAAA2=bbbb

~ ❯ firejail --rmenv="AAAA" env | grep -i aaaa
Reading profile /etc/firejail/default.profile
Reading profile /etc/firejail/disable-common.inc
Reading profile /etc/firejail/disable-passwdmgr.inc
Reading profile /etc/firejail/disable-programs.inc

** Note: you can use --noprofile to disable default.profile **

Parent pid 1352757, child pid 1352764
Warning: /sbin directory link was not blacklisted
Warning: /usr/sbin directory link was not blacklisted
Warning: cleaning all supplementary groups
Child process initialized in 88.72 ms
AAAA2=bbbb

Parent is shutting down, bye...

~ ❯ firejail --noprofile --rmenv="AAAA" env | grep -i aaaa
Parent pid 1354494, child pid 1354497
Child process initialized in 26.68 ms
AAAA2=bbbb

Parent is shutting down, bye...

As expected, AAAA is removed, and AAAA2 is available.

But now let's see what happens if we set AAAA to something really long.

~ ❯ export AAAA=$(tr -dc 'a-zA-Z0-9' < /dev/urandom | head -c 8000)

~ ❯ firejail --noprofile --rmenv="AAAA" env | grep -i aaaa
Error: too long environment variables

I expected same as in the first case to happen, but, as you can see, it just exits with error.

why even do this

My actual use case for it is removing LS_COLORS, which is generated by vivid and about is 8000 characters long.

I know about a workaround with alias as shown here, but LS_COLORS is also used by e.g. lf (which sometimes i launch just by itself, not from interactive shell), and my various scripts (which i usually call from sxhkd).

Of course, it's fixable even from my side, but i think it makes more sense for --rmenv to work before the length check.

That way, i could just add

rmenv LS_COLORS

to ~/.config/firejail/globals.local

And have just one LS_COLORS in .profile, instead of a bunch of local env LS_COLORS="$(vivid ...)" whatever or similar.

So what do you think? Is this expected behavior?

Environment

• Arch linux, linux-zen 5.8.14, everything up-to-date.
• Firejail version: 0ab64e2 (because i want new dbus stuff for firefox, because error "Firefox is already running but is not responding" when using firefox wayland #3290 )

Checklist

• The upstream profile (and redirect profile if exists) have no changes fixing it.
• The program has a profile. (If not, request one in # 1139)
• Programs needed for interaction are listed in the profile.
• A short search for duplicates was performed.
• If it is a AppImage, --profile=PROFILENAME is used to set the right profile.

...Behavior is not related to particular program, in regards to duplicates - i haven't seen anything about length check / --rmenv order.

debug output (with long AAAA)

~ ❯ firejail --debug --rmenv="AAAA" env
Error: too long environment variables

(yes, there is nothing else)

EDIT:

Ops, i didn't actually need to use latest master to fix #3290.

Turns out, it's an error on my side - what i wanted to do is to have 2 separate isolated profiles with different configs and stuff, and naturally i used firefox with -no-remote flag, so two instances could run simultaneously.

And when i tried to open new links in one of them with firejail firefox -no-remote -p pr1 *link* - i was getting the same error, as in #3290.

After messing around in the middle of the night with a launch script (in which i wrapped conditional whitelisting and stuff) and upgrading to the latest master it eventually work, so i assumed that master fixed it and forgot about now missing -no-remote.

Turns out, when you run firefox in firejail, you can run two instances simultaneously without -no-remote.

Well, that's embarrassing. 😅

But i think original question about --rmenv / length checking order is still valid, since i (and probably other vivid users too) eventually will have to deal with it.

In case anyone wants the launching script - here it is:

fjfirefox

#!/usr/bin/env sh

unset LS_COLORS

while [ "$#" -gt 0 ]; do
    case "$1" in
        -p)
            profile="$2"
            shift
            ;;
        -u)
            url="$2"
            shift
            ;;
        *)
            exit 1
            ;;
    esac
    shift
done

profile="${profile:-stable}"
# note that it assumes that profiles are in non default location.
# that way, you can whitelist only one profile and nothing else.
# cache also stored here, as you can check with about:cache
wlprof="$(fd -t d -d 1 "\.$profile$" . ~/.config/firefox-profiles)"

if [ -z "$url" ]; then
    firejail \
        --whitelist="$wlprof" \
        firefox-nightly -p "$profile"
else
    firejail \
        --whitelist="$wlprof" \
        firefox-nightly -p "$profile" "$url"
fi

[topimiettinen]

Currently the environment variables are checked as early as possible, then config files are read, then program arguments including --rmenv and --env. It would make sense to handle --rmenv and --env specially before env var check. That way also env vars added by --env would be subject to the same restrictions.

[topimiettinen]

This seems more tricky than I thought. It makes sense to process --rmenv early (and even remove the environment variables immediately), but --env is not so clear.

--rmenv and --env only apply to the sandboxed program, not to the main program of Firejail. Removing variables from the sandbox (and also from Firejail itself) should be safe. The length checks are made to protect Firejail as it's a set-uid program. But for the sandboxed program it should be OK to pass any kind of variables, so the checks shouldn't be necessary for them. Though similar length checks are also applied to arguments, which makes impossible to pass long variables with --env.

Maybe the argument length checks should be dropped for --env. But #3322 would remove most of the env variables from Firejail (they would still be passed to sandboxes), then the environment variable length checks could be made to apply only to those variables which are retained and the others could be any length.

[MahouShoujoMivutilde]

Wow, that was fast! Thank you for working on it.

I've built your PR against a3d415a, and it seems to work, but only with --rmenv, not with rmenv inside config files.

here is a quick test

~ ❯ export AAAA2=bbbb

~ ❯ cat ~/.config/firejail/globals.local
rmenv AAAA

~ ❯ export AAAA=bbbb

~ ❯ firejail env | grep -i aaaa
Reading profile /etc/firejail/default.profile
Reading profile /home/witch/.config/firejail/globals.local
Reading profile /etc/firejail/disable-common.inc
Reading profile /etc/firejail/disable-passwdmgr.inc
Reading profile /etc/firejail/disable-programs.inc

** Note: you can use --noprofile to disable default.profile **

Parent pid 677723, child pid 677725
Warning: /sbin directory link was not blacklisted
Warning: /usr/sbin directory link was not blacklisted
Warning: cleaning all supplementary groups
Child process initialized in 63.74 ms
AAAA2=bbbb

Parent is shutting down, bye...

As you can see, globals.local is read and applied as expected when AAAA is short.

~ ❯ export AAAA=$(tr -dc 'a-zA-Z0-9' < /dev/urandom | head -c 8000)

~ ❯ firejail env | grep -i aaaa
Error: too long environment variables

~ ❯ firejail --profile=default env | grep -i aaaa
Error: too long environment variables

But firejail still exits with an error when AAAA is long.

~ ❯ firejail --rmenv=AAAA env | grep -i aaaa
Reading profile /etc/firejail/default.profile
Reading profile /home/witch/.config/firejail/globals.local
Reading profile /etc/firejail/disable-common.inc
Reading profile /etc/firejail/disable-passwdmgr.inc
Reading profile /etc/firejail/disable-programs.inc

** Note: you can use --noprofile to disable default.profile **

Parent pid 681426, child pid 681428
Warning: /sbin directory link was not blacklisted
Warning: /usr/sbin directory link was not blacklisted
Warning: cleaning all supplementary groups
Child process initialized in 75.99 ms
AAAA2=bbbb

Parent is shutting down, bye...

~ ❯ firejail --noprofile --rmenv=AAAA env | grep -i aaaa
Parent pid 724839, child pid 724842
Child process initialized in 12.36 ms
AAAA2=bbbb

Parent is shutting down, bye...

--rmenv, however, now works as expected.

[topimiettinen]

I see. The problem with rmenv in profiles is that the profiles are read during processing of program arguments where lots of other stuff also happens, so if the length check would be moved after that, it would be very late. Reading the profiles earlier is also not possible since profiles can be added also with --profile. So for that I'd rather finalize #3322 first, but since we're now closer to a new release, it may not be a good move to do that just now.

The error message could be more helpful and suggest using --rmenv. Maybe the long variables and program arguments could be even removed automatically with a warning, but Firejail would not need to exit. Removing excess variables or arguments could be too random.

[MahouShoujoMivutilde]

So for that I'd rather finalize #3322 first, but since we're now closer to a new release, it may not be a good move to do that just now.

Makes sense.

Maybe the long variables and program arguments could be even removed automatically with a warning, but Firejail would not need to exit. Removing excess variables or arguments could be too random.

But until #3322 is finished, wouldn't you always want to remove them anyway - so firejail will actually run?

If you decide to go this route - i think length check can be just a warning about dropped variables, but if, e.g. user also has too many variables - it's his job to clean them, and there should be an error.

But i get what you mean, as a quick fix a warning instead of error is maybe ok, but in a long run #3322 seems like is a way to go.

[topimiettinen]

I think the workarounds should be good enough for just now, the user could miss the warnings for removing and removing could cause other problems.