Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

profiles: clarify userns comments & fix comment on electron.profile #5304

Closed
wants to merge 2 commits into from
Closed

profiles: clarify userns comments & fix comment on electron.profile #5304

wants to merge 2 commits into from

Conversation

pirate486743186
Copy link
Contributor

clarified description and fixed a comment.

I think it should be renamed to electron-common.profile ...

@kmk3
Copy link
Collaborator

kmk3 commented Aug 9, 2022

@pirate486743186 commented on Aug 7:

And renaming it to electron-common is probably better. You should follow
conventions, or people get confused. Just call all profiles that are meant to
be redirects -common.

@pirate486743186 commented on Aug 9:

clarified description and fixed a comment.

I think it should be renamed to electron-common.profile ...

Makes sense to me.

Feel free to submit it in a new PR after this PR is merged (to avoid
conflicts).

@pirate486743186 pirate486743186 changed the title electron.profile minor corrections profile minor corrections Aug 10, 2022
Copy link
Collaborator

@kmk3 kmk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unprivileged userns clone is not a thing unto itself, so quoting it makes it
more confusing IMO. I think having a reference to the relevant man page should
provide enough context.

Also, an additional suggestion for etc/firejail.config:

 # This logging feature is disabled by default in our implementation.
 # seccomp-log no
 
-# Enable or disable user namespace support, default enabled.
+# Enable or disable user namespace support, default enabled. See
+# user_namespaces(7).
 # userns yes
 
 # Disable whitelist top level directories, in addition to those

Lastly, the commit message could be clearer. Example:

profiles minor edits

->

docs: clarify meaning of userns & fix comment on electron.profile

@kmk3 kmk3 changed the title profile minor corrections docs: clarify meaning of userns & fix comment on electron.profile Aug 11, 2022
@kmk3 kmk3 added the documentation Issues and pull requests related to the documentation label Aug 11, 2022
@rusty-snake
Copy link
Collaborator

Also, an additional suggestion for etc/firejail.config:

We could be more clear that this maps to noroot.

  • dbus maps to dbus*
  • bind maps to bind*
  • apparmor maps to apparmor
  • network maps to net*, ip*, defaultgw and a few other
  • x11 maps to x11
  • seccomp maps to seccomp*
  • ...
  • userns maps to noroot

@kmk3
Copy link
Collaborator

kmk3 commented Aug 11, 2022

@rusty-snake commented on Aug 11:

Also, an additional suggestion for etc/firejail.config:

We could be more clear that this maps to noroot.

  • dbus maps to dbus*
  • bind maps to bind*
  • apparmor maps to apparmor
  • network maps to net*, ip*, defaultgw and a few other
  • x11 maps to x11
  • seccomp maps to seccomp*
  • ...
  • userns maps to noroot

If by "foo maps to bar" you mean "bar depends on foo being enabled",
then I agree.

Though I would add a comment to each command in the man pages instead of in
firejail.config, to avoid potentially having to update a comment in
firejail.config whenever a new command is added (and to avoid adding another
item to the new command checklist).

@pirate486743186
Copy link
Contributor Author

You are overcomplicating it. You need to speak in stupid. You don't need all these details, you speak to a user, not a CPU.

All that is needed is simply to make it clear it's not a typo.

What about "unprivileged user namespaces (userns) clone", without the quotes?

@rusty-snake
Copy link
Collaborator

rusty-snake commented Aug 12, 2022

  1. You speak to an advanced user.
  2. You can not simplify complex topics to a level where everyone understand it without making the statement useless.

What about "unprivileged user namespaces (userns) clone", without the quotes?

Writing user namespace out is good and IMHO we don't need the "(userns)".

@kmk3
Copy link
Collaborator

kmk3 commented Aug 13, 2022

@pirate486743186 commented on Aug 12:

You are overcomplicating it. You need to speak in stupid. You don't need all
these details, you speak to a user, not a CPU.

All that is needed is simply to make it clear it's not a typo.

What about "unprivileged user namespaces (userns) clone", without the quotes?

This is as verbose as I can make it in 3 lines:

-# Add the next line to your chromium-common.local if your kernel allows "unprivileged userns clone".
+# If your kernel allows the creation of user namespaces by unprivileged users
+# (that is, if `sysctl kernel.unprivileged_userns_clone` is >= 1), you can add
+# the next line to your chromium-common.local.

