Paul Theriault [:pauljt] (no longer reading bugmail) Reporter
	Description • 11 years ago

From discussing the Datastore API with Ehsan, my understanding is that Datastore will be certified only for 1.3 in lieu of a proper permission model, which is planned for 1.4.

As far as I can tell, there is no restriction at all though, so I am raising this just to make sure that certified only is enforced.

	Jason Smith [:jsmith]
	Comment 1 • 11 years ago

This probably needs to block 1.3, right?

	(no longer active)
	Comment 2 • 11 years ago

(In reply to comment #1)
> This probably needs to block 1.3, right?

Yes.

	(no longer active)
	Comment 4 • 11 years ago

Comment on attachment 8338506 [details] [diff] [review]
certified.patch

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

I'd like Fabrice to review the Webapps.jsm changes too.

	Andrea Marchesini [:baku] Assignee
	Comment 5 • 11 years ago

renamed the pref to dom.mozApps.allow_app_status_override

	[:fabrice] Fabrice Desré
	Comment 6 • 11 years ago

Comment on attachment 8338536 [details] [diff] [review]
certified.patch

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

I would also use https://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl#443 to check content process status everywhere we enter receiveMessage() in the parent, killing the child if that fails.

::: dom/apps/src/Webapps.jsm
@@ +286,5 @@
>        this.updateDataStore(this.webapps[aId].localId,
>                             this.webapps[aId].origin,
>                             this.webapps[aId].manifestURL,
> +                           aResult[0].manifest,
> +                           this.webapps[aId].appStatus);

doing let app = this.webapps[aId] and reusing app would not hurt ;)

	Andrea Marchesini [:baku] Assignee
	Comment 7 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=69473818adb2

if green I land it.
Fabrice, I don't think we should kill the app if DataStore API is used. It just does not return data.

	[:fabrice] Fabrice Desré
	Comment 8 • 11 years ago

