feature: add keep-xattrs option #5136

dseight · 2022-05-08T20:21:59Z

By default, firejail preserves only xattrs for bind-mounted files. To preserve xattrs on copied files (produced by private-bin, private-etc and private-home options) it's possible to use keep-xattrs option.

keep-xattrs is very useful in systems with signature-based IMA appraisal enabled (especially, when IMA policy prohibits running unsigned binaries).

New command checklist:

Update manpages: firejail(1) and firejail-profile(5)
Update shell completions
Update vim syntax files
Update --help

Edit by @kmk3: Add new command checklist

rusty-snake

🎉

src/zsh_completion/_firejail.in

kmk3

The argument is being named inconsistently:

xattr
xattr_name
xattrs

It would be better to pick just one and stick with it.

Since the command accepts multiple options, I would just use "xattrs"
everywhere for simplicity and clarity. That is:

keep-xattr -> keep-xattrs
xattr / xattr_name -> xattrs

By the way, I noticed that xattr(7) uses "xattr" to refer both to a single
extended attribute and to the "extended attributes" functionality in general,
but I find it clearer to use "xattrs" in the latter case.

On the documentation side, it would be clearer that it takes multiple arguments
if commas were used, for example:

--keep-xattrs=name,name,name

Similar existing options:

--caps.drop=capability,capability,capability
--caps.keep=capability,capability,capability
--cpu=cpu-number,cpu-number,cpu-number
--private-bin=file,file
--protocol=protocol,protocol,protocol

src/man/firejail-profile.txt

src/firejail/usage.c

src/firejail/main.c

src/man/firejail.txt

src/fcopy/main.c

src/zsh_completion/_firejail.in

src/fcopy/main.c

kmk3 · 2022-05-09T19:24:50Z

src/fcopy/main.c

@@ -100,6 +103,79 @@ static void selinux_relabel_path(const char *path, const char *inside_path) {
 #endif
 }

+// Convert string of comma separated values to NULL-terminated string array.
+// Original string is modified in-place.


// Original string is modified in-place.

It already returns the list, so why do that?

Why not e.g.: work on a copy of clist in the function?

Given the way that this is called on fcopy's main():

keep_xattr = csv_to_strv(argv[++i]);

That appears to effectively result in modifying the argv item from something
like "name1,name2,name3" to point to the first item on the resulting list
(which would be "name1" in this case).

Allowing unprivileged users to turn arbitrary strings into globally-accessible
string arrays doesn't sound like a good idea to me.

It already returns the list, so why do that?

Why not e.g.: work on a copy of clist in the function?

To reduce unnecessary allocations and copying.

Given the way that this is called on fcopy's main():

keep_xattr = csv_to_strv(argv[++i]);

That appears to effectively result in modifying the argv item from something like "name1,name2,name3" to point to the first item on the resulting list (which would be "name1" in this case).

It is. But it seems okay since this argument will not be used by anyone else after the call to csv_to_strv.

Allowing unprivileged users to turn arbitrary strings into globally-accessible
string arrays doesn't sound like a good idea to me.

The only thing that I have on my mind is that /proc/pid/cmdline will not reflect the actual argv the process was called with. It doesn't seem like a big issue in a helper tool.

topimiettinen · 2022-05-09T20:20:48Z

Firejail copies SELinux xattrs unconditionally because they are needed for SELinux rules to work. How about copying other xattrs too? There could be instead 'no-keep-xattr' option to disable copying if needed.

dseight · 2022-05-10T13:40:58Z

Firejail copies SELinux xattrs unconditionally because they are needed for SELinux rules to work. How about copying other xattrs too? There could be instead 'no-keep-xattr' option to disable copying if needed.

SELinux xattrs are copied only if SELinux is enabled, so it does not happen unconditionally.

I am not sure that it's worth copying all of the attributes. It feels like it's better to be explicit in what is copied to be sure that no extra data is leaking into the sandbox.

kmk3 · 2022-05-10T15:18:05Z

@dseight commented on May 10:

Firejail copies SELinux xattrs unconditionally because they are needed for
SELinux rules to work. How about copying other xattrs too? There could be
instead 'no-keep-xattr' option to disable copying if needed.

