942639 - Make DataStore API Certified-only for 1.3

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

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.

Comment 1

[Jason Smith [:jsmith]]

This probably needs to block 1.3, right?

Comment 2

[(no longer active)]

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

Yes.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: certified.patch

Comment 4

[(no longer active)]

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.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: certified.patch

renamed the pref to dom.mozApps.allow_app_status_override

Comment 6

[[:fabrice] Fabrice Desré]

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

Comment 7

[Andrea Marchesini [:baku]]

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.

Comment 8

[[:fabrice] Fabrice Desré]

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

Comment 9

[(no longer active)]

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.

Comment 10

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

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

Comment 11

[Andrea Marchesini [:baku]]

Attachment: certified.patch

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

Comment 12

[Andrea Marchesini [:baku]]

Attachment: certified.patch

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

Comment 14

[(no longer active)]

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

Comment 15

[Andrea Marchesini [:baku]]

> 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().

Comment 16

[(no longer active)]

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

Comment 17

[Andrea Marchesini [:baku]]

Attachment: certified.patch

Comment 18

[Andrea Marchesini [:baku]]

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

Comment 19

[(no longer active)]

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?

Comment 20

[Andrea Marchesini [:baku]]

Attachment: cert2.patch

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.

Comment 21

[[:fabrice] Fabrice Desré]

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?

Comment 22

[Andrea Marchesini [:baku]]

Attachment: assert.patch

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.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: assert.patch

Comment 24

[(no longer active)]

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.

Comment 25

[[:fabrice] Fabrice Desré]

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

Comment 26

[Andrea Marchesini [:baku]]

Attachment: cert2.patch

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

Comment 27

[Andrea Marchesini [:baku]]

Attachment: cert2.patch

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

Comment 28

[Andrea Marchesini [:baku]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Andrea Marchesini [:baku]]

Attachment: cert2.patch

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.

Comment 31

[Andrea Marchesini [:baku]]

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

Comment 32

[(no longer active)]

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

Comment 33

[Andrea Marchesini [:baku]]

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

Comment 34

[Carsten Book [:Tomcat]]

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

Comment 35

[Chris Mills [:cmills]]

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