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

RESOLVED FIXED in Firefox 59

Status

defect
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: kubrick, Assigned: stransky)

Tracking

(Depends on 1 bug, {crash, regression})

Trunk
mozilla60
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59+ fixed, firefox60+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

2 years ago
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...
Reporter

Updated

2 years ago
Crash Signature: 1ea5f017-cc7a-441a-b236-775b80171119 17c43b6d-23f9-4189-a567-6c9540171119
Reporter

Comment 1

2 years ago
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 ()
Reporter

Comment 2

2 years ago
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
Summary: abort on startup with nightly post nov 18 (arguments to dbus_bus_request_name() were incorrect) → Lack of dbus name-space sanitization makes Firefox crash at startup
Reporter

Comment 3

2 years ago
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.
Component: Untriaged → General
Keywords: crash
Product: Firefox → Toolkit

Comment 4

Last year
hi, is this still an issue after bug 1420124 has landed?
Flags: needinfo?(kubrick)
Reporter

Comment 5

Last year
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."
Flags: needinfo?(kubrick)
Maybe Martin can take a look when he's back.
Flags: needinfo?(stransky)
Assignee

Updated

Last year
Assignee: nobody → stransky
Flags: needinfo?(stransky)
I'll look at it.
Assignee

Updated

Last year
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 9

Last year
mozreview-review
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.
Attachment #8950192 - Flags: review?(jhorak) → review+
Reporter

Comment 10

Last year
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.
(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.
(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 hidden (mozreview-request)
Assignee

Updated

Last year
Component: General → X-remote
Product: Toolkit → Core
Jimm, can you help get this landed and request uplift?
Flags: needinfo?(jmathies)
Keywords: checkin-needed
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
Attachment #8950192 - Flags: approval-mozilla-beta?
Hm, looks like I forgot to land the commit. Landed now.

Comment 17

Last year
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/44ad70613f51
Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH, r=jhorak
Keywords: checkin-needed

Comment 18

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/44ad70613f51
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8950192 [details]
Bug 1418770 - Truncate DBus names used for remote service to DBUS_MAXIMUM_NAME_LENGTH,

Taking for 59b12.
Flags: needinfo?(jmathies)
Attachment #8950192 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 21

Last year
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.
Flags: needinfo?(stransky)
(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.
Flags: needinfo?(stransky)
This appears to have broken snaps if I understand correctly.
Depends on: 1441894

Comment 24

Last year
(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.

Updated

2 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.