403995 - Possible DOS attack with the notification bar

Status: NEW

(Toolkit :: PopupNotifications and Notification Bars, defect)

Description

[Florian Quèze [:florian]]

Attachment: proposed solution

When calling appendNotification in a loop Minefield can become unresponsive.

This can happen for example from a webpage trying to register a protocol handler in a loop.

Apparently there is a perf issue in the appendNotification method, the code checking where the notification should be inserted according to its priority is O(n).

I can't think of any legitimate use for a huge number of notifications so I think we don't need to bother fixing the perf issue, we can simply limit the number of notifications stored at the same time.

Comment 1

[Damon Sicore (:damons)]

Yeah, would be nice to have, but we won't block the release for this.  Marking as wanted.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #0)
> When calling appendNotification in a loop Minefield can become unresponsive.
> 
> This can happen for example from a webpage trying to register a protocol
> handler in a loop.

Shouldn't we fix that bug? Can't the code that triggers the protocol handler notification check whether there's an existing notification, and if so hide it?

I'm not sure we want to arbitrarily limit the number of notifications if the only problem is that people aren't checking for existing notifications before calling appendNotification.

Comment 3

[Florian Quèze [:florian]]

(In reply to comment #2)

> Shouldn't we fix that bug?

Yes, I agree that the current behavior of registerProtocolHandler is not ideal. But if we want to fix it, it's a different bug.

> Can't the code that triggers the protocol handler
> notification check whether there's an existing notification, and if so hide it?

The same page can register more than one protocol handler so we can't use a single notification.

> I'm not sure we want to arbitrarily limit the number of notifications if the
> only problem is that people aren't checking for existing notifications before
> calling appendNotification.

The problem here is more that appendNotification can become very slow.  I first thought we could improve it by using a binary search instead of the O(n) search that we currently have for the position of the new notification.  But as we don't have any legitimate use case for a high number of notifications, it's easier to just limit the number of notifications.

Also, note that the limit is in a field so that if an extension or a xulrunner app comes up with a need for more than 10 notifications, it will be easy for them to override the limit.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #3)
> The same page can register more than one protocol handler so we can't use a
> single notification.

What's the use case for registering multiple protocol handlers? Why do we need to alert the user with more than one at a time? Seems like restricting it in the protocol-handler-registration code is what we really want to do, rather than putting an arbitrary limit on the generic notification code.

Comment 5

[Florian Quèze [:florian]]

(In reply to comment #4)
> What's the use case for registering multiple protocol handlers?

For example a website may want to register both a mailto and a webcal handler.

> Why do we need to alert the user with more than one at a time?

The notifications are stacked by the notificationbar so we are actually alerting the user of them one at a time.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Why not add the 10 notification limit in the protocol registration code?

There are other ways to DoS the browser that we don't protect against, and I'm not sure that making appendNotification throw after an arbitrary number of notifications have been displayed is the right way to address this.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Once you've got a sufficiently large number of notification bars up, you've got an inherently crummy user-experience (perhaps the entire content area is full, even!).  This problem is inherent to the notification bar widget, so that's the right place to solve it, rather than forcing all the callers to deal manually.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Fair enough. I'm just worried that this could lead to unexpected results, unless callers are expecting that the call might throw. We should at the very least audit the existing callers to make sure that they'll handle it correctly... Or perhaps we should make appendNotification return null if appending the notification failed, rather than throwing.

Comment 9

[Florian Quèze [:florian]]

returning null would lead to even more unexpected results. If a code adds a lot of notifications it's buggy so throwing an exception with a message telling what was wrong seems good to me. If we return null the code will probably go on and fail later with a strange error message.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> returning null would lead to even more unexpected results. If a code adds a lot
> of notifications it's buggy so throwing an exception with a message telling
> what was wrong seems good to me. If we return null the code will probably go on
> and fail later with a strange error message.

We would probably need to audit existing callers either way, yes. But existing callers that don't care about the return value (all of them, as far as I can tell) wouldn't necessarily be affected if it returned null. With an exception, all callers will necessarily be affected in the failure case.

Comment 11

[Florian Quèze [:florian]]

I went quickly over the results of mxr for "appendNotification".

WebContentConverter.js seems to be the only file where notifications are added without first checking if a notification with the same value is already there.
So I think nobody will be affected by the failure case.

Btw, the caller in nsLoginManagerPrompter.js cares about the return value.

Comment 12

[Phil Ringnalda (:philor)]

So on http://dev.philringnalda.com/notif.html which opens ten registerProtocolHandler notifications, then a blocked popup and a blocked xpinstall, you're going to show the ten protocol handler ones, and then throw the popup blocker and the xpinstall on the floor?

That completely turns the whole design of the notificationbar widget on its head - it's intended to show an unbounded number of notifications, the most important on top, and you're redesigning it to show the first ten of any priority, and to then essentially disable the controls for the popup blocker, the xpinstall blocker, the image blocker, the password manager, and who knows how many extensions.

Without even a single test.

Comment 13

[Florian Quèze [:florian]]

(In reply to comment #12)
> So on http://dev.philringnalda.com/notif.html which opens ten
> registerProtocolHandler notifications, then a blocked popup and a blocked
> xpinstall, you're going to show the ten protocol handler ones, and then throw
> the popup blocker and the xpinstall on the floor?

Yes.  It's bad but I can't see any case where this would actually turn into a real problem.  If a page tries to register 10 handlers at once, then it's bogus and the only thing I care about this page is to get out of there, not to see how many popup have been blocked.

> you're redesigning it to show the first ten of any priority

I'm not redesigning it.  The only goal of the limit is to prevent the case that makes Firefox hang.  If you think 10 is not enough I'm fine with changing the default limit to 50.  But still, I don't see any use case for more than 10 notifications at once.

> Without even a single test.

I'm thinking about adding one.

Comment 14

[Dave Townsend [:mossop]]

The number of different notifications that we have just built into Firefox is growing. Currently off the top of my head we have:

Popup blocked.
Plugin missing.
Web content handler.
Password manager.
Extension install.

That's half of your limit already and ignores any extension provided notifications. If we are going to put a limit on the number it should be far higher than 10.

I agree that we really need to look at the callers here, check that throwing isn't going to cause a problem.

I think that if we are going to drop notifications then we should be taking into account the relative priorities of the notifications we are already displaying and those we are dropping.

But really why aren't we fixing the performance problem instead (since it will improve things for all cases) rather than introducing an arbitrary limit on this toolkit widget. 

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

If we do start dropping, we might want to add some sort of notification to observers when that happens.

As far as why set an arbitrary limit, if you actually get to the point where you've got most of the visible window covered with notifications bar, that's a horrible user experience.

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> As far as why set an arbitrary limit, if you actually get to the point where
> you've got most of the visible window covered with notifications bar, that's a
> horrible user experience.

As the example in comment 12 demonstrates, the notification bar never displays more than 1 at a time.

Comment 17

[Florian Quèze [:florian]]

(In reply to comment #14)

> But really why aren't we fixing the performance problem instead (since it will
> improve things for all cases) rather than introducing an arbitrary limit on
> this toolkit widget. 
> 

Because when we have enough notifications to make it really slow, it's already more or less unusable (if we get 100 stacked notifications it's a horrible UI and nearly impossible to close them).

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> (In reply to comment #14)
> 
> > But really why aren't we fixing the performance problem instead (since it will
> > improve things for all cases) rather than introducing an arbitrary limit on
> > this toolkit widget. 
> > 
> 
> Because when we have enough notifications to make it really slow, it's already
> more or less unusable (if we get 100 stacked notifications it's a horrible UI
> and nearly impossible to close them).

So wait, the actual problem you are trying to fix here is a UX one, not a performance issue as the summary and keywords suggest?

Comment 19

[Florian Quèze [:florian]]

I'm not working on this, so anybody can feel free to take it over.

Comment 20

[Matthew N. [:MattN]]

Bulk move to Toolkit::Notifications and Alerts

Filter on notifications-and-alerts-component.