By the way, I could not find the above sysctl variable being documented:

$ man -w 7 user_namespaces | xargs pacman -Qo
/usr/share/man/man7/user_namespaces.7.gz is owned by man-pages 5.13-1
$ man 7 user_namespaces | grep -F unprivileged_userns
$
$ pacman -Q linux-docs
linux-docs 5.18.16.artix1-1
$ pacman -Qlq linux-docs | grep -v '/$' | xargs \
  grep -F unprivileged_userns 2>/dev/null
$

So I think that it would be good to have it written down somewhere in firejail,
even if it's just in the profiles.

@rusty-snake
Copy link
Collaborator

kernel.unprivileged_userns_clone is a Debian patch (which is also used by Arch and others) and does not exists in mainline kernels or RedHat distributions (RHEL, Fedora, Cent OS Stream). If it does not exist userns is either disabled for all users or enabled.

@kmk3
Copy link
Collaborator

kmk3 commented Aug 13, 2022

@rusty-snake commented on Aug 13:

kernel.unprivileged_userns_clone is a Debian patch (which is also used by
Arch and others) and does not exists in mainline kernels or RedHat
distributions (RHEL, Fedora, Cent OS Stream). If it does not exist userns is
either disabled for all users or enabled.

I see, thanks.

Do you know of a more portable way to check if it's allowed?

@glitsj16
Copy link
Collaborator

Do you know of a more portable way to check if it's allowed?

Perhaps testing sysctl user.max_user_namespaces? If that's equal to 0 (zero) one can assume unprivileged namespaces are disallowed.

@rusty-snake
Copy link
Collaborator

@glitsj16 then are all userns disallowed, not only unprivileged. And if it it >0, they can still be disabled for unprivileged processes.

Maybe running unshare -U can work to find it out.

@kmk3
Copy link
Collaborator

kmk3 commented Aug 20, 2022

@rusty-snake commented on Aug 13:

@glitsj16 then are all userns disallowed, not only unprivileged. And if it it

0, they can still be disabled for unprivileged processes.

Maybe running unshare -U can work to find it out.

Nice, that seems to work:

$ sudo sysctl kernel.unprivileged_userns_clone=1
kernel.unprivileged_userns_clone = 1
$ unshare -U true; echo $?
0
$ unshare -U whoami
nobody
$ sudo sysctl kernel.unprivileged_userns_clone=0
kernel.unprivileged_userns_clone = 0
$ unshare -U true; echo $?
unshare: unshare failed: Operation not permitted
1
$ unshare -U whoami
unshare: unshare failed: Operation not permitted

How about this then?

-# Add the next line to your chromium-common.local if your kernel allows "unprivileged userns clone".
+# If your kernel allows the creation of user namespaces by unprivileged users
+# (for example, if running `unshare -U whoami` prints "nobody"), you can add
+# the next line to your chromium-common.local.

@pirate486743186 pirate486743186 changed the title docs: clarify meaning of userns & fix comment on electron.profile [don't merge]docs: clarify meaning of userns & fix comment on electron.profile Aug 22, 2022
@rusty-snake
Copy link
Collaborator

Is there a way whoami can return something else? Maybe we could use unshare -U echo enabled.

@kmk3
Copy link
Collaborator

kmk3 commented Aug 28, 2022

@rusty-snake commented on Aug 28:

Is there a way whoami can return something else?

I wondered about that too, but not sure.

Maybe we could use unshare -U echo enabled.

Agreed; updated diff:

-# Add the next line to your chromium-common.local if your kernel allows "unprivileged userns clone".
+# If your kernel allows the creation of user namespaces by unprivileged users
+# (for example, if running `unshare -U echo enabled` prints "enabled"), you
+# can add the next line to your chromium-common.local.

@pirate486743186 pirate486743186 deleted the electron-profile-minor branch March 3, 2023 01:24
@kmk3 kmk3 changed the title [don't merge]docs: clarify meaning of userns & fix comment on electron.profile docs: clarify meaning of userns & fix comment on electron.profile Sep 5, 2024
@kmk3 kmk3 changed the title docs: clarify meaning of userns & fix comment on electron.profile profiles: clarify meaning of userns & fix comment on electron.profile Sep 5, 2024
@kmk3 kmk3 changed the title profiles: clarify meaning of userns & fix comment on electron.profile profiles: clarify userns comments & fix comment on electron.profile Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues and pull requests related to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants