QuotaManager needs to deal with SQLite path canonicalization/symlink piercing when we upgrade to 3.39
Categories
(Core :: Storage: Quota Manager, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox112 | --- | fixed |
People
(Reporter: asuth, Assigned: janv)
References
Details
Attachments
(1 file)
In bug 1776566 to upgrade SQLite this assertion started firing which appears to be the result of SQLite newly canonicalizing the database path(s) and implies that we should probably canonicalize any paths before handing them to SQLite on linux and OS X so that the telemetryVFS can then perform the lookup in the same canonicalized-space. Another option could be to try and rely on PROFILE/storage
presumably having a consistent root, in which case we just need to be able to recognize and strip the canonicalized prefix in addition to a non-canonicalized prefix. One complication we should be aware of is that on Windows the Firefox profile can potentially exist across the roaming profile and local-machine paths, but I don't believe the windows VFS is impacted.
Note that I have not re-familiarized myself with the way the VFS/GetQuotaObject is working here (yet) so the above is hand-waving.
Assignee | ||
Updated•2 years ago
|
Comment 2•2 years ago
•
|
||
Jan? Would be nice to leverage the improvements made in 3.39.x.
Reporter | ||
Comment 3•2 years ago
•
|
||
Jan is focusing largely on OPFS right now, tentative plan is:
- Jan uploads his existing WIP patches to phabricator and/or via a try push that can be linked here.
- Harveer will take a look once we have those patches up.
Clearing NI to set it again to get a rising edge. Jan should ideally set an NI on Harveer after uploading the WIPs.
Reporter | ||
Updated•2 years ago
|
Assignee | ||
Comment 4•2 years ago
|
||
I might actually have more time for this while I will be waiting for some OPFS reviews.
Comment hidden (off-topic) |
Comment 6•2 years ago
|
||
Sorry to ping, but we're blocked on Sqlite upgrades due to this bug, do we have some quick workaround or fix we can land to unblock the situation?
Reporter | ||
Comment 7•2 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #6)
Sorry to ping, but we're blocked on Sqlite upgrades due to this bug, do we have some quick workaround or fix we can land to unblock the situation?
Naive / quick-fixes potentially risk defeating quota management in the browser entirely and thereby result in DoS of user machines and enable side-channel attacks. A good point made by RyanVM via other channels is that we potentially need to add additional test coverage when addressing this scenario as commenting out this load-bearing assertion potentially still results in green tests because there aren't adversarial tests to deal with split-brain quota management via opaque-box or clear-box testing.
To try and clarify for prioritization, is there something being blocked from landing that depends on the upgrade (other than the upgrade itself) or do we have quantified in-tree benchmarks improvements that we're missing out on? The hygiene benefits of being on the most recent SQLite release are clear, but prioritization is generally driven by OKRs and because we do not expose SQLite directly to content the hygiene benefits of being on the most recent SQLite release don't rise to security concerns.
Comment hidden (advocacy) |
Comment hidden (advocacy) |
Assignee | ||
Comment 10•2 years ago
|
||
FYI, there's an interesting discussion at https://sqlite.org/forum/info/66d7d9ae9a30f05b8cc1082726ac91899e9e1089384572ab5b79d16fae2a1f16
The new Unix VFS implementation now resolves all path components but only if given directory exists which is different from what realpath
does. realpath
is the main supporting function for implementation of nsIFile::Normalize on Unix. So it's quite tricky to use Normalize before we check paths in quota manager (I already tried that).
We already had to disable some of the xFullPathname features on Windows, see https://searchfox.org/mozilla-central/rev/aa3ccd258b64abfd4c5ce56c1f512bc7f65b844c/storage/TelemetryVFS.cpp#561
There are multiple ways how to fix this, but I haven't found a solution which I would like enough. Originally, I wanted to use directory locks for carrying the file path information, but that requires many changes and I don't like that we would use independent file paths when we initialize QuotaObjects with current file sizes (we could however pass the file size from SQLite probably).
Still investigating all the options...
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 11•2 years ago
|
||
Depends on D171440
Comment 12•2 years ago
|
||
Comment 13•2 years ago
|
||
bugherder |
Comment 14•2 years ago
|
||
Thank you, Jan!
Description
•