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.

Sign up for GitHub

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

Update nodejs-common: enable npx, clarify #4172

Closed
wants to merge 2 commits into from
Closed

Update nodejs-common: enable npx, clarify #4172

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f89a87
Update nodejs-common: enable npx, clarify
tredondo Apr 8, 2021
26b0178
Forgot to remove the private-tmp line itself
tredondo Apr 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions etc/profile-m-z/nodejs-common.profile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,16 @@ include nodejs-common.local
# added by caller profile
#include globals.local

blacklist /tmp/.X11-unix
blacklist ${RUNUSER}

ignore noexec ${HOME}

# Required to run `npx`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I understand the reasoning here. We already have noblacklist ${HOME}/.npm in npm.profile. Is npx not working without repeating it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think npm.profile is ever read. cat $(which npx) shows it's a script starting with #!/usr/bin/env node.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think npm.profile is ever read. cat $(which npx) shows it's a script starting with #!/usr/bin/env node.

Indeed it is. In fact, both /usr/bin/npm and /usr/bin/npx are symlinks to scripts with the node shebang (at least that's what I see on Arch Linux). Wouldn't that imply we need a npx.profile too, as well as a node.profile? Just asking, I'm not at all familiar with the Node.js stack.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regardless of the above remark, if we add noblacklist ${HOME}/.npm here, we should drop it from the npm.profile. Can you take care of that please? It's the only thing that needs to be done to merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npx being a node script, means firejail will read node.profile, not npm.profile.

Wouldn't that imply we need a npx.profile too, as well as a node.profile

Correct. Or, we could remove npm.profile, and only create a node.profile which would be used for node, npm and npx. Though @rusty-snake pointed out in #4085 that there's only nodejs-common.profile, so I suppose there might be a good reason for that.

if we add noblacklist ${HOME}/.npm here, we should drop it from the npm.profile. Can you take care of that please?

That would be moot if we get rid of npm.profile. @aidalgol ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be moot if we get rid of npm.profile. @aidalgol ?

It is not moot for a PR that adds it, which this does right?. Please let's not move ahead of ourselves and stick to the PR itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be moot if we get rid of npm.profile. @aidalgol ?

Sorry, I am not familiar enough with firejail's general policies and strategies to comment.

noblacklist ${HOME}/.npm

include allow-bin-sh.inc

blacklist /tmp/.X11-unix
blacklist ${RUNUSER}

include disable-common.inc
include disable-exec.inc
include disable-passwdmgr.inc
Expand Down Expand Up @@ -45,10 +48,8 @@ shell none

disable-mnt
private-dev
# May need to add `passwd` to `private-etc` below to enable debugging with some IDEs
private-etc alternatives,ca-certificates,crypto-policies,host.conf,hostname,hosts,ld.so.cache,ld.so.conf,ld.so.conf.d,ld.so.preload,locale,locale.alias,locale.conf,localtime,login.defs,mime.types,nsswitch.conf,pki,protocols,resolv.conf,rpc,services,ssl,xdg
# May need to be commented out in order to enable debugging with some IDEs
private-tmp
# Pass-through passwd because it's required to run `npx`
private-etc alternatives,ca-certificates,crypto-policies,host.conf,hostname,hosts,ld.so.cache,ld.so.conf,ld.so.conf.d,ld.so.preload,locale,locale.alias,locale.conf,localtime,login.defs,mime.types,nsswitch.conf,passwd,pki,protocols,resolv.conf,rpc,services,ssl,xdg
Comment on lines -48 to +52
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Why only adding a reason for passwd? What about login.defs, xdg, ...? IMHO we should just omit the "Pass-through ...".
  2. I don't understand what's the issue with private-tmp is.

Copy link
Contributor Author

@tredondo tredondo Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I just have no idea why login.defs and the other ones are necessary. Whoever added them should've added a reason :)

private-tmp was necessary in webstorm.profile to be able to debug Node script. Back then, I added it by mistake to nodejs-common.profile as well, but it has no effect there when Node is being debugged by WebStorm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just have no idea why login.defs and the other ones are necessary. Whoever added them should've added a reason :)

Our code is full of stuff like that. Look at other profiles that have login.defs and xdg for background. Let's stick to the PR at hand please. This should have been a one-line thing. Again, I apologize for opening the door on the more general topic of how we handle the Node.js stack, but let's move on towards a merge.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just have no idea why login.defs and the other ones are necessary. Whoever added them should've added a reason :)

Raises the general question if we want to include reasons in profiles. There are a few edge-cases where something looks complete random and a comment makes sense, however passwd is included in 22,86% of our private-etc lines. And there are a lot thing/librarys/progams/... which have trouble w/o it. Do we want profile look like the below?

click me 
# Firejail profile for apostrophe
# Description: Distraction free Markdown editor for GNU/Linux made with GTK+
# This file is overwritten after every install/update
# Persistent local customizations
include apostrophe.local
# Persistent global definitions
include globals.local

# apostrophe is an editor and viewer for markdown document, allowing ${DOCUMENT} ...
noblacklist ${DOCUMENTS}
# allow ${PICTURES} because apostrophe can include pictures in documents
noblacklist ${PICTURES}

# Lua is required by pandoc 
# Allow lua (blacklisted by disable-interpreters.inc)
include allow-lua.inc

# written in python
# Allow python (blacklisted by disable-interpreters.inc)
include allow-python3.inc

# needs no access here
include disable-common.inc
# isn't an IDE
include disable-devel.inc
# see above
include disable-exec.inc
# no other interpreters are used
include disable-interpreters.inc
# isn't a PWMGR
include disable-passwdmgr.inc
# unlikely that makrdownfiles are save there, disbale for security
include disable-programs.inc
# can be used w/o a shell
include disable-shell.inc
# disable the remains XDG dirs
include disable-xdg.inc

# gresource are stored there
whitelist /usr/share/apostrophe
# preview is broken without it
whitelist /usr/share/pandoc-*
# enable wruc for better sanboxing
include whitelist-runuser-common.inc
# wusc is used
include whitelist-usr-share-common.inc
# restrict access to /var
include whitelist-var-common.inc

# make the sandbox stronger
apparmor
# unpriv program
caps.drop all
# apostrophe can't play sound
machine-id
# disable network access for better security and privacy
net none
# no 3d rendering
no3d
# editing files on a ro device make no sense
nodvd
# no supplementary groups are requeired
nogroups
# no suid involved
nonewprivs
# needs no root
noroot
# apostrophe can't play sound
nosound
# apostrophe can't play tv
notv
# no login functionality at all
nou2f
# Disable spying
novideo
protocol unix
seccomp
shell none
# log blacklist violations to detect problems
tracelog

disable-mnt
# apostrophe is written in python and uses pandoc for converting markdown documents
private-bin apostrophe,pandoc,python3*
private-cache
private-dev
private-etc alternatives,dconf,fonts,gtk-3.0,ld.so.cache,ld.so.conf,ld.so.conf.d,ld.so.preload,pango,X11
private-tmp

dbus-user filter
dbus-user.own org.gnome.gitlab.somas.Apostrophe
# settings are imutable w/o it
dbus-user.talk ca.desrt.dconf
dbus-system none

I know this example is a bit extreme.

I added it … to nodejs-common.profile …, but it has no effect there when Node is being debugged by WebStorm.

and if it is not started by WebStorm ...

I would prefer to keep it. And if you say it shouldn't enable by default because of foobar we can still comment it.

Anyway if @glitsj16 is fine with is PR we can merge, I've even less is the npm stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just have no idea why login.defs and the other ones are necessary. Whoever added them should've added a reason :)

Raises the general question if we want to include reasons in profiles.

I honestly have no idea why I needed login.defs for npm to work (with private-etc), and I would not have the foggiest idea where to start trying to find out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I honestly have no idea why I needed login.defs for npm to work (with private-etc), and I would not have the foggiest idea where to start trying to find out.

That's quite understandable. If your uid is 1000 (the default on most OS'es) things will work without the need for login.defs. BUT we cannot assume that each and every Firejail user runs with uid=1000. That's why it is important to check and test our profiles, regardless of the user's uid. We don't nitpick for fun ;)

Copy link
Contributor

@aidalgol aidalgol Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If your uid is 1000 (the default on most OS'es) things will work without the need for login.defs. BUT we cannot assume that each and every Firejail user runs with uid=1000. That's why it is important to check and test our profiles, regardless of the user's uid. We don't nitpick for fun ;)

I actually was running as UID 1000, and I still needed login.defs for npm to work. ¯\_(ツ)_/¯


dbus-user none
dbus-system none