Closed Bug 1781116 Opened 1 year ago Closed 7 months ago

QuotaManager needs to deal with SQLite path canonicalization/symlink piercing when we upgrade to 3.39

Categories

(Core :: Storage: Quota Manager, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
112 Branch
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: nobody → jvarga

Any news on this, Jan?

Flags: needinfo?(jvarga)

Jan? Would be nice to leverage the improvements made in 3.39.x.

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.

Assignee: jvarga → nobody
Flags: needinfo?(jvarga)
Flags: needinfo?(jvarga)

I might actually have more time for this while I will be waiting for some OPFS reviews.

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?

(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.

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...

Flags: needinfo?(jvarga)
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Depends on: 1819535
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d0e58f96dc8
QuotaManager needs to deal with SQLite path canonicalization/symlink piercing; r=asuth,dom-storage-reviewers
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → 112 Branch

Thank you, Jan!

You need to log in before you can comment on or make changes to this bug.