Open Bug 1049920 Opened 6 years ago Updated 1 year ago

support PRAGMA secure_delete

Categories

(Toolkit :: Storage, defect, P3)

All
Linux
defect

Tracking

()

People

(Reporter: lambda.tango, Unassigned, Mentored)

References

Details

Attachments

(1 file)

Some systems prefer to use system libraries instead of bundled ones for reasons related to security (ie faster updates), performance, and/or storage space (avoiding duplicate code). At present, Firefox does support this behavior with regard to the SQLite backend, but the implementation is lacking.

As per bug #546162, Firefox needs to ensure SQLite's SECURE_DELETE is enabled. To do this, SQLite is bundled with Firefox, compiled with SECURE_DELETE enabled for all operations. 

Previously, to use the system sqlite, one had to compile it with SECURE_DELETE enabled for all operations, to the possible detriment of all non-Firefox SQLite operations.

This bug offers an alternative:

Recent SQLite versions support a SECURE_DELETE runtime pragma. This would allow use of the system SQLite without requiring SECURE_DELETE to be enabled at compile-time, instead telling SQLite to enable SECURE_DELETE for Firefox's operations at runtime.

This would ensure SECURE_DELETE is active for Firefox, without forcing it on other users of system SQLite.

Attached is the patch from the aformentioned bug to attempt to implement this behaviour. It runs successfully on Firefox 26.0, but a) has not been updated, and b) is potentially incomplete. It is being included as a potential starting point.
See Also: → 546162
Comment on attachment 8468793 [details] [diff] [review]
Use secure_delete runtime pragma for Firefox 26.0

Review of attachment 8468793 [details] [diff] [review]:
-----------------------------------------------------------------

please add more tests to test_sqlite_secure_delete.js that check PRAGMA secure_delete returns 1 after invoking each of the following APIs

openDatabase
openUnsharedDatabase
openAsyncDatabase
.clone
.asyncClone

::: storage/src/mozStorageConnection.cpp
@@ +679,5 @@
>  
> +  // Set the secure_delete PRAGMA, if sqlite isn't already compiled with it.
> +#ifdef MOZ_SECURE_DELETE_PRAGMA
> +  (void)ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +          "PRAGMA secure_delete = ON;"));

please indent only 4 chars from the (void), see the PRAGMA synchronous indentation as an example

also please use 1 instead of ON, get the rv and use a MOZ_ASSERT on it.

@@ +1163,4 @@
>      "journal_size_limit",
>      "synchronous",
>      "wal_autocheckpoint",
> +    "secure_delete",

I don't think this is needed, because when we clone a connection we call initialize that calls initializeInternal. So it should be fine already.
Please verify that.
Hello.

I've never done Firefox development before, but I would like to see these changes land to master. Since it is already 2015 and nobody seems to be working on it I am asking somebody to guide me towards right direction to resolve this bug. Are points made above still valid?
yes, the above patch and comments are still valid.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Will this even affect something that accesses sqlite via the wrappers defined in sqlite_internal.js ?  Given that it contains an 'open' and 'open2' wrapper I would expect that this pragma query would need to occur on any related open call originating from js too?
(In reply to Ian Stakenvicius from comment #4)
> Will this even affect something that accesses sqlite via the wrappers
> defined in sqlite_internal.js ?  Given that it contains an 'open' and
> 'open2' wrapper I would expect that this pragma query would need to occur on
> any related open call originating from js too?

yes.
OK.  so, thunderbird has a vested interest in something similar to this, due to its need for FTS3_TOKENIZER for gloda and sqlite-3.12 now allowing for it to be enabled via a ' sqlite3_db_config(db,SQLITE_DBCONFIG_ENABLE_FTS3_TOKENIZER,1,0)' call per each connection.  

Does anybody have any thoughts on what the best fit would be to add these calls?  My naive guess would be to add them somehow to the open and open2 wrappers, but i expect there is likely a better way than that..  for the tokenizer support I figure the best method would be to expose sqlite3_db_config via the js api wrappers and add the code to make gloda use it; that doesn't fit the secure_delete pragma very well though...
Re: sqlite_internal.js, my impression is that it's a half-completed implementation.  See bug 853438 for the first steps at sqlite.js which would be where any PRAGMA's would be added when that bug is fixed.  It would likely be helpful to comment on that bug when fixing this one so we don't forget.

I've filed bug 1270882 on the FTS3_TOKENIZER stuff.  It's very related to the goal here of eliminating defines imposed on system SQLite and protecting Firefox from defines imposed by system SQLite that Firefox doesn't need, but is sufficiently different to merit its own bug, I think.  (Especially if we get into debates over Thunderbird imposing on Gecko, although in this case I think it's clear cut that it's a win for everyone to switch to the sqlite3_db_config call.)
Priority: -- → P3
I see that bug #1270882 for FTS3_TOKENIZER is getting some traction, is there any hope for this related bug to move forward too ? i've been shipping http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/www/mozilla-firefox/patches/patch-storage_mozStorageConnection_cpp?rev=1.15&content-type=text/x-cvsweb-markup since forever and would definitely remove it once this is solved.
we'd surely evaluate a patch with a fatal assertion and a simple test.

An heads-up, bug 1523851 is changing the way we detect compile time options from compile time to runtime. You may have to update your patch if you want to exclude SECURE_DELETE.

This bug is still open for a patch, it should be easy to do, it's quite similar to bug 1270882, it just requires a test per the first part of comment 1.

Mentor: mak77

I just realized (after lots of head scratching) that 67.0 betas have been crashing at startup just because of this, without a single helpful message on stderr. What's the use for the strings in https://hg.mozilla.org/releases/mozilla-beta/annotate/tip/storage/mozStorageService.cpp#l200 if they're not displayed to the user ?

sigh

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