1457092 - Implement sandboxing on OpenBSD with pledge()

Status: RESOLVED FIXED

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

Description

[Landry Breuil (:gaston)]

I'm working on implementing process 'sandboxing' on OpenBSD with pledge (cf http://man.openbsd.org/pledge), currently this is a bit rough but sort of works, and of course i have lots of questions:

- pledge is currently only available on OpenBSD, but might (?) be adopted by other oses at some point. Right now i'm enclosing the code within #ifdef __OpenBSD__, but on the long term maybe the configure goo should check for the syscall presence and use it when available
- the idea behind pledge is 'the process itself says what syscall groups it actually needs, and trying to use anything else will get it killed by the kernel'. There's no support for filesystem paths whitelists yet but this is being worked on.
- the current sandboxing infrastructure in mozilla for tier1 platforms is a bit complicated, but i tried to integrate within it the best i could, passing --enable-sandbox --enable-content-sandbox to configure - but since we share code with linux, in some areas (like gfx/thebes/gfxFcPlatformFontList.{h,cpp}) i had to disable inclusion of mozilla/SandboxBroker.h:

-#ifdef MOZ_CONTENT_SANDBOX
+#if defined(MOZ_CONTENT_SANDBOX) && !defined(__OpenBSD__)
 #include "mozilla/SandboxBroker.h"
 #endif

