All users were logged out of Bugzilla on October 13th, 2018

Allow certified apps to call mozID API outside an event handler

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: spenrose, Assigned: jedp)

Tracking

unspecified
mozilla30
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

15.24 KB, patch
ferjm
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
To prevent malicious scripts from hijacking Persona and FxA functionality, the mozID API is restricted to callbacks triggered by user actions. For certified and privileged apps, remove this restriction so they don't need to play the iframe game. Desired for WMF as part of 1.4
(Reporter)

Updated

5 years ago
Blocks: 955952
(Reporter)

Updated

5 years ago
Blocks: 955951
No longer blocks: 955952
(Reporter)

Comment 1

5 years ago
GGP, is this still a blocker for you? (Jed assigning to you so we don't lose track of it.)
Assignee: nobody → jparsons
Flags: needinfo?(ggoncalves)
I believe so. My use case is: if something goes wrong with the Find My Device server, the client will need to re-register, for which the server requires an assertion. Since assertions expire after a short period, I'll need to be able to obtain a fresh one without user interaction (since this is all happening in the System app, no UI involved) when that happens.
Flags: needinfo?(ggoncalves)
Created attachment 8381534 [details] [diff] [review]
Synthetic events always ok with certified apps

Untested! But I think this should do what you want.

:ggp, please give it a try if you have time.  I'll test as soon as I can.
Attachment #8381534 - Flags: feedback?(ggoncalves)
Status: NEW → ASSIGNED
oh ****.  working in the wrong tree.  just a sec.
Created attachment 8381539 [details] [diff] [review]
Synthetic events always ok with certified apps

Ok that's actually a patch this time :)
Attachment #8381534 - Attachment is obsolete: true
Attachment #8381534 - Flags: feedback?(ggoncalves)
Attachment #8381539 - Flags: review?(ggoncalves)
Comment on attachment 8381539 [details] [diff] [review]
Synthetic events always ok with certified apps

Hi Jed, thank you so much for the patch. Would it be too much work to rebase the patch at bug 947374? I would very much like to test this patch along with that one, if possible.
Attachment #8381539 - Flags: review?(ggoncalves) → feedback?(ggoncalves)
Sorry if I accidentally flagged that as review!  Meant to hit feedback.

Yes - I've rebased bug 947374.

Note that we'll need to declare the assertion audience in watch(), not in request().

This patch applies cleanly over that one.

Thanks for looking!
j
Comment on attachment 8381539 [details] [diff] [review]
Synthetic events always ok with certified apps

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

Just tried both patches together.

First of all, for some reason this patch still conflicted with the one at bug 947374 on current m-c (at least when using git bz apply, which downloads the patch and runs it through git am as far as I know). I did manage to apply the patch manually, though. It is also worth noting that both this patch and the other append the same line to services/fxaccounts/tests/moz.build, which causes a build failure.

Anyway, I eventually got to try them, and sadly this didn't work. I called request() from outside an event handler [1], but my onlogin callback never got called, and the console reads "nsDOMIdentity ({e9f59b18-a0b6-b545-84d6-da0d33286544}): request: rejecting non-native event". In case this helps, I also dump()'d aOptions.appStatus to see what was going on, and it seems it was undefined.

1- https://github.com/guilherme-pg/gaia/blob/wimf_fxa/apps/findmydevice/js/findmydevice.js#L137
Attachment #8381539 - Flags: feedback?(ggoncalves) → feedback-
Thanks for the debug line - that's very helpful.

I'm building with your gaia branch and these patches to track down the problem today.  Thanks for your patience.
Created attachment 8382521 [details] [diff] [review]
Synthetic events always ok with certified apps

I am a moron.  This should work.

Tested successfully over the patch for Bug 947374, using your wimf_fxa gaia branch.
Attachment #8381539 - Attachment is obsolete: true
Attachment #8382521 - Flags: feedback?(ggoncalves)
Created attachment 8383361 [details] [diff] [review]
Synthetic events always ok with certified and packaged apps

- Rebased over Bug 947374 attachment 8383328 [details] [diff] [review]
- Added mochitests for installed, packaged, and certified apps
- Refactored error reporting in nsDOMIdentity a bit

:ggp, does this work for you?
spenrose, do you have any feedback?
Thanks!
j
Attachment #8382521 - Attachment is obsolete: true
Attachment #8382521 - Flags: feedback?(ggoncalves)
Attachment #8383361 - Flags: feedback?(spenrose)
Attachment #8383361 - Flags: feedback?(ggoncalves)
Created attachment 8383376 [details] [diff] [review]
Synthetic events always ok with certified and packaged apps

Fixed typos, comments
Attachment #8383361 - Attachment is obsolete: true
Attachment #8383361 - Flags: feedback?(spenrose)
Attachment #8383361 - Flags: feedback?(ggoncalves)
Attachment #8383376 - Flags: feedback?(spenrose)
Attachment #8383376 - Flags: feedback?(ggoncalves)
(Reporter)

Comment 13

5 years ago
Comment on attachment 8383376 [details] [diff] [review]
Synthetic events always ok with certified and packaged apps

This looks good, thanks so much. Excellent ratio of tests to new code :-)
Attachment #8383376 - Flags: feedback?(spenrose) → feedback+
Thank you, Sam!
Comment on attachment 8383376 [details] [diff] [review]
Synthetic events always ok with certified and packaged apps

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

