Closed Bug 1466593 Opened 6 years ago Closed 6 years ago

sandboxing prevents content process to spawn a session dbus

Categories

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

Product:
Core
Component:
Security: Process Sandboxing
Platform:
Unspecified
OpenBSD
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: gaston, Assigned: gaston)

References

(
URL
)

Details

Attachments

(1 file, 1 obsolete file)

Fake a dbus session if none is running when sandboxing the content process
1.37 KB, patch
gcp
: review+
pascalc
: approval-mozilla-beta+
Details | Diff | Splinter Review 
A bit orthogonal to bug #1457092 but opening a distinct bug as the issue is more generic - i'm wondering how this is handled in other sandboxing implementations.

On OpenBSD, with the wip policy i have to implement sandboxing with pledge(), the content processes arent allowed to spawn processes (they shouldnt need to, right ?) - Several users reported me violations of this policy (cf https://marc.info/?l=openbsd-misc&m=152643974704545&w=2 for example), in the following context:

- their X session startup doesnt start a session dbus-daemon for whatever reason (they're running a simple WM, they don't want dbus running, etc)
- they start firefox, pledge policy is applied at process startup
- at some point during their browsing session they focus a password entry field
- this somehow tries to communicate with their password manager (i guess so, as afaik native messaging doesnt really work on openbsd, i havent had time to test it properly, but keepass and its derivate seem popular in the userbase)
- and for some reason, deep inside glib, this ends up trying to spawn the missing session dbus-daemon instance. Boom.

#7  0x00000bce2ac9ec79 in g_spawn_command_line_sync (                                                                                                          
    command_line=<optimized out>, standard_output=0xbce96dac788,                                                                                               
    standard_error=0xbce96dac780, exit_status=0xbce96dac77c,                                                                                                   
    error=0xbce96dac7d8) at gspawn.c:780                                                                                                                       
#8  0x00000bce3edb69e3 in get_session_address_dbus_launch (error=0xbce96dac7d8)                                                                                
    at gdbusaddress.c:1127                                                                                                                                     
#9  0x00000bce3edb64f3 in get_session_address_platform_specific (error=0x0)                                                                                    
    at gdbusaddress.c:1555                                                                                                                                     
#10 g_dbus_address_get_for_bus_sync (bus_type=G_BUS_TYPE_SESSION,                                                                                              
    cancellable=<optimized out>, error=0xbce96dac928) at gdbusaddress.c:1635                                                                                   
#11 0x00000bce3edc3555 in get_uninitialized_connection (                                                                                                       
    bus_type=G_BUS_TYPE_SESSION, cancellable=0x0, error=0xbce96dac928)                                                                                         
    at gdbusconnection.c:7196                                                                                                                                  
#12 0x00000bce3edc34a2 in g_bus_get_sync (bus_type=G_BUS_TYPE_SESSION,                                                                                         
    cancellable=0x0, error=0xbce96dac928) at gdbusconnection.c:7275                                                                                            
#13 0x00000bce34b075ba in ?? ()                                                                                                                                
   from /usr/local/lib/gio/modules/libdconfsettings.so                                                                                                         
#14 0x00000bce34b0725c in ?? ()                                                                                                                                
   from /usr/local/lib/gio/modules/libdconfsettings.so                                                                                                         
#15 0x00000bce2ac51039 in g_main_dispatch (context=<optimized out>)                                                                                            
    at gmain.c:3177                                                                                                                                            
#16 g_main_context_dispatch (context=<optimized out>) at gmain.c:3830                                                                                          
#17 0x00000bce2ac51413 in g_main_context_iterate (context=<optimized out>,                                                                                     
    block=<optimized out>, dispatch=<optimized out>, self=<optimized out>)                                                                                     
    at gmain.c:3903                                                                                                                                            
#12 0x00000bce3edc34a2 in g_bus_get_sync (bus_type=G_BUS_TYPE_SESSION,                                                                                         
    cancellable=0x0, error=0xbce96dac928) at gdbusconnection.c:7275                                                                                            
#13 0x00000bce34b075ba in ?? ()                                                                                                                                
   from /usr/local/lib/gio/modules/libdconfsettings.so                                                                                                         
#14 0x00000bce34b0725c in ?? ()                                                                                                                                
   from /usr/local/lib/gio/modules/libdconfsettings.so                                                                                                         
#15 0x00000bce2ac51039 in g_main_dispatch (context=<optimized out>)                                                                                            
    at gmain.c:3177                                                                                                                                            
#16 g_main_context_dispatch (context=<optimized out>) at gmain.c:3830


I of course am relatively reluctant to allow content processes to spawn other processes in the sandbox policy - so far my workaround has been to document in the package doc (that nobody reads) that running a session dbus instance is a requirement.

So question, how is this handled in other sandboxing implems ? This was never tested, as everybody assumes your distro/os has all the goo to start a session dbus instance ?

Should firefox itself check for a running session dbus instance at startup (*before* the sandboxing policy is applied) and eventually start one ? I see there's already such somewhat similar handling for a11y, cf https://dxr.mozilla.org/mozilla-central/source/accessible/atk/Platform.cpp#241

Another solution that some advertise would be to wrap firefox binary into an horrible wrapper, doing the equivalent of '[ -z $DBUS_SESSION_BUS_ADDRESS ] && dbus-launch firefox $@ || firefox $@' ? I personally despise this.

Right now in the OpenBSD packaging dbus is enabled, but i don't think disabling dbus inside firefox would 'workaround' the issue, as the gio/gmime integration dbus brings doesnt seem to have anything to do with that particular issue.

Are content processes supposed to start processes at all, in the context of sandboxing on other platforms ? Shouldnt the particular case of password prompts (or generically native messaging) be delegated to the main process, or to a distinct process ?
I don't know things about dbus, but I can confirm that Firefox content processes should never need to spawn processes, and indeed are disallowed from doing so on the macOS/Linux/Windows sandboxes.
So what happens on linux if you run a minimalistic X session without dbus-daemon, start firefox and try to focus a password field/trigger the native messaging bits ? The missing link so far is "what inside password field focusing/handling triggers glib to communicate with dbus.."
See Also: → 1259508
It looks like we dealt with a similar problem on Linux with Bug 1259508 "Seccomp sandbox violation: sys_clone called in content process of Firefox desktop" by initializing cubeb (the audio backend) before enabling the sandbox. Bug 1270147 might be relevant too.
Do you have a possibility to make that fork just fail and see if there's noticeable fallout?

Passwords fields somehow triggering password managers are something we'll have to keep an eye on, I presume this is either intentionally from the code...somewhere(?)...or a side effect of using whatever appropriate GTK widget and it trying to be helpful (cough) for us. We're slowly removing widget use from content code so that feature is something that would then stop working in any case, at least in the way that seems to trigger this bug.
>Shouldnt the particular case of password prompts (or generically native messaging) be delegated to the main process

Generally speaking we're trying to get widget use out of the content process, so yes, but this is still ongoing work that won't be finished for a while. (And yes, this makes sandboxing a lot easier!)
This should be GtkEntry with GtkInputPurpose being set to GTK_INPUT_PURPOSE_PASSWORD. I'll have a look what the GTK actually does for that.
In theory, we're not using DBus in content processes (either before or after RecvSetProcessSandbox).  In practice, we never did find out what was going on in bug 1411629.
See Also: 1259508 → 1411629
Here i think the problem is located inside glib itself. We're now building dbus with --disable-x11-autolaunch but i'm not sure this fixes the actual issue.
It seems another option to neuter dbus autospawning is to check if DBUS_SESSION_BUS_ADDRESS is set in the env, and set it to an empty value if it isnt set. An option to test, and to consider ?
There seem to be huge problems on Ubuntu 18.04 with password fields too:
https://bugzilla.mozilla.org/show_bug.cgi?id=1451466
Reading through those bugs, that seems like an unrelated issue (focus bug in ibus).
After a bit more digging & debugging with other openbsd developers, i think the problem lies in glib, and maybe we can do an ugly workaround in firefox.

Building dbus with --disable-x11-autolaunch doesn seem to fix the issue, as glib wants to get the session bus address very hard, in https://gitlab.gnome.org/GNOME/glib/blob/master/gio/gdbusaddress.c#L1555.

