847854 - WebActivity should allow restricting the size of the data passed

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

As web activities are implemented now, there's no way for the activity receiver to restrict the maximum size of the data he wish to receive. Even on some activities like 'dial' or 'websms/sms' where for the number there could be some very definite size restrictions there's no way to restrict that when the activity is declared.

So in the best case, we're giving a way to remotely kill apps (since they'll be killed by the memory watcher process). On the worst case (I tried this and it works):

STR: 
1. Open the dialer (directly or via a web activity, is the same) and dial some number
2. While on the call, go back to another app that sends an activity to the dialer. An activity with a 3MB number did the trick for me.
3. The dialer dies. As did the homescreen, incidentally.
4. The call is still active, but now there's no way to hang it up, other than turning the phone off. There's also no indicator that shows that there's a call in process.

Comment 1

[Jose Manuel Cantera]

Well we have two different scenarios: 

Limiting the amount of data sent to the receiver and limiting the amount of data posted as result to the caller. 

Probably this is something very dependent on the app and could be tricky to set a hard limit at platform level specially on those apps where messages are blobs.

Comment 2

[Antonio Manuel Amaya Calvo (:amac)]

No, I wasn't thinking on setting a hard limit at the platform, more like having some syntax to specify that when declaring the activity.

I'm looking at [1] and maybe it can be restricted now using filters, although I'm not sure who enforces those filters. From the description it seems that the platform is the one that when matching activities to applications applies the filter, and that should suffice (in which case this bug should be re-named to something like 'Modify the Activities for Gaia apps so they have adequate filters').

[1] https://wiki.mozilla.org/WebAPI/WebActivities

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

Ok, I tried it and just using a regexp that includes the maximum length works. It's a... 'small' overkill using a regexp just to control the maximum size, but it works. IMO it would be better to add a 'length' or 'maxsize' or whatever to the filters of Web Activities, but with what we have now we can fix the biggest targets (SMS and Dialer IMO).

I'll open separate bugs for those, leaving this one open in case we decide to add the new field to the filter dictionary.

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

Created bug 847896 to track the manifest changes for the SMS and dialer apps.

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

After Bug 848716#c5 I think we should most definitely fix this bug. The way I see it, we can do one of:

* Allow apps restricting the maximum size of the received *total* data they expect.
or
* Allow apps to specify a strict filter of what attributes they expect to get passed, so any activity invocation that has extra attributes don't match.

I'm not noming this as tef? at this moment because with what was implemented on 847896 and what I'll implement on 848716 the exploits I had found don't work anymore and I haven't been able to find a working exploit to kill another process without the attacker app being killed first by passing not used data. 

I think we should tackle this as soon as realistically possible though. 

That I haven't been able to make the exploit work yet doesn't means that I believe even for a second that it cannot be made to work.

