848716 - [Browser] Passing a huge amount of data via the web activity crashes the phone

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

STR:
* Visit http://sigsegv.es/testW.html with the device browser
* Press Test with the default options

Expected result: 
* Either a new browser window saying the URL is invalid should show, or nothing should happen

Actual result: 
* The parent process gets killed by the memory watcher process, rebooting the phone. Sometimes the phone doesn't reboot completely (I think some other process gets killed first) but the phone stops working until it reboots by itself or it's manually rebooted.

Cause: The browser web activity doesn't control the maximum size of the URL that gets passed. 

Setting tef? because this allow any web page to crash the phone at will, and it has a pretty easy solution.

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

I could take this, it has a pretty easy solution even without fixing bug 847854 but I would like some input about what's a reasonable value for the maximum size of an URL first.

Comment 2

[Daniel Coloma:dcoloma]

Marking as a blocker for TEF

Comment 3

[Andrew Overholt [:overholt]]

(In reply to Antonio Manuel Amaya Calvo from comment #1)
> I could take this

Done :)

I've CC'd some people who may be able to help.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Antonio Manuel Amaya Calvo from comment #1)
> I could take this, it has a pretty easy solution even without fixing bug
> 847854 but I would like some input about what's a reasonable value for the
> maximum size of an URL first.

I would say 4k is reasonable, but I don't have real data to back up that.

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

Ok, I can limit it to 4K for the time being, we can always fine tune that later on.

There's one other thing though... I don't think this is going to cut it, and I think we need a way to limit the *global* size of the data part of a WebActivity. Why? Cause I can restrict using regexps the data the web activity actually expects to get passed and uses (like the URL or the phone number) but I can't limit the data the web activity won't do anything with (like randomAttributeIJustAddedToInjectMemoryOnYourProcess). 

I haven't been able to create a repeatable attack using this vector yet, since as the app doesn't use the extra memory I'm passing it it becomes a issue of injecting enough memory on the victim app without using too much memory on the attacker. Selects not working today isn't helping me much too :)

Comment 6

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

4k is too small since data: URL quickly get much larger. I would prefer a 500K or 1MB limit if we can get away with it. At how large sizes do you start seeing the parent process crashing?

Also note that this isn't specific to webactivities in any way. A normal webpage that the browser navigates to can set location.href to a large URL.

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #6)
> 4k is too small since data: URL quickly get much larger. I would prefer a
> 500K or 1MB limit if we can get away with it. At how large sizes do you
> start seeing the parent process crashing?

I see it crashing consistently at about 3MB. Don't have an exact number since it depends also on how much memory is the parent process consuming previously.


500K of URL, NVM 1MB seems pretty excessive to me. For reference, IE limits the URL length including potential GET data to 2083 chars [1]. The standard [2], section 3.2.1. doesn't set any hard limit on URI sizes but warn against using URI lengths higher than 255 chars.

I think we can reach a compromise here and allow 32K? Much more than current IE implementations, still short enough to not cause any problems on the short term. 

And BTW, IE not allowing URLs bigger than aprox. 2K means that anything bigger than that doesn't work on IE... and I would say it's most probably an attack of some kind.

 
> Also note that this isn't specific to webactivities in any way. A normal
> webpage that the browser navigates to can set location.href to a large URL.

True, but that isn't a propblem per se. This problem *is* specific of webactivities since if a webpage sets it's own href to something huge then it just kill itself, the same way that it'll kill itself eventually if it does 

for(;;) addTillIDie+='1234567890';

But that's not a big problem. The memory watcher kill helpfully kill the offending process, and that's the end of it.

With a web activity though the attacker process is doing:

consumeALongAmountOfMemoryButNotEnoughToKillMe();
injectAllThatMemoryOnAnotherProcessAndHopeItsKilled();

So the memory watcher isn't killing the offending process... it's killing the victim process. Which in this case happens to be the parent process also so it basically resets the phone.

[1] http://support.microsoft.com/kb/208427
[2] http://www.faqs.org/rfcs/rfc2616.html

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