There are lots of cases, and if the X session is not a "login session" (logind/consolekit/etc) and no bus address is found in the env, in the end it will try to launch one via dbus-launch (cf https://gitlab.gnome.org/GNOME/glib/blob/master/gio/gdbusaddress.c#1115) which is forbidden by the sandbox. Building dbus with --disable-x11-autolaunch cant work around that, i think it needs fixing in glib itself.

As for a workaround *for firefox sandboxing on OpenBSD*, i'm currently testing a patch that ensures DBUS_SESSION_BUS_ADDRESS is set when using sandboxing, ie the following lines right after calling pledge() in the content process.

if (!getenv("DBUS_SESSION_BUS_ADDRESS")) {
  printf_stderr("(pid=%d) no session dbus found, faking one\n", getpid());
}
/* dont overwrite an existing session dbus address, but ensure it is set */
setenv("DBUS_SESSION_BUS_ADDRESS","",0);

i havent tested on a linux distro to see if the issue also affect it (i think you need to go to extra length to disable the logind stuff), so i'm not sure this should be specific to OpenBSD sandboxing or common to linux sandboxing.
Setting G_DBUS_DEBUG=all in the env helps understanding things a bit better. The parent/main process tries to spawn a dbus session daemon (when/why, i dont know), but is denied by autolaunch being disabled, but the sandboxing doesnt prevent it as it has rights to spawn processes.

$env G_DBUS_DEBUG=all firefox 
GDBus-debug:Address: In g_dbus_address_get_for_bus_sync() for bus type 'session'
GDBus-debug:Address: env var DBUS_SESSION_BUS_ADDRESS is not set
GDBus-debug:Address: env var DBUS_SYSTEM_BUS_ADDRESS is not set
GDBus-debug:Address: env var DBUS_STARTER_BUS_TYPE is not set
GDBus-debug:Address: Running 'dbus-launch --autolaunch=0d2ffa10e59a7809134db8165a2c2f17 --binary-syntax --close-stderr' to get bus address (possibly autolaunching)
GDBus-debug:Address: dbus-launch output:
  0000: 00 00 00 00  00 00 00 00  00 00 00 00  00             .............
GDBus-debug:Address: dbus-launch stderr output:
X11 autolaunch support disabled at compile time.
GDBus-debug:Address: Cannot look-up address bus type 'session': Erreur lors de la génération de la ligne de commande « dbus-launch --autolaunch=0d2ffa10e59a7809134db8165a2c2f17 --binary-syntax --close-stderr » : Le processus fils s’est terminé avec le code 1

Then, the content process tries to do the same when the password field is focused, and dies.

GDBus-debug:Address: In g_dbus_address_get_for_bus_sync() for bus type 'session'
GDBus-debug:Address: env var DBUS_SESSION_BUS_ADDRESS is not set
GDBus-debug:Address: env var DBUS_SYSTEM_BUS_ADDRESS is not set
GDBus-debug:Address: env var DBUS_STARTER_BUS_TYPE is not set
GDBus-debug:Address: Running 'dbus-launch --autolaunch=0d2ffa10e59a7809134db8165a2c2f17 --binary-syntax --close-stderr' to get bus address (possibly autolaunching)

###!!! [Parent][MessageChannel] Error: (msgtype=0x16007F,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv

so maybe we should also fool the parent process.. or figure out what codepath triggers glib to spawn dbus, in the main process *and* in the content process.
Faking a session dbus if not running works fine at runtime, glib doesnt try spawing dbus-launch and sandboxing doesnt kill hte content process.

/* dont overwrite an existing session dbus address, but ensure it is set */
setenv("DBUS_SESSION_BUS_ADDRESS","",0);

I will probably use that in 61 to workaround the issue in the OpenBSD port, but how acceptable would it be upstream in firefox as a temporary measure until glib knows what to do about this issue ?
Priority: -- → P5
Here's what i've been using since 61 in the package shipped to users. A bit gross, but works for those edge cases....
Assignee: nobody → landry
Attachment #9004768 - Flags: review?(gpascutto)
Comment on attachment 9004768 [details] [diff] [review]
Fake a dbus session if none is running when sandboxing the content process

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

LGTM with small edits.

Jed, would it be worth adding this hack on Linux too to fix some edge cases?

::: dom/ipc/ContentChild.cpp
@@ +1787,5 @@
>    sandboxEnabled = StartOpenBSDSandbox(GeckoProcessType_Content);
> +  if (!getenv("DBUS_SESSION_BUS_ADDRESS")) {
> +      MOZ_LOG(sPledgeLog, LogLevel::Debug, "no session dbus found, faking one\n");
> +  }
> +  /* dont overwrite an existing session dbus address, but ensure it is set */

So this can move inside the if?

@@ +1788,5 @@
> +  if (!getenv("DBUS_SESSION_BUS_ADDRESS")) {
> +      MOZ_LOG(sPledgeLog, LogLevel::Debug, "no session dbus found, faking one\n");
> +  }
> +  /* dont overwrite an existing session dbus address, but ensure it is set */
> +  setenv("DBUS_SESSION_BUS_ADDRESS","",0);

Space between the arguments for consistency.
Attachment #9004768 - Flags: review?(gpascutto)
Attachment #9004768 - Flags: review+
Attachment #9004768 - Flags: feedback?(jld)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #16)
> Comment on attachment 9004768 [details] [diff] [review]
> Fake a dbus session if none is running when sandboxing the content process
> 
> Review of attachment 9004768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM with small edits.
> 
> Jed, would it be worth adding this hack on Linux too to fix some edge cases?

There are still probably die-hard linux users that might be running simple window managers and explicitely disable dbus session daemon, but i doubt the linux content process policy forbids process creation like the one we have now on OpenBSD. Otherwise there would already be reports of such crashes/weird behaviours.. *shrug* someone working on linux sandboxing should test this case :)

