1480517 - AddressSanitizer: heap-use-after-free [@ get] through [@ nsDBusRemoteService::HandleDBusMessage] with READ of size 8

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 63.0a1-20180731105217-https://hg.mozilla.org/mozilla-central/rev/0d72c7996d60a7c07e35c5f90d78b02a47d17460.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Christian Holler (:decoder)]

Can you provide some information on what Linux distribution and version you are using? There are dbus frames on the stack with no symbols. I might be able to symbolize these if I know the exact library and distribution versions.

Comment 4

[Andrew McCreight [:mccr8]]

stransky, do you have any idea what might be going wrong here? It looks like you've been the most active in this Dbus code.

Comment 5

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

Sure, I'll look at it.

Comment 6

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

The affected line is src/toolkit/components/remote/nsDBusRemoteService.cpp:122 is:
ourInterfaceName = nsPrintfCString("org.mozilla.%s", mAppName.get());

looks like nsDBusRemoteService::HandleDBusMessage() is called from DBus message_handler after nsDBusRemoteService::Shutdown() when the nsDBusRemoteService is already released.

Comment 7

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

I don't think we need another data from the reporter, Thanks.

Comment 8

[rugk]

Fedora 28, GNOME 3.28.2, dbus-1.12.8-1.fc28.src.rpm

Comment 9

[rugk]

Ah, now (with Nightly) commenting works. I could not comment this issue here in Firefox stable (but I could comment non-security issues there.).

Comment 10

[rugk]

(no it's still stable, but just a new window … whatever… does not matter… I may report it as a bugzuilla issue when I can reproduce it later)

Comment 12

[rugk]

BTW, if you care, Bugzilla issue reported as https://bugzilla.mozilla.org/show_bug.cgi?id=1480713. Seems to be connected to Mailvelope. So… enough off-topic comments. Sorry for that.

Comment 13

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

Attachment: patch

Unfortunately mozreview does not support to push to security bugs.

Comment 14

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

Attack vector can be:

- Remote service ("@mozilla.org/toolkit/remote-service;1") is stopped (browser shutdown in this case), /org/mozilla/firefox/Remote remains registered.

- attacker calls OpenURL() at /org/mozilla/firefox/Remote DBus interface -> that will call nsDBusRemoteService::HandleDBusMessage() on already freed nsDBusRemoteService object.

- /org/mozilla/firefox/Remote will be removed when firefox completely quits.

So the attack surface is to call /org/mozilla/firefox/Remote -> OpenURL() in small precise time frame or make remote service quit somehow during normal Firefox life and then call the /org/mozilla/firefox/Remote -> OpenURL().

Comment 15

[rugk]

And what's the impact then, i.e. what may be attacker be able to do?

Comment 16

[Christian Holler (:decoder)]

To me this sounds like it is not possible for an attacker to do this because you would already need the ability to send a DBUS message on the remote interface, requiring you to be a process on that machine (or tricking the user into clicking a link somewhere outside of Firefox in the exact right moment of browser shutdown, which the user must also perform).

So overall I'd say this is sec-low at most.

Comment 17

[rugk]

Okay, so it could probably be an issue for sandboxing environments. E.g. Flatpaks on Linux (https://flatpak.org/, FF flatpaks here: https://firefox-flatpak.mojefedora.cz/). I don't know whether dbus messages are isolated/filtered there.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This can't land without a rating and an assessment of which branches are affected (and a sec-approval request if necessary).
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 19

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

Comment on attachment 8997380 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

IMHO hardly if there's no way how to stop particular service and leave browser running.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes.

Which older supported branches are affected by this flaw?
60 ESR.

If not all supported branches, which bug introduced the flaw?
Bug 1360566

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

no risky, the code is (almost) identical.

How likely is this patch to cause regressions; how much testing does it need?

Call dbus_connection_unregister_object_path() at service shutdown should not have any side effect.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Make this a sec-low based on conversation with Decoder. This doesn't need sec-approval to land as a result.

Comment 21

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

Removed the commit comment explaining the issue.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e8c83bddae68cc637af82c1dbe8d792f2e287a8

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/3e8c83bddae6

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please request Beta and ESR60 approval on this when you get a chance.

Comment 24

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

Comment on attachment 8997380 [details] [diff] [review]
patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1360566
[User impact if declined]: sec-low issue
[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]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Call dbus_connection_unregister_object_path() at service shutdown should not have any side effect.
[String changes made/needed]: none

Comment 25

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

Comment on attachment 8997380 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-low issue
User impact if declined: none
Fix Landed on Version: nightly 63
Risk to taking this patch (and alternatives if risky): no risk, Call dbus_connection_unregister_object_path() at service shutdown should not have any side effect.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8997380 [details] [diff] [review]
patch

Fixes a Linux sec issue. Approved for 62.0b17 and ESR 60.2.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b402d46ca93b

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/2d248bce62d7