Oh, something else I forgot. The browser web activity (which will be the only thing affected by this change)only supports http/s URLs.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Can you post stacks from the main process where this crashes from OOM? It would probably be smart to make that allocation site fallible and force-kill the child that sends the data rather than let the main process go down.

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

I don't think the main process is crashing (as in getting a segmentation fault or similar and dieing because of that). What I think is happening is that the OOM watcher process is killing the main process to free memory.

Comment 11

[Justin Lebar (not reading bugmail)]

(In reply to Antonio Manuel Amaya Calvo from comment #10)
> I don't think the main process is crashing (as in getting a segmentation
> fault or similar and dieing because of that). What I think is happening is
> that the OOM watcher process is killing the main process to free memory.

We should use the output from dmesg (adb shell dmesg) to test this hypothesis.

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: b2g-ps output.

The computer clock isn't synchronized with the phone, sorry. I think there's a shift of 8 seconds approximately.

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: dmesg out after the crash

Comment 14

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Justin Lebar [:jlebar] (PTO 3/11-3/15, then GMT+9 until 3/24) from comment #11)
> (In reply to Antonio Manuel Amaya Calvo from comment #10)
> > I don't think the main process is crashing (as in getting a segmentation
> > fault or similar and dieing because of that). What I think is happening is
> > that the OOM watcher process is killing the main process to free memory.
> 
> We should use the output from dmesg (adb shell dmesg) to test this
> hypothesis.

<4>[03-13 15:13:36.157] [109: b2g]select 109 (b2g), adj 0, size 40815, to kill
<4>[03-13 15:13:36.157] [109: b2g]send sigkill to 109 (b2g), adj 0, size 40815

I attached the full dmesg output (from the attack start) and the output of a b2g-ps I had running also that shows how the process grows and grows until it's killed.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ick, we really shouldn't let the lowmem process kill the main b2g process... All child apps will instantly(ish) die too. Justin, can we prevent this?

Comment 16

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to ben turner [:bent] from comment #15)
> Ick, we really shouldn't let the lowmem process kill the main b2g process...
> All child apps will instantly(ish) die too. Justin, can we prevent this?

The problem is that there was nothing else left to kill :). The main b2g process just grew and grew and grew until it was killed. In fact by the time it was killed the homescreen usually has been killed and relaunched also.

Comment 17

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Link to PR 8626

Comment 18

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 724492 [details]
Link to PR 8626

So far, if we go for the gaia solution is looking good to me, r+.

Comment 19

[Daniel Coloma:dcoloma]

what is preventing you from landing this?

Comment 20

[Antonio Manuel Amaya Calvo (:amac)]

For someone with the right permissions to see it :). 

¿Francisco?

Comment 21

[[:fabrice] Fabrice Desré]

Also, the Travis CI is complaining. Can you take a look why?

Comment 22

[Francisco Jordano [:arcturus] [:francisco]]

Taking a look to Travis, once solved I'll land it.

Cheers!

Comment 23

[Francisco Jordano [:arcturus] [:francisco]]

Lintin errors not caused by this PR:

make lint
Skipping 45 file(s).
----- FILE  :  /home/travis/build/mozilla-b2g/gaia/apps/system/js/sound_manager.js -----
Line 8, E:0001: Extra space before ")"
Line 17, E:0001: Extra space before ")"
Found 2 errors, including 0 new errors, in 1 files (599 files OK).

Landed:

https://github.com/mozilla-b2g/gaia/commit/ddcdcc29156c462d62e4f98e166af5bb28571348

Comment 24

[Ryan VanderMeulen [:RyanVM]]

John, can you take care of the uplift on this?

Comment 25

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 6a5637e
v1.0.1: 4f4d059

Comment 26

[Chris Kreinbring [:CKreinbring]]

Verified in both 1.0.1 and 1.1 builds.  Navigating to http://sigsegv.es/testW.html and tapping the Test button has no reaction from the Browser or the device.

1.0.1:
Unagi build 20130403070201
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/512258bc00a3
Gaia: daea430624ec02f8d36a12d581fc4a3278c27cb7

1.1:
Unagi build 20130403070204
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/d467369d1b0c
Gaia: 06e0e5ce42bdfb62bdbe38271de6b5b2d9e40e75