> ::: dom/ipc/ContentChild.cpp
> @@ +1787,5 @@
> >    sandboxEnabled = StartOpenBSDSandbox(GeckoProcessType_Content);
> > +  if (!getenv("DBUS_SESSION_BUS_ADDRESS")) {
> > +      MOZ_LOG(sPledgeLog, LogLevel::Debug, "no session dbus found, faking one\n");
> > +  }
> > +  /* dont overwrite an existing session dbus address, but ensure it is set */
> 
> So this can move inside the if?

Technically yes, but the third argument of setenv ensures the existing value isnt overriden.. your call :)
>but i doubt the linux content process policy forbids process creation like the one we have now on OpenBSD. 

It does prevent this (no fork allowed). We have some weird bugs like bug 1411629, though after rereading that, it seems to suggest the exact opposite as done here.

>Technically yes, but the third argument of setenv ensures the existing value isnt overriden.. your call :)

I think we should just skip the call if we already know it's a no-op anyway.
Comment on attachment 9004768 [details] [diff] [review]
Fake a dbus session if none is running when sandboxing the content process

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

About Linux: I still don't know what's going on there or why we'd be trying to use DBus from a content process.  (I tried ktrace(8)ing on FreeBSD, to see what happens if there's neither an existing daemon nor the possibility of systemd launching things; it appears to be only the parent process that tries to shell out to dbus-launch.)  I suppose this hack isn't likely to make things worse in the case where we're already not touching DBus in content processes.

One difference is that on Linux the network namespace is unshared during clone, so we'd probably want to set the env var in the LaunchOptions.  Also, about not overwriting an explicitly set address: the content process shouldn't be connecting to it in the first place, and will fail if it tries (unless before sandbox start in some cases), so I don't know how useful that is.

::: dom/ipc/ContentChild.cpp
@@ +1788,5 @@
> +  if (!getenv("DBUS_SESSION_BUS_ADDRESS")) {
> +      MOZ_LOG(sPledgeLog, LogLevel::Debug, "no session dbus found, faking one\n");
> +  }
> +  /* dont overwrite an existing session dbus address, but ensure it is set */
> +  setenv("DBUS_SESSION_BUS_ADDRESS","",0);

Are OpenBSD's getenv/setenv thread-safe?  If not, I suggest PR_GetEnv/PR_SetEnv.
Attachment #9004768 - Flags: feedback?(jld)
Same patch carrying over r+, addressing the following review comments:
* using PR_GetEnv to check if DBUS_SESSION_BUS_ADDRESS is set
* using PR_SetEnv only if needed
* adding parenthesis around the debug message
* declaring the static logger

This one builds here, and i'll ship it with my local 63.0b3 build to do some runtime testing.
Attachment #9004768 - Attachment is obsolete: true
works fine at runtime:

env -i DISPLAY=:0 MOZ_LOG=SandboxPledge:5 PATH=:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin firefox

[(null) 99933: 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'
[Child 77460: Main Thread]: D/SandboxPledge pledged content process with promises: 'stdio rpath wpath cpath inet recvfd sendfd prot_exec unix drm ps'
[Child 77460: Main Thread]: D/SandboxPledge no session dbus found, faking one

same messages for all content processes
Comment on attachment 9004768 [details] [diff] [review]
Fake a dbus session if none is running when sandboxing the content process

Switching flag because I'd want to be sure this uses PR_SetEnv.
Attachment #9004768 - Flags: review+ → review-
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f771187eec2d
When sandboxing the content process on OpenBSD, fake a DBUS session if none is running r=gcp
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f771187eec2d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9006977 [details] [diff] [review]
Fake a dbus session if none is running when sandboxing the content process

Approval Request Comment
[Feature/Bug causing the regression]: none
[User impact if declined]: content process killed by sandboxing when no dbus daemon is running
[Is this code covered by automated tests?]: nope
[Has the fix been verified in Nightly?]: yes, and shipped to openbsd users since 61
[Needs manual test from QE? If yes, steps to reproduce]: get an openbsd vm, use a simple desktop session without dbus, try starting beta
[List of other uplifts needed for the feature/fix]: completes #1457092 which is already in 63
[Is the change risky?]: no
[Why is the change risky/not risky?]: NPOTB/Tier3
[String changes made/needed]: none
Attachment #9006977 - Flags: approval-mozilla-beta?
Comment on attachment 9006977 [details] [diff] [review]
Fake a dbus session if none is running when sandboxing the content process

OpenBSD only patch which is tier3, no risk, taking for 63 beta 6
Attachment #9006977 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
We had some difficulties trying to install openbsd on a vm, in the end we did not succeed so we can't actually reproduce and verify this fix. Based on that and the fact that this is a P5, I'll go ahead and remove qe-verify+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: