Closed
Bug 860934
Opened 12 years ago
Closed 12 years ago
Device Storage - Security issues with OOP on FirefoxOS
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: dhylands, Assigned: dhylands)
References
Details
(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [madrid][fixed-in-birch][adv-main23-])
Attachments
(3 files, 3 obsolete files)
65.83 KB,
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
68.59 KB,
patch
|
dhylands
:
review+
|
Details | Diff | Splinter Review |
64.77 KB,
patch
|
dhylands
:
review+
|
Details | Diff | Splinter Review |
As part of some recent work on device storage, I noticed something which I believe is a potential security issue. Right now, when you do certain file operations, like get/add/delete, the child sends a request to the parent. That request includes both a name and a fullpath. If we assume that the child can be compromised, then the child can send arbitrary filenames in the fullpath parameter and get access to pretty much any file on the filesystem. Since the parent runs as root, this gives the child full read access to any file on the filesystem, and write access to any file writable by root. I think that the fullpath parameter should be removed and that the parent should determine the proper root directory based on the storageType/storageName.
Assignee | ||
Updated•12 years ago
|
blocking-b2g: --- → tef?
Comment 1•12 years ago
|
||
There's no known exploit (although this does sound scary) so we wouldn't typically block - Paul/Doug, what do you think? Should we try for a fix in v1.0.1?
Flags: needinfo?(ptheriault)
Flags: needinfo?(doug.turner)
Comment 2•12 years ago
|
||
we should fix it this week and uplift wherever we can.
Assignee: nobody → doug.turner
Flags: needinfo?(doug.turner)
Updated•12 years ago
|
Keywords: sec-critical
Comment 3•12 years ago
|
||
I agree we should fix this asap. Similar bugs (e.g. all the bugs blocking 776652) were basecamp+.
Blocks: 776652
Flags: needinfo?(ptheriault)
Updated•12 years ago
|
Whiteboard: [status: needs ETA]
Assignee | ||
Updated•12 years ago
|
Assignee: doug.turner → dhylands
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #738407 -
Attachment is obsolete: true
Updated•12 years ago
|
Whiteboard: [status: needs ETA] → [status: needs ETA][madrid]
Comment 8•12 years ago
|
||
Comment on attachment 738428 [details] [diff] [review] Changes child/parent to use relative paths instead of absolute paths. v2 Review of attachment 738428 [details] [diff] [review]: ----------------------------------------------------------------- make sure that this passes try and your local testing. Also make sure music, gallery, video still work :) ::: dom/devicestorage/nsDeviceStorage.cpp @@ +345,5 @@ > +void > +DeviceStorageFile::Init(const nsAString& aStorageType) > +{ > + DeviceStorageFile::GetRootDirectoryForType(aStorageType, > + NS_LITERAL_STRING("sdcard") /* todo? */, This is going to change with the composite patch, right? ;) @@ +454,5 @@ > + > + *aFile = f.get(); > + if (f) { > + NS_ADDREF(*aFile); > + } NS_IF_ADDREF()
Attachment #738428 -
Flags: review?(doug.turner) → review+
Assignee | ||
Comment 9•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=2bc5e9c2c93b with the above changes addressed plus I added some changes to IsSafePath so that it works properly again.
Comment on attachment 738428 [details] [diff] [review] Changes child/parent to use relative paths instead of absolute paths. v2 Review of attachment 738428 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/devicestorage/nsDeviceStorage.cpp @@ +317,5 @@ > + , mEditable(false) > +{ > + Init(aStorageType); > + AppendRelativePath(mRootDir); > + if (!mPath.EqualsLiteral("")) { Just FYI, |mPath.IsEmpty()| would be faster here (and there's another place where you do a similar check below). @@ +453,5 @@ > + } > + > + *aFile = f.get(); > + if (f) { > + NS_ADDREF(*aFile); FYI, you could do |f.forget(aFile);| here to avoid all this.
Assignee | ||
Comment 11•12 years ago
|
||
Found the problem causing orange on try. Submitted to try again: https://tbpl.mozilla.org/?tree=Try&rev=d6eb111707be
Comment 13•12 years ago
|
||
Backed out for mochitest assertions and crashes. https://hg.mozilla.org/projects/birch/rev/b390e72c5a54 https://tbpl.mozilla.org/php/getParsedLog.php?id=21961648&tree=Birch https://tbpl.mozilla.org/php/getParsedLog.php?id=21962211&tree=Birch https://tbpl.mozilla.org/php/getParsedLog.php?id=21961984&tree=Birch https://tbpl.mozilla.org/php/getParsedLog.php?id=21962435&tree=Birch BTW, don't forget the [fixed-in-birch] when landing there.
Updated•12 years ago
|
Target Milestone: --- → Madrid (19apr)
Comment 14•12 years ago
|
||
Setting disabled for everything but trunk and b2g18, as we presumably don't care about this except for those.
status-b2g18:
--- → affected
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → affected
status-firefox21:
--- → disabled
status-firefox22:
--- → disabled
status-firefox23:
--- → affected
status-firefox-esr17:
--- → disabled
tracking-b2g18:
--- → ?
tracking-firefox23:
--- → ?
Updated•12 years ago
|
Assignee | ||
Comment 15•12 years ago
|
||
This should fix the orange failures in debug builds. Changes from previous r+ - Make nsVolumeService thread safe, so that GetVolumeByXxx can be called from any thread. Pushed to try (including debug builds): https://tbpl.mozilla.org/?tree=Try&rev=09bc5c41d4f0
Attachment #738428 -
Attachment is obsolete: true
Attachment #739509 -
Flags: review?(doug.turner)
Updated•12 years ago
|
Attachment #739509 -
Flags: review?(doug.turner) → review+
Updated•12 years ago
|
Whiteboard: [status: needs ETA][madrid] → [status: needs new patch (test failures)][madrid]
Assignee | ||
Comment 16•12 years ago
|
||
Moves the directory services call back to the main thread. Cleaned up the nsVolumeService stuff to use a regular monitor instead of a ReentrantMonitor. Removed called to NotifyObservers out of the monitor. https://tbpl.mozilla.org/?tree=Try&rev=6815bb6a2804
Attachment #739509 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #739638 -
Flags: review?(doug.turner)
Comment 17•12 years ago
|
||
Comment on attachment 739638 [details] [diff] [review] Changes child/parent to use relative paths instead of absolute paths. v4 Review of attachment 739638 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/devicestorage/nsDeviceStorage.cpp @@ +66,5 @@ > using namespace mozilla::dom::devicestorage; > > #include "nsDirectoryServiceDefs.h" > > +struct GlobalDirs : RefCounted<GlobalDirs> use class @@ +78,5 @@ > +#endif > + nsCOMPtr<nsIFile> temp; > +}; > + > +static StaticRefPtr<GlobalDirs> sDirs; no need to clear this on shutdown? ClearOnShutdown(&sDirs) ::: dom/system/gonk/nsIVolumeService.idl @@ +16,5 @@ > interface nsIVolumeService : nsISupports > { > nsIVolume getVolumeByName(in DOMString volName); > nsIVolume getVolumeByPath(in DOMString path); > + nsIVolume getExistingVolumeByPath(in DOMString path); I don't like this interface. I think having a 'get' call that magically creates the volume is probably bad -- especially now that you have different needs here. Would you consider an explict call: nsIVolume create(in DOMString .....) ::: dom/system/gonk/nsVolumeService.cpp @@ +158,5 @@ > return NS_OK; > } > + nsRefPtr<nsVolume> vol; > + { > + MonitorAutoLock autoLock(mArrayMonitor); Triple check that you aren't calling out while holding these locks. ::: dom/system/gonk/nsVolumeService.h @@ +51,5 @@ > > + void CheckMountLock(const nsAString& aMountLockName, > + const nsAString& aMountLockState); > + already_AddRefed<nsVolume> FindVolumeByName(const nsAString& aName); > + already_AddRefed<nsVolume> FindAddVolumeByName(const nsAString& aName); CreateOrFindVolumeByName()
Attachment #739638 -
Flags: review?(doug.turner) → review+
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Doug Turner (:dougt) from comment #17) > Comment on attachment 739638 [details] [diff] [review] > Changes child/parent to use relative paths instead of absolute paths. v4 > > Review of attachment 739638 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/devicestorage/nsDeviceStorage.cpp > @@ +66,5 @@ > > using namespace mozilla::dom::devicestorage; > > > > #include "nsDirectoryServiceDefs.h" > > > > +struct GlobalDirs : RefCounted<GlobalDirs> > > use class I will - although It's not clear to me that there is any advantage in doing this. > @@ +78,5 @@ > > +#endif > > + nsCOMPtr<nsIFile> temp; > > +}; > > + > > +static StaticRefPtr<GlobalDirs> sDirs; > > no need to clear this on shutdown? ClearOnShutdown(&sDirs) Indeed. > ::: dom/system/gonk/nsIVolumeService.idl > @@ +16,5 @@ > > interface nsIVolumeService : nsISupports > > { > > nsIVolume getVolumeByName(in DOMString volName); > > nsIVolume getVolumeByPath(in DOMString path); > > + nsIVolume getExistingVolumeByPath(in DOMString path); > > I don't like this interface. I think having a 'get' call that magically > creates the volume is probably bad -- especially now that you have > different needs here. > > Would you consider an explict call: > > nsIVolume create(in DOMString .....) I've decided to rename GetVolumeByPath to be CreateOrGetVolumeByPath and I'll make GetExistingVolumeByPath just be GetVolumeByPath. > ::: dom/system/gonk/nsVolumeService.cpp > @@ +158,5 @@ > > return NS_OK; > > } > > + nsRefPtr<nsVolume> vol; > > + { > > + MonitorAutoLock autoLock(mArrayMonitor); > > Triple check that you aren't calling out while holding these locks. Yep - I decided to add a FindVolumeByMountLockName just to ensure that we're holding the locks only while iterating the array, and not while performing an operation on the result of the iteration. > ::: dom/system/gonk/nsVolumeService.h > @@ +51,5 @@ > > > > + void CheckMountLock(const nsAString& aMountLockName, > > + const nsAString& aMountLockState); > > + already_AddRefed<nsVolume> FindVolumeByName(const nsAString& aName); > > + already_AddRefed<nsVolume> FindAddVolumeByName(const nsAString& aName); > > CreateOrFindVolumeByName() Done.
Assignee | ||
Comment 19•12 years ago
|
||
Try run is looking green. Once everything has finished, I'll push to birch https://tbpl.mozilla.org/?tree=Try&rev=eb7c911ebf6f
Assignee | ||
Comment 20•12 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/1071ca655e43
Whiteboard: [status: needs new patch (test failures)][madrid] → [madrid][fixed-in-birch]
Comment 21•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1071ca655e43 Should this have tests?
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Assignee | ||
Comment 22•12 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #21) > https://hg.mozilla.org/mozilla-central/rev/1071ca655e43 > > Should this have tests? Its using the existing tests. This particular bug basically reimplemented the code in a more secure manner. So the existing tests provide the required test coverage.
Comment 23•12 years ago
|
||
WFM, thanks for the quick reply! https://hg.mozilla.org/releases/mozilla-b2g18/rev/e9f86fcb3197 This is going to need a branch-specific patch for v1.0.1. It applied to b2g18 mostly OK with a few fixups.
Flags: in-testsuite? → in-testsuite+
Comment 25•12 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #24) > Ryan - sure - I'll work on a 1.0.1 patch. Make that a b2g18 patch too :) https://hg.mozilla.org/releases/mozilla-b2g18/rev/92bc5f26a1d3
Assignee | ||
Comment 26•12 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=071a4af83418 Carrying forward r+
Attachment #741021 -
Flags: review+
Assignee | ||
Comment 27•12 years ago
|
||
b2g18_v1.0.1 version of the patch Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c80bf46756e5 Carrying forward r+
Attachment #741069 -
Flags: review+
Assignee | ||
Comment 28•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/8183424c9e91 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a53c6262936d
Updated•12 years ago
|
tracking-b2g18:
? → ---
Comment 29•11 years ago
|
||
Lowering the severity from sec-critical: this bug by itself is not a remote exploit. Still bad, however, because when combined with another bug leading to a client process compromise (a separate sec-critical bug in it's own right because that could compromise a Firefox desktop client) then this could be used to completely compromise a device.
Updated•11 years ago
|
Whiteboard: [madrid][fixed-in-birch] → [madrid][fixed-in-birch][adv-main23-]
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•