1514683 - Evaluate enabling SQLITE_DBCONFIG_DEFENSIVE

Status: NEW

(Toolkit :: Storage, enhancement, P2)

Description

[Marco Bonardo [:mak]]

SQLITE_DBCONFIG_DEFENSIVE is a new flag that disallows a few dangerous features of Sqlite that may corrupt databases.
We surely can take this for our products, my only concern is that we may end up forcing this on system Sqlite. It doesn't seem particularly bad but it will disallow those features more widely.

Comment 1

[Marco Bonardo [:mak]]

A possible take on this may be that system sqlite should be safer, while apps willing to expose dangerous features should use their own compile, rather than the opposite.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1514683 - Enable SQLITE_DBCONFIG_DEFENSIVE. r=mak

Comment 3

[Ryan VanderMeulen [:RyanVM]]

As noted in the previous bug, this change passes in our CI. So like you said, it seems like the main unresolved question is what to do with downstream distros.

Comment 4

[Marco Bonardo [:mak]]

Imho we should take this, my opinion is in comment 1. It's worth getting more opinions though, Andrew, what do you think?

Comment 5

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

I also think we should take this.

In terms of system SQLite, I think that one of the main arguments of using system packaged SQLite is that they're kept up-to-date for security reasons.  I don't think there's any sane/safe reason to not want SQLITE_DBCONFIG_DEFENSIVE, so I think we're aligned with disto's here.  In the event a distro doesn't want it, they can patch out the configure check which is our only enforcement here.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7408e4b312e9
Enable SQLITE_DBCONFIG_DEFENSIVE. r=mak

Comment 7

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/7408e4b312e9

Comment 8

[Alyssa Ross]

I don’t think this patch was correct. From my reading of the SQLite source, SQLITE_DBCONFIG_DEFENSIVE isn’t a compile time option, but rather a bitflag that can be supplied to SQLite at runtime when establishing a connection.

As a result of this, I can no longer build Firefox Nightly from source, even if I build SQLite with -DSQLITE_DBCONFIG_DEFENSIVE. It’s possible -DSQLITE_DBCONFIG_DEFENSIVE=1 will build siccessfully (I’m trying that now), but I still don’t think it will do the right thing.

If I’m correct, and SQLITE_DBCONFIG_DEFENSIVE isn’t a compile-time option to SQLite, this change should be reverted, and then as a follow up, Firefox can take advantage of this flag by passing it when it creates a SQLite connection.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Indeed, thanks for pointing it out!

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/079be36cc55e
Backed out changeset 7408e4b312e9 because it's not the correct way to enable SQLITE_DBCONFIG_DEFENSIVE.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Setting connection-time options isn't something I'm familiar with - can you please point me in the right direction, Marco?

Comment 12

[Mike Hommey [:glandium]]

SQLITE_DBCONFIG_* options are set with sqlite3_db_config.
https://www.sqlite.org/c3ref/c_dbconfig_defensive.html

Comment 13

[Marco Bonardo [:mak]]