(In reply to Andrea Marchesini (:baku) from comment #7)
> https://tbpl.mozilla.org/?tree=Try&rev=69473818adb2
> 
> if green I land it.
> Fabrice, I don't think we should kill the app if DataStore API is used. It
> just does not return data.

We do that for all other apis when apps don't have the permission to access data. I don't see how this is different here. Let's see what Paul thinks before landing.

	(no longer active)
	Comment 9 • 11 years ago

Comment on attachment 8338536 [details] [diff] [review]
certified.patch

Wait, I don't know what I was thinking when reviewing this patch...  In addition to all of this, we should not expose navigator.getDataStores and any of the data store interfaces to non-certified apps.

	Paul Theriault [:pauljt] (no longer reading bugmail) Reporter
	Comment 10 • 11 years ago

(In reply to Fabrice Desré [:fabrice] from comment #8)
> (In reply to Andrea Marchesini (:baku) from comment #7)
> > https://tbpl.mozilla.org/?tree=Try&rev=69473818adb2
> > 
> > if green I land it.
> > Fabrice, I don't think we should kill the app if DataStore API is used. It
> > just does not return data.
> 
> We do that for all other apis when apps don't have the permission to access
> data. I don't see how this is different here. Let's see what Paul thinks
> before landing.

My understanding of the standard approach was to check permissions (or I guess status in this case) in both parent and child. Child to decide what interfaces to expose to web content (i.e. what is said in comment 9) and parent to enforce the permissions on all messages since the child could be compromised. In that model, an app without permissions shouldn't ever be able to send a datastore:* message to the parent, and if it does, something is very wrong and the app should probably be killed. If its not too difficult, it would be safer to do that I think. 

To phrase the question another way: what damage could a compromised child process do by spoofing DataStore:Get, DataStore:RegisterForMessages, or DataStore:Changed messages?

	Andrea Marchesini [:baku] Assignee
	Comment 11 • 11 years ago

Here getDataStores() is not exposed if the app is not certified and checkAppStatus is used in any receiveMessage() calls.

	Andrea Marchesini [:baku] Assignee
	Comment 12 • 11 years ago

This patch changes just 1 line in nsFrameMessageManager.h:

-    return false;
+    return true;

This is needed because otherwise checking the appStatus when the app runs on the main process always fails. It seems green on try.

https://tbpl.mozilla.org/?tree=Try&rev=62ed7aeabb22

	(no longer active)
	Comment 14 • 11 years ago

Comment on attachment 8339366 [details] [diff] [review]
certified.patch

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

I'm mostly minusing because of the incorrect certified check, and also because of Paul's comment.  I also think that we should kill the child if it sends any datastore related messages and is not certified, especially now that it cannot access the API.  That may be a sign of a compromised child process trying to immitate a real one.

::: content/base/src/nsFrameMessageManager.h
@@ +94,5 @@
>    }
>  
>    virtual bool CheckAppHasStatus(unsigned short aStatus)
>    {
> +    return true;

I don't understand this change.  Fabrice, can you please make sure that this is correct?

::: dom/base/Navigator.cpp
@@ +1882,5 @@
> +  if (NS_FAILED(app->GetAppStatus(&status))) {
> +    return false;
> +  }
> +
> +  return status == nsIPrincipal::APP_STATUS_CERTIFIED;

Instead of this, please get the principal of the document and check its appStatus.

::: dom/datastore/DataStore.jsm
@@ +8,5 @@
>  
>  this.EXPORTED_SYMBOLS = ["DataStore"];
>  
>  function debug(s) {
> +  //dump('DEBUG DataStore: ' + s + '\n');

Please remove these hunks from the patch...

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 11 years ago

> I'm mostly minusing because of the incorrect certified check, and also
> because of Paul's comment.  I also think that we should kill the child if it
> sends any datastore related messages and is not certified, especially now
> that it cannot access the API.  That may be a sign of a compromised child
> process trying to immitate a real one.

The kill is done by CheckAppHasStatus when the API is used from a child process.

> >    virtual bool CheckAppHasStatus(unsigned short aStatus)
> >    {
> > +    return true;
> 
> I don't understand this change.  Fabrice, can you please make sure that this
> is correct?

I moved this change to 
SameParentProcessMessageManagerCallback().

> 
> ::: dom/base/Navigator.cpp
> @@ +1882,5 @@
> > +  if (NS_FAILED(app->GetAppStatus(&status))) {
> > +    return false;
> > +  }
> > +
> > +  return status == nsIPrincipal::APP_STATUS_CERTIFIED;
> 
> Instead of this, please get the principal of the document and check its
> appStatus.

I tried. but appStatus is always 0 if I get it from the document->NodePrincipal().

	(no longer active)
	Comment 16 • 11 years ago

(In reply to comment #15)
> > I'm mostly minusing because of the incorrect certified check, and also
> > because of Paul's comment.  I also think that we should kill the child if it
> > sends any datastore related messages and is not certified, especially now
> > that it cannot access the API.  That may be a sign of a compromised child
> > process trying to immitate a real one.
> 
> The kill is done by CheckAppHasStatus when the API is used from a child
> process.

OK, then that sounds good.

> > >    virtual bool CheckAppHasStatus(unsigned short aStatus)
> > >    {
> > > +    return true;
> > 
> > I don't understand this change.  Fabrice, can you please make sure that this
> > is correct?
> 
> I moved this change to 
> SameParentProcessMessageManagerCallback().
> 
> > 
> > ::: dom/base/Navigator.cpp
> > @@ +1882,5 @@
> > > +  if (NS_FAILED(app->GetAppStatus(&status))) {
> > > +    return false;
> > > +  }
> > > +
> > > +  return status == nsIPrincipal::APP_STATUS_CERTIFIED;
> > 
> > Instead of this, please get the principal of the document and check its
> > appStatus.
> 
> I tried. but appStatus is always 0 if I get it from the
> document->NodePrincipal().

Hmm, that's really weird.  Can you please debug why this happens?  We do this kind of check in other places where we need to determine whether the caller is a certified app.

	Andrea Marchesini [:baku] Assignee
	Comment 18 • 11 years ago

> Hmm, that's really weird.  Can you please debug why this happens?  We do
> this kind of check in other places where we need to determine whether the
> caller is a certified app.

Ok. It turned out that the principal->getAppStatus() does an additional check on the origin URL, that was failing in my mochitests. Now they are fix.

You will find a CSP modification, this is needed because when the appStatus if overwritten, the CSP should still work as a normal app. It was orange on try.

	(no longer active)
	Comment 19 • 11 years ago

Comment on attachment 8340040 [details] [diff] [review]
certified.patch

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

I'm not sure why you're removing file_changes2.html here, but I'm trust you, so r=me with the below.  If you think that I will be surprised by the reason you're doing this, please request another review.  :-)

Thanks!

::: content/base/src/nsDocument.cpp
@@ +2623,5 @@
> +       cspHeaderValue.IsEmpty() &&
> +       cspROHeaderValue.IsEmpty() &&
> +       cspOldHeaderValue.IsEmpty() &&
> +       cspOldROHeaderValue.IsEmpty()) ||
> +      Preferences::GetBool("dom.mozApps.allow_app_status_override", false)) {

Please get review on this bit from someone who understands CSP.

::: dom/base/Navigator.cpp
@@ +1868,5 @@
> +  if (NS_FAILED(doc->NodePrincipal()->GetAppStatus(&status))) {
> +    return false;
> +  }
> +
> +  return status == nsIPrincipal::APP_STATUS_CERTIFIED;

Hmm, you could rewrite this like below:

return NS_SUCCEEDED(doc->NodePrincipal()->GetAppStatus(&status)) &&
       status == nsIPrincipal::APP_STATUS_CERTIFIED;

I find that a bit more readable.  Please update the patch if you agree.

::: dom/datastore/tests/file_changes.html
@@ +66,5 @@
>      is(/[0-9a-zA-Z]{8}-[0-9a-zA-Z]{4}-[0-9a-zA-Z]{4}-[0-9a-zA-Z]{4}-[0-9a-zA-Z]{12}/.test(evt.revisionId), true, "event.revisionId returns something");
>      is(evt.id, gChangeId, "OnChangeListener is called with the right ID: " + evt.id);
>      is(evt.operation, gChangeOperation, "OnChangeListener is called with the right operation:" + evt.operation + " " + gChangeOperation);
> +
> +    alert('EVENT');

Is this needed?

	Andrea Marchesini [:baku] Assignee
	Comment 20 • 11 years ago

This patch does EXACTLY the opposite of the previous one.
There is a pref that enables DataStore for any kind of app. My mochitest are kept as they are with this pref enabled. Then there is a new mochitest for testing that DataStore is disabled by default without that pref.

	[:fabrice] Fabrice Desré
	Comment 21 • 11 years ago

Comment on attachment 8340142 [details] [diff] [review]
cert2.patch

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

Could be r+, but https://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl#443 doesn't say that we kill the child if that call fails. Can you double check that and with the IDL comment accordingly?

	Andrea Marchesini [:baku] Assignee
	Comment 22 • 11 years ago

This is an ID that implements the assertion. There are not other piece of code using checkAppHasStatus so I renamed it assertAppHasStatus as the rest of nsIProcessChecker interface.

	(no longer active)
	Comment 24 • 11 years ago

Comment on attachment 8342261 [details] [diff] [review]
assert.patch

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

r=me but Fabrice is probably a better reviewer anyway.

	[:fabrice] Fabrice Desré
	Comment 25 • 11 years ago

Comment on attachment 8342261 [details] [diff] [review]
assert.patch

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

::: content/base/public/nsIMessageManager.idl
@@ +434,2 @@
>    /**
>     * Return true iff the "remote" process' principal has an appStatus equal to

nit: can you fix the iff while we update this file?

::: dom/ipc/AppProcessChecker.h
@@ +39,5 @@
>                   AssertAppProcessType aType,
>                   const char* aCapability);
>  
> +/**
> + * Return true iff the specified app has the specified status.

nit: if

@@ +57,5 @@
>                   AssertAppProcessType aType,
>                   const char* aCapability);
>  
> +/**
> + * Return true iff any of the PBrowsers loaded in this content process

nit: if

	Andrea Marchesini [:baku] Assignee
	Comment 26 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=d0f8e133c8bf

	Andrea Marchesini [:baku] Assignee
	Comment 27 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb6d1811696

	Andrea Marchesini [:baku] Assignee
	Comment 28 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=83eeaeb9ea60

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 11 years ago

Backed out for B2G mochitest-3 timeouts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e805a81f0a1c

https://tbpl.mozilla.org/php/getParsedLog.php?id=31501779&tree=Mozilla-Inbound

	Andrea Marchesini [:baku] Assignee
	Comment 30 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=ed40076286ce

I add to disable the test for b2g emulator because the mochitest app is installed before the pref for enabling DataStore everywhere is set.

	Andrea Marchesini [:baku] Assignee
	Comment 31 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/49da013f914e

	(no longer active)
	Comment 32 • 11 years ago

(In reply to comment #30)
> I add to disable the test for b2g emulator because the mochitest app is
> installed before the pref for enabling DataStore everywhere is set.

Why is that?  Can you please file a follow-up to enable this test?

	Andrea Marchesini [:baku] Assignee
	Comment 33 • 11 years ago

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #32)
> (In reply to comment #30)
> > I add to disable the test for b2g emulator because the mochitest app is

s/add/had/
I'll try to ri-enable it in a follow-up.

	Carsten Book [:Tomcat]
	Comment 34 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/49da013f914e

	Chris Mills [:cmills]
	Comment 35 • 10 years ago

Data Store API documented: https://developer.mozilla.org/en-US/docs/Web/API/Data_Store_API.