1000337 - Notification.get() returns notification of other applications in non-OOP

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[:gerard-majax]

While running tests on a b2g desktop with OOP enabled for bug 963234, I noticed some test failing.

In the test defined at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/marionette/notification_get_test.js#L140, we switch to the system app and do a Notification.get({tag: tag}). However, the check for the title is performed against the value emailTitle, instead of systemTitle, since we are fetching a notification in the system app, we will only get notifications from system app, not email app.

Comment 1

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/18600

Please find attached a link to the trivial github pull request that fixes the test.

Comment 2

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8411170 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18600

This is failing on Travis:
https://travis-ci.org/mozilla-b2g/gaia/jobs/23610607#L2795

What I think is happening is that without OOP, Notification.get is returning the wrong notification for the email app (ie. it also returns system app notifications). This is obviously wrong, and we should either investigate this non-OOP failure, or make your change but disable this test until they all run OOP.

Comment 3

[:gerard-majax]

(In reply to Michael Henretty [:mhenretty] from comment #2)
> Comment on attachment 8411170 [details] [review]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/18600
> 
> This is failing on Travis:
> https://travis-ci.org/mozilla-b2g/gaia/jobs/23610607#L2795
> 
> What I think is happening is that without OOP, Notification.get is returning
> the wrong notification for the email app (ie. it also returns system app
> notifications). This is obviously wrong, and we should either investigate
> this non-OOP failure, or make your change but disable this test until they
> all run OOP.

This code passes test when I run OOP B2G Desktop with my changes for mulet, for bug 963234. So if you agree with the fix itself, we can just skip the test for now and re-enable it once bug 963234 lands

Comment 4

[:gerard-majax]

I suspect it's because NotificationStorage.js' caching byTag is not origin-aware :)

Comment 5

[:gerard-majax]

I fear this is present in the Gecko of v1.3

Comment 6

[Anne (:annevk)]

This seems like a privacy/security bug of sorts.

Comment 7

[Stephanie Ouillon [:arroway] (needinfo me)]

I'll take a look at it, thanks.

Comment 8

[:gerard-majax]

Attachment: 0001-Bug-1000337-Make-NotificationStorage-cache-origin-aw.patch

Gecko-side fix in NotificationStorage: adding a layer of origin into the this._byTag caching. So far that I could test, B2G Desktop non OOP and OOP passes all notification tests (mochitest and gaia integration tests).

Comment 9

[Jason Smith [:jsmith]]

notification.get was first supported in 1.3.

Comment 10

[Jason Smith [:jsmith]]

Paul - I need a sec-rating on this to make a blocking call & an assessment if you think this is a blocker on the security side.

Comment 11

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Jason Smith [:jsmith] from comment #10)
> Paul - I need a sec-rating on this to make a blocking call & an assessment
> if you think this is a blocker on the security side.

From reading this bug, my understanding that if an app is not OOP, then notifcations.get might return another apps notifications? Apps are always OOP on Firefox OS, except for system and browser. So this doesnt sound like it is worth blocking production for. (correct me if my understanding is wrong though)

Comment 12

[Jason Smith [:jsmith]]

Backlog per comment 11.

Comment 13

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8411827 [details] [diff] [review]
0001-Bug-1000337-Make-NotificationStorage-cache-origin-aw.patch

This is a good start, and thanks for fixing this so fast!

We also need to cache `this._notifications` by origin too, since Notification.get without specifying a tag will also get cross origin notifications.

Comment 14

[:gerard-majax]

(In reply to Michael Henretty [:mhenretty] from comment #13)
> Comment on attachment 8411827 [details] [diff] [review]
> 0001-Bug-1000337-Make-NotificationStorage-cache-origin-aw.patch
> 
> This is a good start, and thanks for fixing this so fast!
> 
> We also need to cache `this._notifications` by origin too, since
> Notification.get without specifying a tag will also get cross origin
> notifications.

I think I already addressed this issue in my patch:

>      } else if (!tag) {
>        for (var id in this._notifications) {
> -        notifications.push(this._notifications[id]);
> +        if (this._notifications[id].origin === origin) {
> +          notifications.push(this._notifications[id]);
> +        }
>        }
>      }

This should make sure we only return notifications with the matching origin. Did I missed it at some other place?

