Closed Bug 1580271 Opened 5 years ago Closed 5 years ago

enhance sandbox on OpenBSD with unveil()

Categories

(Core :: Security: Process Sandboxing, enhancement, P5)

69 Branch
All
OpenBSD
enhancement

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: jcs, Assigned: jcs)

References

Details

Attachments

(7 files, 8 obsolete files)

Attached patch ff-unveil.diff (obsolete) (deleted) — Splinter Review

Use the unveil system call on OpenBSD to lock down filesystem access. Our OpenBSD package of Firefox will contain 3 files listing the files that the main, content, and GPU processes are allowed to access. These are loaded in at sandbox initialization time, and each passed to unveil(). Once the pledge call is executed after this, filesystem access is locked down.

Depends on #1580268

Assignee: nobody → jcs
Status: UNCONFIRMED → NEW
Component: General → Security: Process Sandboxing
Ever confirmed: true
Product: Firefox → Core
Hardware: x86_64 → All
Comment on attachment 9091834 [details] [diff] [review] ff-unveil.diff I doubt hardcoding /etc/firefox/unveil.main in the source code will be accepted as is, since etc/firefox doesnt exist. :gcp, what would be the preferred way to add a (potentially user-customizable) config file listing paths and corresponding perms ? Right now, filesystem isolation is configured this way in our chromium port but i'm not comfortable with adding /etc/firefox. A default file under browser/defaults/preferences/, overridable by the user ? for pledge we use about:config strings for that wont work for unveil paths lists. I know for linux & macos iirc the list is hardcoded in the source code but that doesnt leave a way for the user to add paths. Oh and of course, see http://man.openbsd.org/unveil for details on the unveil() API.
Attachment #9091834 - Flags: feedback?(gpascutto)

See the unveil.* files in https://github.com/openbsd/ports/tree/master/www/chromium/files for the list of paths whitelisted/allowed per process type in our chromium port.

Attached file paths list for the main process (deleted) —

Updating unveil.main, it also needs read access to /dev/urandom or the new bookmark window won't draw correctly (?!)

for pledge we use about:config strings for that wont work for unveil paths lists.

Why not?

FYI we do offer that option on Linux so "weird" distros and users with unusual setups can help themselves:
https://searchfox.org/mozilla-central/source/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp#494
It's a comma-separated list.

Right now, filesystem isolation is configured this way in our chromium port but i'm not comfortable with adding /etc/firefox. A default file under browser/defaults/preferences/, overridable by the user ?

Some kind of prefs file seems like it would work, but by default those are in the profile. I don't think we expect prefs in a system-wide location, but I'm far from the expert here. As far as bundling, it seems only channel-prefs is shipped, and the default prefs.js is reconstructed when we write out the profile.

I think I'll need to ask someone who knows our prefs system better to see what the best option is.

Nicholas, see above comments for context. Do you have any advice where a "system-wide" Firefox preference file should live, or if that would even be a good idea (as opposed to a per-profile config) in this case?

Flags: needinfo?(n.nethercote)

We don't currently have any system-wide prefs file that I know of, and I am reluctant to add one. It sounds challenging to get right.

mkaply might have opinions. Does this relate to any of our enterprise policy stuff? I don't understand that well.

Flags: needinfo?(n.nethercote) → needinfo?(mozilla)
Comment on attachment 9091834 [details] [diff] [review] ff-unveil.diff Review of attachment 9091834 [details] [diff] [review]: ----------------------------------------------------------------- For the final review, add a DOM IPC peer: https://wiki.mozilla.org/Modules/All#IPC ::: dom/ipc/ContentChild.cpp @@ +4052,5 @@ > > +#define MAXTOKENS 3 > +#define _UNVEIL_MAIN "/etc/firefox/unveil.main" > +#define _UNVEIL_CONTENT "/etc/firefox/unveil.content" > +#define _UNVEIL_GPU "/etc/firefox/unveil.gpu" Pending discussion in the other bug. @@ +4060,5 @@ > bool StartOpenBSDSandbox(GeckoProcessType type, char *passedPledge) { > nsAutoCString promisesString; > nsAutoCString processTypeString; > + FILE *fp; > + char *line = NULL, *home = NULL, **ap, *tokens[MAXTOKENS]; Use nullptr. (The only other instance of NULL in this file is other OpenBSD code) @@ +4095,5 @@ > return false; > } > > + if (!PR_GetEnv("MOZ_DISABLE_UNVEIL")) { > + fp = fopen(ufile, "r"); A more Firefoxy way of doing this can be found in: https://searchfox.org/mozilla-central/source/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp#130 Which would avoid the fiddling with C-style strings here. @@ +4120,5 @@ > + mozilla::ipc::FatalError(nsPrintfCString("%s line %zu: must supply " > + "value", ufile, lineno).get(), false); > + > + if (tokens[0][0] == '~') { > + if ((home = getenv("HOME")) == NULL || *home == '\0') PR_GetEnv or https://searchfox.org/mozilla-central/source/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp#334
Attachment #9091834 - Flags: feedback?(gpascutto) → feedback-

