823965 - App can delete files from a storage type where it does not have permission

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

While working on a patch for the Gaia UI tests that clears all media from the sdcard between tests I encountered this permissions bug.

If I create a navigator.getDeviceStorage('music') from within the System all (readcreate permission) then I can retrieve all files but not delete them. This is what I'd expect, however if I then create a navigator.getDeviceStorage('pictures') I am able to delete the music files previously obtained.

Steps to reproduce:

var pictures = navigator.getDeviceStorage('pictures');
var music = navigator.getDeviceStorage('music');
mreq = music.enumarate();
mreq.onsuccess = function() {
  dreq = pictures.delete(mreq.result.name); // note this uses pictures to delete music
  dreq.onerror = function () {
    console.error('failed to remove file!');
  };
  dreq.onerror = function () {
    console.log('success removing file!');
  };
};

Expected: As we're trying to delete a file that does not exist in the pictures storage type then I would expect the delete to fail.

Actual: Music files are deleted, even though the app does not have permissions to do this

Comment 1

[Doug Turner (:dougt)]

Yeah.. i see what you're doing.  We don't protect against that.  We probably can.

Comment 2

[Doug Turner (:dougt)]

This isn't a great concern as most permissions are going to be handed out as r/w, and everything on your sdcard is basically exposed with any w permissions.  We'll fix this post v.1

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 699075 [details] [diff] [review]
patch

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

looks good.  I worry a bit about someone storing a blob that doesn't have the correct mime type set.  before this change, we were lax in checking.  now that we are enforcing this, we might see gaia bustage.  Lets keep an eye out for that.

It is better we get this change in now, then to start enforcing this later (and causing lots of bustage)  Marking approved for b2g18.

Comment 5

[Doug Turner (:dougt)]

david/mike, please keep an eye out for any bustage when trying to use device storage to save blobs.

Comment 6

[Andrea Marchesini [:baku]]

Waiting for green on try, then I push it to inbound.

Comment 7

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4536e2bf1cd0

https://hg.mozilla.org/releases/mozilla-b2g18/rev/aa93bb472ff6

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4536e2bf1cd0