1429093 - (CVE-2018-5141) Push API allows DOM event without user interaction

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[Zohar]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.84 Safari/537.36

Steps to reproduce:

I've created an application that uses the notification Push API. In the service worker JS. I've created a listener for the 'NotificationClose' event, which launches a DOM interaction (opens a new tab on browser).

Then, I've registered to the application with Firefox (allowing it to send me push notification). 

Finally, I've sent few consecutive push notifications to the endpoint in a short time (for example 5 notification in 4 seconds).

Example code fro service worker is attached. 


Actual results:

When sending few consecutive notifications, I was able to invoke the 'notification close' event without user interaction. Furthermore, each notification launched a new Tab (invoking a DOM event). 

In essence:
1) If the browser was opened, I was able to remotely open an arbitrary amount of tabs on the browser without user interaction.

2) If the browser was closed, all the tabs open upon next launch.

This behavior puts the user in risk:

1) Adware: If the user once allowed a malicious application to send notification, now the ad providers can launch as many ads as they choose without the user interaction.

2) Persistence for attackers: if once I was able to get a user to register to my app, I can now redirect him to  malicious link (for example containing CSRF or XSS) whenever I choose (for example I will send the user to a CSRF link again and again without his interaction, until he has a valid session with the target application).

3) Client side denial of service: I can launch infinite numbers of tabs every time the user opens the browser, effectively rendering the browser unusable. 

It is important to stress that the user cannot stop the attack once the notifications were allowed, I.e. he cannot prevent the attacker from opening a tab on his browser


Expected results:

Events (such as 'close notification') should require user interaction. Furthermore:
1. close / push events should not allowed DOM interaction (opening a tab, for example). note that this is enforced in Chrome. 

2. DOM intercations should be limited to 'one at a time', and not queued without restriction.

Comment 1

[Zohar]

Attachment: javascript code for service worker. Use this to register a push notifications, then send consecutive requests to the endpoit

Comment 2

[Zohar]

Attachment: A short video clip demonstrating the issue

the issue is demonstrated both when the browser is opened, and closed. The notifications are sent via fiddler (standard web proxy) and you can see the firefox tabs open in the background. Hope that makes the issue more clear :)

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

The issue is that we "allow window interaction" for any kind of notification event.  We need to restrict this notification clicks.

https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/dom/workers/ServiceWorkerPrivate.cpp#1255

We can add a check like this at the above call site:

https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/dom/workers/ServiceWorkerPrivate.cpp#1288

Tom, do you have any time to look at this?

Comment 4

[Tom Tung [:tt, :ttung]]

Sure

Comment 5

[Tom Tung [:tt, :ttung]]

(In reply to Zohar from comment #2)
> Created attachment 8941142 [details]
> A short video clip demonstrating the issue
> 
> the issue is demonstrated both when the browser is opened, and closed. The
> notifications are sent via fiddler (standard web proxy) and you can see the
> firefox tabs open in the background. Hope that makes the issue more clear :)

Thanks for reporting the issue and providing the STR & video! That does make the issue more clear.

Comment 6

[Zohar]