It's working for me, thanks Jed!
Attachment #8383376 - Flags: feedback?(ggoncalves) → feedback+
Created attachment 8385005 [details] [diff] [review]
Synthetic events always ok with certified and packaged apps

Rebased over bug 947374 attachment 8384776 [details] [diff] [review]
Attachment #8383376 - Attachment is obsolete: true
Created attachment 8387834 [details] [diff] [review]
Synthetic events always ok with certified and packaged apps

This patch:

- confirms that packaged and certified apps can request() a firefox accounts assertion outside a user input handler.  That is, they can call navigator.mozId.request() at any time, without a user click, because we trust them.

- reconfirms that an installed app cannot use fxa.  (Since it gets shut down on watch(), it never gets to call request() anywhere.)

- confirms that a Persona RP cannot call request() when not handling user input.

Thanks for this and all your other reviews lateley, Fernando!
j
Attachment #8385005 - Attachment is obsolete: true
Attachment #8387834 - Flags: review?(ferjmoreno)
Comment on attachment 8387834 [details] [diff] [review]
Synthetic events always ok with certified and packaged apps

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

LGTM, but I'd like to get an OK from the security team before landing this change.

::: dom/identity/DOMIdentity.jsm
@@ +171,5 @@
> +   * Mockable, for testing
> +   */
> +  _mockIdentityService: null,
> +  get IdentityService() {
> +    if (this._mockIdentityService) { 

nit: trailing whitespace.

@@ +330,5 @@
>  
>    _watch: function DOMIdentity__watch(message, targetMM) {
>      log("DOMIdentity__watch: " + message.id);
>      let context = this.newContext(message, targetMM);
> +    let service = this.getService(message);

It seems that you don't need this unless you use 'service' in the next line.

::: dom/identity/nsDOMIdentity.js
@@ +194,3 @@
>      let message = this.DOMIdentityMessage(aOptions);
>  
> +    // We permit calling of request() outside of a user input handler is when

s/handler is when/handler when ?

@@ +198,5 @@
> +    // which make use of an RP context marked as _internal, or when a certified
> +    // or privileged app is calling.
> +    if (!aOptions._internal &&
> +        this._appStatus !== Ci.nsIPrincipal.APP_STATUS_CERTIFIED &&
> +        this._appStatus !== Ci.nsIPrincipal.APP_STATUS_PRIVILEGED) {

I would like to get a verification from the security team that they are ok with this.

@@ +633,5 @@
>      // Currently, we only permit certified and privileged apps to use
>      // Firefox Accounts.
> +    if (aOptions.wantIssuer == "firefox-accounts") {
> +      if (this._appStatus !== principal.APP_STATUS_PRIVILEGED &&
> +          this._appStatus !== principal.APP_STATUS_CERTIFIED) {

Can you group the condition in one if, please?

::: dom/identity/tests/mochitest/test_syntheticEvents.html
@@ +35,5 @@
> +    return Promise.resolve("here~you.go.dude");
> +  }
> +};
> +
> +let originalManager = FirefoxAccounts.fxAccountsManager;

You are not restoring the original manager.

::: toolkit/identity/FirefoxAccounts.jsm
@@ +32,5 @@
>  }
>  
>  let log = Log.repository.getLogger("Identity.FxAccounts");
>  log.level = LOG_LEVEL;
> +//log.addAppender(new Log.ConsoleAppender(new Log.BasicFormatter()));

Uncomment this line
Attachment #8387834 - Flags: review?(ptheriault)
Attachment #8387834 - Flags: review?(jonas)
Attachment #8387834 - Flags: review?(ferjmoreno)
Attachment #8387834 - Flags: review+
(In reply to Fernando Jiménez Moreno [:ferjm] (Work week from 10/3 to 14/3. Low bugmail activity). from comment #19)
> Comment on attachment 8387834 [details] [diff] [review]
> Synthetic events always ok with certified and packaged apps
> 
> Review of attachment 8387834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, but I'd like to get an OK from the security team before landing this
> change.

Thanks for your review, Fernando.  I'm particularly grateful to have your feedback while you are participating in a work-week on a completely unrelated topic.

> ::: dom/identity/DOMIdentity.jsm
> @@ +171,5 @@
> > +   * Mockable, for testing
> > +   */
> > +  _mockIdentityService: null,
> > +  get IdentityService() {
> > +    if (this._mockIdentityService) { 
> 
> nit: trailing whitespace.

Oops.  Sorry

> @@ +330,5 @@
> >  
> >    _watch: function DOMIdentity__watch(message, targetMM) {
> >      log("DOMIdentity__watch: " + message.id);
> >      let context = this.newContext(message, targetMM);
> > +    let service = this.getService(message);
> 
> It seems that you don't need this unless you use 'service' in the next line.

Quite right.  Removed.

> ::: dom/identity/nsDOMIdentity.js
> @@ +194,3 @@
> >      let message = this.DOMIdentityMessage(aOptions);
> >  
> > +    // We permit calling of request() outside of a user input handler is when
> 
> s/handler is when/handler when ?

Wow, that sentence is broken.  Fixed.

> @@ +198,5 @@
> > +    // which make use of an RP context marked as _internal, or when a certified
> > +    // or privileged app is calling.
> > +    if (!aOptions._internal &&
> > +        this._appStatus !== Ci.nsIPrincipal.APP_STATUS_CERTIFIED &&
> > +        this._appStatus !== Ci.nsIPrincipal.APP_STATUS_PRIVILEGED) {
> 
> I would like to get a verification from the security team that they are ok
> with this.

Ok.

> @@ +633,5 @@
> >      // Currently, we only permit certified and privileged apps to use
> >      // Firefox Accounts.
> > +    if (aOptions.wantIssuer == "firefox-accounts") {
> > +      if (this._appStatus !== principal.APP_STATUS_PRIVILEGED &&
> > +          this._appStatus !== principal.APP_STATUS_CERTIFIED) {
> 
> Can you group the condition in one if, please?

Fixed

> ::: dom/identity/tests/mochitest/test_syntheticEvents.html
> @@ +35,5 @@
> > +    return Promise.resolve("here~you.go.dude");
> > +  }
> > +};
> > +
> > +let originalManager = FirefoxAccounts.fxAccountsManager;
> 
> You are not restoring the original manager.

Oh thanks for catching that!  Fixed.

> ::: toolkit/identity/FirefoxAccounts.jsm
> @@ +32,5 @@
> >  }
> >  
> >  let log = Log.repository.getLogger("Identity.FxAccounts");
> >  log.level = LOG_LEVEL;
> > +//log.addAppender(new Log.ConsoleAppender(new Log.BasicFormatter()));
> 
> Uncomment this line

Ah - sorry about that.  Fixed.

Thanks for your review, Fernando.

So that we can both give the security team time to review the powers this grants to privileged apps, and also unblock certified apps like Where's My Fox, would you be ok with me splitting this patch into two parts, one for certified apps that we can land now, and a follow-up for privileged apps?
Flags: needinfo?(ferjmoreno)
Created attachment 8389272 [details] [diff] [review]
Synthetic events always ok with certified and packaged apps

r=ferjm

needs security review for packaged apps bits
Attachment #8387834 - Attachment is obsolete: true
Attachment #8387834 - Flags: review?(ptheriault)
Attachment #8387834 - Flags: review?(jonas)
Opened Bug 982460 for granting these privileges to packaged apps.  I will pursue the security review there.
Summary: Allow certified and privileged apps to call mozID API outside an event handler → Allow certified apps to call mozID API outside an event handler
Created attachment 8389578 [details] [diff] [review]
Synthetic events always ok with certified apps

Fernando, I've removed packaged apps from this patch so that it only grants the privilege in question to certified apps.  I've opened Bug 982460 to deal with the security issues surrounding packaged apps separately.

Does this remove your security concerns for the present patch?  I'd like to be able to land something that unblocks our certified apps as soon as possible.

Thanks!
j
Attachment #8389272 - Attachment is obsolete: true
Attachment #8389578 - Flags: feedback?(ferjmoreno)
Comment on attachment 8389578 [details] [diff] [review]
Synthetic events always ok with certified apps

I guess that we are ok allowing this to certified apps.
Attachment #8389578 - Flags: feedback?(ferjmoreno) → feedback+
Flags: needinfo?(ferjmoreno)
Cool.  I thought so, too.  After all, they are the system and can do whatever they want anyway.

Thanks for your feedback, Fernando.  I'll check this in and then pursue bug 982460.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7a6be642b729
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.