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

qdii · 2024-06-22T11:42:22Z

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.

rusty-snake

LGTM

There has to be an follow-up PR that adds notpm to all profiles with private-dev.

qdii · 2024-06-22T13:12:30Z

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

glitsj16

LGTM

qdii · 2024-06-23T06:22:35Z

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

etc/templates/profile.template

src/man/firejail.1.in

glitsj16

LGTM. Some ordering nitpicks...

qdii · 2024-06-23T06:43:29Z

Amended and force-pushed with alphabetical ordering.

kmk3

Sorting.

src/fbuilder/build_profile.c

src/firejail/firejail.h

src/firejail/fs_dev.c

src/firejail/main.c

src/firejail/sandbox.c

src/firejail/usage.c

kmk3 · 2024-06-23T14:07:30Z

There are some other missing changes:

Add notpm to profile.c
Add notpm to firejail-profile.5.in
Add #notpm to default.profile (keep it commented like #nou2f for
consistency)
Run make syntax and commit the resulting changes

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

qdii · 2024-06-24T21:10:33Z

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

kmk3

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.

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

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

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

kmk3

Misc

src/firejail/fs_dev.c

src/man/firejail-profile.5.in

src/man/firejail.1.in

qdii · 2024-07-03T19:31:12Z

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 · 2024-07-04T07:30:15Z

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.

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 netblue30#6379). So add a new `notpm` command and keep tpm devices in /dev by default with `private-dev` unless `notpm` is used.

Relates to #6372 #6379 #6380 #6382 #6387 #6390 #6398.

This command is deprecated and may be confused for a hardening option. This amends commit 5a61202 ("rename noautopulse to keep-config-pulse", 2021-05-13) / PR netblue30#4278. This is a follow-up to netblue30#6390.

This command is deprecated and may be confused for a hardening option. This amends commit 5a61202 ("rename noautopulse to keep-config-pulse", 2021-05-13) / PR #4278. This is a follow-up to #6390.

Remove the newer #6390 item as it is already on the list, remove the older #6307 item (modif) and sort the new #6307 item (bugfix). This amends commit 9ebecd0 ("readme/relnotes update", 2024-07-13).

rusty-snake reviewed Jun 22, 2024

View reviewed changes

glitsj16 approved these changes Jun 22, 2024

View reviewed changes

qdii force-pushed the master branch from 596cea3 to fe6bcde Compare June 23, 2024 06:12

glitsj16 reviewed Jun 23, 2024

View reviewed changes

etc/templates/profile.template Show resolved Hide resolved

glitsj16 reviewed Jun 23, 2024

View reviewed changes

src/man/firejail.1.in Show resolved Hide resolved

glitsj16 reviewed Jun 23, 2024

View reviewed changes

src/man/firejail.1.in Outdated Show resolved Hide resolved

glitsj16 approved these changes Jun 23, 2024

View reviewed changes

qdii force-pushed the master branch from fe6bcde to e643453 Compare June 23, 2024 06:43

kmk3 requested changes Jun 23, 2024

View reviewed changes

kmk3 changed the title ~~Add TPM devices to private-dev~~ feature: add notpm command & keep tpm devices in private-dev Jun 23, 2024

qdii force-pushed the master branch 3 times, most recently from 0a57213 to 6d50555 Compare June 24, 2024 21:09

kmk3 requested changes Jun 29, 2024

View reviewed changes

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

src/man/firejail-profile.5.in Outdated Show resolved Hide resolved

src/man/firejail-profile.5.in Outdated Show resolved Hide resolved

src/man/firejail.1.in Outdated Show resolved Hide resolved

kmk3 force-pushed the master branch 2 times, most recently from 4da75ef to 1d80642 Compare July 4, 2024 07:10

kmk3 approved these changes Jul 4, 2024

View reviewed changes

This was referenced Jul 4, 2024

ssh: cannot access private key stored in TPM (private-dev) #6379

Closed

docs: man: format and sort some private- items #6398

Merged

kmk3 force-pushed the master branch from 1d80642 to 6b97aad Compare July 7, 2024 20:05

kmk3 force-pushed the master branch from 6b97aad to 8bfc334 Compare July 7, 2024 20:07

kmk3 merged commit 0013202 into netblue30:master Jul 9, 2024
14 checks passed

kmk3 added a commit that referenced this pull request Jul 9, 2024

RELNOTES: add feature, modif and docs items

aed94a3

Relates to #6372 #6379 #6380 #6382 #6387 #6390 #6398.

kmk3 added the enhancement New feature request label Jul 9, 2024

kmk3 mentioned this pull request Jul 9, 2024

bugfix: remove --noautopulse from --help and zsh comp #6401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

qdii commented Jun 22, 2024 •

edited by kmk3

Loading

rusty-snake left a comment

qdii commented Jun 22, 2024

glitsj16 left a comment

qdii commented Jun 23, 2024

glitsj16 left a comment

qdii commented Jun 23, 2024

kmk3 left a comment

kmk3 commented Jun 23, 2024 •

edited

Loading

qdii commented Jun 24, 2024

kmk3 left a comment

kmk3 left a comment

qdii commented Jul 3, 2024

kmk3 commented Jul 4, 2024

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

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

Conversation

qdii commented Jun 22, 2024 • edited by kmk3 Loading

rusty-snake left a comment

Choose a reason for hiding this comment

qdii commented Jun 22, 2024

glitsj16 left a comment

Choose a reason for hiding this comment

qdii commented Jun 23, 2024

glitsj16 left a comment

Choose a reason for hiding this comment

qdii commented Jun 23, 2024

kmk3 left a comment

Choose a reason for hiding this comment

kmk3 commented Jun 23, 2024 • edited Loading

qdii commented Jun 24, 2024

kmk3 left a comment

Choose a reason for hiding this comment

kmk3 left a comment

Choose a reason for hiding this comment

qdii commented Jul 3, 2024

kmk3 commented Jul 4, 2024

qdii commented Jun 22, 2024 •

edited by kmk3

Loading

kmk3 commented Jun 23, 2024 •

edited

Loading