Open Bug 1514683 Opened Last year Updated 10 months ago



(Toolkit :: Storage, enhancement, P2)





(Reporter: mak, Unassigned, NeedInfo)




(1 obsolete file)

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.
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.
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.
Imho we should take this, my opinion is in comment 1. It's worth getting more opinions though, Andrew, what do you think?
Flags: needinfo?(bugmail)
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.
Flags: needinfo?(bugmail)
Assignee: nobody → ryanvm
No longer blocks: 1514408
Depends on: SQLIte3.26.0
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
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.
Flags: needinfo?(ryanvm)
Indeed, thanks for pointing it out!
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---
Target Milestone: mozilla66 → ---
Backout by
Backed out changeset 7408e4b312e9 because it's not the correct way to enable SQLITE_DBCONFIG_DEFENSIVE.
Setting connection-time options isn't something I'm familiar with - can you please point me in the right direction, Marco?
Flags: needinfo?(mak77)
SQLITE_DBCONFIG_* options are set with sqlite3_db_config.
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);
in the same call points
Flags: needinfo?(mak77)

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
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 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);

Attachment #9032134 - Attachment is obsolete: true

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

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

Flags: needinfo?(mak77)

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.

Flags: needinfo?(mak77) → needinfo?(bugmail)

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.

Assignee: ryanvm → nobody
You need to log in before you can comment on or make changes to this bug.