support PRAGMA secure_delete

NEW
Unassigned

Status

()

Toolkit
Storage
P3
normal
4 years ago
8 days ago

People

(Reporter: Michael Rowell, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Created attachment 8468793 [details] [diff] [review]
Use secure_delete runtime pragma for Firefox 26.0

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

Updated

4 years ago
See Also: → bug 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.

Comment 2

3 years ago
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

Comment 4

2 years ago
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.

Comment 6

2 years ago
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...
See Also: → bug 1270882
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
You need to log in before you can comment on or make changes to this bug.