Comment 15

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Alexandre LISSY :gerard-majax from comment #14)
> (In reply to Michael Henretty [:mhenretty] from comment #13)
> > Comment on attachment 8411827 [details] [diff] [review]
> > 0001-Bug-1000337-Make-NotificationStorage-cache-origin-aw.patch
> > 
> > This is a good start, and thanks for fixing this so fast!
> > 
> > We also need to cache `this._notifications` by origin too, since
> > Notification.get without specifying a tag will also get cross origin
> > notifications.
> 
> I think I already addressed this issue in my patch:
> 
> >      } else if (!tag) {
> >        for (var id in this._notifications) {
> > -        notifications.push(this._notifications[id]);
> > +        if (this._notifications[id].origin === origin) {
> > +          notifications.push(this._notifications[id]);
> > +        }
> >        }
> >      }
> 
> This should make sure we only return notifications with the matching origin.
> Did I missed it at some other place?

This is why I shouldn't do reviews at midnight. Sorry, I'll take a look again tomorrow.

Comment 16

[:gerard-majax]

Michael, I've updated the PR to skip the test for now, with a comment stating to re-enable it once gecko part lands. My proposal would be that you review this gaia patch and that we merge it before the gecko part lands. This way, we avoid breaking travis when gecko part lands and we can re-enable the test at this time.

Comment 17

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Alexandre LISSY :gerard-majax from comment #16)
> Michael, I've updated the PR to skip the test for now, with a comment
> stating to re-enable it once gecko part lands. My proposal would be that you
> review this gaia patch and that we merge it before the gecko part lands.
> This way, we avoid breaking travis when gecko part lands and we can
> re-enable the test at this time.

Makes sense to me.

Comment 18

[:gerard-majax]

PR merged: https://github.com/mozilla-b2g/gaia/commit/4457f14677f3a5ef626b7905dca584681401acc3

Comment 19

[Julien Wajsberg [:julienw]]

STR for Firefox:
* go to http://jsfiddle.net/PhR9C/
* accept that the notification is displayed
* go to http://everlong.org/mozilla/testcase-notification-nsa/
* accept that the notification is displayed

=> The page on everlong.org can access the notification created on jsfiddle.net

Comment 20

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8411827 [details] [diff] [review]
0001-Bug-1000337-Make-NotificationStorage-cache-origin-aw.patch

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

LGTM.

Thinking about it, I would prefer to also keep `this._notifications` grouped by origin as well, so that would wouldn't have to loop through every origin's notifications when one page calls Notification.get(). But in the end, that's a micro-optimization, so I will leave it up to you.

Comment 21

[Gregor Wagner [:gwagner]]

Alex, also ask for sec-approval please.

Comment 22

[:gerard-majax]

Comment on attachment 8411827 [details] [diff] [review]
0001-Bug-1000337-Make-NotificationStorage-cache-origin-aw.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
> Very easily, according to comment 19

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

Which older supported branches are affected by this flaw?
> Nothing older than october 2013

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
> This patch is simple enough to be applied on all branches

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

Comment 23

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

We need to have a security rating on this before it is approved. Do you have a suggested rating?

Comment 24

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

This is sec-high on discussion with Dveditz. Please check this in on May 20 for trunk. I'll give sec-approval+ now though.

After it goes in, we should make Aurora and Beta patches for this.

Comment 25

[:gerard-majax]

Paul, I know some stuff running on Tarako is not OOP, for example Keyboard. So the impact on v1.3 may be more important than what has been considered at first.

Comment 26

[Julien Wajsberg [:julienw]]

Right, I think the browser chrome is not OOP either (but the content is!).

Comment 27

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Alexandre LISSY :gerard-majax from comment #25)
> Paul, I know some stuff running on Tarako is not OOP, for example Keyboard.
> So the impact on v1.3 may be more important than what has been considered at
> first.

Right, I wasn't aware of that, thanks. I think that this also means that third-party keyboards are disabled too? (I tried to check this here: [1] and it looks like that case, unless its enabled somewhere else). 

I don't have a Tarako yet (one is on route) but can someone else confirm if there are any other apps which are not oop, apart from:
- system
- browser(pages loaded remotely though)
- keyboard (3rd party keyboards disabled)

You can check with b2g-ps I think.

If there is any chance that 3rd party content will be run in the parent process, then its probably worth fixing this - but we really shouldn't have this situation I think (since it basically means browsing the web as root).


[1] https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/system/js/keyboard_manager.js#L107

Comment 28

[:gerard-majax]

