681930 - IndexedDB: unopened sqlite databases don't count towards the quota

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jan Varga [:janv]]

We have discovered that the quota applies only to opened sqlite databases and databases are opened only for the lifetime of a transaction!
So even when I have two pages http://localhost/test.html and http://localhost/test2.html loaded in the browser and both pages open an indexeddb database in onload handler, the quota applies only to pages that run a transaction.

The fix for this bug should probably take into account that we need to store files in the same directory. These files should be restricted by the quota too.

Comment 1

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

The sqlite guys have patched this, I'll upload their changes later today as soon as I've had a chance to test it.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that this isn't your mothers DoS. This can DoS the users system more or less permanently by filling up the hard drive with databases.

Comment 3

[Jan Varga [:janv]]

Are you talking about sqlite3_quota_file() which was added only several hours ago ?
http://www.sqlite.org/cgi/src/timeline

Note, that we need to include non sqlite files in the quota

Comment 4

[Lucas Adamski [:ladamski]]

Even tho we discussed this earlier, I took another look at https://wiki.mozilla.org/Security_Severity_Ratings and am updating the sg rating accordingly.

Comment 5

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

I'm nearly done here, I think.

Comment 6

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

Attachment: Patch, v1

The SQLite changes are from the SQLite folks, they're checked into trunk and we'll get it on our next update I presume.

The other changes are pretty simple but I had to move some code around so the patch looks a little messy.

Basically we keep track of the origins we've already opened and enumerate the directory contents for origins that we haven't yet opened. I've confirmed that filling the target directory with junk before writing to a database triggers quota warnings/

Comment 7

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/6eeb384db995

Comment 8

[Ian Melven :imelven]

https://hg.mozilla.org/mozilla-central/rev/6eeb384db995

Comment 9

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

Comment on attachment 555966 [details] [diff] [review]
Patch, v1

I think we should patch this everywhere we can. Every single IndexedDB mochitest hits this code (so it's well tested) and it only affects sites that the user has allowed to use IndexedDB (not too many).

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 555966 [details] [diff] [review]
Patch, v1

Unless I'm missing something, this should just ride the normal trains.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 555966 [details] [diff] [review]
Patch, v1

Asa: Why wouldn't we want to take security fixes onto branches as to protect users as soon as possible?

Or is there a concern about risk? I'll leave it to ben to give a judgement as to what risk is involved here.

Comment 12

[[e-mail]]

Comment on attachment 555966 [details] [diff] [review]
Patch, v1

>+  nsCString path;
>+  nsresult rv = aFile->GetNativePath(path);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  int rc = ::sqlite3_quota_file(PromiseFlatCString(path).get());
...

Comment 13

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

(In reply to Jonas Sicking (:sicking) from comment #11)
> Or is there a concern about risk? I'll leave it to ben to give a judgement
> as to what risk is involved here.

See comment 9, I think the risk is low.

Comment 14

[[e-mail]]

Comment on attachment 555966 [details] [diff] [review]
Patch, v1

>+  nsCString path;
>+  nsresult rv = aFile->GetNativePath(path);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  int rc = ::sqlite3_quota_file(PromiseFlatCString(path).get());
Also, sqlite3 uses UTF8, not platform encoding...

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 555966 [details] [diff] [review]
Patch, v1

Minusing again per todays triage meeting. The reasoning being that this is a bug we've shipped several times already, and afawk nobody has been hurt by this yet, and the worst case isn't that bad, and we're fixing this already. Speeding this up isn't worth the risk here, even if that risk is low. This will be fixed in ~2 months either way.