1418770 - Lack of dbus name-space sanitization makes Firefox crash at startup

Status: RESOLVED FIXED

(Core Graveyard :: X-remote, defect)

Description

[Francois Guerraz]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171117100127

Steps to reproduce:

After the latest update on nightly, firefox crashes on startup
report 1ea5f017-cc7a-441a-b236-775b80171119 for example
reverting to 2017-11-17 works.


Actual results:

dbus[17992]: arguments to dbus_bus_request_name() were incorrect, assertion "_dbus_check_is_valid_bus_name (name)" failed in file dbus-bus.c line 1122.
This is normally a bug in some application using the D-Bus library.

  D-Bus not built with -rdynamic so unable to print a backtrace
Redirecting call to abort() to mozalloc_abort

ExceptionHandler::GenerateDump cloned child 18102
ExceptionHandler::SendContinueSignalToChild sent continue signal to child
ExceptionHandler::WaitForContinueSignal waiting for continue signal...

Comment 1

[Francois Guerraz]

This is the backtrace I get if I disable google breakpad:
#0  0x000000000040896b in mozalloc_abort(char const*) ()
#1  0x0000000000408935 in mozalloc_abort(char const*) ()
#2  0x00007fffef1384e7 in _dbus_abort () at /usr/lib/libdbus-1.so.3
#3  0x00007fffef12e9a3 in  () at /usr/lib/libdbus-1.so.3
#4  0x00007fffef1107c8 in dbus_bus_request_name () at /usr/lib/libdbus-1.so.3
#5  0x00007fffe93e449d in nsDBusRemoteService::Startup(char const*, char const*) () at /home/francois/bin/firefox-nightly/libxul.so
#6  0x00007fffe93e4d9c in nsRemoteService::Startup(char const*, char const*) () at /home/francois/bin/firefox-nightly/libxul.so
#7  0x00007fffe94b46ab in XREMain::XRE_mainRun() [clone .cold.89] () at /home/francois/bin/firefox-nightly/libxul.so
#8  0x00007fffea46b958 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) () at /home/francois/bin/firefox-nightly/libxul.so
#9  0x00007fffea46b5b6 in XRE_main(int, char**, mozilla::BootstrapConfig const&) () at /home/francois/bin/firefox-nightly/libxul.so
#10 0x0000000000421bf9 in do_main(int, char**, char**) ()
#11 0x0000000000415bdf in main ()

Comment 2

[Francois Guerraz]

This was introduced by
https://hg.mozilla.org/mozilla-central/diff/8a07d34a17b6/toolkit/components/remote/nsDBusRemoteService.cpp#l1.177
not checking if the profile name is a valid name for DBUS and crashing if the profile name contains spaces or other characters not in "[A-Z][a-z][0-9]_" as per
https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-names
Also, the total length must be checked not to be over 255 characters long

Comment 3

[Francois Guerraz]

My opinion is that the random string prefix of the profile directory should be used as it is unique (just removing invalid characters would open the door to name collisions) and short.

Comment 4

[[:philipp]]

hi, is this still an issue after bug 1420124 has landed?

Comment 5

[Francois Guerraz]

Yes this is still an issue.
If the profile name is long enough to produce a base64 string longer than 256 character, the crash still occurs.
You could truncate it, but then name collisions can occur.

I maintain that "the random string prefix of the profile directory should be used as it is unique (just removing invalid characters would open the door to name collisions) and short."

Comment 6

[Andrew Overholt [:overholt]]

Maybe Martin can take a look when he's back.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

I'll look at it.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1418770 - Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH,

Review commit: https://reviewboard.mozilla.org/r/219454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219454/

Comment 9

[Jan Horak [:jhorak]]

Comment on attachment 8950192 [details]
Bug 1418770 - Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH,

https://reviewboard.mozilla.org/r/219454/#review225544

r+ with issue fixed.

::: commit-message-84151:1
(Diff revision 1)
> +Bug 1418770 - Truncate DBus names to DBUS_MAXIMUM_NAME_LENGTH, r?jhorak

maybe little more details there:
Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH

::: toolkit/components/remote/nsDBusRemoteService.cpp:116
(Diff revision 1)
>      (strcmp("org.freedesktop.DBus.Introspectable", iface) == 0)) {
>      return Introspect(msg);
>    }
>  
>    nsAutoCString ourInterfaceName;
>    ourInterfaceName = nsPrintfCString("org.mozilla.%s", mAppName.get());

I guess we need to check also this to be extra sure.

Comment 10

[Francois Guerraz]

What if two long profile names start the same? You'll have a dbus name collision.

Why don't you just use the random string prefix of the profile directory? it's there, it's unique, identifiable and dependably short.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

(In reply to Francois Guerraz from comment #10)
> What if two long profile names start the same? You'll have a dbus name
> collision.
> 
> Why don't you just use the random string prefix of the profile directory?
> it's there, it's unique, identifiable and dependably short.

Unfortunately the random string prefix is not available for remote code. We have only the profile name given as command line argument (-P profile_name). We even don't have default profile name which is solved at Bug 1434544.

Comment 12

[Martin Stránský [:stransky] (ni? me)]

(In reply to Martin Stránský [:stransky] from comment #11)
> (In reply to Francois Guerraz from comment #10)
> > What if two long profile names start the same? You'll have a dbus name
> > collision.
> > 
> > Why don't you just use the random string prefix of the profile directory?
> > it's there, it's unique, identifiable and dependably short.
> 
> Unfortunately the random string prefix is not available for remote code. We
> have only the profile name given as command line argument (-P profile_name).
> We even don't have default profile name which is solved at Bug 1434544.

Err, we actually have the default profile name but for remote service only (not for remote client).

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8950192 [details]
Bug 1418770 - Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219454/diff/1-2/

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Jimm, can you help get this landed and request uplift?

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8950192 [details]
Bug 1418770 - Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH,

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1360566
[User impact if declined]: Firefox can't start with very long profile name (longer than ~230 chars)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no one
[Is the change risky?]: no
[Why is the change risky/not risky?]: It only truncates profile name which is passed to DBus interface. The truncated name is not used by Firefox itself.
[String changes made/needed]: none

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Hm, looks like I forgot to land the commit. Landed now.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44ad70613f51
Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH, r=jhorak

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/44ad70613f51

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8950192 [details]
Bug 1418770 - Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH,

Taking for 59b12.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/28b5b075c5e6

Comment 21

[Francois Guerraz]

I have 3 profiles,

*default
*aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc
*aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccd

I launched all three of them, but as predicted, there's a dbus name collision
$ dbus-send --session             --dest=org.freedesktop.DBus   --type=method_call            --print-reply                 /org/freedesktop/DBus         org.freedesktop.DBus.ListNames | grep firefox
      string "org.mozilla.firefox.YWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJjY2NjY2NjY2N"
      string "org.mozilla.firefox.ZGVmYXVsdA__"

I do not know what are the consequences of that but it can't be good.

Comment 22

[Martin Stránský [:stransky] (ni? me)]

(In reply to Francois Guerraz from comment #21)
> I have 3 profiles,
> 
> *default
> *aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbb
> bbbbbbbbbbbbbbcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc
> *aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbb
> bbbbbbbbbbbbbbcccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccd
> 
> I launched all three of them, but as predicted, there's a dbus name collision
> $ dbus-send --session             --dest=org.freedesktop.DBus  
> --type=method_call            --print-reply                
> /org/freedesktop/DBus         org.freedesktop.DBus.ListNames | grep firefox
>       string
> "org.mozilla.firefox.
> YWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhY
> WFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYW
> FhYWFhYWFhYWJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJiYmJjY2NjY2N
> jY2N"
>       string "org.mozilla.firefox.ZGVmYXVsdA__"
> 
> I do not know what are the consequences of that but it can't be good.

Consequence is that if you have already running a Firefox with profile name longer that ~230 chars and you start another firefox instance with different profile name longer than ~230 chars the DBus remote service fails to start for the second one.

Comment 23

[Mike Kaply [:mkaply]]

This appears to have broken snaps if I understand correctly.

Comment 24

[Ken VanDine]

(In reply to Mike Kaply [:mkaply] from comment #23)
> This appears to have broken snaps if I understand correctly.

I don't think this change actually broke the snap, but snap isn't allowing it to bind to those dbus names.  This would have been an issue before as well.