801855 - [OTA update][Security Review] Check update download progress to prevent overly large update from being downloaded

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P2)

Description

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

An potential issue was raised in the secreview that an if the update delivered is larger than expected it could cause problems on the device. The update is delivered over HTTP so could be tampered with, or it could be the wrong update etc.

The proposed fix was to have a check in the downloader code that if the amount downloaded significantly exceeds the expected size, then cancel the download and try again later.

Comment 1

[Brian R. Bondy [:bbondy]]

What is a sane limit here considering FOTA updates may be quite large (I'm assuming they will at least)? 1GB?

rstrong do you want this limit applied to all code or only to B2G?

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

The reason I raised this issue is that I have seen cases where devices and/or operating systems completely freeze/fail/brick because of 100% full disks. That is the attack that we should prevent.

I think two things need to be done:

1) Checking if the offered download (Content-Length) is equal to the expected size as a specified in in the update manifest xml. Immediately abort if it is not.

2) Check in onProgress if the actual download size exceeds the value specified in Content-Length (this assumes the content-length is correct and equal to what was specified in the initial update manifest xml)

I think 2 is important because AFAIK we gracefully deal with content that does not match Content-Length. Ie, we download whatever we are offered.

The most important thing here is that we prevent the disk from filling up.

I think hardcoded values are not a good idea and also not needed. The update is either the correct size *and* passes the disk space test, or it is not and it is rejected. Or it downloads beyond the expected size and is also rejected.

Note that this is not just an attack. It could also happen because of some network error (like a confused proxy server sending the wrong or corrupt file).

Comment 3

[Brian R. Bondy [:bbondy]]

Thanks for the clarification. I'll make those changes.

The reason I was thinking a max limit constant value (control by a pref), is because an attacker could potentially (perhaps not as easily) also modify response headers if they can send extra data after a response.  Perhaps they can generate a very large file that would not be over the disk limit even, but would make the device soon reach that limit.

Also they could send us an invalid manifest with whatever values they want.

Even without an attack, in the case of a a bad MAR file being uploaded to the update server, maybe that MAR file could be several GB in size for example.

Is there a reason why these things are not a concern?

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Still need to add some tests, once I do that I'll request review.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Implementation

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Tests

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 673069 [details] [diff] [review]
Patch v2 - Implementation

Looks fine. Did you verify that the about dialog UI does something sane?

Comment 8

[Robert Strong (they/them - no direct email)]

Moving over to app update since that is where the code is

Comment 9

[Brian R. Bondy [:bbondy]]

Yup I tried with an app update url override and the about dialog.  I'm going to push this one to oak as well for unit tests on all platforms and to do some extra testing though.

Comment 10

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7e8a1440a598
http://hg.mozilla.org/integration/mozilla-inbound/rev/3a20b8a911dc

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e8a1440a598
https://hg.mozilla.org/mozilla-central/rev/3a20b8a911dc

Comment 12

[Brian R. Bondy [:bbondy]]

Was this meant to be blocking basecamp? If so do we need to uplift to aurora?

Comment 13

[Brian R. Bondy [:bbondy]]

Comment on attachment 673069 [details] [diff] [review]
Patch v2 - Implementation

I'd like to land Bug 811120, but I think this bug should be landed soon. I'd rather avoid a risky change to that code so it will apply cleanly.  And I think this bug should be on Aurora anyway since security asked for it.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Security asked for us to add this for B2G updates, we never had it before
User impact if declined: Less secure updates, more risky to land bug 811120 which needs to go to aurora. 
Testing completed (on m-c, etc.): This has a lot of bake time on m-c. 
Risk to taking this patch (and alternatives if risky): Low since if there were problems we would have heard about them by now 
String or UUID changes made by this patch: none

Comment 14

[Brian R. Bondy [:bbondy]]

Comment on attachment 673069 [details] [diff] [review]
Patch v2 - Implementation

Ignore Comment 13, I used the wrong bug links. 
I'll retry the request in a minute :)

Comment 15

[Brian R. Bondy [:bbondy]]

Comment on attachment 673069 [details] [diff] [review]
Patch v2 - Implementation

I'd like to land Bug 791829 on Aurora, but I think this bug should be landed on Aurora first. 
Without this bug, the code for bug 791829 will not apply cleanly to Aurora.
It is safest to put this bug on Aurora and then land Bug 791829.


[Approval Request Comment]
Bug caused by (feature/regressing bug #): Security asked for us to add this for B2G updates, we never had it before
User impact if declined: Less secure updates, more risky landing for Aurora for a bug that doesn't apply cleanly. 
Testing completed (on m-c, etc.): This has a lot of bake time on m-c. 
Risk to taking this patch (and alternatives if risky): Low since if there were problems we would have heard about them by now 
String or UUID changes made by this patch: none

Comment 16

[Brian R. Bondy [:bbondy]]

Comment on attachment 673070 [details] [diff] [review]
Patch v1 - Tests

[Approval Request Comment]
Comment 15, except this patch is for the tests, and it is safest to land with tests.

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 673069 [details] [diff] [review]
Patch v2 - Implementation

[Triage Comment]
Approving to limit updater risk for B2G, and because this has had significant bake time on central already. Not making a call on b-b though, since it's not clear this is a hard requirement.

Comment 18

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/fe0e5fdcaa52
http://hg.mozilla.org/releases/mozilla-aurora/rev/6c9fc008e25c

Is there some reason that b-b is important for tracking reasons even know this is already landed on aurora now? If not, then I think we can clear that flag now.