813995 - Need additional security checks for the "device-storage" permissions

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

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

Some notes on the discussion I had with dougt:

  device-storage:apps:
    in child process check for webapp-manage, need to check in parent too
    no 'access' checks
    nsIContentPermissionPrompt can be bypassed from child
    maybe assert permission when handing out blobs (maybe /data only)

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

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

Comment on attachment 685300 [details] [diff] [review]
patch v.1

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

This stuff looks fine, but it only addresses a piece of the problem (for "apps"). Are you planning on doing the other type checks in another bug?

Comment 3

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

Comment on attachment 685300 [details] [diff] [review]
patch v.1

Actually, hm. It's weird that you're doing this in a constructor since you can't bail out if the app doesn't have permission. The child process will get killed regardless but you keep on going as if nothing happened...

Comment 4

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

Comment on attachment 685300 [details] [diff] [review]
patch v.1

Actually, yeah, can we do this in ContentParent::AllocPDeviceStorageRequest instead? Then we can bail out correctly by returning null.

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 7

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

Comment on attachment 686217 [details] [diff] [review]
patch v.3

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

Looks good!

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +182,5 @@
> +      return false;
> +    }
> +  }
> +
> +  // the 'apps' type is special.  We only want this exposed

Nit: Capitalize 'the'

@@ +190,5 @@
> +      return false;
> +    }
> +  }
> +
> +  nsCString permissionName;

Since you're doing some appends below you should use nsAutoCString.

::: dom/devicestorage/DeviceStorageRequestParent.h
@@ +25,5 @@
>  
>    NS_IMETHOD_(nsrefcnt) AddRef();
>    NS_IMETHOD_(nsrefcnt) Release();
> +
> +  bool EnsureRequiredPermissions(mozilla::dom::ContentParent* aParent);  

Nit: trailing whitespace

::: dom/ipc/ContentParent.cpp
@@ +1192,5 @@
>  ContentParent::AllocPDeviceStorageRequest(const DeviceStorageParams& aParams)
>  {
> +  nsRefPtr<DeviceStorageRequestParent> result = new DeviceStorageRequestParent(aParams);
> +  if (!result->EnsureRequiredPermissions(this)) {
> +      return nullptr; 

Nit: trailing whitespace

Comment 8

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0690146190f1

Comment 9

[Doug Turner (:dougt)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/13fa37380b51
https://hg.mozilla.org/releases/mozilla-beta/rev/fbf8dcadab61

Comment 10

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0690146190f1