Closed Bug 1480517 Opened Last year Closed Last year

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

Categories

(Toolkit :: General, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 62+ fixed
firefox61 --- wontfix
firefox62 + fixed
firefox63 + fixed

People

(Reporter: decoder, Assigned: stransky)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, sec-low, Whiteboard: [post-critsmash-triage][adv-main62+][adv-esr60.2+])

Attachments

(2 files)

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.
See Also: → 1480226
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.
Flags: needinfo?(c4609174)
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.
Flags: needinfo?(stransky)
Sure, I'll look at it.
Flags: needinfo?(stransky)
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.
I don't think we need another data from the reporter, Thanks.
Flags: needinfo?(c4609174)
Fedora 28, GNOME 3.28.2, dbus-1.12.8-1.fc28.src.rpm
Ah, now (with Nightly) commenting works. I could not comment this issue here in Firefox stable (but I could comment non-security issues there.).
(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)
Duplicate of this bug: 1480226
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.
Attached patch patchSplinter Review
Unfortunately mozreview does not support to push to security bugs.
Assignee: nobody → stransky
Attachment #8997380 - Flags: review?(jhorak)
Attachment #8997380 - Flags: review?(jhorak) → review+
Keywords: checkin-needed
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().
And what's the impact then, i.e. what may be attacker be able to do?
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.
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.
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
Flags: needinfo?(stransky)
Keywords: checkin-needed
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.
Flags: needinfo?(stransky)
Attachment #8997380 - Flags: sec-approval?
Make this a sec-low based on conversation with Decoder. This doesn't need sec-approval to land as a result.
Keywords: sec-low
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3e8c83bddae6
Group: core-security → core-security-release
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Please request Beta and ESR60 approval on this when you get a chance.
Flags: needinfo?(stransky)
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
Flags: needinfo?(stransky)
Attachment #8997380 - Flags: approval-mozilla-beta?
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.
Attachment #8997380 - Flags: approval-mozilla-esr60?
Comment on attachment 8997380 [details] [diff] [review]
patch

Fixes a Linux sec issue. Approved for 62.0b17 and ESR 60.2.
Attachment #8997380 - Flags: approval-mozilla-esr60?
Attachment #8997380 - Flags: approval-mozilla-esr60+
Attachment #8997380 - Flags: approval-mozilla-beta?
Attachment #8997380 - Flags: approval-mozilla-beta+
Whiteboard: [adv-main62+][adv-esr60.2+]
Flags: qe-verify-
Whiteboard: [adv-main62+][adv-esr60.2+] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.