Make DataStore API Certified-only for 1.3

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: pauljt, Assigned: baku)

Tracking

({dev-doc-complete})

Trunk
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 fixed)

Details

(Whiteboard: ucid:System103)

Attachments

(1 attachment, 10 obsolete attachments)

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.
No longer blocks: 938976
This probably needs to block 1.3, right?
blocking-b2g: --- → 1.3?
Blocks: 916089
(In reply to comment #1)
> This probably needs to block 1.3, right?

Yes.
blocking-b2g: 1.3? → 1.3+
Depends on: 880043
Posted patch certified.patch (obsolete) — Splinter Review
Attachment #8338506 - Flags: review?(ehsan)
Assignee: nobody → amarchesini
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.
Attachment #8338506 - Flags: review?(fabrice)
Attachment #8338506 - Flags: review?(ehsan)
Attachment #8338506 - Flags: review+
Posted patch certified.patch (obsolete) — Splinter Review
renamed the pref to dom.mozApps.allow_app_status_override
Attachment #8338506 - Attachment is obsolete: true
Attachment #8338506 - Flags: review?(fabrice)
Attachment #8338536 - Flags: review?(fabrice)
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 ;)
Attachment #8338536 - Flags: review?(fabrice) → feedback+
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.
(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.
Flags: needinfo?(ptheriault)
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.
Attachment #8338536 - Flags: review-
(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?
Flags: needinfo?(ptheriault)
Posted patch certified.patch (obsolete) — Splinter Review
Here getDataStores() is not exposed if the app is not certified and checkAppStatus is used in any receiveMessage() calls.
Attachment #8338536 - Attachment is obsolete: true
Attachment #8339195 - Flags: review?(ehsan)
Attachment #8339195 - Flags: feedback?(fabrice)
Posted patch certified.patch (obsolete) — Splinter Review
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
Attachment #8339195 - Attachment is obsolete: true
Attachment #8339195 - Flags: review?(ehsan)
Attachment #8339195 - Flags: feedback?(fabrice)
Attachment #8339366 - Flags: review?(fabrice)
Attachment #8339366 - Flags: review?(ehsan)
Duplicate of this bug: 940394
Whiteboard: ucid:System103
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...
Attachment #8339366 - Flags: review?(ehsan) → 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.

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().
(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.
Posted patch certified.patch (obsolete) — Splinter Review
Attachment #8339366 - Attachment is obsolete: true
Attachment #8339366 - Flags: review?(fabrice)
Attachment #8340040 - Flags: review?(fabrice)
Attachment #8340040 - Flags: review?(ehsan)
> 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 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?
Attachment #8340040 - Flags: review?(ehsan) → review+
Posted patch cert2.patch (obsolete) — Splinter Review
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.
Attachment #8340040 - Attachment is obsolete: true
Attachment #8340040 - Flags: review?(fabrice)
Attachment #8340142 - Flags: review?(fabrice)
Attachment #8340142 - Flags: review?(ehsan)
Attachment #8340142 - Flags: review?(ehsan) → review+
Blocks: 945797
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?
Attachment #8340142 - Flags: review?(fabrice) → feedback+
Posted patch assert.patch (obsolete) — Splinter Review
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.
Attachment #8342245 - Flags: review?(fabrice)
Attachment #8342245 - Flags: review?(ehsan)
Posted patch assert.patch (obsolete) — Splinter Review
Attachment #8342245 - Attachment is obsolete: true
Attachment #8342245 - Flags: review?(fabrice)
Attachment #8342245 - Flags: review?(ehsan)
Attachment #8342261 - Flags: review?(fabrice)
Attachment #8342261 - Flags: review?(ehsan)
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.
Attachment #8342261 - Flags: review?(ehsan) → review+
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
Attachment #8342261 - Flags: review?(fabrice) → review+
Posted patch cert2.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=d0f8e133c8bf
Attachment #8340142 - Attachment is obsolete: true
Attachment #8342261 - Attachment is obsolete: true
Blocks: 942642
Posted patch cert2.patchSplinter Review
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.
Attachment #8343052 - Attachment is obsolete: true
(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?
(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.
Blocks: 947134
https://hg.mozilla.org/mozilla-central/rev/49da013f914e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.