Messages app and Keyboard opened:
> $ adb shell b2g-ps
> APPLICATION      USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
> b2g              root      83    1     152268 37872 ffffffff 4003f724 R /system/b2g/b2g
> (Nuwa)           root      337   83    50536  3552  ffffffff 40097518 S /system/b2g/plugin-container
> Homescreen       app_554   554   337   65028  14132 ffffffff 40097518 S /system/b2g/plugin-container
> Messages         app_742   742   337   66352  20752 ffffffff 40097518 S /system/b2g/plugin-container
> (Preallocated a  root      956   337   54624  9636  ffffffff 40d7e882 R /system/b2g/plugin-container

So builtin keyboard is NOT OOP.

Comment 29

[:gerard-majax]

Browser opened with two tabs loaded (google.com):
> $ adb shell b2g-ps
> APPLICATION      USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
> b2g              root      83    1     146464 38936 ffffffff 400a8518 S /system/b2g/b2g
> (Nuwa)           root      340   83    50536  6860  ffffffff 400b5518 S /system/b2g/plugin-container
> Homescreen       app_360   360   340   66052  12344 ffffffff 400b5518 S /system/b2g/plugin-container
> Browser          app_530   530   340   68080  25940 ffffffff 400b5518 S /system/b2g/plugin-container
> (Preallocated a  root      538   340   54624  9752  ffffffff 400b45cc R /system/b2g/plugin-container

Comment 30

[Julien Wajsberg [:julienw]]

Maybe you should try 2 websites in different origins.

Comment 31

[:gerard-majax]

Attachment: 0001-Bug-1000337-Make-NotificationStorage-cache-origin-aw.patch

Patch rebased against master.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b15c3dcd60

Calling this in-testsuite+ since it was caught by our existing tests. Please nominate this for Aurora/Beta approval as soon as you get a chance :)

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[:gerard-majax]

Comment on attachment 8420519 [details] [diff] [review]
0001-Bug-1000337-Make-NotificationStorage-cache-origin-aw.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: leaking of data to third party
Testing completed (on m-c, etc.): unit tests included, tested on device
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6660863c0aa
https://hg.mozilla.org/releases/mozilla-beta/rev/d520b0344613
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/c7e94b2d3718

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d520b0344613

Comment 37

[:gerard-majax]

Comment on attachment 8411170 [details] [review]
Link to Github https://github.com/mozilla-b2g/gaia/pull/18600

This gaia commit should have been uplifted to v1.3t also. This is making some tests failing: https://travis-ci.org/mozilla-b2g/gaia/jobs/25839681

Comment 38

[:gerard-majax]

Attachment: Link to github pull request https://github.com/mozilla-b2g/gaia/pull/19592

Comment 39

[:gerard-majax]

Comment on attachment 8427650 [details]
Link to github pull request https://github.com/mozilla-b2g/gaia/pull/19592

>https://github.com/mozilla-b2g/gaia/pull/19592

Comment 40

[:gerard-majax]

This new PR should take care of just fixing the test.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8427650 [details]
Link to github pull request https://github.com/mozilla-b2g/gaia/pull/19592

1.3t: https://github.com/mozilla-b2g/gaia/commit/44aeb45296737299a988b4d91e3f0c1059caec08

Comment 42

[:gerard-majax]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #41)
> Comment on attachment 8427650 [details]
> Link to github pull request https://github.com/mozilla-b2g/gaia/pull/19592
> 
> 1.3t:
> https://github.com/mozilla-b2g/gaia/commit/
> 44aeb45296737299a988b4d91e3f0c1059caec08

Thans Ryan. Green travis for this PR is https://travis-ci.org/mozilla-b2g/gaia/builds/25858585

Comment 43

[Julien Wajsberg [:julienw]]

Seems like we have the same tests issue on v1.3: [1].

So here is a PR for v1.3 [2] and a travis run in [3].

[1] https://travis-ci.org/mozilla-b2g/gaia/jobs/26215343
[2] https://github.com/mozilla-b2g/gaia/pull/19878
[3] https://travis-ci.org/mozilla-b2g/gaia/builds/26579673

Comment 44

[Julien Wajsberg [:julienw]]

v1.3: 56f15bef5b23dcb239bfa14bd691f530ab46e603

Comment 45

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #43)
> Seems like we have the same tests issue on v1.3: [1].
> 
> So here is a PR for v1.3 [2] and a travis run in [3].
> 
> [1] https://travis-ci.org/mozilla-b2g/gaia/jobs/26215343
> [2] https://github.com/mozilla-b2g/gaia/pull/19878
> [3] https://travis-ci.org/mozilla-b2g/gaia/builds/26579673

And again on v1.4 ...

Comment 46

[Julien Wajsberg [:julienw]]

test failure on v1.4: https://travis-ci.org/mozilla-b2g/gaia/jobs/27294892
I don't know why this happens only now.

Comment 47

[Julien Wajsberg [:julienw]]

PR: https://github.com/mozilla-b2g/gaia/pull/20365
Waiting for Travis

Comment 48

[Julien Wajsberg [:julienw]]

Travis is green (I canceled all tests beside the marionette one), landing
v1.4: df75050ea29bc6ffd627f200eeb91ff99503163f

Comment 49

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-release/rev/972b1d6b4f1b