SELinux xattrs are copied only if SELinux is
enabled,
so it does not happen unconditionally.

I am not sure that it's worth copying all of the attributes. It feels like
it's better to be explicit in what is copied to be sure that no extra data is
leaking into the sandbox.

It sounds like a good idea to avoid leaking unnecessarily by default. One
example that came to mind is that maybe (that is, not sure if this would
actually happen) ACLs could leak information about other users/groups even with
noroot + nogroups.

Instead of copying all the attributes, how about just the security.* ones?

Or, why not do the same conditional copying as SELinux for the other
security.* attributes? If this sounds desirable and feasible and if there
are no obvious disadvantages to it, maybe it could be implemented without
needing any "keep" / "nokeep" options.

src/firejail/usage.c

rusty-snake · 2022-05-10T15:59:40Z

Instead of copying all the attributes, how about just the security.* ones?

This includes security.capability i.e. setcap-executables.

rusty-snake · 2022-05-10T16:05:00Z

user.org.gnome.audio.* seems to be worth to copy by default.

rusty-snake · 2022-05-10T16:06:52Z

sudo getfattr -m- -d -R / > xattrs && grep -v "^#" xattrs | grep ".*=" | cut -d= -f1 | sort -u:

security.capability
security.selinux
system.posix_acl_access
system.posix_acl_default
system.sockprotoname
trusted.delegate
trusted.invocation_id
trusted.SGI_ACL_DEFAULT
trusted.SGI_ACL_FILE
user.birthtime
user.crtime_usec
user.flatpak.http
user.Librepo.checksum.mtime
user.Librepo.checksum.sha256
user.Librepo.checksum.sha512
user.Librepo.DownloadInProgress
user.org.gnome.audio.artist
user.org.gnome.audio.title
user.overlay.impure
user.overlay.opaque
user.overlay.origin
user.random-seed-creditable
user.xdg.inactive-since
user.Zif.MdChecksum[1234567890]

kmk3 · 2022-05-10T17:00:15Z

@rusty-snake commented on May 10:

Instead of copying all the attributes, how about just the security.* ones?

This includes security.capability i.e. setcap-executables.

How about copying just the security attributes that only serve restrict (such
as security.selinux AFAIK) and ignoring the ones that can expand
capabilities? No idea on which side security.ima and security.SMACK64
would fall into.

@rusty-snake commented on May 10:

user.org.gnome.audio.* seems to be worth to copy by default.

In this case, it seems that adding option would be warranted, as hardcoding the
copying of such attributes could end up unintentionally leaking data. Which is
arguably worse than usual in the case of xattrs, as their existence is usually
not apparent.

rusty-snake · 2022-05-10T17:06:23Z

How about copying just the security attributes that only serve restrict and ignoring the ones that can expand capabilities?

How can this be detected?

rusty-snake · 2022-05-10T17:09:22Z

as hardcoding the copying of such attributes could end up unintentionally leaking data.

The filename could unintentionally leaking data. Not talking about the file content ...

What do you think to happen here? I don't understand you point.

By default, firejail preserves only xattrs for bind-mounted files. To preserve xattrs on copied files (produced by private-bin, private-etc and private-home options) it's possible to use keep-xattrs option. keep-xattrs is very useful in systems with signature-based IMA appraisal enabled (especially, when IMA policy prohibits running unsigned binaries).

dseight · 2022-05-13T12:33:55Z

Or, why not do the same conditional copying as SELinux for the other security.* attributes? If this sounds desirable and feasible and if there are no obvious disadvantages to it, maybe it could be implemented without needing any "keep" / "nokeep" options.

This will require adding extra checks, which are different for each attribute. Unconditional copying of well-known attributes will be easier both for verification and maintenance.

So, maybe proceed with keep-xattrs option and add some well-known attributes as a default value? Or, perhaps, just keep this list of attributes inside of the fcopy, without exposing it to firejail options.

The list of xattrs might include following:

security.selinux — SELinux context.
security.ima — signature or hash that could be used for IMA appraisal.
security.SMACK64 (and related SMACK attributes) — worth copying, as it behaves somewhat like SELinux. But I couldn't verify that, as I have no system with SMACK enabled.
user.org.gnome.audio.artist, .title, .genre, .duration — no idea how much they are used, but it's possible to keep them as well.

Also, by copying SELinux context with plain xattr copy it would be possible to remove libselinux dependency from fcopy, as fcopy is only used for regular files anyway.

rusty-snake · 2022-05-13T15:18:03Z

So, maybe proceed with keep-xattrs option and add some well-known attributes as a default value?

What about adding an keep-xattrs-default abc.def,at.tr to firejail.config?

kmk3 · 2022-05-14T19:54:29Z

@rusty-snake commented on May 10:

How about copying just the security attributes that only serve restrict and
ignoring the ones that can expand capabilities?

How can this be detected?

I mean, if it is known a priori that a given extended attribute has no way of
expanding capabilities beyond what the user already has.

If security.selinux is always copied (when SELinux is enabled), I would
assume that it doesn't.

Any idea if that's indeed the case? What about for the other 2 attributes?

security.selinux
security.ima
security.SMACK64

Now re-reading the first post:

By default, firejail preserves only xattrs for bind-mounted files. To
preserve xattrs on copied files (produced by private-bin, private-etc and
private-home options) it's possible to use keep-xattr option.

It seems that keep-xattrs would only affect system paths, in which case I
don't think any of them would have something like user.org.gnome.audio.*
anyway.

So how about copying each of them if the respective feature is enabled (like it
is done for SELinux)? Something like this:

if (selinux.enabled)
    // copy selinux
if (ima.enabled)
    // copy ima
if (smack.enabled)
    // copy smack

If that does not work, then how about just unconditionally copying the 3
attributes?

@dseight commented on May 13:

Or, why not do the same conditional copying as SELinux for the other
security.* attributes? If this sounds desirable and feasible and if there
are no obvious disadvantages to it, maybe it could be implemented without
needing any "keep" / "nokeep" options.

This will require adding extra checks, which are different for each
attribute. Unconditional copying of well-known attributes will be easier both
for verification and maintenance.

So, maybe proceed with keep-xattrs option and add some well-known
attributes as a default value?

Not sure about this, but if a hardcoding were to be implemented for a
profile-level command, it would be good to do it as a profile_add() (meaning
that it could be easily ignored/overridden by the user).

Or, perhaps, just keep this list of attributes inside of the fcopy, without
exposing it to firejail options.

If the other approaches do not work out and a keep-xattrs command is to be
implemented, it would be nice to leave it just on fcopy; I have some thoughts
on the API of keep-xattrs and on copying them for user vs system paths (I
might expand on this later if needed).

The list of xattrs might include following:

security.selinux — SELinux context.

security.ima — signature or hash that could be used for IMA appraisal.

security.SMACK64 (and
related
SMACK attributes) — worth copying, as it behaves somewhat like SELinux.
But I couldn't verify that, as I have no system with SMACK enabled.

user.org.gnome.audio.artist, .title, .genre, .duration — no idea
how much they are used, but it's possible to keep them as well.

Also, by copying SELinux context with plain xattr copy it would be possible
to remove libselinux dependency from fcopy, as fcopy is only used for regular
files anyway.