(In reply to Tom Tung [:tt] from comment #5)
> (In reply to Zohar from comment #2)
> > Created attachment 8941142 [details]
> > A short video clip demonstrating the issue
> > 
> > the issue is demonstrated both when the browser is opened, and closed. The
> > notifications are sent via fiddler (standard web proxy) and you can see the
> > firefox tabs open in the background. Hope that makes the issue more clear :)
> 
> Thanks for reporting the issue and providing the STR & video! That does make
> the issue more clear.

My pleasure, 
if there are any questions or clarifications needed please feel free to ask. 

Cheers, 
Zohar

Comment 7

[Tom Tung [:tt, :ttung]]

Ben, I collect the things I understand so far and present them below. Could you correct me if I put anything wrong? And maybe give me some feedback? Thanks!

IIUC, we are going to disallow client.openWindow() on "notificationclose" event, but keep allowing it work on "notificationonclick" event. 

After skimming the code, I found there are two places to increase the counter for WindowInteraction. One is in the WorkerRun [1] while the other is inside the AllowWindowInteractionHandler [2]. And the code for consuming the counter [3] (opposite to [1]) and [4] (opposite to [2]).

I guess [1, 3] cover all the microtasks in the event listener. But I'm not sure about whether should we ensure client.openWindow() in another task. And [2] covers even more.  
For example, in the following case:
  onnotificationclick = function(event) {
    setTimeout(client.openWindow("foo.com"), 0);          // Place A
    Promise.resolve().then(client.openWindow("foo.com")); // Place B
    client.openWindow("foo.com");                         // Place C
    notfication.close();         
  }
  
  onnotificationclose = function(event) {
    client.openWindow("foo.com");                         // Place D 
  }

The order of executing client.openWindow is: 
  click's [1] -> click 's [2] -> C -> B  -> A -> click's [3] -> close's [1] -> close's [2] -> D -> close's [3] -> close's [4] -> click's [4]

I think, ideally, we should resolve the promise at Place A, B, and C, but reject the promise at Place D. However, I'm sure whether shall we allow the window interaction in other tasks (e.g. set a timeout too long to make user realize the opening window is triggered by the notification click event).

If we shouldn't, then the solution would be as your suggestion in comment #3 which is adding if-statement on [1, 3]. Besides, we might consider removing [2, 4].

[1] https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/dom/workers/ServiceWorkerPrivate.cpp#1117
[2] https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/dom/workers/ServiceWorkerPrivate.cpp#1255
[3] https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/dom/workers/ServiceWorkerPrivate.cpp#1267
[4] https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/dom/workers/ServiceWorkerPrivate.cpp#1093

Comment 8

[Tom Tung [:tt, :ttung]]

BTW, chromium seems to only allow window interaction only when the event is notificationclick or payment:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp?l=120-121

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Right, I think we want to look for the notificationclick event type string and only do the logic then.

I don't think you would do the check in StartClearWindowTimer(), though.  I think instead it would be better to not create the AllowWindowInteractionHandler() at all.

Whether to remove the extra Allow/Consume calls might be a bit orthogonal.  You could try to remove them, but if its easier we could also just make them all respect the event name check.

Something like:

  bool allowInteraction = mEventName == NAME;

  RefPtr<AllowWindowInteractionHandler> allowWindowInteraction
  if (allowInteraction) {
    aWorkerPrivate->GlobalScope()->AllowWindowInteraction();
    allowWindowInteraction = new AllowWindowInteractionHandler(aWorkerPrivate);
  }

  nsresult rv = DispatchExtendableEventOnWorkerScope(aCx,
                                                     aWorkerPrivate->GlobalScope(),
                                                     event,
                                                     allowWindowInteraction);

  /* ... */

  if (allowInteraction) {
    aWorkerPrivate->GlobalScope()->ConsumeWindowInteraction();
  }

Does that help?

Comment 10

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> I don't think you would do the check in StartClearWindowTimer(), though.  I
> think instead it would be better to not create the
> AllowWindowInteractionHandler() at all.

Yeah, we shouldn't create AllowWindowInteractionHandler for the notificationclose event if we don't want it to allow window interaction. That makes sense.

> Whether to remove the extra Allow/Consume calls might be a bit orthogonal. 
> You could try to remove them, but if its easier we could also just make them
> all respect the event name check.
> 
> Something like:
> 
>   bool allowInteraction = mEventName == NAME;
> 
>   RefPtr<AllowWindowInteractionHandler> allowWindowInteraction
>   if (allowInteraction) {
>     aWorkerPrivate->GlobalScope()->AllowWindowInteraction();
>     allowWindowInteraction = new
> AllowWindowInteractionHandler(aWorkerPrivate);
>   }
> 
>   nsresult rv = DispatchExtendableEventOnWorkerScope(aCx,
>                                                     
> aWorkerPrivate->GlobalScope(),
>                                                      event,
>                                                      allowWindowInteraction);
> 
>   /* ... */
> 
>   if (allowInteraction) {
>     aWorkerPrivate->GlobalScope()->ConsumeWindowInteraction();
>   }

But it might allow clients to open window even in the notificationclose event when that event is triggered by a notificationclick event.
For example, just like the example in comment 7:

onnotificationclick = function(event) {
    notfication.close();
}
onnotificationclose = function(event) {
    client.openWindow("foo.com");                
}

In this example, the window will open. Do we allow that? I was thinking that we don't, but I'm not sure about it.

If we do, then I believe I don't need to remove the allow/consume pair in the AllowWindowInteractionHandler. If not, then we probably want to remove that pair.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

The allow interaction is timer based.  It should be permitted from other async callbacks if it's within the time window.

I think the extra calls around the dispatch might just support calling openWindow synchronously in a long running event handler.

Comment 12

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #11)
> The allow interaction is timer based.  It should be permitted from other
> async callbacks if it's within the time window.
> 
> I think the extra calls around the dispatch might just support calling
> openWindow synchronously in a long running event handler.

Oh, I see. So, in this case, the window can be opened is because of the timer hasn't expired rather than triggered by the click event. And we do allow this happen.

Thanks for clearing my confusion!

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Well, we start the timer right before firing the event handler.  We should only do that for the click event and not the other events.

Comment 14

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #13)
> Well, we start the timer right before firing the event handler.  We should
> only do that for the click event and not the other events.

Hmm, then I dive into the code a bit more because it seems that we clear the window allow after running into handler for the close event in this case. 

Sorry for misreading your comment :(

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

The timer is started as a side effect of creating the AllowWindowInteractionHandler():

https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/workers/ServiceWorkerPrivate.cpp#1168

We create the AllowWindowInteractionHandler() right before dispatching the event:

https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/workers/ServiceWorkerPrivate.cpp#1258

Right now we do that before all notification events, but the bug here is to only do it before notificationclick events.

Looking at the code now, though, it seems we cancel the timer and call ConsumeWindowInteraction() when the function event's waitUntil() completes:

https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/workers/ServiceWorkerPrivate.cpp#1117
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/workers/ServiceWorkerPrivate.cpp#1093

So I think maybe I was confused.  We want to allow async code to call clients.openWindow() until the notificationevent's waitUntil() promise resolves.

Sorry for my confusion here.  This code is a bit hard to follow.

So I think you are mostly right about comment 7, but you probably need to include a waitUntil() that covers A, B, and C.

Assuming notification.close() does not fire the notificationclose event synchronously (I don't think it should), then D should reject as you suggest.

I still think comment 9 changes probably achieve this, but clearly this code could use some cleanup.  Not sure if we want to do that here or in a follow-up that is not marked private.

Does that help at all or confuse things further?  Thanks again for looking at this.

Comment 16

[Zohar]

Hey guys, 
I just want to add my opinion as a security researcher, 
I think  the potential impact of this bug is quite severe (I saw you marked the bug as a 'low risk' security event)

The situation is that I, the attacker, can cause a victim to visit any link I choose, whenever I choose, and as many times as I choose.

So any reflected cross site scripting I detect in any website - I can make sure the user is impacted:
1) I detect a reflected XSS in website A
2) I create a link the exploits the XSS
3) I send the link to the user inside a notification (for example as the notification data), and force the user to visit the link
using the 'close' event.
4) I monitor the exploitation from my CNC server:
  a. If the user had a session with website A when he visited it, I win (as i get to execute my javascript in the context of website A domain). 
  b IF the user did not have a logged in session, I simply wait some time and fire the attack again. 