how acceptable would this be ? I didnt need to add anything openbsd-related to security/sandbox/* yet, and would like to avoid it unless really needed.

I added pledge() calls in two areas, one for the main process in https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4426 and one for the content process in https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#1715 - are those the right places ?

+#elif defined(__OpenBSD__)
+  sandboxCapable = true;
+  nsAutoCString promisesString;
+  Preferences::GetCString("security.sandbox.pledge.main",
+                          promisesString);
+  if (pledge(promisesString.get(), NULL) == -1) {
+    if (errno == EINVAL)
+        printf_stderr("pledge promises for main process is a malformed string: '%s'\n", promisesString.get());
+    if (errno == EPERM)
+        printf_stderr("pledge promises for main process cant elevate priviledges: '%s'\n", promisesString.get());
+  } else
+      printf_stderr("pledged main process (pid=%d) with promises: '%s'\n", getpid(), promisesString.get());
 #endif


On top of this, locally for debugging im using a pref for the pledge strings so that its simpler to change the amount of promises 'on the fly' - i dunno how commitable would that be, but i use this in a browser/defaults/preferences/all-openbsd.js file:

// enable pledging the content process
pref("security.sandbox.content.level", 1);
pref("security.sandbox.pledge.main","stdio rpath wpath cpath inet proc exec prot_exec flock ps sendfd recvfd dns vminfo tty");
pref("security.sandbox.pledge.content","stdio rpath wpath cpath inet sendfd prot_exec unix");

And finally, even if i pledge(tty) some sysctls are denied, mostly KERN_PROC_PID which is being called in 3 places (some ppl would say the codebase dont need the info) so i temporarly neutered them:
- https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsMemoryReporterManager.cpp#221
- https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsDebugImpl.cpp#179
- https://dxr.mozilla.org/mozilla-central/source/mozglue/misc/TimeStamp_posix.cpp#311

Those are neutered like this:

+#if defined(__OpenBSD__) && defined(MOZ_SANDBOX)
+  printf_stderr("%s called when pledged, noop\n", __func__);
+  return 0;
+#endif

This is of course a bit awkward, are there other occurences of non-critical code being disabled like this when running sandboxed in the tree ?

All this is WIP and in the debugging phase, so i'm all ears for suggestions/directions on how to properly integrate this. I've not yet handled the case for device access (ie audio/webcam/webrtc..)

Comment 1

[Gian-Carlo Pascutto [:gcp]]

>but since we share code with linux, in some areas (like gfx/thebes/gfxFcPlatformFontList.{h,cpp}) i had to disable inclusion of mozilla/SandboxBroker.h:

The use of the Sandbox there is so our fonts code knows which fonts won't be accessible in the content process. This was an issue on some Linux distributions that installed some TeX fonts in a weird place. I think OpenBSD is probably a bit more unified in that regard...

Comment 2

[Gian-Carlo Pascutto [:gcp]]

>'ve not yet handled the case for device access (ie audio/webcam/webrtc..)

Webcam access is remoted through the parent, so that should just work. Audio relies on an IPC layer in Rust that remotes but must be enabled in prefs (which we do by default on Linux, because else PulseAudio is a PITA).

Comment 3

[Jan Beich]

audioipc is currently built[1] only on Linux and Darwin. On OpenBSD you may need a patch from bug 1439197.

https://searchfox.org/mozilla-central/rev/78dbe34925f0/toolkit/library/rust/gkrust-features.mozbuild#23
https://searchfox.org/mozilla-central/rev/78dbe34925f0/dom/media/CubebUtils.cpp#49

Comment 4

[Landry Breuil (:gaston)]

I dunno which process does it, but as of now i can play a youtube video and have sound through sndio cubeb backend, even with pledge() enabled.

Comment 5

[Landry Breuil (:gaston)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> >'ve not yet handled the case for device access (ie audio/webcam/webrtc..)
> 
> Webcam access is remoted through the parent, so that should just work. 

The issue here is that even if it's done in the parent/main process the webcam device is accessed very late (afaict, only before prompting the user with the doorhanger) in the process lifetime, and there's no 'video' pledge subset yet to allow access to the device and the corresponding ioctls. Putting the webcam initialization code earlier in the process is awkward, since why should you be accessing it if you dont plan to use it.. 

Are there plans to remote the webcam access in its own process ? Saw nothing related in https://wiki.mozilla.org/Security/Sandbox

> Audio relies on an IPC layer in Rust that remotes but must be enabled in prefs
> (which we do by default on Linux, because else PulseAudio is a PITA).

I thought i needed nothing for audio sandboxing, since cubeb talks to sndiod over unix socket, but when sndiod is not running, libsndio tries to do ioctls on the audio device, so i'm pondering if i should add the 'audio' pledge subset.
On the other hand, i should 'soon' be able to enable cubeb remoting on OpenBSD, it might work ootb.

So far, the other takeaways from a month of using pledge and having users crash it in various environments:
- something in the password prompts focusing triggers dbus-launch in the content process, cf bug #1466593
- i've had to disable/neuter shmget calls in widget/nsShmImage.cpp, since they are called several times in the content process lifetime (and no pledge class allows for shm stuff), disabling it seems to have a certain impact of gfx performance. That one is puzzling, i got a bit lost in all the codepaths calling into XSHM (widget/gtk/WindowSurfaceProvider.cpp ?), but ideally there should only be one shm segment creation at process startup before the sandbox is initialized. How is this handled in the linux sandboxing ? Should we be using XSHM by default, or rather Xrender ?

Will try to start pushing some easy patches..

Comment 6

[Landry Breuil (:gaston)]

Attachment: only include SandboxBroker.h on linux sandbox

Comment 7

[Landry Breuil (:gaston)]

About that shmget bits that seems random, i'm not sure if i'm hitting bug #1438401 (which would be the system cairo via gtk) but that doesnt match what i've been seeing (ie direct calls to nsShmImage) and its fix from https://hg.mozilla.org/mozilla-central/rev/936b73ae6e3c isnt possible with pledge, we're not wrapping syscalls.
Jed, since you worked on it, any idea ?

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8984574 [details] [diff] [review]
only include SandboxBroker.h on linux sandbox

Review of attachment 8984574 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM to unblock you. But perhaps configure should not set MOZ_CONTENT_SANDBOX by default on platforms where it's not supported?

Comment 9

[Landry Breuil (:gaston)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #8)
> Comment on attachment 8984574 [details] [diff] [review]
> only include SandboxBroker.h on linux sandbox
> 
> Review of attachment 8984574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM to unblock you. But perhaps configure should not set
> MOZ_CONTENT_SANDBOX by default on platforms where it's not supported?

It isn't, i'm explicitely passing --enable-sandbox --enable-content-sandbox...

Comment 10

[Landry Breuil (:gaston)]

Attachment: Add pledge() syscalls

Not really putting for review yet but feedback on the code welcome. Of course, this needs an all.js diff to set the default pledge sets...

Comment 11

[Landry Breuil (:gaston)]

Attachment: Disable codepaths forbidden by pledge()

access to KERN_PROC_PID is forbidden when pledged, and well, accessing the info about the process being traced is optional. Same thing for GetKinfoProcSelf..

the one that annoys me more is the shmget one. It's done by the parent process, and i havent been able to figure out if one shm segment was created by child process creation, or if it's related to a repainting event, or... wonder how this works on linux.

Same thing, not putting for review but feedback welcome!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1151c924c029
Only include SandboxBroker.h header on Linux sandbox. r=gcp

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/1151c924c029

Comment 14

[Gian-Carlo Pascutto [:gcp]]

>Are there plans to remote the webcam access in its own process ?

This has been talked about, but mostly from a stability point of view. I know of no concrete implementation plans.

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

We didn't appear to be using nsShmImage in content processes on Linux as far as I could tell — when shmget was fatal on Nightly we had other problems, but not that.  A stack trace for the shmget might be useful, but I'd probably need help from someone who knows the graphics stack better to interpret it.

Comment 16

[Landry Breuil (:gaston)]

could the nsShmImage use be related to the fact we're using layers.enable-tiles on OpenBSD ? This was done in bug #1345899 because we were crashing otherwise (problem in our semaphore implementation...)

Comment 17

[Gian-Carlo Pascutto [:gcp]]

Maybe..maybe not. There's enough moving parts involved here that a stacktrace is probably much more helpful than any guessing we'd be doing,

Comment 18

[Landry Breuil (:gaston)]

gdb on OpenBSD isnt cooperative at all so i have a hard time stepping in the process, but it seems setting layers.acceleration.force-enabled=yes avoids most of the shmget calls(). With it defaulting to false, shmget() is called at first window paint and firefox crashes.

Comment 19

[Landry Breuil (:gaston)]

Here's the backtrace of the shmget() call at first window paint, when layers.acceleration.force-enabled defaults to false:

#0  shmget () at -:3
#1  0x00000dabdbb9e350 in CreateShmSegment () at /usr/obj/ports/firefox-61.0rc3-debug/firefox-61.0/widget/nsShmImage.cpp:78
#2  0x00000dabdbb9e5d7 in nsShmImage::CreateImage(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&) () from /usr/local/lib/firefox/libxul.so.78.0
#3  0x00000dabdbb9e733 in nsShmImage::CreateDrawTarget(mozilla::gfx::IntRegionTyped<mozilla::LayoutDevicePixel> const&) () from /usr/local/lib/firefox/libxul.so.78.0
#4  0x00000dabdbbdcc68 in mozilla::widget::WindowSurfaceProvider::StartRemoteDrawingInRegion(mozilla::gfx::IntRegionTyped<mozilla::LayoutDevicePixel>&, mozilla::layers::BufferMode*)
    () from /usr/local/lib/firefox/libxul.so.78.0
#5  0x00000dabda7c6478 in mozilla::layers::BasicCompositor::BeginFrame(mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits>*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits>*) () at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34
#6  0x00000dabda816e19 in mozilla::layers::LayerManagerComposite::Render(mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&) () at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34
#7  0x00000dabda8168bc in mozilla::layers::LayerManagerComposite::UpdateAndRender() () at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34
#8  0x00000dabda8161ef in mozilla::layers::LayerManagerComposite::EndTransaction(mozilla::TimeStamp const&, mozilla::layers::LayerManager::EndTransactionFlags) ()
    at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34
#9  0x00000dabda82d808 in mozilla::layers::CompositorBridgeParent::CompositeToTarget(mozilla::gfx::DrawTarget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) ()
    at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34
#10 0x00000dabda834b73 in mozilla::layers::CompositorVsyncScheduler::Composite(mozilla::TimeStamp) ()
    at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34
#11 0x00000dabda8467bb in mozilla::detail::RunnableMethodImpl<mozilla::layers::CompositorVsyncScheduler*, void (mozilla::layers::CompositorVsyncScheduler::*)(mozilla::TimeStamp), true, (mozilla::RunnableKind)1, mozilla::TimeStamp>::Run() () at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34
#12 0x00000dabda176bc8 in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&) ()
    at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Landry Breuil (:gaston) from comment #19)
> #9  0x00000dabda82d808 in mozilla::layers::CompositorBridgeParent::CompositeToTarget(mozilla::gfx::DrawTarget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) ()
>     at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34
> #10 0x00000dabda834b73 in mozilla::layers::CompositorVsyncScheduler::Composite(mozilla::TimeStamp) ()
>     at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34

This looks like main process code — PCompositorBridge.ipdl says, “the CompositorBridgeParent lives in the GPU process (if there is one) or the UI process otherwise.”

Comment 21

[Landry Breuil (:gaston)]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #20)
> (In reply to Landry Breuil (:gaston) from comment #19)
> > #9  0x00000dabda82d808 in mozilla::layers::CompositorBridgeParent::CompositeToTarget(mozilla::gfx::DrawTarget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) ()
> >     at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34
> > #10 0x00000dabda834b73 in mozilla::layers::CompositorVsyncScheduler::Composite(mozilla::TimeStamp) ()
> >     at /usr/obj/ports/firefox-61.0rc3-debug/build-amd64-debug/dist/include/nsPrintfCString.h:34
> 
> This looks like main process code — PCompositorBridge.ipdl says, “the
> CompositorBridgeParent lives in the GPU process (if there is one) or the UI
> process otherwise.”

What would be needed to enable the GPU process by default on OpenBSD, and is it a good idea to do so ?

Using layers.acceleration.force-enabled=true disables completely those 'compositor' codepaths ?

Comment 22

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8984656 [details] [diff] [review]
Add pledge() syscalls

Review of attachment 8984656 [details] [diff] [review]:
-----------------------------------------------------------------

This could probably be cleaned up a little.

::: dom/ipc/ContentChild.cpp
@@ +1745,5 @@
>    mozilla::SandboxTarget::Instance()->StartSandbox();
>  #elif defined(XP_MACOSX)
>    sandboxEnabled = StartMacOSContentSandbox();
> +#elif defined(__OpenBSD__)
> +  nsAutoCString promisesString;

Factor into an StartOpenBSDContentSandbox()?

@@ +1748,5 @@
> +#elif defined(__OpenBSD__)
> +  nsAutoCString promisesString;
> +  Preferences::GetCString("security.sandbox.pledge.content",
> +                          promisesString);
> +  if (pledge(promisesString.get(), NULL) == -1) {

Do you want to handle the case where the pref isn't set specifically?

@@ +1755,5 @@
> +    if (errno == EPERM)
> +        printf_stderr("pledge promises for content process cant elevate priviledges: '%s'\n", promisesString.get());
> +  } else {
> +    if (getenv("MOZ_SANDBOX_LOGGING")) {
> +      printf_stderr("(pid=%d) pledged content process with promises: '%s'\n", getpid(), promisesString.get());

This should probably use macros and our standard logging like SANDBOX_LOG_ERROR(). You can use that even if it's informational, despite the name. There's an open bug for us to fix things so there would be SANDBOX_LOG_INFO() etc.

::: toolkit/xre/nsAppRunner.cpp
@@ +4452,5 @@
>    sandboxCapable = true;
>  #elif defined(XP_LINUX)
>    sandboxCapable = SandboxInfo::Get().CanSandboxContent();
> +#elif defined(__OpenBSD__)
> +  sandboxCapable = true;

StartOpenBSDContentSandbox("security.sandbox.pledge.main") perhaps? Or perhaps StartOpenBSDContentSandbox(GeckoProcessType_Content)

https://searchfox.org/mozilla-central/source/xpcom/build/nsXULAppAPI.h#365

Comment 23

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8984657 [details] [diff] [review]
Disable codepaths forbidden by pledge()

Review of attachment 8984657 [details] [diff] [review]:
-----------------------------------------------------------------

Same remark as in the other patch, i.e. better to use standard logging macros than just dumping everything on stderr.

Comment 24

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #22)
> > +      printf_stderr("(pid=%d) pledged content process with promises: '%s'\n", getpid(), promisesString.get());
> 
> This should probably use macros and our standard logging like
> SANDBOX_LOG_ERROR(). You can use that even if it's informational, despite
> the name. There's an open bug for us to fix things so there would be
> SANDBOX_LOG_INFO() etc.

The most “standard” logging here would be MOZ_LOG, I think.  SANDBOX_LOG_ERROR from the Linux sandbox depends on code from security/sandbox/chromium, and the main reason it exists is because it can be called in async signal context and normal logging facilities tend to use malloc, locales, etc.; that's not an issue here.

Comment 25

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Landry Breuil (:gaston) from comment #21)
> What would be needed to enable the GPU process by default on OpenBSD, and is
> it a good idea to do so ?

We're not even using the GPU process on Linux yet, but I don't know the details; someone in #gfx should know more.

> Using layers.acceleration.force-enabled=true disables completely those
> 'compositor' codepaths ?

My understanding is that this selects a different compositor implementation that uses OpenGL instead of compositing in software and sending the result to the X server, so it would be creating GL textures instead of X images.

There's a reason why accelerated compositing isn't enabled by default on X11 platforms, but I don't know what it is.

Comment 26

[Landry Breuil (:gaston)]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #24)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #22)
> > > +      printf_stderr("(pid=%d) pledged content process with promises: '%s'\n", getpid(), promisesString.get());
> > 
> > This should probably use macros and our standard logging like
> > SANDBOX_LOG_ERROR(). You can use that even if it's informational, despite
> > the name. There's an open bug for us to fix things so there would be
> > SANDBOX_LOG_INFO() etc.
> 
> The most “standard” logging here would be MOZ_LOG, I think. 
> SANDBOX_LOG_ERROR from the Linux sandbox depends on code from
> security/sandbox/chromium, and the main reason it exists is because it can
> be called in async signal context and normal logging facilities tend to use
> malloc, locales, etc.; that's not an issue here.

It seems windows sandboxing does its own macros on top in chromium-shim/sandbox/win/loggingCallbacks.h:

static LazyLogModule sSandboxTargetLog("SandboxTarget");

#define LOG_D(...) MOZ_LOG(sSandboxTargetLog, LogLevel::Debug, (__VA_ARGS__))

then also directly uses an ostringstream, and its initialization does check that logging is enabled via env or the pref:

  if (Preferences::GetBool("security.sandbox.logging.enabled") ||
      PR_GetEnv("MOZ_SANDBOX_LOGGING")) 

I'm fine with using whatever is standard in m-c, but i suppose if using MOZ_LOG("sandboxPledge") or whatever other identifier i should set another variable in the env to enable this logging class ?

The 'boolean' MOZ_SANDBOX_LOGGING/security.sandbox.logging.enabled suits me well, but i'm not really fan of adding layers of macros wrapping macros..

(In reply to Gian-Carlo Pascutto [:gcp] from comment #22)
> Comment on attachment 8984656 [details] [diff] [review]
> Add pledge() syscalls
> 
> Review of attachment 8984656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This could probably be cleaned up a little.
> 
> ::: dom/ipc/ContentChild.cpp
> @@ +1745,5 @@
> >    mozilla::SandboxTarget::Instance()->StartSandbox();
> >  #elif defined(XP_MACOSX)
> >    sandboxEnabled = StartMacOSContentSandbox();
> > +#elif defined(__OpenBSD__)
> > +  nsAutoCString promisesString;
> 
> Factor into an StartOpenBSDContentSandbox()?

Sure, i can reorganize the code the calls. I just wanted to avoid creating a new subdir/file under security/sandbox (and dealing with the build system goop), because we know we wont need dozens of functions for OpenBSD, but if it's preferred...

> 
> @@ +1748,5 @@
> > +#elif defined(__OpenBSD__)
> > +  nsAutoCString promisesString;
> > +  Preferences::GetCString("security.sandbox.pledge.content",
> > +                          promisesString);
> > +  if (pledge(promisesString.get(), NULL) == -1) {
> 
> Do you want to handle the case where the pref isn't set specifically?

I was planning to set the default value in init/all.js when all the building bits are in.

> ::: toolkit/xre/nsAppRunner.cpp
> @@ +4452,5 @@
> >    sandboxCapable = true;
> >  #elif defined(XP_LINUX)
> >    sandboxCapable = SandboxInfo::Get().CanSandboxContent();
> > +#elif defined(__OpenBSD__)
> > +  sandboxCapable = true;
> 
> StartOpenBSDContentSandbox("security.sandbox.pledge.main") perhaps? Or
> perhaps StartOpenBSDContentSandbox(GeckoProcessType_Content)
> 
> https://searchfox.org/mozilla-central/source/xpcom/build/nsXULAppAPI.h#365

Sure, and while here is it okay to set the 'main' process pledge directly in AddSandboxAnnotations() or there's a better spot to 'start sandboxing' on the main process ? I see other platforms doing some sandbox init in XREMain::XRE_mainInit or XREMain::XRE_mainRun...

Comment 27

[Gian-Carlo Pascutto [:gcp]]

>i should set another variable in the env to enable this logging class

Yes, MOZ_LOG controls which modules log.

>we know we wont need dozens of functions for OpenBSD, but if it's preferred..

I don't think you need a separate file, IIRC the other platforms are in the same file. Just a separate function.

>Sure, and while here is it okay to set the 'main' process pledge directly in AddSandboxAnnotations() or there's a better spot to 'start sandboxing' on the main process?

This looked fine to me.

Comment 28

[Landry Breuil (:gaston)]

Attachment: Add pledge() syscalls

Refactored to address review comments:
* declare a single StartOpenBSDSandbox() function, use a switch to get the correct value/process type
* convert to use MOZ_LOG, creating a new 'SandboxPledge' logger
* call StartOpenBSDSandbox() where appropriate with the correct GeckoProcessType

should be functionally equivalent. Builds fine, tests to be done. I'm not sure about the processTypeString assignation ...

Comment 29

[Landry Breuil (:gaston)]

Attachment: Disable codepaths forbidden by pledge()

Same as previous, convert to use MOZ_LOG. Testing pending...

Comment 30

[Landry Breuil (:gaston)]

Hmm i though i had it build-tested but it seems not, as it fails to pass a nsautocstring to a macro.

error: cannot pass non-trivial object of type 'nsAutoCString'                
      (aka 'nsTAutoStringN<char, 64>') to variadic function; expected type from format string was 'char *'

i would have expected the macro to deal with it but i gues i'll have to add .get() to those strings, unless there's a simpler way to handle those strings across the tree and the macros ?

Comment 31

[Landry Breuil (:gaston)]

Attachment: Add pledge() syscalls

This one just adds .get() to the three MOZ_LOG calls so that strings are properly accessed. Should build this time :)

Comment 32

[Landry Breuil (:gaston)]

Bah. I thought i had run a full mach build but it turns out mach lied to me and didnt build all my changes. This doesnt build at all.. as nsAppRunner.cpp doesnt know StartOpenBSDSandbox which isnt exported.

error: use of undeclared identifier 'StartOpenBSDSandbox'                  
  StartOpenBSDSandbox(GeckoProcessType_Default);

Comment 33

[Landry Breuil (:gaston)]

gcp: where should i move the common code shared by both codepaths ? That's why i didnt want to refactor all this... now it feels i need a new file in security/sandbox/openbsd; unless i can declare this function in a header common to nsAppRunner.cpp & ContentChild.cpp... but it seems only SandboxSettings.h is common.

Comment 34

[Landry Breuil (:gaston)]

If i declare StartOpenBSDSandbox() in SandboxSettings.h like this, insie the mozilla namespace block:

#if defined(__OpenBSD__)
bool StartOpenBSDSandbox(GeckoProcessType type);
#endif

With its implementation in dom/ipc/ContentChild.cpp per Attachment #8997734 [details] [diff] , without the 'static' keyword, then libxul.so doesnt link:


73:11.40 /usr/bin/ld.lld: error: undefined symbol: mozilla::StartOpenBSDSandbox(GeckoProcessType)                                         
73:11.40 >>> referenced by nsAppRunner.cpp
73:11.40 >>>               ../xre/nsAppRunner.o:(AddSandboxAnnotations())
73:11.40 /usr/bin/ld.lld: error: undefined symbol: mozilla::StartOpenBSDSandbox(GeckoProcessType)                                         
73:11.40 >>> referenced by nsAppRunner.cpp
73:11.40 >>>               ../xre/nsAppRunner.o:(XREMain::XRE_mainRun())

I think i also tried putting the implementation in SandboxSettings.cpp but still that wouldnt link. It feels like i'm missing something obvious...

Comment 35

[Landry Breuil (:gaston)]

Doh. found it, namespace clashes...

Comment 36

[Gian-Carlo Pascutto [:gcp]]

If you get something working again, feel free to update the patch. I have a day or two now to help you with this.

Comment 37

[Landry Breuil (:gaston)]

Attachment: only include SandboxBroker.h on linux sandbox

Trivial one, unbitrotted

Comment 38

[Landry Breuil (:gaston)]

Attachment: Disable codepaths forbidden by pledge()

Updated to remove getpid() calls, as the pid is provided by MOZ_LOG.

Comment 39

[Landry Breuil (:gaston)]

Attachment: Add startOpenBSDSandbox and call it where appropriate

Differences from previous patch:

* declare StartOpenBSDSandbox in common/SandboxSettings.h
* move the implementation in the mozilla namespace, instead of mozilla::dom - that's what was causing the linking issues
* remove getpid() calls as MOZ_LOG provides the pid

Comment 40

[Landry Breuil (:gaston)]

Attachment: Set defaults for openbsd sandboxing params

This one doesnt work for some reason, the default prefs values arent set for OpenBSD. At first i would have put them in modules/libpref/init/all.js but then saw that the defaults for sandboxing are set in browser/app/profile/firefox.js...

i guess OS_OPENBSD isnt defined when firefox.js is processed..but strangely that is the case when modules/libpref/init/all.js is processed. Any idea ?

Comment 41

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 9003114 [details] [diff] [review]
Add startOpenBSDSandbox and call it where appropriate

Review of attachment 9003114 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, adding Jed who's an IPC peer (even though dom/ipc may not belong to that, technically...)

::: dom/ipc/ContentChild.cpp
@@ +3923,5 @@
>  
>  } // namespace dom
>  
> +#if defined(__OpenBSD__) && defined(MOZ_CONTENT_SANDBOX)
> +#include <unistd.h>

I think this file typically has the includes near the top, protected by platform ifdefs as needed.

Comment 42

[Nathan Froyd [:froydnj]]

Comment on attachment 9003113 [details] [diff] [review]
Disable codepaths forbidden by pledge()

Review of attachment 9003113 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me, though I'm curious about the below.

::: widget/nsShmImage.cpp
@@ +70,5 @@
> +#if defined(__OpenBSD__) && defined(MOZ_SANDBOX)
> +  static mozilla::LazyLogModule sPledgeLog("SandboxPledge");
> +  MOZ_LOG(sPledgeLog, mozilla::LogLevel::Debug,
> +         ("%s called when pledged, returning false\n", __func__));
> +  return false;

How many things break as a result of doing this?

Alternatively, we may just want to change nsShmImage.h to completely undefine MOZ_HAVE_SHMIMAGE for sandboxed OpenBSD, and then perhaps other codepaths would work properly?

Comment 43

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 9003116 [details] [diff] [review]
Set defaults for openbsd sandboxing params

Review of attachment 9003116 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/profile/firefox.js
@@ +1081,5 @@
>  pref("security.sandbox.content.read_path_whitelist", "");
>  pref("security.sandbox.content.syscall_whitelist", "");
>  #endif
>  
> +#if defined(OS_OPENBSD) && defined(MOZ_SANDBOX)

Looks like we don't set OS_OPENBSD generally (just in some subtrees).

I think you want to modify this: https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/build/moz.configure/init.configure#952

Add add a similar logic to add an XP_OPENBSD flag, then test on that here.

Comment 44

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #43)
> Looks like we don't set OS_OPENBSD generally (just in some subtrees).

The OS_* defines are for the imported Chromium IPC code, as I understand it.  (In theory they could all replaced with XP_*, if someone wanted to write that patch.)

Comment 45

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 9003114 [details] [diff] [review]
Add startOpenBSDSandbox and call it where appropriate

Review of attachment 9003114 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentChild.cpp
@@ +3958,5 @@
> +               ("pledge promises for %s process is a malformed string: '%s'\n",
> +                processTypeString.get(), promisesString.get()));
> +    } else if (errno == EPERM) {
> +        MOZ_LOG(sPledgeLog, LogLevel::Error,
> +               ("pledge promises for %s process can't elevate priviledges: '%s'\n",

Typo nit: “privileges”

Comment 46

[Landry Breuil (:gaston)]

(In reply to Nathan Froyd [:froydnj] from comment #42)
> Comment on attachment 9003113 [details] [diff] [review]
> Disable codepaths forbidden by pledge()
> 
> Review of attachment 9003113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me, though I'm curious about the below.
> 
> ::: widget/nsShmImage.cpp
> @@ +70,5 @@
> > +#if defined(__OpenBSD__) && defined(MOZ_SANDBOX)
> > +  static mozilla::LazyLogModule sPledgeLog("SandboxPledge");
> > +  MOZ_LOG(sPledgeLog, mozilla::LogLevel::Debug,
> > +         ("%s called when pledged, returning false\n", __func__));
> > +  return false;
> 
> How many things break as a result of doing this?

Iirc, without shmimage gfx performance is terrible on most chipsets, and that is worked around by forcing acceleration on. Problem is shmget() and this family of syscalls arent covered (yet?) by a pledge keyword, so if you're pledged you *cant* use them. If there was such family, i could add it to the list of allowed syscalls and remove that kludge.. right now, there hasnt been a real incentive to add it in OpenBSD, as there are no real users of shm in base that would benefit from such class (ie being possibly sandboxed and using shm).

I dont remember if those nsShmImage codepaths are still used in majority, and if on the longterm non-accelerated codepaths will still be supported, i tried following the codepaths leading to it but got lost..

We also dont have shared semaphores so other codepaths for gfx acceleration are currently broken, cf Bug 1345899 and bug 1485371 - that's also why we force-enable tiling.

> Alternatively, we may just want to change nsShmImage.h to completely
> undefine MOZ_HAVE_SHMIMAGE for sandboxed OpenBSD, and then perhaps other
> codepaths would work properly?

Maybe that would be 'nicer' build-system wise, but more complicated to understand ? What other codepaths would that be ?

Comment 47

[Landry Breuil (:gaston)]

Attachment: Define XP_*BSD for the BSDs

Lets define those XP_ macros for the bsds once and for all..

Comment 48

[Nathan Froyd [:froydnj]]

(In reply to Landry Breuil (:gaston) from comment #46)
> (In reply to Nathan Froyd [:froydnj] from comment #42)
> > Alternatively, we may just want to change nsShmImage.h to completely
> > undefine MOZ_HAVE_SHMIMAGE for sandboxed OpenBSD, and then perhaps other
> > codepaths would work properly?
> 
> Maybe that would be 'nicer' build-system wise, but more complicated to
> understand ? What other codepaths would that be ?

Sorry, I was not very clear here.  I was assuming:

1. There is some other non-MOZ_HAVE_SHMIMAGE path;
2. The current patch turns on the MOZ_HAVE_SHMIMAGE path, but then effectively neuters it;
2a. (I don't know if we just fallback to #1, or we do something else)

and I'm wondering if we should just avoid turning on the MOZ_HAVE_SHMIMAGE path on sandboxed OpenBSD, because it's always going to fail, and then we can take path #1.  If the error case that OpenBSD now forces on is the same path that !MOZ_HAVE_SHMIMAGE would have taken, I guess it doesn't matter too much.  But turning off MOZ_HAVE_SHMIMAGE on sandboxed OpenBSD is just a bit of #ifdef magic in nsShmImage.h, so I don't think it's terribly complicated...

Comment 49

[Landry Breuil (:gaston)]

With the XP_xxx patch, and my stack of patches, i'm finally able to start nightly sandboxed with defaults settings coming from firefox.js, and logging works fine:

$MOZ_LOG=SandboxPledge:5 nightly
[(null) 22327: Main Thread]: D/SandboxPledge pledged main process with promises: 'stdio rpath wpath cpath inet proc exec prot_exec flock ps sendfd recvfd dns vminfo tty drm unix fattr getpw mcast'
[(null) 22327: Compositor]: D/SandboxPledge CreateShmSegment called when pledged, returning false
[Child 65210: Main Thread]: D/SandboxPledge pledged content process with promises: 'stdio rpath wpath cpath inet recvfd sendfd prot_exec unix drm ps'
[Child 63293: Main Thread]: D/SandboxPledge pledged content process with promises: 'stdio rpath wpath cpath inet recvfd sendfd prot_exec unix drm ps'
[Child 83204: Main Thread]: D/SandboxPledge pledged content process with promises: 'stdio rpath wpath cpath inet recvfd sendfd prot_exec unix drm ps'
[Parent 22327: Main Thread]: D/SandboxPledge GetKinfoProcSelf called when pledged, returning NS_ERROR_FAILURE                             
[Parent 22327: Main Thread]: D/SandboxPledge GetKinfoProcSelf called when pledged, returning NS_ERROR_FAILURE                             
[Parent 22327: Main Thread]: D/SandboxPledge GetKinfoProcSelf called when pledged, returning NS_ERROR_FAILURE                             
[Child 63293: Main Thread]: D/SandboxPledge GetKinfoProcSelf called when pledged, returning NS_ERROR_FAILURE                              
[Child 63293: Main Thread]: D/SandboxPledge GetKinfoProcSelf called when pledged, returning NS_ERROR_FAILURE                              
[Child 83204: Main Thread]: D/SandboxPledge GetKinfoProcSelf called when pledged, returning NS_ERROR_FAILURE                              
[Child 83204: Main Thread]: D/SandboxPledge GetKinfoProcSelf called when pledged, returning NS_ERROR_FAILURE                              
[Parent 22327: Main Thread]: D/SandboxPledge GetKinfoProcSelf called when pledged, returning NS_ERROR_FAILURE                             
[Parent 22327: Main Thread]: D/SandboxPledge GetKinfoProcSelf called when pledged, returning NS_ERROR_FAILURE                             
[Parent 22327: Main Thread]: D/SandboxPledge GetKinfoProcSelf called when pledged, returning NS_ERROR_FAILURE
[Parent 22327: Compositor]: D/SandboxPledge CreateShmSegment called when pledged, returning false
[Child 12086: Main Thread]: D/SandboxPledge pledged content process with promises: 'stdio rpath wpath cpath inet recvfd sendfd prot_exec unix drm ps'

Comment 50

[Landry Breuil (:gaston)]

Attachment: Set defaults for openbsd sandboxing params

This one depends on attachment 9003428 [details] [diff] [review], updated to use XP_OPENBSD

Comment 51

[Landry Breuil (:gaston)]

Attachment: Add startOpenBSDSandbox and call it where appropriate

Address review comments on attachment 9003114 [details] [diff] [review], move the unistd.h inclusion to the top of ContentChild.cpp, and fix typo on privileges. Carrying over r+ from jld and gcp

Comment 52

[Landry Breuil (:gaston)]

setting checkin-needed for attachment 9003474 [details] [diff] [review] (r+ jld & gcp), attachment 9003112 [details] [diff] [review] (r+ gcp) & attachment 9003113 [details] [diff] [review] (r+ froydnj)

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5c17ac83aa
Implement sandboxing on OpenBSD. r=gcp, r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/411427c1f5fe
Content sandbox codepaths are Linux only. r=gcp
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad8c54c6dc8
Disable codepaths forbidden by pledge() when being sandboxed on OpenBSD. r=froydnj

Comment 54

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8f5c17ac83aa
https://hg.mozilla.org/mozilla-central/rev/411427c1f5fe
https://hg.mozilla.org/mozilla-central/rev/3ad8c54c6dc8

Comment 55

[Landry Breuil (:gaston)]

Setting checking-needed for Attachment #9003428 [details] [diff] & Attachment #9003472 [details] [diff]

Comment 56

[Landry Breuil (:gaston)]

Remove leave-open as once those two last patches have landed, all the ones from this bug will be in. I'll probably ask for beta approval for them so that all bits are in 63 for this testing cycle...

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ba529cbce59
Define XP_*BSD for OpenBSD/NetBSD/FreeBSD. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/152b2a1144ae
set conservative default values for pledge() sandboxing on OpenBSD. r=gcp

Comment 58

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5ba529cbce59
https://hg.mozilla.org/mozilla-central/rev/152b2a1144ae

Comment 59

[Landry Breuil (:gaston)]

Comment on attachment 9003428 [details] [diff] [review]
Define XP_*BSD for the BSDs

Approval Request Comment
[Feature/Bug causing the regression]: none, new improvement
[User impact if declined]: failure to use sandboxing on OpenBSD
[Is this code covered by automated tests?]: nope
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Use sandboxing on OpenBSD
[List of other uplifts needed for the feature/fix]: other attachment on the same bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: Tier3
[String changes made/needed]: None

This way all patches are in 63..

Comment 60

[Landry Breuil (:gaston)]

Comment on attachment 9003472 [details] [diff] [review]
Set defaults for openbsd sandboxing params


Approval Request Comment
[Feature/Bug causing the regression]: none, new improvement
[User impact if declined]: failure to use sandboxing on OpenBSD
[Is this code covered by automated tests?]: nope
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Use sandboxing on OpenBSD
[List of other uplifts needed for the feature/fix]: other attachment on the same bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: Tier3
[String changes made/needed]: None

This way all patches are in 63..

Comment 61

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9003428 [details] [diff] [review]
Define XP_*BSD for the BSDs

63 is being kept in sync with Beta until the version number is bumped on m-c to 64 next week.

Comment 62

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #14)
> >Are there plans to remote the webcam access in its own process ?
> 
> This has been talked about, but mostly from a stability point of view. I
> know of no concrete implementation plans.

None at this moment.  It's possible this might occur as part of fission, but isn't required by fission