Interesting; that would be an improvement (though could be tricky to implement
considering the makefile improvements that I've been working on, which depends
on #5140).

@rusty-snake commented on May 14:

So, maybe proceed with keep-xattrs option and add some well-known
attributes as a default value?

What about adding an keep-xattrs-default abc.def,at.tr to firejail.config?

If this is only intended to affect system/read-only paths as stated in the
first post, then this indeed sounds more appropriate than a profile-level
option, as I don't see why someone would enable this for one profile but not
another.

With all that said, the following is what looks like the most obvious solution
to me at the moment:

if (selinux.enabled)
    // copy selinux
if (ima.enabled)
    // copy ima
if (smack.enabled)
    // copy smack

But would such checks require additional IMA/SMACK dependencies?

If the above does not work, I have the following questions:

Is there any harm (other than unnecessary leakage) in always unconditionally
copying the security.selinux xattr (instead of only when SELinux is
enabled)?
What about the other 2 xattrs?
When an xattr is applied to a directory, does it effectively get applied to
everything under it? Or does it have to be applied on every single file/dir?

rusty-snake · 2022-05-15T16:17:46Z

It seems that keep-xattrs would only affect system paths,

private-home

dseight · 2022-05-16T09:51:53Z

With all that said, the following is what looks like the most obvious solution to me at the moment:
if (selinux.enabled)
    // copy selinux
if (ima.enabled)
    // copy ima
if (smack.enabled)
    // copy smack
But would such checks require additional IMA/SMACK dependencies?

No, this wouldn't require any additional dependencies.

But there is a problem: IMA status (enabled or not) is determined by the presence of a /sys/kernel/security/ima directory/symlink. This check works fine for private-bin, but fails on private-etc because private-etc copy is done after /sys remount.

SELinux detection works fine after remount because it also checks /proc/filesystems for selinuxfs presence. But there is no similar way for IMA.

Is there any harm (other than unnecessary leakage) in always unconditionally
copying the security.selinux xattr (instead of only when SELinux is
enabled)?

I see no harm, security.selinux attribute is not used by anything except SELinux itself. This attribute will not give any extra capabilities to the sandboxed process.

What about the other 2 xattrs?

SMACK behaves just like the SELinux (but in a simplified manner), so it harmless as well.
security.ima neither gives any extra capabilities, it contains only hash or signature of the file. There are no ways to abuse this information.

When an xattr is applied to a directory, does it effectively get applied to
everything under it? Or does it have to be applied on every single file/dir?

In SELinux and SMACK, directories and files under them may have different attributes, so xattr should be applied on every single file or directory. IMA has no xattrs on directories, and each file has its distinct xattr value.

kmk3 · 2022-05-16T16:55:38Z

@rusty-snake commented on May 15:

It seems that keep-xattrs would only affect system paths,

private-home

Thanks; looks like that part didn't register with me at all.

@dseight commented on May 16:

With all that said, the following is what looks like the most obvious solution to me at the moment:
if (selinux.enabled)
    // copy selinux
if (ima.enabled)
    // copy ima
if (smack.enabled)
    // copy smack
But would such checks require additional IMA/SMACK dependencies?
No, this wouldn't require any additional dependencies.

Nice.

But there is a problem: IMA status (enabled or not) is determined by the
presence of a /sys/kernel/security/ima directory/symlink. This check works
fine for private-bin, but fails on private-etc because private-etc copy
is done
after
/sys remount.

The issue of checking for paths after bind-mounting them has bitten me before
(see whitelist-run-common.inc on #4930).

To fix this, I created a function (in a WIP branch) that does path-related
feature checks on firejail startup. It currently only checks for features that
affect group handling, but it could probably be expanded to check for anything
that can be checked at startup, likely including IMA status. The branch has
other fixes that I still want to test, but the one about the paths looks good
enough I suppose, so I'll try to open a PR for that.

SELinux detection works fine after remount because it also
checks
/proc/filesystems for selinuxfs presence. But there is no similar way for
IMA.

Is there any harm (other than unnecessary leakage) in always
unconditionally copying the security.selinux xattr (instead of only
when SELinux is enabled)?

I see no harm, security.selinux attribute is not used by anything except
SELinux itself. This attribute will not give any extra capabilities to the
sandboxed process.

What about the other 2 xattrs?

SMACK behaves just like the SELinux (but in a simplified manner), so it
harmless as well. security.ima neither gives any extra capabilities, it
contains only hash or signature of the file. There are no ways to abuse this
information.

When an xattr is applied to a directory, does it effectively get applied
to everything under it? Or does it have to be applied on every single
file/dir?

In SELinux and SMACK, directories and files under them may have different
attributes, so xattr should be applied on every single file or directory. IMA
has no xattrs on directories, and each file has its distinct xattr value.

Thanks for the detailed explanations.

One thing that I am still unsure about is if the xattr copy is currently done
recursively.

For example, given the following directory structure:

/etc/foo
/etc/foo/bar1/baz
/etc/foo/bar2/baz

If private-etc foo is used, does firejail copy security.selinux for every
path under /etc/foo or just for /etc/foo itself?

Skimming through fs_etc.c and fcopy/main.c, at first glance it looks like it
would only copy it for the directory itself.

dseight · 2022-05-16T19:43:19Z

One thing that I am still unsure about is if the xattr copy is currently done recursively.

For example, given the following directory structure:
/etc/foo
/etc/foo/bar1/baz
/etc/foo/bar2/baz
If private-etc foo is used, does firejail copy security.selinux for every path under /etc/foo or just for /etc/foo itself?

Skimming through fs_etc.c and fcopy/main.c, at first glance it looks like it would only copy it for the directory itself.

Copying will be recursive. The following will get done, considering your case:

/etc/foo is a directory → call duplicate_dir
Here, in duplicate_dir, nftw will walk through each entry (/etc/foo, /etc/foo/bar1, /etc/foo/bar1/baz, /etc/foo/bar2, /etc/foo/bar2/baz) and call fs_copydir on that entry:
1. If the entry is a file, copy_file will be called, which then will call selinux_relabel_path and copy_xattrs
2. If the entry is a directory, a directory will be created, but no xattrs will be copied (perhaps this is a problem that no one has encountered yet)

Simplify fcopy args processing

7db169b

rusty-snake reviewed May 9, 2022

View reviewed changes

src/zsh_completion/_firejail.in Outdated Show resolved Hide resolved

kmk3 requested changes May 9, 2022

View reviewed changes

kmk3 reviewed May 9, 2022

View reviewed changes

Add --keep-xattrs option to fcopy

055168c

dseight force-pushed the keep-xattr branch from 2bfa613 to c74b3e2 Compare May 10, 2022 13:41

kmk3 reviewed May 10, 2022

View reviewed changes

src/firejail/usage.c Outdated Show resolved Hide resolved

kmk3 changed the title ~~Add keep-xattr option~~ Add keep-xattrs option May 10, 2022

dseight force-pushed the keep-xattr branch from c74b3e2 to 624517b Compare May 10, 2022 17:23

kmk3 added the enhancement New feature request label Aug 25, 2024

kmk3 changed the title ~~Add keep-xattrs option~~ feature: add keep-xattrs option Aug 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add keep-xattrs option #5136

feature: add keep-xattrs option #5136

dseight commented May 8, 2022 •

edited

Loading

rusty-snake left a comment

kmk3 left a comment

kmk3 May 9, 2022

dseight May 10, 2022

topimiettinen commented May 9, 2022

dseight commented May 10, 2022

kmk3 commented May 10, 2022

rusty-snake commented May 10, 2022

rusty-snake commented May 10, 2022

rusty-snake commented May 10, 2022

kmk3 commented May 10, 2022

rusty-snake commented May 10, 2022

rusty-snake commented May 10, 2022

dseight commented May 13, 2022

rusty-snake commented May 13, 2022

kmk3 commented May 14, 2022

rusty-snake commented May 15, 2022

dseight commented May 16, 2022

kmk3 commented May 16, 2022

dseight commented May 16, 2022

feature: add keep-xattrs option #5136

Are you sure you want to change the base?

feature: add keep-xattrs option #5136

Conversation

dseight commented May 8, 2022 • edited Loading

rusty-snake left a comment

Choose a reason for hiding this comment

kmk3 left a comment

Choose a reason for hiding this comment

kmk3 May 9, 2022

Choose a reason for hiding this comment

dseight May 10, 2022

Choose a reason for hiding this comment

topimiettinen commented May 9, 2022

dseight commented May 10, 2022

kmk3 commented May 10, 2022

rusty-snake commented May 10, 2022

rusty-snake commented May 10, 2022

rusty-snake commented May 10, 2022

kmk3 commented May 10, 2022

rusty-snake commented May 10, 2022

rusty-snake commented May 10, 2022

dseight commented May 13, 2022

rusty-snake commented May 13, 2022

kmk3 commented May 14, 2022

rusty-snake commented May 15, 2022

dseight commented May 16, 2022

kmk3 commented May 16, 2022

dseight commented May 16, 2022

dseight commented May 8, 2022 •

edited

Loading