Closed Bug 805354 Opened 7 years ago Closed 7 years ago

Add read/write permission checks to chrome databases from other processes

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(1 file, 1 obsolete file)

We need to make sure that a random process can't read and write chrome databases unless they've been explicitly granted this permission.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attached patch Patch, v1 (obsolete) — Splinter Review
Jonas, is this about what you had in mind?
Attachment #675009 - Flags: review?(jonas)
Ben told me this is a big security hole for the settings db.
blocking-basecamp: --- → +
Comment on attachment 675009 [details] [diff] [review]
Patch, v1

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

Sicking is out of town, need to get this looked at ASAP.
Attachment #675009 - Flags: review?(jonas) → review?(khuey)
Comment on attachment 675009 [details] [diff] [review]
Patch, v1

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

::: dom/apps/src/PermissionsInstaller.jsm
@@ +434,5 @@
> +    // Handle additional permissions.
> +    if (data.additional) {
> +      perms = perms.concat(data.additional);
> +    }
> + 

Extra whitespace.

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +124,5 @@
>                                                 PIndexedDBDatabaseParent* aActor,
>                                                 const nsString& aName,
>                                                 const uint64_t& aVersion)
>  {
> +  MOZ_ASSERT(!mDisconnected);

No.  This is wrong until we fix all this shit to not race.

@@ +445,5 @@
> +      AsyncConnectionHelper::GetCurrentTransaction();
> +    MOZ_ASSERT(transaction);
> +
> +    if (!CheckWritePermission(databaseConcrete)) {
> +      if (NS_FAILED(transaction->Abort())) {

Please add a comment about how if we enter this branch the child is a dead man walking.

@@ +728,5 @@
>                                      PIndexedDBObjectStoreParent* aActor,
>                                      const ObjectStoreConstructorParams& aParams)
>  {
> +  if (!mTransaction) {
> +    // This can happen if the verion change transaction was aborted.

by the parent.

Also spell version correctly please.
Attachment #675009 - Flags: review?(khuey) → review+
Attached patch Patch, v2Splinter Review
Fabrice, could you look over the PermissionsInstaller.jsm changes? I had to do a little extra work once Gregor's settings permission patch landed.
Attachment #675009 - Attachment is obsolete: true
Attachment #676878 - Flags: review?(fabrice)
Comment on attachment 676878 [details] [diff] [review]
Patch, v2

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

::: dom/apps/src/PermissionsInstaller.jsm
@@ +296,5 @@
> +
> +  if (tableEntry.access && aAccess) {
> +    let requestedSuffixes = [];
> +    switch(aAccess) {
> +    case READONLY:

Nit: indent the 'case...'
Attachment #676878 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/1008f2b00216
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 807779
You seem to have pushed a diff-w patch here :( Indentation in the PermissionsInstaller.jsm changes are all whacky!
(In reply to Jonas Sicking (:sicking) from comment #10)
> You seem to have pushed a diff-w patch here :( Indentation in the
> PermissionsInstaller.jsm changes are all whacky!

Eh? https://hg.mozilla.org/integration/mozilla-inbound/rev/1008f2b00216 looks fine to me...
(In reply to ben turner [:bent] from comment #11)
> (In reply to Jonas Sicking (:sicking) from comment #10)
> > You seem to have pushed a diff-w patch here :( Indentation in the
> > PermissionsInstaller.jsm changes are all whacky!
> 
> Eh? https://hg.mozilla.org/integration/mozilla-inbound/rev/1008f2b00216
> looks fine to me...

No, the indentations in the patches for central and aurora are different.

  https://hg.mozilla.org/mozilla-central/rev/1008f2b00216 (bad)
  https://hg.mozilla.org/releases/mozilla-aurora/rev/840c1b46ad0b (good)

Anyway, the later patch at bug Bug 805655 has already cleaned them up. However, it also means the patch cannot cleanly be applied from central to aurora/beta.
You need to log in before you can comment on or make changes to this bug.