Closed Bug 940684 (CVE-2014-1507) Opened 6 years ago Closed 6 years ago

Device Storage - Additional security issue with OOP on FirefoxOS

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
blocking-b2g -
Tracking Status
firefox26 - wontfix
firefox27 - wontfix
firefox28 - fixed
firefox29 --- fixed
firefox-esr24 --- unaffected
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: dhylands, Assigned: dhylands)

Details

(Keywords: sec-moderate, Whiteboard: [fxos:media][adv-main28+])

Attachments

(1 file)

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.
This should probably be backported as far back as possible...
blocking-b2g: --- → koi?
Paul - Can you make a blocking call on this bug?
Flags: needinfo?(ptheriault)
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
Yeh I agree Dan - I would say moderate since attack scenarios is a compromised client that _has_ the devicestorage permission.
Flags: needinfo?(ptheriault)
The comments above read off that this is not a blocker for release. Renom if you disagree.
blocking-b2g: koi? → -
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 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.
Attachment #8349759 - Flags: review?(bent.mozilla)
Added [fxos:media] whiteboard for bugs assigned to me so that they can be triaged/prioritized, etc.
Whiteboard: [fxos:media]
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.
Attachment #8349759 - Flags: review?(bent.mozilla) → review+
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
Attachment #8349759 - Flags: approval-mozilla-aurora?
Attachment #8349759 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Does this need to be explicitly backported for b2g?
(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.
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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
Attachment #8349759 - Flags: approval-mozilla-b2g26?
Attachment #8349759 - Flags: approval-mozilla-b2g18?
Attachment #8349759 - Flags: approval-mozilla-b2g26?
Attachment #8349759 - Flags: approval-mozilla-b2g26+
Attachment #8349759 - Flags: approval-mozilla-b2g18?
Attachment #8349759 - Flags: approval-mozilla-b2g18+
Whiteboard: [fxos:media] → [fxos:media][adv-main28+]
Alias: CVE-2014-1507
Group: core-security
You need to log in before you can comment on or make changes to this bug.