Closed Bug 1457092 Opened 2 years ago Closed Last year

Implement sandboxing on OpenBSD with pledge()

Categories

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

Unspecified
OpenBSD
enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: gaston, Assigned: gaston)

References

(Depends on 1 open bug)

Details

(Whiteboard: sb+)

Attachments

(5 files, 8 obsolete files)

2.47 KB, patch
gcp
: review+
Details | Diff | Splinter Review
2.62 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.13 KB, patch
glandium
: review+
gaston
: feedback?
jbeich
martin
: feedback+
Details | Diff | Splinter Review
1.58 KB, patch
gcp
: review+
Details | Diff | Splinter Review
4.95 KB, patch
Details | Diff | Splinter Review
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..)
Assignee: nobody → landry
>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...
>'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).
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.
Priority: -- → P1
Whiteboard: sb+
(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..
Attachment #8984574 - Flags: review?(gpascutto)
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 ?
Flags: needinfo?(jld)
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?
Attachment #8984574 - Flags: review?(gpascutto) → review+
(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...
Depends on: 1467982
Attached patch Add pledge() syscalls (obsolete) — Splinter Review
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...
Attachment #8984656 - Flags: feedback?(gpascutto)
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!
Attachment #8984657 - Flags: feedback?(gpascutto)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1151c924c029
Only include SandboxBroker.h header on Linux sandbox. r=gcp
Keywords: checkin-needed
>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.
Flags: needinfo?(rjesup)
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.
Flags: needinfo?(jld)
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...)
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,
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.
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
(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.”
(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 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
Attachment #8984656 - Flags: feedback?(gpascutto)
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.
Attachment #8984657 - Flags: feedback?(gpascutto)
(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.
(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.
(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...
>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.
Attached patch Add pledge() syscalls (obsolete) — Splinter Review
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 ...
Attachment #8984656 - Attachment is obsolete: true
Attachment #8997714 - Flags: review?(gpascutto)
Same as previous, convert to use MOZ_LOG. Testing pending...
Attachment #8984657 - Attachment is obsolete: true
Attachment #8997715 - Flags: review?(gpascutto)
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 ?
Attached patch Add pledge() syscalls (obsolete) — Splinter Review
This one just adds .get() to the three MOZ_LOG calls so that strings are properly accessed. Should build this time :)
Attachment #8997714 - Attachment is obsolete: true
Attachment #8997714 - Flags: review?(gpascutto)
Attachment #8997734 - Flags: review?(gpascutto)
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);
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.
Flags: needinfo?(gpascutto)
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...
Doh. found it, namespace clashes...
If you get something working again, feel free to update the patch. I have a day or two now to help you with this.
Flags: needinfo?(gpascutto)
Trivial one, unbitrotted
Attachment #8984574 - Attachment is obsolete: true
Attachment #9003112 - Flags: review?(gpascutto)
Updated to remove getpid() calls, as the pid is provided by MOZ_LOG.
Attachment #8997715 - Attachment is obsolete: true
Attachment #8997715 - Flags: review?(gpascutto)
Attachment #9003113 - Flags: review?(gpascutto)
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
Attachment #8997734 - Attachment is obsolete: true
Attachment #8997734 - Flags: review?(gpascutto)
Attachment #9003114 - Flags: review?(gpascutto)
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 ?
Attachment #9003116 - Flags: review?(gpascutto)
Attachment #9003112 - Flags: review?(gpascutto) → review+
Attachment #9003113 - Flags: review?(gpascutto) → review?(nfroyd)
Attachment #9003114 - Attachment is patch: true
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.
Attachment #9003114 - Flags: review?(jld)
Attachment #9003114 - Flags: review?(gpascutto)
Attachment #9003114 - Flags: review+
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?
Attachment #9003113 - Flags: review?(nfroyd) → review+
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.
Attachment #9003116 - Flags: review?(gpascutto) → review-
(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 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”
Attachment #9003114 - Flags: review?(jld) → review+
(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 ?
Lets define those XP_ macros for the bsds once and for all..
Attachment #9003428 - Flags: review?(ted)
Attachment #9003428 - Flags: review?(mh+mozilla)
Attachment #9003428 - Flags: feedback?(martin)
Attachment #9003428 - Flags: feedback?(jbeich)
(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...
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'
This one depends on attachment 9003428 [details] [diff] [review], updated to use XP_OPENBSD
Attachment #9003116 - Attachment is obsolete: true
Attachment #9003472 - Flags: review?(gpascutto)
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
Attachment #9003114 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Attachment #9003472 - Flags: review?(gpascutto) → review+
Attachment #9003428 - Flags: review?(ted)
Attachment #9003428 - Flags: review?(mh+mozilla)
Attachment #9003428 - Flags: review+
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...
Keywords: leave-open
Pushed by ebalazs@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5ba529cbce59
https://hg.mozilla.org/mozilla-central/rev/152b2a1144ae
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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..
Attachment #9003428 - Flags: approval-mozilla-beta?
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..
Attachment #9003472 - Flags: approval-mozilla-beta?
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.
Attachment #9003428 - Flags: approval-mozilla-beta?
Attachment #9003472 - Flags: approval-mozilla-beta?
(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
Flags: needinfo?(rjesup)
Attachment #9003428 - Flags: feedback?(martin) → feedback+
See Also: → 1580268
See Also: → 1580271
You need to log in before you can comment on or make changes to this bug.