github-starred/firejail
2
0
Fork 0
mirror of https://github.com/netblue30/firejail.git synced 2026-05-15 14:16:14 -06:00
Code Issues 531 Projects Packages Wiki Activity Actions 6

[GH-ISSUE #2200] --read-only recursiveness does not cross filesystem boundaries #1471

New issue
Closed
opened 2026-05-05 08:08:15 -06:00 by gitea-mirror · 10 comments
gitea-mirror commented 2026-05-05 08:08:15 -06:00
Owner
Copy link

Originally created by @iskunk on GitHub (Oct 17, 2018).
Original GitHub issue: https://github.com/netblue30/firejail/issues/2200

Consider a path /foo/bar/disk/, where disk is a mounted filesystem:

skunk@image-ubuntu64:~$ ls >/foo/good.txt
skunk@image-ubuntu64:~$ ls >/foo/bar/good.txt
skunk@image-ubuntu64:~$ ls >/foo/bar/disk/good.txt
skunk@image-ubuntu64:~$ find /foo -name good.txt
/foo/bar/disk/good.txt
find: ‘/foo/bar/disk/lost+found’: Permission denied
/foo/bar/good.txt
/foo/good.txt

skunk@image-ubuntu64:~$ firejail --hostname=fjtest --read-only=/foo
Reading profile /etc/firejail/default.profile
Reading profile /etc/firejail/disable-common.inc
Reading profile /etc/firejail/disable-passwdmgr.inc
Reading profile /etc/firejail/disable-programs.inc

** Note: you can use --noprofile to disable default.profile **

Parent pid 2469, child pid 2470
Child process initialized in 16.93 ms
skunk@fjtest:~$ ls >/foo/bad.txt
bash: /foo/bad.txt: Read-only file system
skunk@fjtest:~$ ls >/foo/bar/bad.txt
bash: /foo/bar/bad.txt: Read-only file system
skunk@fjtest:~$ ls >/foo/bar/disk/bad.txt
skunk@fjtest:~$ ls -l /foo/bar/disk/bad.txt
-rw-r--r-- 1 skunk skunk 86 Oct 16 14:47 /foo/bar/disk/bad.txt

My expectation would be that --read-only=/foo prevents writing to /foo/bar/disk/, much as --blacklist=/foo prevents access to same.

This relates to issue #402, but the problem as described there relates to the /media directory and has a simple solution (use a wildcard). I'd like to bring attention to a trickier scenario: auto-mounted NFS directories. The situation is similar to the example presented above, except that mounts-within-mounts are common, and the depth at which they occur is arbitrary and unpredictable.

I'll share an example patterned after a real automount setup at my organization:

fileserve.example.com:/ on /nfs/fileserve.example.com type nfs (rw,nodev,addr=10.10.10.10)
fileserve.example.com:/vol/vol123 on /nfs/fileserve.example.com/vol/vol123 type nfs (rw,nodev,addr=10.10.10.10)

Not only would --read-only=/nfs not be sufficient, --read-only=/nfs/* also falls short. And --read-only=/nfs/*/*/* et al. is not practical.

I'd like the entire NFS tree to be accessible, but in a read-only manner to prevent writing anywhere I'd normally be able to. The current implementation of --read-only can't do this, and as the documentation gives the impression that it can, users may be caught unaware of the limitation.

Originally created by @iskunk on GitHub (Oct 17, 2018). Original GitHub issue: https://github.com/netblue30/firejail/issues/2200 Consider a path `/foo/bar/disk/`, where `disk` is a mounted filesystem: ``` skunk@image-ubuntu64:~$ ls >/foo/good.txt skunk@image-ubuntu64:~$ ls >/foo/bar/good.txt skunk@image-ubuntu64:~$ ls >/foo/bar/disk/good.txt skunk@image-ubuntu64:~$ find /foo -name good.txt /foo/bar/disk/good.txt find: ‘/foo/bar/disk/lost+found’: Permission denied /foo/bar/good.txt /foo/good.txt skunk@image-ubuntu64:~$ firejail --hostname=fjtest --read-only=/foo Reading profile /etc/firejail/default.profile Reading profile /etc/firejail/disable-common.inc Reading profile /etc/firejail/disable-passwdmgr.inc Reading profile /etc/firejail/disable-programs.inc ** Note: you can use --noprofile to disable default.profile ** Parent pid 2469, child pid 2470 Child process initialized in 16.93 ms skunk@fjtest:~$ ls >/foo/bad.txt bash: /foo/bad.txt: Read-only file system skunk@fjtest:~$ ls >/foo/bar/bad.txt bash: /foo/bar/bad.txt: Read-only file system skunk@fjtest:~$ ls >/foo/bar/disk/bad.txt skunk@fjtest:~$ ls -l /foo/bar/disk/bad.txt -rw-r--r-- 1 skunk skunk 86 Oct 16 14:47 /foo/bar/disk/bad.txt ``` My expectation would be that `--read-only=/foo` prevents writing to `/foo/bar/disk/`, much as `--blacklist=/foo` prevents access to same. This relates to issue #402, but the problem as described there relates to the `/media` directory and has a simple solution (use a wildcard). I'd like to bring attention to a trickier scenario: auto-mounted NFS directories. The situation is similar to the example presented above, except that mounts-within-mounts are common, and the depth at which they occur is arbitrary and unpredictable. I'll share an example patterned after a real automount setup at my organization: ``` fileserve.example.com:/ on /nfs/fileserve.example.com type nfs (rw,nodev,addr=10.10.10.10) fileserve.example.com:/vol/vol123 on /nfs/fileserve.example.com/vol/vol123 type nfs (rw,nodev,addr=10.10.10.10) ``` Not only would `--read-only=/nfs` not be sufficient, `--read-only=/nfs/*` also falls short. And `--read-only=/nfs/*/*/*` et al. is not practical. I'd like the entire NFS tree to be accessible, but in a read-only manner to prevent writing anywhere I'd normally be able to. The current implementation of `--read-only` can't do this, and as the documentation gives the impression that it can, users may be caught unaware of the limitation.
gitea-mirror commented 2026-05-05 08:08:18 -06:00
Author
Owner
Copy link

@smitsohu commented on GitHub (Oct 17, 2018):

@iskunk Thank you for the report. Can I ask with which Firejail version you tried?

@netblue30 I wonder if Firejail could walk /proc/self/mountinfo bottom-up in order to remount all the child mount points. Its not perfect, but maybe it could be a pragmatic solution to all these issues with --noexec and --read-only.

<!-- gh-comment-id:430570981 --> @smitsohu commented on GitHub (Oct 17, 2018): @iskunk Thank you for the report. Can I ask with which Firejail version you tried? @netblue30 I wonder if Firejail could walk /proc/self/mountinfo bottom-up in order to remount all the child mount points. Its not perfect, but maybe it could be a pragmatic solution to all these issues with `--noexec` and `--read-only`.
gitea-mirror commented 2026-05-05 08:08:18 -06:00
Author
Owner
Copy link

@netblue30 commented on GitHub (Oct 17, 2018):

We need to fix this. Let's walk /proc/self/mountinfo!

<!-- gh-comment-id:430620548 --> @netblue30 commented on GitHub (Oct 17, 2018): We need to fix this. Let's walk /proc/self/mountinfo!
gitea-mirror commented 2026-05-05 08:08:19 -06:00
Author
Owner
Copy link

@iskunk commented on GitHub (Oct 17, 2018):

Hello @smitsohu, @netblue30!

I am using Firejail 0.9.52 as shipped with Ubuntu Bionic/18.04.

<!-- gh-comment-id:430704615 --> @iskunk commented on GitHub (Oct 17, 2018): Hello @smitsohu, @netblue30! I am using Firejail 0.9.52 as shipped with Ubuntu Bionic/18.04.
gitea-mirror commented 2026-05-05 08:08:21 -06:00
Author
Owner
Copy link

@smitsohu commented on GitHub (Oct 21, 2018):

Ok, so there is a proof-of-concept at smitsohu@8a7293cf76bc20976bbc97a5e6110300e51fabfb

It looks quite ugly and there is probably plenty of room for optimizations, but it seems to work for most cases out of #402, #158, #833, #2029, #1529 and #1628 (not tested everything)

I don't have an NFS at hand, so I can't say if that would work as well.

<!-- gh-comment-id:431676920 --> @smitsohu commented on GitHub (Oct 21, 2018): Ok, so there is a proof-of-concept at smitsohu@8a7293cf76bc20976bbc97a5e6110300e51fabfb It looks quite ugly and there is probably plenty of room for optimizations, but it seems to work for most cases out of #402, #158, #833, #2029, #1529 and #1628 (not tested everything) I don't have an NFS at hand, so I can't say if that would work as well.
gitea-mirror commented 2026-05-05 08:08:22 -06:00
Author
Owner
Copy link

@crass commented on GitHub (Oct 25, 2018):

Should we have a read-only-recursive or perhaps read-only-norec? I'm not actually sure if it would be useful, but it allows for backwards compatibility if it is. Also should read-write also be recursive for consistency?

<!-- gh-comment-id:432882658 --> @crass commented on GitHub (Oct 25, 2018): Should we have a `read-only-recursive` or perhaps `read-only-norec`? I'm not actually sure if it would be useful, but it allows for backwards compatibility if it is. Also should `read-write` also be recursive for consistency?
gitea-mirror commented 2026-05-05 08:08:23 -06:00
Author
Owner
Copy link

@smitsohu commented on GitHub (Oct 25, 2018):

It looks quite ugly and there is probably plenty of room for optimizations,

Maybe the solution is to open the mount target, pick the mount id from /proc/self/fdinfo and jump right to the interesting lines in /proc/self/mountinfo. Then combine that with mount flag extraction, in order to not do unnecessary remounts. If this works it should avoid most of the problems with the current quick and dirty patch.

<!-- gh-comment-id:432993480 --> @smitsohu commented on GitHub (Oct 25, 2018): > It looks quite ugly and there is probably plenty of room for optimizations, Maybe the solution is to open the mount target, pick the mount id from /proc/self/fdinfo and jump right to the interesting lines in /proc/self/mountinfo. Then combine that with mount flag extraction, in order to not do unnecessary remounts. If this works it should avoid most of the problems with the current quick and dirty patch.
gitea-mirror commented 2026-05-05 08:08:23 -06:00
Author
Owner
Copy link

@smitsohu commented on GitHub (Oct 25, 2018):

I think I have something, I will send a pull request.

<!-- gh-comment-id:433137611 --> @smitsohu commented on GitHub (Oct 25, 2018): I think I have something, I will send a pull request.
gitea-mirror commented 2026-05-05 08:08:24 -06:00
Author
Owner
Copy link

@smitsohu commented on GitHub (Oct 26, 2018):

Should we have a read-only-recursive or perhaps read-only-norec? I'm not actually sure if it would be useful, but it allows for backwards compatibility if it is. Also should read-write also be recursive for consistency?

The new behaviour is not without problems in case of read-write. Take baloo_file.profile as an example. There we have

include disable-common.inc
...
# read-only  ${HOME}
# read-write ${HOME}/.local/share

If a user enables the last two lines, it will override the read-only ${HOME}/.local/share/applications and a number of other directives from disable-common.inc. In other words, the sequence of the directives is getting more important. This is probably something to keep in mind.

I'm going to update some profiles in the pull request to account for this (by moving read-write to the top).

<!-- gh-comment-id:433411903 --> @smitsohu commented on GitHub (Oct 26, 2018): > Should we have a read-only-recursive or perhaps read-only-norec? I'm not actually sure if it would be useful, but it allows for backwards compatibility if it is. Also should read-write also be recursive for consistency? The new behaviour is not without problems in case of `read-write`. Take baloo_file.profile as an example. There we have ``` include disable-common.inc ... # read-only ${HOME} # read-write ${HOME}/.local/share ``` If a user enables the last two lines, it will override the `read-only ${HOME}/.local/share/applications` and a number of other directives from disable-common.inc. In other words, the sequence of the directives is getting more important. This is probably something to keep in mind. I'm going to update some profiles in the pull request to account for this (by moving `read-write` to the top).
gitea-mirror commented 2026-05-05 08:08:25 -06:00
Author
Owner
Copy link

@smitsohu commented on GitHub (Oct 26, 2018):

Moving to read-write to the top does not solve the problem if users have included it via a .local file. EDIT: doesn't apply, as all read-only directives are coming later when read-write is in the .local file.

Should read-write keep the old behavior? There could be a second option that does the mounts recursively.

<!-- gh-comment-id:433422964 --> @smitsohu commented on GitHub (Oct 26, 2018): ~~Moving to `read-write` to the top does not solve the problem if users have included it via a `.local` file.~~ EDIT: doesn't apply, as all `read-only` directives are coming later when `read-write` is in the `.local` file. Should `read-write` keep the old behavior? There could be a second option that does the mounts recursively.
gitea-mirror commented 2026-05-05 08:08:26 -06:00
Author
Owner
Copy link

@iskunk commented on GitHub (Oct 26, 2018):

I was hoping, in #2202, that the order of options could be made less important rather than more. But I don't see a good alternative approach here.

It might be good if e.g. read-write ${HOME}/.local/share causes a warning to be printed that ${HOME}/.local/share/applications was previously marked read-only, so that at least the overrides are obvious to the user.

<!-- gh-comment-id:433469375 --> @iskunk commented on GitHub (Oct 26, 2018): I was hoping, in #2202, that the order of options could be made less important rather than more. But I don't see a good alternative approach here. It might be good if e.g. `read-write ${HOME}/.local/share` causes a warning to be printed that `${HOME}/.local/share/applications` was previously marked read-only, so that at least the overrides are obvious to the user.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
master
serialize_remounts_v2
landlock_v4
landlock
release-0.9.64
release-0.9.62
gitlab_ci
LTSbase
fred_ctests
docs
0.9.80
0.9.80-rc1
0.9.78
0.9.76
0.9.76-rc4
0.9.76-rc3
0.9.76-rc2
0.9.76-rc1
0.9.74
landlock-split
0.9.72
0.9.72rc1
0.9.70
0.9.68
0.9.68rc1
0.9.66
0.9.66rc1
0.9.64.4
0.9.64.2
0.9.64
0.9.64rc1
0.9.62.4
0.9.62.2
0.9.62
0.9.56.2-LTS
0.9.60
0.9.60-rc1
0.9.58.2
0.9.58
0.9.58-rc1
0.9.56-LTS-release
0.9.56-LTS
0.9.56
0.9.56-rc1
0.9.54
0.9.54-rc2
0.9.54-rc1
0.9.52
0.9.38.12
0.9.50
0.9.50-rc1
0.9.48
0.9.46
0.9.46-rc1
0.9.44.10
0.9.44.8
0.9.44.6
0.9.38.10
0.9.44.4
0.9.38.8
0.9.44.2
0.9.44
0.9.44-rc1
0.9.38.4
0.9.42
0.9.42-rc2
0.9.38.2
0.9.42-rc1
disable-globalcfg
0.9.40
0.9.40-rc1
0.9.38
0.9.38-rc1
0.9.36
0.9.36-rc1
0.9.34
0.9.34-rc1
0.9.32
0.9.32-rc1
0.9.30
0.9.30-rc1
Labels
Clear labels   
LTS merge

   
LTS merge

   
bug

   
bug

   
converted-to-discussion

   
doc-todo

   
documentation

   
duplicate

   
enhancement

   
file-transfer

   
firecfg

   
firejail-in-firejail

   
firetools

   
graphics

   
help wanted

   
information_old

   
installation

   
invalid

   
modif

   
moved

   
needinfo

   
networking

   
notabug

   
notourbug

   
old-version

   
overlayfs

   
packaging

   
profile-request

   
pull-request

Mirrored from GitHub Pull Request

  
question

   
question_old

   
removal

   
runtime-permissions

   
sandbox-ipc

   
security

   
stale

   
wiki

   
wiki

   
wontfix

   
wordpress

   
workaround
No labels
LTS merge
LTS merge
bug
bug
converted-to-discussion
doc-todo
documentation
duplicate
enhancement
file-transfer
firecfg
firejail-in-firejail
firetools
graphics
help wanted
information_old
installation
invalid
modif
moved
needinfo
networking
notabug
notourbug
old-version
overlayfs
packaging
profile-request
pull-request
question
question_old
removal
runtime-permissions
sandbox-ipc
security
stale
wiki
wiki
wontfix
wordpress
workaround
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/firejail#1471
No description provided.
Delete branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?