Dave Hylands [:dhylands] Assignee
	Description • 12 years ago

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.

	Alex Keybl [:akeybl]
	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?

	Doug Turner (:dougt)
	Comment 2 • 12 years ago

we should fix it this week and uplift wherever we can.

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 3 • 12 years ago

I agree we should fix this asap. Similar bugs (e.g. all the bugs blocking 776652) were basecamp+.

	Johnny Stenback (:jst)
	Comment 4 • 12 years ago

tef+ per triage meeting.

	Dave Hylands [:dhylands] Assignee
	Comment 7 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=cf7746a1c06b

	Doug Turner (:dougt)
	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()

	Dave Hylands [:dhylands] 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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 10 • 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]:
-----------------------------------------------------------------

::: 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.

	Dave Hylands [:dhylands] 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

	Dave Hylands [:dhylands] Assignee
	Comment 12 • 12 years ago

https://hg.mozilla.org/projects/birch/rev/8f7ef60b6f5a

	Ryan VanderMeulen [:RyanVM]
	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.

	Andrew McCreight [:mccr8]
	Comment 14 • 12 years ago

Setting disabled for everything but trunk and b2g18, as we presumably don't care about this except for those.

	Dave Hylands [:dhylands] 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

	Dave Hylands [:dhylands] 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

	Doug Turner (:dougt)
	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()

	Dave Hylands [:dhylands] Assignee
	Comment 18 • 11 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.

	Dave Hylands [:dhylands] Assignee
	Comment 19 • 11 years ago

Try run is looking green. Once everything has finished, I'll push to birch
https://tbpl.mozilla.org/?tree=Try&rev=eb7c911ebf6f

	Dave Hylands [:dhylands] Assignee
	Comment 20 • 11 years ago

https://hg.mozilla.org/projects/birch/rev/1071ca655e43

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/1071ca655e43

Should this have tests?

	Dave Hylands [:dhylands] Assignee
	Comment 22 • 11 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 11 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.

	Dave Hylands [:dhylands] Assignee
	Comment 24 • 11 years ago

Ryan - sure - I'll work on a 1.0.1 patch.

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 11 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

	Dave Hylands [:dhylands] Assignee
	Comment 26 • 11 years ago

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=071a4af83418

Carrying forward r+

	Dave Hylands [:dhylands] Assignee
	Comment 27 • 11 years ago

b2g18_v1.0.1 version of the patch

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=c80bf46756e5

Carrying forward r+

	Dave Hylands [:dhylands] Assignee
	Comment 28 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8183424c9e91
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a53c6262936d

	Daniel Veditz [:dveditz]
	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.

	Daniel Veditz [:dveditz]
	Comment 30 • 11 years ago

> ... could be ...

could have been