search

netblue30 / firejail

Linux namespaces and seccomp-bpf sandbox
https://firejail.wordpress.com
GNU General Public License v2.0
5.83k stars 568 forks source link

feature: add notpm command & keep tpm devices in private-dev #6390

Closed qdii closed 4 months ago

qdii commented 5 months ago

An ssh private key may be stored in a Trusted Platform Module (TPM) device and private-dev in ssh.profile currently breaks this use-case, as it does not keep tpm devices (see #6379).

So add a new notpm command and keep tpm devices in /dev by default with private-dev unless notpm is used.

Tested locally with:

❯ /usr/local/bin/firejail --private-dev --noprofile stat /dev/tpm0
firejail version 0.9.73

Parent pid 34059, child pid 34060
Base filesystem installed in 0.03 ms
Child process initialized in 13.23 ms
  File: /dev/tpm0
  Size: 0           Blocks: 0          IO Block: 4096   character special file
Device: 0,6 Inode: 150         Links: 1     Device type: 10,224
Access: (0600/crw-------)  Uid: (  973/     tss)   Gid: (  973/     tss)
Access: 2024-06-22 11:20:44.017230197 +0200
Modify: 2024-06-22 11:20:44.017230197 +0200
Change: 2024-06-22 11:20:44.017230197 +0200
 Birth: 2024-06-22 11:20:10.406666659 +0200

Parent is shutting down, bye...

Fixes #6379.

qdii commented 5 months ago

@rusty-snake Added a commit that adds notpm statement to all the profiles which contain private-dev.

qdii commented 5 months ago

After this is committed, we should also update the Wiki: https://github.com/netblue30/firejail/wiki/Comparison-of-firejail-and-systemd's-hardening-options

qdii commented 5 months ago

Amended and force-pushed with alphabetical ordering.

kmk3 commented 5 months ago

There are some other missing changes:

See commit 760f50f78 ("landlock: move commands into profile and add landlock.enforce", 2023-11-17) / PR #6125 for a recent example of a new command.

qdii commented 5 months ago

It looks like force-pushing is making other updates obsolete. I'm not used to working on github just yet, sorry.

qdii commented 4 months ago

This PR already makes substantial changes by adding the new command; leave the profile changes (other than the ones for default.profile and profile.template) for after this PR to make reviewing easier.

OK, reverted the long list of profile changes. I stored it in another branch to propose later.

In the parts of the code that are mostly alphabetically sorted (such as in the man pages), it makes sense to put notpm before notv.

But in some parts of the code, for whatever reason the multimedia-related options (nosound, noautopulse, no3d, notv, nodvd) are sorted together, so for consistency put notpḿ before nou2f, as was suggested in the previous review.

That is:

  • notv, nou2f -> notpm, notv, nou2f
  • nodvd, nou2f -> nodvd, notpm, nou2f

OK, I see.

Also, avoid sorting things in this PR (especially in the same commit).

This sentence confuses me, because I interpreted the two previous paragraphs as a ask to actually sort these options in a certain way.

Or do you want to merge this PR, and do the sorting in a different one? I'm happy either way.

After this PR I might try to make these parts more consistent.

kmk3 commented 4 months ago

Also, avoid sorting things in this PR (especially in the same commit).

This sentence confuses me, because I interpreted the two previous paragraphs as a ask to actually sort these options in a certain way.

Sorry, I meant avoid sorting/moving existing lines while adding new lines at the same time, such as in these cases:

In general it seems better to avoid refactoring in PRs that just add a new thing, as they will mostly contain hunks that add new lines, which are easier to review. Hunks that sort lines might be wrong/inconsistent with how the sorting is done elsewhere (this applies to the first link above).

Or do you want to merge this PR, and do the sorting in a different one? I'm happy either way.

I fixed the sorting, squashed the commits, edited the commit message and force-pushed.

Let me know if there are any issues.