This is obviously a very simple example. The same can be used to exploit many other vulnerabilities (CSRF, Phishing, adware, Exploit kits, etc.). 

The point is that having the ability to force a client to visit any website of my choosing again and again is a very strong asset to the attacker and puts the client in quite a serious risk. 

So, please consider giving the issue a higher security rating.

Thanks

Comment 17

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #15)
> So I think you are mostly right about comment 7, but you probably need to
> include a waitUntil() that covers A, B, and C.
> 
> Assuming notification.close() does not fire the notificationclose event
> synchronously (I don't think it should), then D should reject as you suggest.
> 
> I still think comment 9 changes probably achieve this, but clearly this code
> could use some cleanup.  Not sure if we want to do that here or in a
> follow-up that is not marked private.

That makes sense to me. Thanks for elaborating on it! 
For correction, if I apply the comment 9:
onnotificationclick = function(event) {
    event.notfication.close();
}
onnotificationclose = function(event) {
    client.openWindow("foo.com");      // the window won't be opened           
}

I guess what I get stuck is that I accidently found a strage way to open window in the close evnet after applying comment 9. 
For example, if I specify to close the notification in the existing test [1] just above the waitUntil:
onnotificationclick = function(e) {
    ...

    e.notfication.close();
    e.waitUntil(Promise.All(promises).then(() => client.postMessage(results)));
}
onnotificationclose = function(event) {
    client.openWindow("foo.com");                
}

Then, I found the window is still opened even after I do the check as your comment 9. 

I guess maybe it's an another issue for waitUtil(f) while the "f" takes tons of time? I'll investigate more about this.

[1] https://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/openWindow_worker.js#111

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Well, if the notificationclose event handler is fired before the waitUntil promises resolve, then in theory it should be allowed to open the window.  But this only really matters if the notificationclick did not open a window.  If the click opened a window then it should have consumed the interaction.

The important thing is we only allow one additional "open a window" token per click dispatch.

If you try to open a window in both click and close, do you get two windows?

(Maybe this does argue for trying to remove any extraneous AllowWindowInteraction() calls as that might allow two windows to be synchronously opened in the click handler.)

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Daniel, please see comment 16 about possible impact.

Comment 20

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #18)
I'm afraid I might misunderstand something. I put them as follows:

> Well, if the notificationclose event handler is fired before the waitUntil
> promises resolve, then in theory it should be allowed to open the window. 

Yeah, it looks like the close event is fired before the promises in waitUntil() is resolved.

> But this only really matters if the notificationclick did not open a window.

Because the close event is fired before the promise in waitUntil() is resolved, we can make client's window be opened even if the click event doesn't call clients.openWindow().
For example:
onnotificationclick = function(event) {
    event.notfication.close();
    event.waitUntil(client.focus());
}
onnotificationclose = function(event) {
    client.openWindow("foo.com");      // The window will be opened        
}

I guess my problem is whether should allow this window to be opened?

> If the click opened a window then it should have consumed the interaction.

Sorry, I'm a little confused here. I reckon I might misunderstand something. Please correct me if there is anything wrong.

If we don't call the clients.windowOpen() in the click event handler, the window interaction will still be consumed.

The open window doesn't consume the interaction. The current implementation consumes the interaction at the end of WorkerRun() [1] and ClearWindowAllowed() [2]. And it seems we don't trigger either of them after calling openWindow() [3]. Therefore, we can open few windows in the current test test_openWindow.html [4] (testForUrl calls clients.openWindow). (Note: I run the test with lldb and there are two allow & consume pairs. The first interaction is consumed by the end of WorkerRun, and the second interaction is consumed by timer's callback [5]). 

[1] https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#1267 
[2] https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#1093
[3] https://searchfox.org/mozilla-central/source/dom/clients/api/Clients.cpp#219
[4] https://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/openWindow_worker.js#97-110
[5] https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#1134-1141

> The important thing is we only allow one additional "open a window" token
> per click dispatch.

I'm not sure about one additional "open a window" token. Do you mean we allow open a window in the close event handler if it's trigger by the click event handler? Can you elaborate on that?

> If you try to open a window in both click and close, do you get two windows?

The answer is yes.
But the behavior doesn't change whether opening a window in the click event or not.
For example:
// get client from onmessage (client = event.source)
onnotificationclick = function(event) {
    clients.openWindow("foo.com");     // A
    event.notfication.close();
}
onnotificationclose = function(event) {
    client.openWindow("foo.com");      // The client will open a window by A (Won't open a window if without A)       
}

//-------

onnotificationclick = function(event) {
    clients.openWindow("foo.com");     // A
    event.notfication.close();
    event.waitUntil(client.focus());
}
onnotificationclose = function(event) {
    client.openWindow("foo.com");      // The client will open two windows (Will open a window if without A).        
}

Comment 21

[Tom Tung [:tt, :ttung]]

Attachment: P1

patch for only allowing window interaction for the notification click.

Comment 22

[Tom Tung [:tt, :ttung]]

Attachment: P2

Test for verifying P1

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

I guess I'm trying to say when a notificationclick event occurs we should only increase WorkerScope::mWindowInteractionAllowed by one:

https://searchfox.org/mozilla-central/source/dom/workers/WorkerScope.h#65

Currently we are increasing it by two because we call AllowWindowInteraction() twice.  We consume one of those synchronously after the event handler runs, but don't consume the other one until either waitUntil() promise resolves or the timer fires.

So I think we should remove the AllowWindowInteraction() and ConsumeWindowInteraction() calls here:

https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/dom/workers/ServiceWorkerPrivate.cpp#1256
https://searchfox.org/mozilla-central/rev/41925c0b6c6d58578690121de439d2a8d3d690f3/dom/workers/ServiceWorkerPrivate.cpp#1267

And then only create the AllowWindowInteractionHandler if the event is exactly notificationclick instead of any notification event.

Comment 24

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #23)
> And then only create the AllowWindowInteractionHandler if the event is
> exactly notificationclick instead of any notification event.

Oh, I see! So, you are right in comment 15, which is comment 9 can already achieve this. The call for AllowWindowInteraction() in WorkerRun() seems to be redundant. But, I'd like to do it together with comment 9, since the change is small.

Sorry for taking so much time to understand...

I was thinking to avoid allowing the window interaction in the close event (even if it's triggered by the click event). But, I find out I don't need to worry about that because the click event needs users' interaction to trigger.

Comment 25

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1429093 - P1: Only allow notification click to call allowWindowInteraction. r=bkelly

Hi Ben,

This patch is for only allowing notificationclick to increase the mWindowInteractionAllowed token. Also, it removes the extra adding/consuming token in WorkerRun. Would you mind helping me to review this? Thanks!

Comment 26

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1429093 - P2: A test to verify the close event is not allowed to window interaction. r=bkelly

This patch is a test for verifying the P1.

Comment 27

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11c58ba6541ea92bf2368523a4eecb4f9220c2fb

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8943108 [details] [diff] [review]
Bug 1429093 - P1: Only allow notification click to call allowWindowInteraction. r=bkelly

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

Thanks!

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8943109 [details] [diff] [review]
Bug 1429093 - P2: A test to verify the close event is not allowed to window interaction. r=bkelly

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

Since this is currently marked sec-low I guess we can land the test with the change.

::: dom/workers/test/serviceworkers/notificationclose.js
@@ +2,5 @@
>  // http://creativecommons.org/publicdomain/zero/1.0/
>  //
> +onnotificationclose = async function(e) {
> +  let windowOpened = true;
> +  await clients.openWindow("hello.html")

This should probable use waitUntil.

Comment 30

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #29)
> ::: dom/workers/test/serviceworkers/notificationclose.js
> @@ +2,5 @@
> >  // http://creativecommons.org/publicdomain/zero/1.0/
> >  //
> > +onnotificationclose = async function(e) {
> > +  let windowOpened = true;
> > +  await clients.openWindow("hello.html")
> 
> This should probable use waitUntil.

Could you tell me more about it?

I have considered using waitUntil(), but I was not sure how to do it's better.
The main document (test_notificationclose.html) will wait for its iframe's callback (notificationclose.html). The iframe is the client of the service worker (notificationclose.js) and it is waiting for the postMessage() from the service worker. The iframe will notify the main document after receiving the message.

So, I decided to the pass extra information in the postMessage and used await to make sure the promise for openWindow() is rejected before posting the message to the client.

If I want to add waitUtil(), I thought I should add it at the place of client.postMessage(). But, since it's in a for-loop (though, it should just have only one client), I was afraid it would lead the test to be confusing.

BTW, should I uplift this to Beta after I got the security-approval?

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

You can just do the waitUntil around the whole thing.  Instead of passing the async function as the event handler do:

  onnotificationclose = function(evt) {
    evt.waitUntil(async function() {
      // handler body
    }());
  };

If we can land this before next week, then no uplift is necessary.  It's too late to get it in 58.

Comment 32

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #31)
> You can just do the waitUntil around the whole thing.  Instead of passing
> the async function as the event handler do:
> 
>   onnotificationclose = function(evt) {
>     evt.waitUntil(async function() {
>       // handler body
>     }());
>   };

Oh, I see! Thanks!

I was restricting myself to use waitUtil() for a single line...

> If we can land this before next week, then no uplift is necessary.  It's too
> late to get it in 58.

Got it!

Comment 33

[Tom Tung [:tt, :ttung]]

Sorry, but I guess I don't have enough time to fix this (ask for sec-approval, ... etc)

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Thank you Tom.  You've done really great work.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

I'll get this landed.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2: A test to verify the close event is not allowed to window interaction. r=bkelly

Updated based on my review feedback.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Pushed since this is sec-low and doesn't require sec-approval:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/20b048f7a32fc9d76827c2f06be1d50afb8bbd00
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/abd11ceb68ca7f0b04a4e4b3dc49090c523ad067

Comment 38

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

https://hg.mozilla.org/mozilla-central/rev/20b048f7a32f
https://hg.mozilla.org/mozilla-central/rev/abd11ceb68ca

Comment 39

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 40

[Zohar]

Hey, Glad you got this sorted! 
thanks for the hard work!

Will there be a security bounty attached to this bug? 

Cheers, 
Zohar

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

This is not for me to decide.  The security team will does a periodic review to determine that.

I will request beta uplift.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8943108 [details] [diff] [review]
Bug 1429093 - P1: Only allow notification click to call allowWindowInteraction. r=bkelly

Approval Request Comment
[Feature/Bug causing the regression]: Service worker push notifications
[User impact if declined]: A malicious website with push permissions could spam the user with notifications and new tabs.
[Is this code covered by automated tests?]: Test in P2 patch.
[Has the fix been verified in Nightly?]: The fix has been in nightly for a day and it has a test.  I don't think we have formally verified it, though.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: The change is contained to only notification related code and relatively small.  The risk of functional regressions is low.
[String changes made/needed]: None

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8944821 [details] [diff] [review]
P2: A test to verify the close event is not allowed to window interaction. r=bkelly

See comment 42.

Comment 44

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8943108 [details] [diff] [review]
Bug 1429093 - P1: Only allow notification click to call allowWindowInteraction. r=bkelly

Regression from 58, has test coverage. Let's uplift this for 59 beta 6.

Comment 45

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ca41b3ad3577
https://hg.mozilla.org/releases/mozilla-beta/rev/57b52fb5a9a5

Comment 46

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

(In reply to Zohar from comment #16)
> Hey guys, 
> I just want to add my opinion as a security researcher, 
> I think  the potential impact of this bug is quite severe (I saw you marked
> the bug as a 'low risk' security event)

DOS attacks are generally rated as "low" per our rating guidelines at https://wiki.mozilla.org/Security_Severity_Ratings, linked to from our security pages at http://www.mozilla.org/security/. DOS attacks are only rated about sec-low if they are persistent, which this attack isn't. 

(In reply to Zohar from comment #40)

> Will there be a security bounty attached to this bug? 

Not by default, no. If you are interested in the bug bounty program, you need to write to us at our [e-mail] email address and ask it to be considered for bounty. Please read the program guidelines at https://www.mozilla.org/en-US/security/client-bug-bounty/. We will not automatically flag bugs for bounty except in very unusual circumstances as there is no guarantee that people from the bounty committee will see all issues.