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

RESOLVED FIXED

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: janv, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
The sqlite guys have patched this, I'll upload their changes later today as soon as I've had a chance to test it.
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.
Whiteboard: [sg:dos]
(Reporter)

Comment 3

6 years ago
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
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.
Whiteboard: [sg:dos] → [sg:moderate]
I'm nearly done here, I think.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Created attachment 555966 [details] [diff] [review]
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/
Attachment #555966 - Flags: review?(jonas)
Attachment #555966 - Flags: review?(jonas) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/6eeb384db995

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/6eeb384db995
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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).
Attachment #555966 - Flags: approval-mozilla-release?
Attachment #555966 - Flags: approval-mozilla-beta?
Attachment #555966 - Flags: approval-mozilla-aurora?

Comment 10

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

Unless I'm missing something, this should just ride the normal trains.
Attachment #555966 - Flags: approval-mozilla-release?
Attachment #555966 - Flags: approval-mozilla-release-
Attachment #555966 - Flags: approval-mozilla-beta?
Attachment #555966 - Flags: approval-mozilla-beta-
Attachment #555966 - Flags: approval-mozilla-aurora?
Attachment #555966 - Flags: approval-mozilla-aurora-
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.
Attachment #555966 - Flags: approval-mozilla-beta?
Attachment #555966 - Flags: approval-mozilla-beta-
Attachment #555966 - Flags: approval-mozilla-aurora?
Attachment #555966 - Flags: approval-mozilla-aurora-
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());
...
(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 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 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.
Attachment #555966 - Flags: approval-mozilla-beta?
Attachment #555966 - Flags: approval-mozilla-beta-
Attachment #555966 - Flags: approval-mozilla-aurora?
Attachment #555966 - Flags: approval-mozilla-aurora-
Group: core-security
Component: DOM → DOM: IndexedDB
You need to log in before you can comment on or make changes to this bug.