Closed Bug 1000337 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
tracking-b2g backlog
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 + fixed
firefox31 + fixed
firefox32 + fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
seamonkey2.26 --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Keywords: csectype-sop, sec-high, Whiteboard: [systemsfe][adv-main30-])

Attachments

(3 files, 1 obsolete file)

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.
Flags: needinfo?(mhenretty)
Please find attached a link to the trivial github pull request that fixes the test.
Assignee: nobody → lissyx+mozillians
Status: NEW → ASSIGNED
Attachment #8411170 - Flags: review?(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.
Attachment #8411170 - Flags: review?(mhenretty)
Flags: needinfo?(mhenretty)
Whiteboard: [systemsfe]
(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
Flags: needinfo?(mhenretty)
I suspect it's because NotificationStorage.js' caching byTag is not origin-aware :)
I fear this is present in the Gecko of v1.3
blocking-b2g: --- → 1.3?
Summary: Notification.get() gaia integration test issue on cross domain testing → Notification.get({tag: tag}) returns notification of other applications
This seems like a privacy/security bug of sorts.
I'll take a look at it, thanks.
Group: b2g-core-security
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).
Attachment #8411827 - Flags: review?(mhenretty)
Group: b2g-core-security → core-security
Component: Gaia::System → General
Product: Firefox OS → Core
notification.get was first supported in 1.3.
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.
Flags: needinfo?(ptheriault)
Blocks: 899574
(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)
Flags: needinfo?(ptheriault)
Backlog per comment 11.
blocking-b2g: 1.3? → backlog
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.
Attachment #8411827 - Flags: review?(mhenretty) → review-
Flags: needinfo?(mhenretty)
(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?
Flags: needinfo?(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.
Flags: needinfo?(mhenretty)
Summary: Notification.get({tag: tag}) returns notification of other applications → Notification.get() returns notification of other applications in non-OOP
Attachment #8411827 - Flags: review- → review?(mhenretty)
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.
Flags: needinfo?(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.
Flags: needinfo?(mhenretty)
Depends on: 997949
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 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.
Attachment #8411827 - Flags: review?(mhenretty) → review+
Alex, also ask for sec-approval please.
Flags: needinfo?(lissyx+mozillians)
Target Milestone: --- → 2.0 S1 (9may)
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,
Attachment #8411827 - Flags: sec-approval?
Flags: needinfo?(lissyx+mozillians)
We need to have a security rating on this before it is approved. Do you have a suggested rating?
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.
Whiteboard: [systemsfe] → [systemsfe][checkin on 5/20/14]
Attachment #8411827 - Flags: sec-approval? → sec-approval+
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.
Flags: needinfo?(ptheriault)
Right, I think the browser chrome is not OOP either (but the content is!).
(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
Flags: needinfo?(ptheriault)
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.
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
Maybe you should try 2 websites in different origins.
Patch rebased against master.
Attachment #8411827 - Attachment is obsolete: true
Attachment #8420519 - Flags: sec-approval?
Attachment #8420519 - Flags: review+
Attachment #8420519 - Flags: sec-approval? → sec-approval+
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 :)
Flags: needinfo?(lissyx+mozillians)
Flags: in-testsuite+
Whiteboard: [systemsfe][checkin on 5/20/14] → [systemsfe]
Target Milestone: 2.0 S1 (9may) → ---
https://hg.mozilla.org/mozilla-central/rev/f7b15c3dcd60
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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
Attachment #8420519 - Flags: approval-mozilla-beta?
Attachment #8420519 - Flags: approval-mozilla-aurora?
Flags: needinfo?(lissyx+mozillians)
Attachment #8420519 - Flags: approval-mozilla-beta?
Attachment #8420519 - Flags: approval-mozilla-beta+
Attachment #8420519 - Flags: approval-mozilla-aurora?
Attachment #8420519 - Flags: approval-mozilla-aurora+
Blocks: 1013793
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
Attachment #8411170 - Flags: checkin?(cbook)
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
Attachment #8427650 - Attachment description: Link → Link to github pull request https://github.com/mozilla-b2g/gaia/pull/19592
Attachment #8427650 - Flags: checkin?(cbook)
This new PR should take care of just fixing the test.
Attachment #8411170 - Flags: checkin?(cbook)
Attachment #8427650 - Flags: checkin+
(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
Depends on: 1015572
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
v1.3: 56f15bef5b23dcb239bfa14bd691f530ab46e603
Whiteboard: [systemsfe] → [systemsfe][adv-main30-]
(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 ...
test failure on v1.4: https://travis-ci.org/mozilla-b2g/gaia/jobs/27294892
I don't know why this happens only now.
Travis is green (I canceled all tests beside the marionette one), landing
v1.4: df75050ea29bc6ffd627f200eeb91ff99503163f
No longer depends on: 1015572
blocking-b2g: backlog → ---
Group: core-security
You need to log in before you can comment on or make changes to this bug.