If you feel we can tackle this in time (I can even take this once we've agreed on what to do), we can nom it then.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think there are two things to protect against here:

1. The size of the memory allocation needed in both the parent process and the
   receiver process. Either of these can cause the processes to run out of memory.
2. The size of an individual property which the receiver would use to take some form
   of action. For example the length of the url property that the browser uses to
   navigate.

I think we can solve 1 by using a hard-coded limit on the structured clone data. We can probably make the limit pretty high, say 1MB.

To solve 2, I think applications can easily check the length of whatever property they are worried about using JS. We could add some form of max-length property in the filters section of the activity registration as a convenience, but this doesn't seem strictly needed.

But I'm definitely for solving 1.

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #6)
> I think there are two things to protect against here:
> 
> 1. The size of the memory allocation needed in both the parent process and
> the receiver process. Either of these can cause the processes to run out of
> memory.

True but the allocation on the sender process cannot be controlled, since there are tons of way he can kill itself by excessive memory usage. What we can (and should) control is the amount of memory that an "innocent" process will get when he processes a web activity.

> 2. The size of an individual property which the receiver would use to take
> some form of action. For example the length of the url property that the 
> browser uses to  navigate.
> 
> I think we can solve 1 by using a hard-coded limit on the structured clone
> data. We can probably make the limit pretty high, say 1MB.

Hmm more like having a hard-coded limit for the data I would prefer to have a per-activity limit, defined by the receiver. Something like:

"dial": {
 "filters": {
    "maxSize": "1K",
    "type": "webtelephony/number",
    "number": { "regexp":"/^[\\d\\s+#*().-]{0,50}$/" }
  },
  "href": "/dialer/index.html#keyboard-view",
  "disposition": "window"
}

Meaning that for the filter to be satisfied, the size of the data should be less or equal to maxSize. If maxSize is absent, then we can hardcode a default value of 1Mb if whatever.

I prefer allowing apps to specify their own limit if they wish because:

* It doesn't seem very efficient to allow passing me 1Mb of data to an app that will use only 100 bytes, tops.
* This way we limit the attack surface. The attacks I had implemented worked even on a otherwise idle phone (the only apps running where the attacker and the victim). If the phone had more apps running, I would have been able to make it work using even less memory. So this way we can more effectively protect the certified apps against this type of DoS attacks.


> To solve 2, I think applications can easily check the length of whatever
> property they are worried about using JS. We could add some form of
> max-length property in the filters section of the activity registration as a
> convenience, but this doesn't seem strictly needed.

This can currently be controlled just by adding a regexp to the filter for the to-be-controlled properties. That's what I intend to do on bug 848716 in fact as soon as we agree on a size :).


> But I'm definitely for solving 1.

Great! I've assigned this for the time being to Carmen. If you agree with this approach (having an optional maxSize parameter to the filter with a default value of 1Mb for example) we'll get to this on Monday.

BTW and out of curiosity, is there a better way to calculate the size of a JS object (or JS::Value in C++) than iterate through the properties and adding the length/estimated size?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Antonio Manuel Amaya Calvo from comment #7)
> (In reply to Jonas Sicking (:sicking) from comment #6)
> > I think there are two things to protect against here:
> > 
> > 1. The size of the memory allocation needed in both the parent process and
> > the receiver process. Either of these can cause the processes to run out of
> > memory.
> 
> True but the allocation on the sender process cannot be controlled, since
> there are tons of way he can kill itself by excessive memory usage. What we
> can (and should) control is the amount of memory that an "innocent" process
> will get when he processes a web activity.

I didn't mention the sender process. I agree there's no point in trying to do something about that.

What I talked about is the parent process and the receiver process. All data goes through the parent process currently.

> > 2. The size of an individual property which the receiver would use to take
> > some form of action. For example the length of the url property that the 
> > browser uses to  navigate.
> > 
> > I think we can solve 1 by using a hard-coded limit on the structured clone
> > data. We can probably make the limit pretty high, say 1MB.
> 
> Hmm more like having a hard-coded limit for the data I would prefer to have
> a per-activity limit, defined by the receiver. Something like:
> 
> "dial": {
>  "filters": {
>     "maxSize": "1K",
>     "type": "webtelephony/number",
>     "number": { "regexp":"/^[\\d\\s+#*().-]{0,50}$/" }
>   },
>   "href": "/dialer/index.html#keyboard-view",
>   "disposition": "window"
> }

If we're having a maxSize property, it should go outside of the filter and instead be a property directly of "dial".

But I don't really see what having a maxSize gets us. In your case you're having to limit the size using the regexp anyway. And the OOM issue should be taken care of by making a limit of 1MB.

So what problem are you trying to solve using maxSize?

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #8)
> (In reply to Antonio Manuel Amaya Calvo from comment #7)
> > (In reply to Jonas Sicking (:sicking) from comment #6)
> > > I think there are two things to protect against here:
> > True but the allocation on the sender process cannot be controlled, since
> > there are tons of way he can kill itself by excessive memory usage. What we
> > can (and should) control is the amount of memory that an "innocent" process
> > will get when he processes a web activity.
> 
> I didn't mention the sender process. I agree there's no point in trying to
> do something about that.
> 
> What I talked about is the parent process and the receiver process. All data
> goes through the parent process currently.

My bad, I had assumed that in this context parent == sender.

> > > 2. The size of an individual property which the receiver would use to take
> > > some form of action. For example the length of the url property that the 
> > > browser uses to  navigate.
> > > 
> > > I think we can solve 1 by using a hard-coded limit on the structured clone
> > > data. We can probably make the limit pretty high, say 1MB.
> > 
> > Hmm more like having a hard-coded limit for the data I would prefer to have
> > a per-activity limit, defined by the receiver. Something like:
> > 
> > "dial": {
> >  "filters": {
> >     "maxSize": "1K",
> >     "type": "webtelephony/number",
> >     "number": { "regexp":"/^[\\d\\s+#*().-]{0,50}$/" }
> >   },
> >   "href": "/dialer/index.html#keyboard-view",
> >   "disposition": "window"
> > }
> 
> If we're having a maxSize property, it should go outside of the filter and
> instead be a property directly of "dial".

I thought it should be on the filter since we could have several apps offering the same activity with different filters, and one of the things they could offer was different maxSize. It doesn't make much sense having it outside of the filter since in that case, if I have several 'dial' activities defined, which one do I use to control the size?


> But I don't really see what having a maxSize gets us. In your case you're
> having to limit the size using the regexp anyway. And the OOM issue should
> be taken care of by making a limit of 1MB.
> 
> So what problem are you trying to solve using maxSize?

Invoking the activity as:

var reallyLongString=<several MBs of trash>
new MozActivity({
  name:'dial',
  data:{
    number: '12345689',
    doesntReallyMatter: reallyLongString
  }
});

That is, passing to the activity an attribute that isn't actually used by the activity (and thus can't be controlled by filtering). Other way to fix that would be to allow the filter to specify the list of attributes it uses and making everything with more/less attributes don't match. 

Now that I think about it, probably the second way would be better. Something like:

"dial": {
 "filters": {
    "type": "webtelephony/number",
    "attributes": ["number"],
    "number": { "regexp":"/^[\\d\\s+#*().-]{0,50}$/" }
  },
  "href": "/dialer/index.html#keyboard-view",
  "disposition": "window"
}

That way if someone tries to invoke the activity with extra attributes it won't satisfy the filter and thus the receiver app won't be invoked.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Antonio Manuel Amaya Calvo from comment #9)
> (In reply to Jonas Sicking (:sicking) from comment #8)
> > (In reply to Antonio Manuel Amaya Calvo from comment #7)
> > > (In reply to Jonas Sicking (:sicking) from comment #6)
> > > > I think there are two things to protect against here:
> > > True but the allocation on the sender process cannot be controlled, since
> > > there are tons of way he can kill itself by excessive memory usage. What we
> > > can (and should) control is the amount of memory that an "innocent" process
> > > will get when he processes a web activity.
> > 
> > I didn't mention the sender process. I agree there's no point in trying to
> > do something about that.
> > 
> > What I talked about is the parent process and the receiver process. All data
> > goes through the parent process currently.
> 
> My bad, I had assumed that in this context parent == sender.
> 
> > > > 2. The size of an individual property which the receiver would use to take
> > > > some form of action. For example the length of the url property that the 
> > > > browser uses to  navigate.
> > > > 
> > > > I think we can solve 1 by using a hard-coded limit on the structured clone
> > > > data. We can probably make the limit pretty high, say 1MB.
> > > 
> > > Hmm more like having a hard-coded limit for the data I would prefer to have
> > > a per-activity limit, defined by the receiver. Something like:
> > > 
> > > "dial": {
> > >  "filters": {
> > >     "maxSize": "1K",
> > >     "type": "webtelephony/number",
> > >     "number": { "regexp":"/^[\\d\\s+#*().-]{0,50}$/" }
> > >   },
> > >   "href": "/dialer/index.html#keyboard-view",
> > >   "disposition": "window"
> > > }
> > 
> > If we're having a maxSize property, it should go outside of the filter and
> > instead be a property directly of "dial".
> 
> I thought it should be on the filter since we could have several apps
> offering the same activity with different filters, and one of the things
> they could offer was different maxSize.

Even if we put the maxSize property outside of the "filters" property can we use it as a required condition for displaying the handler in the activity list.

The "filters" property describes which values are acceptable for which properties. The syntax you have used above would with the current list be interpreted as the "maxSize" property having to have the value "1K". We shouldn't overload that meaning.

> > But I don't really see what having a maxSize gets us. In your case you're
> > having to limit the size using the regexp anyway. And the OOM issue should
> > be taken care of by making a limit of 1MB.
> > 
> > So what problem are you trying to solve using maxSize?
> 
> Invoking the activity as:
> 
> var reallyLongString=<several MBs of trash>
> new MozActivity({
>   name:'dial',
>   data:{
>     number: '12345689',
>     doesntReallyMatter: reallyLongString
>   }
> });

Why is it a problem if someone does that?

Comment 11

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #10)
> (In reply to Antonio Manuel Amaya Calvo from comment #9)
> > > So what problem are you trying to solve using maxSize?
> > 
> > Invoking the activity as:
> > 
> > var reallyLongString=<several MBs of trash>
> > new MozActivity({
> >   name:'dial',
> >   data:{
> >     number: '12345689',
> >     doesntReallyMatter: reallyLongString
> >   }
> > });
> 
> Why is it a problem if someone does that?

The doesntReallyMatter parameter will be passed to the activity destination process along with the number... potentially getting it on the 'to-kill' list of the OOM manager. That's the attack I wanted to mitigate by maxSize or just by explicitly defining which are the valid attributes (I think the second option is better, in fact).

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, that is point 1 in comment 6.

Like I proposed in comment 6, I think that can be solved by simply putting a 1MB limit on the size.

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #12)
> Yes, that is point 1 in comment 6.
> 
> Like I proposed in comment 6, I think that can be solved by simply putting a
> 1MB limit on the size.

Yeah, that would be a one-size-fits-all solution. I would prefer each app being able to explicitly say what it can/expect to receive. Seems kinda wasteful to allow passing 1MB of data to the dial activity, which only processes numbers up to 50 chars of length, for example. And opens the way to try to kill it by sending several concurrent activities instead of just one. If the process gets killed at 3MB of data consumption, and I inject 3 activities on the same time frame, I win.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wasteful in what sense? Writing code to support a feature that isn't needed sounds more wasteful to me.

Let's keep things simple and add a 1MB hardcoded limit for now. We'll need such a limit anyway since not everyone will opt in to a maxSize.

If it turns out that there's still problems with this approach, we can look at adding a maxSize at that point.

Comment 15

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #14)
> Wasteful in what sense? Writing code to support a feature that isn't needed
> sounds more wasteful to me.

In the sense that we're moving 1MB around (from the attacker to the parent process, and from the parent process to the attacked process (or to itself if the attacked app is the browser) when all that the victim app wants is 100 bytes, tops.

As I said before, this isn't high priority for me ATM because the approach I was using (1 simple attacker injecting all the data in one go) doesn't work if the data isn't copied at the destiny (the attacker dies of bloating itself). But if I have time I wanted to try sending several small packages to a single app at the same time (for some value of 'same time'). 

> Let's keep things simple and add a 1MB hardcoded limit for now. We'll need
> such a limit anyway since not everyone will opt in to a maxSize.
> 
> If it turns out that there's still problems with this approach, we can look
> at adding a maxSize at that point.

Ok, will do that then.

Comment 16

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Jonas Sicking (:sicking) from comment #14)
> 
> Let's keep things simple and add a 1MB hardcoded limit for now. We'll need
> such a limit anyway since not everyone will opt in to a maxSize.

Would this affect passing image data around via web activities as well?  Those can get much larger than 1MB, depending on resolution and encoding.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

This is a nice to have, please nominate a low risk patch for uplift when one is available.

Comment 18

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Mike Habicher [:mikeh] from comment #16)
> (In reply to Jonas Sicking (:sicking) from comment #14)
> > 
> > Let's keep things simple and add a 1MB hardcoded limit for now. We'll need
> > such a limit anyway since not everyone will opt in to a maxSize.
> 
> Would this affect passing image data around via web activities as well? 
> Those can get much larger than 1MB, depending on resolution and encoding.
If it's a global value for all apps and all activities then it will affect that also. That's one of the reasons why I didn't like the one size fits all approach.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We shouldn't count data living in Blobs towards the size. The receiving application won't end up loading the Blob data into memory unless it actually reads from the Blob. And it can see the size of the Blob before doing so.

So simply measuring the size of the structured-clone buffer, and ignoring the attached array of Blobs, should do the trick.

Comment 20

[Mounir Lamouri (:mounir)]

I join Jonas here: I think we should just have a clever platform and we shouldn't rely on applications to be clever because they will not.

As a side note, I wonder if the change made for the sms and communication apps [1] was a good idea because those activities are web facing [by definition] and you just changed their behaviour.

[1] https://github.com/mozilla-b2g/gaia/pull/8481 and https://bugzilla.mozilla.org/show_bug.cgi?id=847896

Comment 21

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Mounir Lamouri (:mounir) from comment #20)
> I join Jonas here: I think we should just have a clever platform and we
> shouldn't rely on applications to be clever because they will not.

Hmm having a one size fits all on the platform barely qualifies as clever in my book :). But unless I can find a working exploit to DoS the receiver apps by using several ~1M simultaneous activity, we'll just implement that.


> As a side note, I wonder if the change made for the sms and communication
> apps [1] was a good idea because those activities are web facing [by
> definition] and you just changed their behaviour.

If the activities are documented somewhere, I'll be happy to update the documentation. If they aren't, then the manifest *is* the documentation and so they're documented already.

As for changing the behavior, what's been done is making them reject data that wouldn't work anyway. If the application is legit then it's doing something wrong and this should help them to fix the problem. If it isn't legit, then frankly I don't mind it not working anymore :P


> 
> [1] https://github.com/mozilla-b2g/gaia/pull/8481 and
> https://bugzilla.mozilla.org/show_bug.cgi?id=847896

Comment 22

[BMO Automation]

Firefox OS is not being worked on