1248329 - (CVE-2016-2829) Requesting permissions in short succession can lead to the microphone icon displayed for an unrelated notification

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Tim McCormack]

Attachment: demo-6194b06b95d2c580b7.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 Iceweasel/38.6.1
Build ID: 20160214081214

Steps to reproduce:

I created a web page to request as many permissions as possible within a short span of time. (Geolocation, pointer lock, microphone and camera access, notifications.) Demo attached.


Actual results:

Under some conditions, the notifications dialogs have the wrong icon, e.g. microphone icon for notifications or (more worryingly) for geolocation.

This is probably a race condition. If it can be controlled, it could lead to a user "consenting" to the wrong permission. (Marking as Security bug out of caution for now, feel free to remove flag.)

The boxes also flash past in a way that indicates that sometimes one permission cancels display of another.


Expected results:

Notifications boxes should appear in quick succession (but do not replace each other) or form a single stack. They should also have the correct icons.

Comment 1

[Tim McCormack]

Attachment: Demonstration of local (file://) wrong-icon behavior at demo commit 6194b06b95d2c580b7

Adding screenshot of behavior occurring when I view the demo locally via file://. This is fairly easy to reproduce; loading the page and then hitting F5 is usually sufficient. Since I'm triggering one of the permission requests on keyup, hitting spacebar in the page may be required before hitting F5.

I'm on Debian 7 (Wheezy) with XGCE, if it matters.

Comment 2

[Tim McCormack]

Attachment: demo-6194b06b95d2c580b7-remote.png

Adding screenshot of behavior occurring over HTTPS at same demo commit. Much more difficult to reproduce than over file://.

Comment 3

[Tim McCormack]

Was also able to reproduce on Firefox 44 over file://; have not confirmed over https. Race condition harder to trigger, or different in permissions is changing things.

Comment 4

[:Gijs (he/him)]

I expect this is an issue with PopupNotifications.jsm, but I'm not sure. Florian, have you seen something like this before?

Comment 5

[Florian Quèze [:florian]]

The problem seems to be specifically with the microphone icon, which isn't very surprising, as that icon has specific code. The notification for camera (or camera+microphone) and microphone is the same, and the icon to show in the panel is selected at the time the popup is shown.

The code doing this is at http://hg.mozilla.org/mozilla-central/annotate/e355cacefc88/browser/modules/webrtcUI.jsm#l361 and I would guess a fix involves replacing ".firstChild" on that line.

I didn't know it was possible to have several notifications shown at once, is this a recent change related to the permission center?

Comment 6

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> The problem seems to be specifically with the microphone icon, which isn't
> very surprising, as that icon has specific code. The notification for camera
> (or camera+microphone) and microphone is the same, and the icon to show in
> the panel is selected at the time the popup is shown.
> 
> The code doing this is at
> http://hg.mozilla.org/mozilla-central/annotate/e355cacefc88/browser/modules/
> webrtcUI.jsm#l361 and I would guess a fix involves replacing ".firstChild"
> on that line.

OK, moving the bug accordingly, then. :-)

> I didn't know it was possible to have several notifications shown at once,
> is this a recent change related to the permission center?

Considering this was reported against ESR, I would not expect so.

Comment 7

[Tim McCormack]

Attachment: Demo v2, deterministic

Attaching a test case that is deterministic, using Florian's discovery. Clicking the demo button brings up three requests in quick succession, microphone second. Each preempts any existing dialog. Cancelling the third one shows the first two stacked, and the icon problem appears reliably.

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch

Comment 9

[:Gijs (he/him)]

Comment on attachment 8719852 [details] [diff] [review]
Patch

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

TBH, could just use getElementById here - Firefox code really shouldn't have multiple items with the same ID, and hopefully there are tests for this working? :-)

Comment 10

[Florian Quèze [:florian]]

Attachment: Patch v2

Using getElementById instead of querySelector.

(In reply to :Gijs Kruitbosch from comment #9)
> hopefully there are tests for this working? :-)

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_devices_get_user_media.js?rev=43a902e35df7#67 covers the microphone icon being shown.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8720216 [details] [diff] [review]
Patch v2

Carrying forward Gijs' r+ from comment 9.

Comment 12

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/a8cd1f16ed6411dfe8e768596f3e594760050235
Bug 1248329 - Requesting permissions in short succession can lead to the microphone icon displayed for an unrelated notification, r=Gijs.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8cd1f16ed64

Comment 14

[Paul Silaghi, QA [:pauly]]

Reproduced the issue on FX 46.0.1 using the second testcase.
Verified fixed FX 47 RC, 49.0a1 (2016-06-01) on Win 7.