Technically any file in default/pref is a system wide preference file.

So we know what kinds of things users will need to add for this?

Flags: needinfo?(mozilla)

If we can use an about:config string that's fine, but the current tuples (path, perms) list could get large and not easily editable in about:config. I would personally prefer that of course, just not sure if we'd hit the limit.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #8)

FYI we do offer that option on Linux so "weird" distros and users with unusual setups can help themselves:
https://searchfox.org/mozilla-central/source/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp#494
It's a comma-separated list.

so there's a "default list" that is hardcoded, and on top of that two prefs for extra exclusions/inclusions - good, didnt know that. Maybe we could reuse this idea, once we're comfortable with the final paths lists..

(In reply to Nicholas Nethercote [:njn] from comment #10)

We don't currently have any system-wide prefs file that I know of, and I am reluctant to add one. It sounds challenging to get right.

Totally agree with that, /etc/firefox feels deeply wrong to me. If we need a systemwide extra pref file, default/pref under the install dir is the way to go imo. And if the user wants to override it, he copies the file to his profile and edits it there.

Comment on attachment 9091834 [details] [diff] [review] ff-unveil.diff ` nsCOMPtr<nsIFile> ldconfig(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv));` fails inside the GPU process (but not in the main or content process). Is there something specific that would be preventing that?

If we can use an about:config string that's fine, but the current tuples (path, perms) list could get large and not easily editable in about:config. I would personally prefer that of course, just not sure if we'd hit the limit.

There is a hard 1 MiB limit for string prefs. Furthermore, any string pref that exceeds 4 KiB will not be sent to content processes; I don't know if that's relevant here.

Is there something specific that would be preventing that?

My first guess would be the sandbox, but it's just a guess.

Attached patch ff-unveil2.diff (obsolete) (deleted) — Splinter Review

Here's a rewrite of the patch that uses more of the C++ API for file IO and string manipulation, but it fails in the GPU process because the first nsCOMPtr<nsIFile> lf(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv)) line returns a failure. It works fine in the main and content processes.

Priority: -- → P5

My first guess would be the sandbox, but it's just a guess.

They're on OpenBSD, so they don't see our Linux seccomp filter & broker. In fact, this config file that won't load contains the paths that are supposed to be loadable after they do the unveil calls.

Is the GPU process actually enabled on OpenBSD? Could it be that it's not and the second call fails because we already restricted filesystem access in this process?

If we can use an about:config string that's fine, but the current tuples (path, perms) list could get large and not easily editable in about:config. I would personally prefer that of course, just not sure if we'd hit the limit.

Would you or wouldn't you prefer that? I'm fine with either approach suggested so far (i.e. defaults/prefs, even though that seems basically empty on my Linux installs, or prefs). We can do as for Linux and take a default list that we know is going to be needed in the source too.

Is the GPU process actually enabled on OpenBSD?

It is in that it starts up (see #1580268) but I'm not sure whether you mean something else by "enabled".

(In reply to Gian-Carlo Pascutto [:gcp] from comment #18)

If we can use an about:config string that's fine, but the current tuples (path, perms) list could get large and not easily editable in about:config. I would personally prefer that of course, just not sure if we'd hit the limit.

Would you or wouldn't you prefer that? I'm fine with either approach suggested so far (i.e. defaults/prefs, even though that seems basically empty on my Linux installs, or prefs). We can do as for Linux and take a default list that we know is going to be needed in the source too.

I would be fine with going with two string prefs for user-level overrides, and a default hardcoded list in the source if we had a minimal & stable "trusted list" of paths/perms - iirc (but i might be wrong, please correct me jcs) the current lists has been taken straight from the chromium port, with some additions, and might still need refinement in some corner cases that will only be discovered once users start dogfooding the feature.

(In reply to Nicholas Nethercote [:njn] from comment #15)

There is a hard 1 MiB limit for string prefs. Furthermore, any string pref that exceeds 4 KiB will not be sent to content processes; I don't know if that's relevant here.

from my understanding the content process receives its config on the cmdline, and that should include the list of paths to unveil with which perms. How are the >4kb prefs sent then ?

Anyway, right now the paths lists with comments are 1.3kb for main/content processes so we should be fine.

The failure from do_CreateInstance was NS_ERROR_NOT_INITIALIZED, which lead me to find that NS_InitMinimalXPCOM needed to be called first. This is actually done already in GPUParent::Init (from the child, not the parent), so I just need to delay the call to StartOpenBSDSandbox until after NS_InitMinimalXPCOM is called. This fixed the preferences not loading in #1580268, and now the error I get from do_CreateInstance is NS_ERROR_FACTORY_NOT_REGISTERED which seems right if the GPU process is not supposed to be loading files.

So I can revert back to using the C FILE API instead, unless I am supposed to be allowed to access files from the GPU child and there's something else wrong here.

I suppose if we're moving the unveil strings into a preference, I won't need the file API anyway. I'll work moving these to preferences.

Attached patch ff-unveil3.diff (obsolete) (deleted) — Splinter Review

3rd revision of unveil patch, now reading file lists from preferences.

Attachment #9091834 - Attachment is obsolete: true
Attachment #9092362 - Attachment is obsolete: true

from my understanding the content process receives its config on the cmdline, and that should include the list of paths to unveil with which perms. How are the >4kb prefs sent then ?

That was true at one point, but has since changed. Prefs are now all sent via proper IPC mechanisms such as shared memory.

So I can revert back to using the C FILE API instead, unless I am supposed to be allowed to access files from the GPU child and there's something else wrong here.

I think that should work but it's quite possible there's nothing else using our File APIs in GPUParent (in the child, sigh!) right now.

That was true at one point, but has since changed. Prefs are now all sent via proper IPC mechanisms such as shared memory.

FWIW this passing/setup happens here, which is close to where you originally added the pref transfer in the other bug:
https://searchfox.org/mozilla-central/source/gfx/ipc/GPUProcessImpl.cpp#67

Comment on attachment 9092480 [details] [diff] [review] ff-unveil3.diff Review of attachment 9092480 [details] [diff] [review]: ----------------------------------------------------------------- rwc in ~/Downloads from content seems seriously wrong. ::: browser/app/profile/firefox.js @@ +1137,1 @@ > pref("security.sandbox.pledge.content", "stdio rpath wpath cpath inet recvfd sendfd prot_exec unix drm ps"); Wonder how close our SocketProcess work brings us to being able to nuke the inet pledge here. Aside from that, drm is the biggie to get rid of? I would think wpath isn't so critical if unveil works? @@ +1139,5 @@ > pref("security.sandbox.pledge.gpu", "stdio rpath wpath cpath ps sendfd recvfd drm dns unix prot_exec"); > + > +// default file paths unveiled to each process, bug 1580271 > +pref("security.sandbox.unveil.main", "/dev/urandom r,/dev/video rw,/etc/fonts r,/etc/machine-id r,/tmp rwc,/usr/bin/lpr rx,/usr/local/bin/xdg-desktop-menu rx,/usr/local/bin/xdg-open rx,/usr/local/bin/xdg-settings rx,/usr/local/lib r,/usr/local/firefox r,/usr/local/lib/firefox rx,/usr/local/share r,/usr/share/locale r,/var/cache/fontconfig r,/usr/X11R6/lib r,/usr/X11R6/share r,/var/run rw,~/.XCompose r,~/.Xauthority r,~/.Xdefaults r,~/.fontconfig r,~/.fonts r,~/.fonts.conf r,~/.fonts.conf.d r,~/.icons r,~/.mozilla rwc,~/.pki rwc,~/.sndio rwc,~/.terminfo r,~/.cache/dconf rwc,~/.cache/thumbnails rwc,~/.config/dconf r,~/.config/fontconfig r,~/.config/gtk-3.0 r,~/.config/mimeapps.list r,~/.config/mozilla rwc,~/.config/user-dirs.dirs r,~/.local/share/applications rwc,~/.local/share/applnk r,~/.local/share/fonts r,~/.local/share/glib-2.0 r,~/.local/share/icons r,~/.local/share/mime r,~/.local/share/recently-used.xbel rwc,~/.local/share/themes r,~/Downloads rwc"); > +pref("security.sandbox.unveil.content", "/dev/drm0 rw,/etc/fonts r,/etc/machine-id r,/tmp rwc,/usr/bin/lpr rx,/usr/local/bin/xdg-desktop-menu rx,/usr/local/bin/xdg-open rx,/usr/local/bin/xdg-settings rx,/usr/local/lib r,/usr/local/firefox r,/usr/local/lib/firefox rx,/usr/local/share r,/usr/share/locale r,/var/cache/fontconfig r,/usr/X11R6/lib r,/usr/X11R6/share r,/var/run rw,~/.XCompose r,~/.Xauthority r,~/.Xdefaults r,~/.fontconfig r,~/.fonts r,~/.fonts.conf r,~/.fonts.conf.d r,~/.icons r,~/.mozilla rwc,~/.pki rwc,~/.sndio rwc,~/.terminfo r,~/.cache/dconf rwc,~/.cache/thumbnails rwc,~/.config/dconf r,~/.config/fontconfig r,~/.config/gtk-3.0 r,~/.config/mimeapps.list r,~/.config/mozilla rwc,~/.config/user-dirs.dirs r,~/.local/share/applications rwc,~/.local/share/applnk r,~/.local/share/fonts r,~/.local/share/glib-2.0 r,~/.local/share/icons r,~/.local/share/mime r,~/.local/share/recently-used.xbel rwc,~/.local/share/themes r,~/Downloads rwc"); See remark below about /tmp rwc ~/.sndio rwc seems potentially problematic. No idea how easy it is to get the audio remoting working on OpenBSD though. ~/.cache/dconf rwc similar issues. ~/Downloads rwc seems like a total no-go? I doubt it's needed?! @@ +1140,5 @@ > + > +// default file paths unveiled to each process, bug 1580271 > +pref("security.sandbox.unveil.main", "/dev/urandom r,/dev/video rw,/etc/fonts r,/etc/machine-id r,/tmp rwc,/usr/bin/lpr rx,/usr/local/bin/xdg-desktop-menu rx,/usr/local/bin/xdg-open rx,/usr/local/bin/xdg-settings rx,/usr/local/lib r,/usr/local/firefox r,/usr/local/lib/firefox rx,/usr/local/share r,/usr/share/locale r,/var/cache/fontconfig r,/usr/X11R6/lib r,/usr/X11R6/share r,/var/run rw,~/.XCompose r,~/.Xauthority r,~/.Xdefaults r,~/.fontconfig r,~/.fonts r,~/.fonts.conf r,~/.fonts.conf.d r,~/.icons r,~/.mozilla rwc,~/.pki rwc,~/.sndio rwc,~/.terminfo r,~/.cache/dconf rwc,~/.cache/thumbnails rwc,~/.config/dconf r,~/.config/fontconfig r,~/.config/gtk-3.0 r,~/.config/mimeapps.list r,~/.config/mozilla rwc,~/.config/user-dirs.dirs r,~/.local/share/applications rwc,~/.local/share/applnk r,~/.local/share/fonts r,~/.local/share/glib-2.0 r,~/.local/share/icons r,~/.local/share/mime r,~/.local/share/recently-used.xbel rwc,~/.local/share/themes r,~/Downloads rwc"); > +pref("security.sandbox.unveil.content", "/dev/drm0 rw,/etc/fonts r,/etc/machine-id r,/tmp rwc,/usr/bin/lpr rx,/usr/local/bin/xdg-desktop-menu rx,/usr/local/bin/xdg-open rx,/usr/local/bin/xdg-settings rx,/usr/local/lib r,/usr/local/firefox r,/usr/local/lib/firefox rx,/usr/local/share r,/usr/share/locale r,/var/cache/fontconfig r,/usr/X11R6/lib r,/usr/X11R6/share r,/var/run rw,~/.XCompose r,~/.Xauthority r,~/.Xdefaults r,~/.fontconfig r,~/.fonts r,~/.fonts.conf r,~/.fonts.conf.d r,~/.icons r,~/.mozilla rwc,~/.pki rwc,~/.sndio rwc,~/.terminfo r,~/.cache/dconf rwc,~/.cache/thumbnails rwc,~/.config/dconf r,~/.config/fontconfig r,~/.config/gtk-3.0 r,~/.config/mimeapps.list r,~/.config/mozilla rwc,~/.config/user-dirs.dirs r,~/.local/share/applications rwc,~/.local/share/applnk r,~/.local/share/fonts r,~/.local/share/glib-2.0 r,~/.local/share/icons r,~/.local/share/mime r,~/.local/share/recently-used.xbel rwc,~/.local/share/themes r,~/Downloads rwc"); > +pref("security.sandbox.unveil.gpu", "/dev/drm0 rw,/tmp rwc,/usr/local/lib/firefox r,/usr/local/lib/gdk-pixbuf-2.0 r,/usr/X11R6/lib r,/usr/share/locale r,/usr/local/share r,~/.Xauthority r"); Do you need the rwc in /tmp? The GPU process is unsandboxed now on other platforms (temporarily!), but when it was still in content we remapped /tmp access to a specific other dir. AFAIK the only thing that needed access to it were shader caches (which are optional) and fontconfig that wanted to write out a random file. See for example: https://bugzilla.mozilla.org/show_bug.cgi?id=1386404 You might be able to outright block it without any real ill effects, which is worth considering. ::: dom/ipc/ContentChild.cpp @@ +4155,5 @@ > + > + nUnveils++; > + } > + > + if (!nUnveils) { This is used as a boolean only, so you could make it one.
Attachment #9092480 - Flags: feedback-

(In reply to Gian-Carlo Pascutto [:gcp] from comment #26)

Comment on attachment 9092480 [details] [diff] [review]
ff-unveil3.diff

Review of attachment 9092480 [details] [diff] [review]:

rwc in ~/Downloads from content seems seriously wrong.

::: browser/app/profile/firefox.js
@@ +1137,1 @@

pref("security.sandbox.pledge.content", "stdio rpath wpath cpath inet recvfd sendfd prot_exec unix drm ps");

Wonder how close our SocketProcess work brings us to being able to nuke the
inet pledge here.

I've been running without inet for many days since another OpenBSD developer mentioned it and I haven't seen any fallout from it. I'm not sure what to expect to break without it, so I didn't want to include it here, but I can open it as another bug if there is no fallout.

Aside from that, drm is the biggie to get rid of? I would think wpath isn't
so critical if unveil works?

Yeah, the unveil really makes the *path ones not so scary.

@@ +1139,5 @@

pref("security.sandbox.pledge.gpu", "stdio rpath wpath cpath ps sendfd recvfd drm dns unix prot_exec");
+
+// default file paths unveiled to each process, bug 1580271
+pref("security.sandbox.unveil.main", "/dev/urandom r,/dev/video rw,/etc/fonts r,/etc/machine-id r,/tmp rwc,/usr/bin/lpr rx,/usr/local/bin/xdg-desktop-menu rx,/usr/local/bin/xdg-open rx,/usr/local/bin/xdg-settings rx,/usr/local/lib r,/usr/local/firefox r,/usr/local/lib/firefox rx,/usr/local/share r,/usr/share/locale r,/var/cache/fontconfig r,/usr/X11R6/lib r,/usr/X11R6/share r,/var/run rw,/.XCompose r,/.Xauthority r,/.Xdefaults r,/.fontconfig r,/.fonts r,/.fonts.conf r,/.fonts.conf.d r,/.icons r,/.mozilla rwc,/.pki rwc,/.sndio rwc,/.terminfo r,/.cache/dconf rwc,/.cache/thumbnails rwc,/.config/dconf r,/.config/fontconfig r,/.config/gtk-3.0 r,/.config/mimeapps.list r,/.config/mozilla rwc,/.config/user-dirs.dirs r,/.local/share/applications rwc,/.local/share/applnk r,/.local/share/fonts r,/.local/share/glib-2.0 r,/.local/share/icons r,/.local/share/mime r,/.local/share/recently-used.xbel rwc,/.local/share/themes r,/Downloads rwc");
+pref("security.sandbox.unveil.content", "/dev/drm0 rw,/etc/fonts r,/etc/machine-id r,/tmp rwc,/usr/bin/lpr rx,/usr/local/bin/xdg-desktop-menu rx,/usr/local/bin/xdg-open rx,/usr/local/bin/xdg-settings rx,/usr/local/lib r,/usr/local/firefox r,/usr/local/lib/firefox rx,/usr/local/share r,/usr/share/locale r,/var/cache/fontconfig r,/usr/X11R6/lib r,/usr/X11R6/share r,/var/run rw,
/.XCompose r,/.Xauthority r,/.Xdefaults r,/.fontconfig r,/.fonts r,/.fonts.conf r,/.fonts.conf.d r,/.icons r,/.mozilla rwc,/.pki rwc,/.sndio rwc,/.terminfo r,/.cache/dconf rwc,/.cache/thumbnails rwc,/.config/dconf r,/.config/fontconfig r,/.config/gtk-3.0 r,/.config/mimeapps.list r,/.config/mozilla rwc,/.config/user-dirs.dirs r,/.local/share/applications rwc,/.local/share/applnk r,/.local/share/fonts r,/.local/share/glib-2.0 r,/.local/share/icons r,/.local/share/mime r,/.local/share/recently-used.xbel rwc,/.local/share/themes r,/Downloads rwc");

See remark below about /tmp rwc

~/.sndio rwc seems potentially problematic. No idea how easy it is to get
the audio remoting working on OpenBSD though.
~/.cache/dconf rwc similar issues.
~/Downloads rwc seems like a total no-go? I doubt it's needed?!

Changed to 'r' so downloaded files can be viewed.

@@ +1140,5 @@

+// default file paths unveiled to each process, bug 1580271
+pref("security.sandbox.unveil.main", "/dev/urandom r,/dev/video rw,/etc/fonts r,/etc/machine-id r,/tmp rwc,/usr/bin/lpr rx,/usr/local/bin/xdg-desktop-menu rx,/usr/local/bin/xdg-open rx,/usr/local/bin/xdg-settings rx,/usr/local/lib r,/usr/local/firefox r,/usr/local/lib/firefox rx,/usr/local/share r,/usr/share/locale r,/var/cache/fontconfig r,/usr/X11R6/lib r,/usr/X11R6/share r,/var/run rw,/.XCompose r,/.Xauthority r,/.Xdefaults r,/.fontconfig r,/.fonts r,/.fonts.conf r,/.fonts.conf.d r,/.icons r,/.mozilla rwc,/.pki rwc,/.sndio rwc,/.terminfo r,/.cache/dconf rwc,/.cache/thumbnails rwc,/.config/dconf r,/.config/fontconfig r,/.config/gtk-3.0 r,/.config/mimeapps.list r,/.config/mozilla rwc,/.config/user-dirs.dirs r,/.local/share/applications rwc,/.local/share/applnk r,/.local/share/fonts r,/.local/share/glib-2.0 r,/.local/share/icons r,/.local/share/mime r,/.local/share/recently-used.xbel rwc,/.local/share/themes r,/Downloads rwc");
+pref("security.sandbox.unveil.content", "/dev/drm0 rw,/etc/fonts r,/etc/machine-id r,/tmp rwc,/usr/bin/lpr rx,/usr/local/bin/xdg-desktop-menu rx,/usr/local/bin/xdg-open rx,/usr/local/bin/xdg-settings rx,/usr/local/lib r,/usr/local/firefox r,/usr/local/lib/firefox rx,/usr/local/share r,/usr/share/locale r,/var/cache/fontconfig r,/usr/X11R6/lib r,/usr/X11R6/share r,/var/run rw,
/.XCompose r,/.Xauthority r,/.Xdefaults r,/.fontconfig r,/.fonts r,/.fonts.conf r,/.fonts.conf.d r,/.icons r,/.mozilla rwc,/.pki rwc,/.sndio rwc,/.terminfo r,/.cache/dconf rwc,/.cache/thumbnails rwc,/.config/dconf r,/.config/fontconfig r,/.config/gtk-3.0 r,/.config/mimeapps.list r,/.config/mozilla rwc,/.config/user-dirs.dirs r,/.local/share/applications rwc,/.local/share/applnk r,/.local/share/fonts r,/.local/share/glib-2.0 r,/.local/share/icons r,/.local/share/mime r,/.local/share/recently-used.xbel rwc,/.local/share/themes r,/Downloads rwc");
+pref("security.sandbox.unveil.gpu", "/dev/drm0 rw,/tmp rwc,/usr/local/lib/firefox r,/usr/local/lib/gdk-pixbuf-2.0 r,/usr/X11R6/lib r,/usr/share/locale r,/usr/local/share r,~/.Xauthority r");

Do you need the rwc in /tmp?

The GPU process is unsandboxed now on other platforms (temporarily!), but
when it was still in content we remapped /tmp access to a specific other
dir. AFAIK the only thing that needed access to it were shader caches (which
are optional) and fontconfig that wanted to write out a random file.

See for example: https://bugzilla.mozilla.org/show_bug.cgi?id=1386404

You might be able to outright block it without any real ill effects, which
is worth considering.

Yes, rwc is needed for tmp for the shm files. Without all 3 one gets:

[Child 77282, Main Thread] WARNING: failed to open shm: Permission denied: file /home/jcs/code/firefox/ipc/chromium/src/base/shared_memory_posix.cc, line 142
Crash Annotation GraphicsCriticalError: |[C0][GFX1-]: Failed to connect WebRenderBridgeChild. (t=1.44932) |[C151][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (t=10.8195) |[C152][GFX1]: ValidateTile failed (t=10.8195) |[C153][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (t=10.8711) |[C154][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (t=11.0112) |[C155][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (t=11.0814) |[C156][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (t=11.1069) |[C142][GFX1]: ValidateTile failed (t=10.8187) |[C143][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (t=10.8188) |[C144][GFX1]: ValidateTile failed (t=10.8189) |[C145][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (t=10.819) |[C146][GFX1]: ValidateTile failed (t=10.8191) |[C147][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (t=10.8192) |[C148][GFX1]: ValidateTile failed (t=10.8193) |[C149][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (t=10.8193) |[C150][GFX1]: ValidateTile failed (t=10.8194) [GFX1]: [Tiling:Client] Failed to allocate a TextureClient

::: dom/ipc/ContentChild.cpp
@@ +4155,5 @@

  •  nUnveils++;
    
  • }
  • if (!nUnveils) {

This is used as a boolean only, so you could make it one.

Ok, will change.

Attached patch ff-unveil4.diff (obsolete) (deleted) — Splinter Review
Attachment #9092480 - Attachment is obsolete: true

(In reply to Gian-Carlo Pascutto [:gcp] from comment #26)

~/.sndio rwc seems potentially problematic. No idea how easy it is to get
the audio remoting working on OpenBSD though.

I had a start at it in bug #1467982 but that got stalled on bug #1467872 - now that the latter is fixed, maybe i could revisit this if i find hacking time...

(oh, and many thanks for the review/feedback on unveil paths/pledge promises !)

(In reply to jcs from comment #27)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #26)

@@ +1137,1 @@

pref("security.sandbox.pledge.content", "stdio rpath wpath cpath inet recvfd sendfd prot_exec unix drm ps");

Wonder how close our SocketProcess work brings us to being able to nuke the
inet pledge here.

I've been running without inet for many days since another OpenBSD developer mentioned it and I haven't seen any fallout from it. I'm not sure what to expect to break without it, so I didn't want to include it here, but I can open it as another bug if there is no fallout.

I looked at bit in my git history and inet was in my first wip on pledging the content process (https://cgit.rhaalovely.net/mozilla-firefox/commit/files/all-openbsd.js?id=db0ed17a814399422112130000d8e6508ea6ea9f), and i have absolutely zero memory why.

If we're fine without it that's great. Will test that on my installs.

(In reply to jcs from comment #27)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #26)

~/Downloads rwc seems like a total no-go? I doubt it's needed?!

Changed to 'r' so downloaded files can be viewed.

If this is about navigating to file:/// URLs, there's a dedicated content process for those (see FILE_REMOTE_TYPE); with the existing sandbox implementations that process gets global file read access and others don't.

[Child 77282, Main Thread] WARNING: failed to open shm: Permission denied: file /home/jcs/code/firefox/ipc/chromium/src/base/shared_memory_posix.cc, line 142

There's a per-process prefix for the shm_open names (see base::SharedMemory::AppendPosixShmPrefix), if that would help restrict the /tmp access. (Also, that error message from line 142 looks like an older version of the source.)

(In reply to Landry Breuil (:gaston) from comment #30)

I looked at bit in my git history and inet was in my first wip on pledging the content process (https://cgit.rhaalovely.net/mozilla-firefox/commit/files/all-openbsd.js?id=db0ed17a814399422112130000d8e6508ea6ea9f), and i have absolutely zero memory why.

Remote X11, maybe? (Including ssh forwarding, assuming that OpenBSD ssh uses IP loopback like portable OpenSSH does.) See also bug 1376559.

There's a larger problem with unveil: as I understand it, it builds up the allowed list non-atomically, and adding the first entry changes the policy from allow-all to default-deny, which means that if another thread is trying to access the filesystem during this process, it can get spurious denials.

This could be avoided by unveiling early in startup when the process is single-threaded (note that starting IPC creates threads), but that might require adding more permissions to the policies.

Are the preferences available for reading that early?

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #31)

(In reply to jcs from comment #27)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #26)

~/Downloads rwc seems like a total no-go? I doubt it's needed?!

Changed to 'r' so downloaded files can be viewed.

If this is about navigating to file:/// URLs, there's a dedicated content process for those (see FILE_REMOTE_TYPE); with the existing sandbox implementations that process gets global file read access and others don't.

Without the 'r' for ~/Downloads, the tab just shows an error page saying it can't read the file.

(In reply to jcs from comment #35)

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #31)

(In reply to jcs from comment #27)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #26)

~/Downloads rwc seems like a total no-go? I doubt it's needed?!

Changed to 'r' so downloaded files can be viewed.

If this is about navigating to file:/// URLs, there's a dedicated content process for those (see FILE_REMOTE_TYPE); with the existing sandbox implementations that process gets global file read access and others don't.

Without the 'r' for ~/Downloads, the tab just shows an error page saying it can't read the file.

Iirc some release ago, i tried triggerring the dedicated content process for file:// urls and failed to have anything clearly separated out from the other content processes, but maybe i tried wrong.

Attached patch ff-unveil5.diff (obsolete) (deleted) — Splinter Review

Here's a fifth version that honors $XDG_CONFIG_HOME, $XDG_CACHE_HOME, and $XDG_DATA_HOME being set in the user's environment, replacing hard-coded paths ~/.config, ~/.cache, and ~/.local/share in the unveil list with those variables (and then expanding them).

Attachment #9092692 - Attachment is obsolete: true
Attached patch ff-unveil6.diff (obsolete) (deleted) — Splinter Review

Landry pointed out that executing 3rd party apps for MIME handlers (such as mupdf for viewing PDFs) broke, so this adds a workaround.

Any handler not in the unveil list won't show up in the "Open With" dialog because at least Glib checks that the binary is accessible, so this adds a generic system default entry which executes xdg-open on the temporary file. Since xdg-open will be outside the unveil/pledge, it can execute whatever application is registered as the default. This is kind of confusing because Glib executes that handler (xdg-open in this case) by way of executing gio-launch-desktop, so only gio-launch-desktop needs to be in the pledge list.

This also adds /etc/mailcap and ~/.mailcap to the unveil list, so that older-style mailcap handlers still work if the executable is explicitly added to the unveil list by the user. For example, adding application/pdf; /usr/local/bin/xpdf %s to ~/.mailcap and then adding /usr/local/bin/xpdf rx to security.sandbox.unveil.main will allow it to show up in the list and get executed when selected in the Firefox dialog.

Attachment #9092944 - Attachment is obsolete: true
Attached patch ff-unveil7.diff (obsolete) (deleted) — Splinter Review

A new revision, rebased on top of #1584839

Attachment #9093624 - Attachment is obsolete: true

Landry pointed out that executing 3rd party apps for MIME handlers (such as mupdf for viewing PDFs) broke, so this adds a workaround.

IIRC We had it as a task to make sure MIME remoting is enabled on all platforms. Let me nag haik to find the bug...

Iirc some release ago, i tried triggerring the dedicated content process for file:// urls and failed to have anything clearly separated out from the other content processes, but maybe i tried wrong.

I'm not sure what you mean exactly, but you can grep the source for FILE_REMOTE_TYPE to see the related logic and how to detect that you're in this process.

Flags: needinfo?(haftandilian)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #40)

Landry pointed out that executing 3rd party apps for MIME handlers (such as mupdf for viewing PDFs) broke, so this adds a workaround.

IIRC We had it as a task to make sure MIME remoting is enabled on all platforms. Let me nag haik to find the bug...

We have bug 1446549. My plan has been to enable bug 1452278 for all platforms to address this.

This can be tested by changing nsExternalHelperAppService::GetSingleton() to use nsOSHelperAppServiceChild for child processes like it does for Mac on other platforms.

Flags: needinfo?(haftandilian)
Attached patch ff-unveil8.diff (obsolete) (deleted) — Splinter Review

New patch against Firefox 70

Attachment #9097220 - Attachment is obsolete: true

(In reply to Landry Breuil (:gaston) from comment #36)

(In reply to jcs from comment #35)

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #31)

If this is about navigating to file:/// URLs, there's a dedicated content process for those (see FILE_REMOTE_TYPE); with the existing sandbox implementations that process gets global file read access and others don't.

Without the 'r' for ~/Downloads, the tab just shows an error page saying it can't read the file.

Iirc some release ago, i tried triggerring the dedicated content process for file:// urls and failed to have anything clearly separated out from the other content processes, but maybe i tried wrong.

Fwiw, outside of this unveil work, just tried in 71.0b5, navigating to a file:/// url in a new tab indeed creates a new distinct process, and about:support shows a 'local file : 1' line in 'remote processes' section, so pledge/unveil can/should be applied with distinct params (ie full filesystem access but sensitive dirs/files ? or restricted to usr/local/share where most html/doc files live ? what about opening html files in ~ ?) to those FILE_REMOTE_TYPE processes.

If i open another file:// url in a new tab, it seems the same process is reused.

Hmm..i think i'll need to retract what i just said, apparently FILE_REMOTE_TYPE is not a value of the GeckoProcessType enum in https://searchfox.org/mozilla-central/source/xpcom/build/GeckoProcessTypes.h#25 so we have no way to distinguish a 'file://' tab process from a regular tab process.. unless we use a similar construct to https://searchfox.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1696, but we have no access to the ContentChild object in StartOpenBSDSandbox().

ExpandUnveilPath() takes care of expanding potentially
environment-specific XDG_DATA/CONFIG/CACHE_HOME dirs.

The unveil config files lists the allowed paths & modes.

'disable' in the files will disable the corresponding pledge/unveil
syscall.

Depends on D51386

g_app_info_get_default_for_type() will fail on OpenBSD's veiled
filesystem since we most likely don't have direct access to the binaries
that are registered as defaults for this type. Fake it up by just
executing xdg-open via gio-launch-desktop (which we do have access to)
and letting it figure out which program to execute for this MIME type.

This has the side-effect of ignoring/losing MIME types associations
registered in firefox prefs only.

Depends on D51387

Attached patch ff-unveil9.diff (deleted) — Splinter Review

New patch relative to #1580268

Attachment #9104537 - Attachment is obsolete: true

At that point, i think it's simpler to move the patches to phabricator as i started doing, that's the required (painful?) process for proper review/upstreaming.. and im pretty sure several ppl can work incrementally on the same patchset, creating independent revisions/diffs on phabricator. I still need to get used to it :)

I tried keeping authorship in the hg commits, but somehow bugzilla/phabricator seem to show me as author ? weird, at least that's not the case on the commits.

One thing i noted - to disable unveil, one needs to put 'disable ' (with a trailing space) otherwise it wont start up and complain about firefox: /etc/firefox/unveil.main: line 1: invalid format.

I need to figure out what paths i need to unveil now that i have a trunk build with the complete patch stack..

And with /home/landry/firefox r in unveil.main & unveil.content + /home/landry/firefox/firefox rx in unveil.main, my trunk build untarred in ~/firefox runs fine, yay.

This should fix the disable problem.  I'm not going to keep uploading new patches since you're working on these somewhere else.

diff --git dom/ipc/ContentChild.cpp dom/ipc/ContentChild.cpp
index e42c4453b5..91bd60fa64 100644
--- dom/ipc/ContentChild.cpp
+++ dom/ipc/ContentChild.cpp
@@ -4223,17 +4223,17 @@ OpenBSDUnveilPaths(const nsACString& uPath, const nsACString& pledgePath) {
       continue;
     }
 
+    if (linenum == 1 && line.EqualsLiteral("disable")) {
+      disabled = true;
+      break;
+    }
+
     int32_t space = line.FindChar(' ');
     if (space <= 0) {
       errx(1, "%s: line %d: invalid format", PromiseFlatCString(uPath).get(),
         linenum);
     }
 
-    if (linenum == 1 && line.EqualsLiteral("disable")) {
-      disabled = true;
-      break;
-    }
-
     nsAutoCString uPath(Substring(line, 0, space));
     ExpandUnveilPath(uPath);

Fwiw, i think $XDG_CACHE_HOME/mozilla/ (or only the firefox subdir of that dir ?) is also needed as i got crashes on

console.error: services.settings: 
  Message: Unix error 2 during operation makeDir on file /home/landry/.cache/mozilla/firefox/qu6wv3d0.empty/settings (No such file or directory)

/usr/share/zoneinfo r might also be needed for the main process, as icu seems to use it (and we link with it), ktrace shows that the dir is accessed, https://searchfox.org/mozilla-central/source/js/src/vm/DateTime.cpp points at it, and i dunno what we gain (or what we break) at preventing access to the system timezones - /etc/localtime is also accessed, but its not needed to unveil() it as it's bypassed.

Apparently the macos sandbox gives access to it too: https://searchfox.org/mozilla-central/source/security/sandbox/mac/SandboxPolicyUtility.h#57

This can be tested by changing nsExternalHelperAppService::GetSingleton() to use nsOSHelperAppServiceChild for child processes like it does for Mac on other platforms.

I will do this in bug 1594679 to see if we can avoid the xdg-open hack here.

Pushed by gpascutto@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/faf2b623b315 enhance sandbox on OpenBSD with unveil() r=gcp https://hg.mozilla.org/integration/autoland/rev/0e0f33fd72b8 defer to xdg-open when opening files on OpenBSD r=gcp
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Regressions: 1596546
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: