Closed Bug 860934 Opened 7 years ago Closed 7 years ago

Device Storage - Security issues with OOP on FirefoxOS

Categories

(Core :: DOM: Device Interfaces, defect)

19 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
1.0.1 Madrid (19apr)
blocking-b2g tef+
Tracking Status
firefox21 --- disabled
firefox22 --- disabled
firefox23 + fixed
firefox-esr17 --- disabled
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

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)

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.
blocking-b2g: --- → tef?
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)
we should fix it this week and uplift wherever we can.
Assignee: nobody → doug.turner
Flags: needinfo?(doug.turner)
I agree we should fix this asap. Similar bugs (e.g. all the bugs blocking 776652) were basecamp+.
Blocks: 776652
Flags: needinfo?(ptheriault)
tef+ per triage meeting.
blocking-b2g: tef? → tef+
Whiteboard: [status: needs ETA]
Assignee: doug.turner → dhylands
Attachment #738407 - Attachment is obsolete: true
Whiteboard: [status: needs ETA] → [status: needs ETA][madrid]
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+
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.
Found the problem causing orange on try. Submitted to try again:
https://tbpl.mozilla.org/?tree=Try&rev=d6eb111707be
Target Milestone: --- → Madrid (19apr)
Setting disabled for everything but trunk and b2g18, as we presumably don't care about this except for those.
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)
Attachment #739509 - Flags: review?(doug.turner) → review+
Whiteboard: [status: needs ETA][madrid] → [status: needs new patch (test failures)][madrid]
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
Attachment #739638 - Flags: review?(doug.turner)
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+
(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.
Try run is looking green. Once everything has finished, I'll push to birch
https://tbpl.mozilla.org/?tree=Try&rev=eb7c911ebf6f
https://hg.mozilla.org/projects/birch/rev/1071ca655e43
Whiteboard: [status: needs new patch (test failures)][madrid] → [madrid][fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/1071ca655e43

Should this have tests?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(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.
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+
Ryan - sure - I'll work on a 1.0.1 patch.
(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
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=071a4af83418

Carrying forward r+
Attachment #741021 - Flags: review+
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+
Depends on: 865255
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.
> ... could be ...

could have been
Whiteboard: [madrid][fixed-in-birch] → [madrid][fixed-in-birch][adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.