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

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement)

Description

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Any news on this, Jan?

Comment 2

[Arthur K. (he/him)]

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

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 4

[Jan Varga [:janv]]

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

Comment 5

[Arthur K. (he/him)]

(In reply to Jan Varga [:janv] from comment #4)

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

Not so much?

Comment 6

[Marco Bonardo [:mak]]

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?

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

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

[Arthur K. (he/him)]

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

Dare I ask if it is possible to update Windows builds to 3.39.x only and keep Mac and Linux using 3.38.x until a proper fix is produced? 3 months is getting to be a long time for fix. Is this canonicalizing issue really that bad?

Comment 9

[Arthur K. (he/him)]

Jan,

Just wanting to keep this on your radar. Any chance you'll have some cycles to peek at a solution?

Comment 10

[Jan Varga [:janv]]

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

Comment 11

[Jan Varga [:janv]]

Attachment: Bug 1781116 - QuotaManager needs to deal with SQLite path canonicalization/symlink piercing; r=#dom-storage,asuth

Depends on D171440

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d0e58f96dc8
QuotaManager needs to deal with SQLite path canonicalization/symlink piercing; r=asuth,dom-storage-reviewers

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1d0e58f96dc8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Thank you, Jan!