oops, this is totally my fault, I missed the DBCONFIG bit :( This is even better, we won't touch system sqlite.

You can have a look at the patch in bug 1270882 that, modulo the define we don't need here, does pretty much the same (also, check my review comment on it). The only difference is that, based on the documentation, SQLITE_DBCONFIG_DEFENSIVE doesn't have any additional argument, so it should be enough to:
srv = ::sqlite3_db_config(mDBConn, SQLITE_DBCONFIG_DEFENSIVE);
MOZ_ASSERT(srv == SQLITE_OK, "SQLITE_DBCONFIG_DEFENSIVE should be enabled");
in the same call points

Comment 14

[Arfrever Frehtes Taifersar Arahesis]

In SQLite, sqlite3_db_config() handles SQLITE_DBCONFIG_DEFENSIVE in the same way as most operations (only SQLITE_DBCONFIG_MAINDBNAME and SQLITE_DBCONFIG_LOOKASIDE are handled slightly differently).
See calls to va_arg() in sqlite3_db_config() in https://sqlite.org/src/file?name=src/main.c
int onoff = va_arg(ap, int);
int pRes = va_arg(ap, int);
...
if( onoff>0 ){
db->flags |= aFlagOp[i].mask;
}else if( onoff==0 ){
db->flags &= ~(u64)aFlagOp[i].mask;
}

va_arg(3) man page says:
"If there is no next argument, or if type is not compatible with the type of the actual next argument (as promoted according to the default argument promotions), random errors will occur."

And https://sqlite.org/capi3ref.html#sqlitedbconfigdefensive says that SQLITE_DBCONFIG_DEFENSIVE option can be used for both activating or deactivating the "defensive" flag for a database connection.

So usage is:

• Activation:
  sqlite3_db_config(db, SQLITE_DBCONFIG_DEFENSIVE, 1, 0);
• Deactivation:
  sqlite3_db_config(db, SQLITE_DBCONFIG_DEFENSIVE, 0, 0);

In this case you should use:
srv = ::sqlite3_db_config(mDBConn, SQLITE_DBCONFIG_DEFENSIVE, 1, 0);

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I've got a patch which appears to be working, at least going off the Try results. Unfortunately, it's because I'm seeing real-looking test failures :(

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=221235976&repo=try&lineNumber=1974

Of particular interest:
INFO - PID 9637 | [9637, DOMCacheThread] WARNING: The SQL statement 'UPDATE sqlite_master SET sql=:sql WHERE name='entries'' could not be compiled due to an error: table sqlite_master may not be modified: file /builds/worker/workspace/build/src/storage/mozStorageConnection.cpp, line 1158

Comment 16

[Marco Bonardo [:mak]]

It looks like DOM Cache is using writable_schema to remove DEFAULT from NOT NULL columns that are added (through ALTER TABLE) in schema migrations. That has the advantage to force statements to always provide a value for each column, Sqlite will do the checks and fail for you.
An alternative to this may be to create a new table, copy over the data to it, and remove the old one, that is surely a little bit more annoying than a simple ALTER TABLE.
Second alternative may be adding NULLable columns and shutdown debug-only asserts that ensure the added columns are not null, then a test harness with a decent coverage would basically guarantee the same (that's what we did for Places GUIDs for example).
The last alternative would be to expose a Storage API to remove DEFAULT values from columns of a table, then we could temporarily unset SQLITE_DBCONFIG_DEFENSIVE just for that op and ensure others won't abuse it.
Unfortunately none of these is a trivial change.

The last DOM cache schema migration using writable schema happened in Firefox 57, if there has been a watershed version after it (was Firefox 60 one of those?), we could just remove all the migrations doing this, and for future migrations use one of the above approaches.
Not sure who can evaluate DOM cache storage changes nowadays, Andrew should know though.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

I'm not going to be able to take this bug any further, sadly. Here's the patch for anybody looking to resurrect it down the road.
https://hg.mozilla.org/try/rev/a3f1840105b431db2d3134d238e5f746c214e6f1

Comment 18

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

I can evaluate DOM Cache API things. Currently all QuotaManager clients (of which Cache API is one) have extensive upgrade logic. We're on board with declaring some things too old to migrate, but it'll be at least a few more months until we're able to have test coverage for this and to rip out the old logic. It's likely this will be part of the QuotaManager v4 effort, which I'm marking this as dependent upon.

Comment 19

[Marco Bonardo [:mak]]

we seem to be far enough from Firefox 57, can we make this a mentored bug to remove all the migrations up to MigrateFrom25To26 included, remove aRewriteSchema support from DOM:Cache, and then enable this setting? Now we don't even have anymore the system SQLite concern.

Comment 20

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

Amending my previous comment somewhat; I don't think it's a given that we're ready to remove the existing schema migration and validation mechanism from the Cache API implementation. I'd like to revisit how it does migrations and validates database sanity at the next schema change given that we do know that the current approach had problems in at least some upgrades and that manual validation of SQLite behavior in this space can be misleading.

However, given that sqlite3_db_config() can be called at any time and the Cache API is using the synchronous mozStorage API, it's absolutely fine to:

1. Change the default so that the SQLITE_DBCONFIG_DEFENSIVE is on by default.
2. Add a [noscript] exposure of the ability to turn this on/off to mozIStorageConnection.
3. Change the RewriteEntriesSchema so that it manually turns the protection off and then turns it back on again afterwards.

I should also note that we likely will be in the market for removing older migrations soon, but those will happen in component-specific bugs and that's somewhat orthogonal to the issue at hand in this bug. (And I had some proposals in the bug discussing the broken Cache API upgrade.)

cc'ing Eden and Jan for context as I don't see them on the cc list right now and they may have different thoughts.

Comment 21

[Marco Bonardo [:mak]]

I'm ok with that, provided the [noscript] IDL method clarifies no new uses of it should be added. writable schema is a footgun with workarounds, it's ok to retain consumers until necessary, but we should not add new ones.