940684 - (CVE-2014-1507) Device Storage - Additional security issue with OOP on FirefoxOS

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Dave Hylands [:dhylands]]

This is really a follow on to bug 860934

This affects a compromised child.

In bug 860934, the child was able to send a fully qualified path to the parent.
This was addressed by splitting the full path into a storageName which identifies the root directory, and a relative path.

However, if the child were compromised, it could pass in a path which includes ../ and escape out of the storageName'd root directory.

Currently, device storage does check for ..'s in the pathname, but this check is being done in the child, and not in the parent.

Thanks goes to bent for identifying this.

Comment 1

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

This should probably be backported as far back as possible...

Comment 2

[Jason Smith [:jsmith]]

Paul - Can you make a blocking call on this bug?

Comment 3

[Daniel Veditz [:dveditz]]

Paul: we're guessing sec-moderate because it would require a compromised client first, but it does allow the reading of potentially confidential info so maybe sec-high could be justified

Comment 4

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

Yeh I agree Dan - I would say moderate since attack scenarios is a compromised client that _has_ the devicestorage permission.

Comment 5

[Jason Smith [:jsmith]]

The comments above read off that this is not a blocker for release. Renom if you disagree.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

With the info so far this doesn't appear to be severe enough to block release and instead a low risk uplift nomination could be considered when ready.

Comment 7

[Dave Hylands [:dhylands]]

Attachment: Don't allow unsafe paths when constructing DevcieStorageFile object.

Comment 8

[Dave Hylands [:dhylands]]

Comment on attachment 8349759 [details] [diff] [review]
Don't allow unsafe paths when constructing DevcieStorageFile object.

This patch just causes illegal paths to not be appended.

So if a child were to send something like:

type: sdcard, volume: sdcard, relpath: ../somedir

it will wind up being edffectively the same thing as if they passed:

type: sdcard, volume: sdcard, relpath: "" (empty string)

which means that they won't be able to access anything outside of the root directory of the storage area.

Comment 9

[Dave Hylands [:dhylands]]

Added [fxos:media] whiteboard for bugs assigned to me so that they can be triaged/prioritized, etc.

Comment 10

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

Comment on attachment 8349759 [details] [diff] [review]
Don't allow unsafe paths when constructing DevcieStorageFile object.

Review of attachment 8349759 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +900,5 @@
> +    // if a non-safe path is entered. This check here is done in the parent
> +    // and prevents a compromised child from bypassing the check. It shouldn't
> +    // be possible for this code path to be taken with a non-compromised
> +    // child.
> +    return;

Let's at least add a NS_WARNING here.

Comment 11

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/00967488c6bb - commit with incorrect bug number
https://hg.mozilla.org/integration/b2g-inbound/rev/677be012a2aa - backout of previous commit
https://hg.mozilla.org/integration/b2g-inbound/rev/bf42b0e06976 - committed with correct bug number

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf42b0e06976  landed on central

Comment 13

[Dave Hylands [:dhylands]]

Comment on attachment 8349759 [details] [diff] [review]
Don't allow unsafe paths when constructing DevcieStorageFile object.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Has been present since DeviceStorage has been OOP (probably 1.0 timeframe).
User impact if declined: A compromised child process can get access to any file on the system.
Testing completed (on m-c, etc.): Since there is no tests involving a compromised child, I did a manual test by detecting the string foo and replacing with .., and verified that the warning message generated by the patch was trigged, and that the normal client received an error.
Risk to taking this patch (and alternatives if risky): Extremely low
String or IDL/UUID changes made by this patch: None

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Does this need to be explicitly backported for b2g?

Comment 15

[Dave Hylands [:dhylands]]

(In reply to Al Billings [:abillings] from comment #14)
> Does this need to be explicitly backported for b2g?

I'm not sure what you mean?

We should backport this as far as we reasonably can. I'm not familiar with the policies around backporting security problems.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4382683168ee

To get this on b2g18 (v1.1.x) and b2g26 (v1.2.x), you'll need to nominate the patch for approval. See below for more information:
https://wiki.mozilla.org/Release_Management/B2G_Landing

Comment 17

[Dave Hylands [:dhylands]]

Comment on attachment 8349759 [details] [diff] [review]
Don't allow unsafe paths when constructing DevcieStorageFile object.

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Has been present since DeviceStorage has been OOP (probably 1.0 timeframe).
User impact if declined: A compromised child process can get access to any file on the system.
Testing completed (on m-c, etc.): Since there is no tests involving a compromised child, I did a manual test by detecting the string foo and replacing with .., and verified that the warning message generated by the patch was trigged, and that the normal client received an error.
Risk to taking this patch (and alternatives if risky): Extremely low
String or IDL/UUID changes made by this patch: None

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/bc2181968930
https://hg.mozilla.org/releases/mozilla-b2g18/rev/aa0ceb07a73e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/aa0ceb07a73e