WIP: improve firejail's error messaging

[glitsj16]

One of those days on a rolling release OS. Both kernel and systemd just got upgraded, together with a few dozen other packages. After a reboot frustration levels upped as my main dev machine was only barely alive. After taking a long outdoor reflective breather I started to rebuild a few mission-critical packages. Error: too many environment variables. Exit. No sender. No return address. No nothing.

This strangely mixed crystal clear and utterly cryptic message had apparently started to pop-up left and right. Sure, I use plenty of these env vars (134 according to env), but never had I been told I use too many :) After more digging it turned out I had been bitten by a newly introduced sane maximum number of environment variables. Going over the commit in question rapidly improved my mood. At last I had found something I could relate to: didn't pass the 'sanity check' :) Took care of upping the number to accomodate my 134 vars in my firejail-git PKGBUILD.

Nothing wrong with setting a limit, nor with drawing the line at 100. In fact probably a very wise thing to do as a sandbox application. All sarcasm aside, a slightly more contextual indication of the originator of the error message(s) would have helped. This WIP is a minimalistic first attempt to do just that, nothing more, nothing less. For now it only touched error messages directly followed by exit(1) that didn't already mention {F,f}irejail in the error text. I did notice the errExit("foo") defined in common.h but decided to not touch that yet and await comments/suggestions.

[topimiettinen]

Heh, classic "640k is enough for everybody" case: I had 64 variables, so I thought 100 would be more than enough for everybody... Perhaps the limit should be higher, say 256? This would total 256 * (4096+32) ~ 1M, which should not be enough for a stack smashing attack, at least on 64 bit systems.

I'd perhaps improve errExit() and use it everywhere than add prefix to everywhere. That can be done independently of this commit too.

[glitsj16]

Perhaps the limit should be higher, say 256? This would total 256 * (4096+32) ~ 1M, which should not be enough for a stack smashing attack, at least on 64 bit systems.

@topimiettinen Thanks for pointing out the rationale behind the limit. I was not familiar with the notion of a stack smashing attack and its relation to env vars. Luckily I didn't go overboard in my PKGBUILD patch and have it set to 160 currently heh. Please don't feel the need to adjust the 'sanity level' purely on my one-time puzzled state when I didn't connect the error message with firejail. Use whatever you deem needed as an expert to keep firejail as safe as possible I'd say 👍.

I'd perhaps improve errExit() and use it everywhere than add prefix to everywhere. That can be done independently of this commit too.

That would be awesome! In this context I noticed several C files also have the following construct:

errexit:
	fprintf(stderr, "Error: blah \"%s\" blah\n", foo);
	exit(1);
}

I suppose these are different than errExit(), or can they be integrated /rewritten using that function too? Apologies for the 'absolute-beginner-C' question. A basic understanding of the language and its constructs have been on my todo list for a while, but life always seems to find a way to creep in :) Pointers to sound introductory stuff is still very welcome though.

[topimiettinen]

Variadic or printf-like functions, which can take any number of arguments and then process them (typically based on a format string as first and mandatory argument), need to use special C constructs for variadic arguments, see man stdarg. It should be possible to make errExit() variadic.

Since Firejail is only targeting Linux, we could simplify errExit() a lot by using "%m" instead of sprintf(), perror() etc. Also it should be an inlined function (static inline errExit() {stuff}) instead of a preprocessor macro, so the the C compiler can check the syntax.

[Ryujinra]

I think there could be some issue with how environment variables are being counted in the original commit: 8825856. I am also being considered over the MAX_ENVS of 100, but I do not actually have anywhere near that number env variables.

What I do have set is the env variable for configuring ls called LS_COLORS that looks like this:
LS_COLORS=so=0;38;2;0;0;0;48;2;255;106;193:ln=0;38;2;255;106;193:no=0:or=0;38;2;0;0;0;48;2;255;92;87:fi=0:bd=0;38;2;154;237;254;48;2;51;51;51:pi=0;38;2;0;0;0;48;2;87;199;255:cd=0;38;2;255;106;193;48;2;51;51;51:di=0;38;2;87;199;255:ex=1;38;2;255;92;87: ... and so on for quite awhile. The total env var is quite an ugly large block of text, but this is how you configure colors for ls.

If I remove this env var, firejail works again as expected.

Could it be that this variable is wrongly being counted as a large number of env variables, exceeding the limit?

[glitsj16]

Could it be that this variable is wrongly being counted as a large number of env variables, exceeding the limit?

@Ryujinra Good question. In the current code MAX_ENVS and MAX_ENV_LEN are defined here. As you can see, the value of MAX_ENV_LEN is defined as (PATH_MAX + 32). On Linux PATH_MAX = 4096 (see /usr/include/linux/limits.h). So if your LS_COLORS block is larger than that, it fails the test, hence exit(1). I just checked and - luckily - this time I'm within bounds, having LS_COLORS counting 3205 characters. Have you checked yours?

[Ryujinra]

So if your LS_COLORS block is larger than that, it fails the test, hence exit(1).

Yep, this makes sense. My env character count is over 10,000 thanks to my LS_COLORS block. I use vivid which autogenerates the LS_COLORS variable and it can become quite large due to the breadth of its theming.

[glitsj16]

@Ryujinra So you seem only affected by MAX_ENV_LEN. Perhaps cutting up the large LS_COLORS var into pieces of <= 4096 chars (hence individually falling below the threshold) might work. Have you tried that yet?

LS_COLORS="4096-chars-long-string"
LS_COLORS+="second-4096-long-string"
LS_COLORS+="final-string-to-get-favo-theme-working-within-firejail-limits"

[topimiettinen]

Splitting does not help, the environment variable will be the same.

This bash alias seems to work:

alias ls="LS_COLORS='rs=0:...[cut]...xspf=00;36:' /bin/ls --color=auto"

It means that the environment variable is only set for the /bin/ls command, not for every possible executable (also saves memory and copying time, though in the order of kilobytes and nanoseconds...). Using a full path ensures that a possibly firejailed version in /usr/local/bin is not used (which of course is not so great).

[Ryujinra]

It means that the environment variable is only set for the /bin/ls command, not for every possible executable (also saves memory and copying time, though in the order of kilobytes and nanoseconds...).

The issue with this is if you use something like vivid to autogenerate the LS_COLORS env var, you will end up needing to regenerate it every time ls is called, rather than once when .bashrc is sourced. Could be a performance consideration although I haven't tested it thoroughly yet.

[topimiettinen]

My example is static, there's no regeneration. If you want vivid to be used only when .bashrc is read, something like this should work:

v=$(vivid)
alias ls="LS_COLORS=$v /bin/ls --color=auto"

[Ryujinra]

My example is static, there's no regeneration. If you want vivid to be used only when .bashrc is read, something like this should work:

v=$(vivid)
alias ls="LS_COLORS=$v /bin/ls --color=auto"

Yes, this workaround works for me now. Tested with latest master and no longer get the env var length error.

[matu3ba]

If we want users to reduce number of environments, we may hint to [shameless copied].

System wide

/etc/environment: specifically meant for environment variables
/etc/env.d/*: environment variables, split in multiple files
/etc/profile: all types of initialization scripts
/etc/profile.d/*: initialization scripts
/etc/bashrc, /etc/bash.bashrc: meant for functions and aliases

User specific

~/.bash_profile: initialization for login (bash-)shells
~/.bashrc: initialization for all interactive (bash-)shells
~/.profile: used for all shells
~/.cshrc, ~/.zshrc, ~/.tcshrc: similar for non-bash shells

It is a shame that it is cluttered like that.