971379 - Allow certified apps to call mozID API outside an event handler

Status: RESOLVED FIXED

(Core Graveyard :: Identity, defect)

Description

[Sam Penrose]

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

Comment 1

[Sam Penrose]

GGP, is this still a blocker for you? (Jed assigning to you so we don't lose track of it.)

Comment 2

[Guilherme Gonçalves [:ggp]]

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.

Comment 3

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 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.

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

oh ****.  working in the wrong tree.  just a sec.

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Synthetic events always ok with certified apps

Ok that's actually a patch this time :)

Comment 6

[Guilherme Gonçalves [:ggp]]

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.

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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 8

[Guilherme Gonçalves [:ggp]]

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

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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.

Comment 10

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 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.

Comment 11

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 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

Comment 12

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Synthetic events always ok with certified and packaged apps

Fixed typos, comments

Comment 13

[Sam Penrose]

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 :-)

Comment 14

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Thank you, Sam!

Comment 15

[Guilherme Gonçalves [:ggp]]

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!

Comment 16

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Great news!

Comment 17

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Synthetic events always ok with certified and packaged apps

Rebased over bug 947374 attachment 8384776 [details] [diff] [review]

Comment 18

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 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

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 20

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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?

Comment 21

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Synthetic events always ok with certified and packaged apps

r=ferjm

needs security review for packaged apps bits

Comment 22

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Opened Bug 982460 for granting these privileges to packaged apps.  I will pursue the security review there.

Comment 23

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 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

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 25

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6be642b729

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a6be642b729