Closed Bug 546162 Opened 10 years ago Closed 10 years ago

SQLITE_SECURE_DELETE Should not be Mandatory

Categories

(Toolkit :: Storage, defect, major)

x86_64
Linux
defect
Not set
major

Tracking

()

VERIFIED WONTFIX

People

(Reporter: Jason, Unassigned)

References

Details

Attachments

(3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/532.9 (KHTML, like Gecko) Chrome/5.0.307.7 Safari/532.9
Build Identifier: 

This commit should be reverted or replaced with a warning instead of an error when SQLITE_SECURE_DELETE is not enabled: http://hg.mozilla.org/mozilla-central/rev/06dd18a3647011415874f3261e53347cecb4f029

For many users with SSDs who are not hyper paranoid about security, this is far from ideal. Furthermore, many distros, such as Gentoo for example, have a policy of using system libraries, which means that by having SQLITE_SECURE_DELETE be required for firefox, all apps that use sqlite experience the performance issues associated with it.

In short, the issue of whether or not SQLITE_SECURE_DELETE should be enabled on SYSTEM libraries should be left up to distros, and not enforced as mandatory by Mozilla. Mozilla ought not to force it. Instead, print a large warning.

Reproducible: Always
downstream is only cc'd for tracking purpose.
Tracked by Gentoo as well: http://bugs.gentoo.org/show_bug.cgi?id=304913
Assignee: Jan.Varga → nobody
Component: SQL → Private Browsing
Product: Core → Firefox
QA Contact: omarshall → private.browsing
Version: unspecified → 3.6 Branch
Absolutely not.  Too much code in Mozilla depends on secure delete being enabled.  We added the configure test because distros were clearly not running our own unit tests that would have failed.  This causes breakage that the users see, and then file bug reports here.
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Component: Private Browsing → Storage
Product: Firefox → Toolkit
QA Contact: private.browsing → storage
Resolution: --- → WONTFIX
Version: 3.6 Branch → Trunk
(In reply to comment #3)
> Too much code in Mozilla depends on secure delete being enabled.

I understand why you want to force distros to ensure that SECURE_DELETE is enabled, but could you please elaborate on how exactly the Mozilla code "depends" on the data being zeroed out?
Largely it's the contract between uses about deleting something means it's no longer easily findable on the hard drive.  This holds for cookies, downloads, history, and many other things.

If SQLITE_SECURE_DELETE is not defined, data that was deleted can easily be grepped for.  We have had bugs filed about this from distros who didn't ship with either system SQLite with secure delete enabled or used our own (see bug 455164, bug 435418, bug 498263, and possibly a few others).

On top of that, extensions depend on certain behaviors like this, and they suddenly don't work as advertised on some linux distros (we had this problem with fts3 as well).
(In reply to comment #5)
> Largely it's the contract between uses about deleting something means it's no
> longer easily findable on the hard drive.  This holds for cookies, downloads,
> history, and many other things.

Seems like false sense of security to me. If I look at http://www.sqlite.org/compile.html I see:

"This option does not cause deleted data is securely removed from the underlying storage media."

> If SQLITE_SECURE_DELETE is not defined, data that was deleted can easily be
> grepped for.  We have had bugs filed about this from distros who didn't ship
> with either system SQLite with secure delete enabled or used our own (see bug
> 455164, bug 435418, bug 498263, and possibly a few others).

If users are really paranoid, they should use some encryption, IMHO :)

> On top of that, extensions depend on certain behaviors like this, and they
> suddenly don't work as advertised on some linux distros (we had this problem
> with fts3 as well).

I can understand fts3, as that's adding a visible feature. But I cannot imagine how something could rely on the fact that the database zeroes out unused data?

IMHO making the check default is fine. It can make the self-compiling users or distro packagers aware of the problem, and make it the default on the distro too. But there could be an overriding switch for people who "understand the risks" and care more about performance, SSD durability or whatever.

Well, ideally sqlite could make this runtime option instead of compile-time, but that's another story.
(In reply to comment #6)
> Seems like false sense of security to me. If I look at
> http://www.sqlite.org/compile.html I see:
Sure, you can use more advanced tools to find out more, but this doesn't prevent tools like grep from working.

> If users are really paranoid, they should use some encryption, IMHO :)
However said they were paranoid?  They were complaining about how trivial it was to find out information firefox was supposed to delete.

> I can understand fts3, as that's adding a visible feature. But I cannot imagine
> how something could rely on the fact that the database zeroes out unused data?
And I bet you'd also find it hard to imagine people wanting to use WEP security for their wireless, and yet many, many people do.

> Well, ideally sqlite could make this runtime option instead of compile-time,
> but that's another story.
Even if it did (SQLite 3.6.23 will), I'm not sure I'd take a patch that adds a "system SQLite only" code path to our code.  And I certainly do not want to make every user have to run that code when 95+% of the time a user is going to be using our provided SQLite compiled the way we expect it.
Looks like this has been taken care of:

http://www.sqlite.org/pragma.html#pragma_secure_delete

Will mozilla get up to date?
(In reply to comment #8)
> Will mozilla get up to date?
No.  Running a pragma on every database connection just to support system SQLite is not something I'm willing to impose on all our users.  Adding an ifdef is not something I'm willing to do because it adds unnecessary code complexity for our contributors and reviewers to worry about about.
Shawn- Can't you compromise at some level? That pragma option was added practically because of this debate. But you reject it? Is there really any performance penalty calling it? And would a single ifdef be that awful? C'mon... this is such a good middle ground for all of us.
I was just looking at the source code and it seems that sqlite3_open is called in only 2 places (as of firefox 3.6):

security/nss/lib/softoken/sdb.c:606
storage/src/mozStorageConnection.cpp:319

I think it should be fairly easy to issue a "PRAGMA secure_delete=1" right after each of these occurrences of sqlite3_open. I don't think that would complicate the code that much nor that it would incur a significant performance penalty. And as you said you can always wrap it with a "#ifndef MOZ_NATIVE_SQLITE".

Don't you think that this would be less ugly than the current solution of forcing people use the included sqlite.
(In reply to comment #11)
> I think it should be fairly easy to issue a "PRAGMA secure_delete=1" right
> after each of these occurrences of sqlite3_open. I don't think that would
> complicate the code that much nor that it would incur a significant performance
> penalty. And as you said you can always wrap it with a "#ifndef
> MOZ_NATIVE_SQLITE".
Did you not even read comment 9?

> Don't you think that this would be less ugly than the current solution of
> forcing people use the included sqlite.
We're not forcing you to use system SQLite.  We're requiring that SQLite is compiled with secure delete.
@sdwilsh

But as Marios pointed out, it's TWO PLACES in the entire source code. This would be such a little amount of code to add for such a wanted (the community has spoken, and adjusted to Firefox, by introducing the pragma statement) feature. In fact, you'd end up removing more code than you add, because the AC test wouldn't be needed.

> We're not forcing you to use system SQLite.  We're requiring that SQLite is
compiled with secure delete.

And we're asking you to support system SQLite that is compiled without secure delete -- while still retaining the secure delete functionality via pragma -- since this is now an extremely easy thing to do. In fact, that pragma statement was added just for us.
(In reply to comment #12)
> Did you not even read comment 9?

Yes, I was trying to respond to your argument about the unnecessary code
complexity. Could you please explain why my idea in comment #11 is wrong (i.e. what changes other than the proposed 2 ones are required?).

Let me also give you another perspective. Since firefox can be configured to use the system sqlite, we must make sure that secure delete is enabled. There are 2 ways this can be done:
* at compile time
* at run-time
The former solution is not reliable because users can change the system sqlite at a later time. The latter is more reliable but incurs a small performance penalty. But if we are willing to pay the cost of this run-time check, we might as well do a "PRAGMA secure_delete=1" instead (which can be wrapped with a "#ifndef MOZ_NATIVE_SQLITE" to avoid the overhead for the 95+% of the users). This is the safest approach and offers the greatest flexibility possible.
(In reply to comment #13)
> But as Marios pointed out, it's TWO PLACES in the entire source code. This
> would be such a little amount of code to add for such a wanted (the community
> has spoken, and adjusted to Firefox, by introducing the pragma statement)
> feature. In fact, you'd end up removing more code than you add, because the AC
> test wouldn't be needed.
I'm sorry if it has come across as me caring about lines of code.  What I care about is code complexity.  When someone makes a change to storage code (or possibly even code that uses storage) I have to then think about how system SQLite will interact with it with all of its differences.  I know what you are going to say next - "but this is only one option!"  While that is true, you aren't the only people requesting options like this.  If we always add an option because its only modifying code in a few places, we'll kill ourselves with complexity (death by a thousand needles).

(In reply to comment #14)
> Yes, I was trying to respond to your argument about the unnecessary code
> complexity. Could you please explain why my idea in comment #11 is wrong (i.e.
> what changes other than the proposed 2 ones are required?).
I think I've addressed this above.

> Let me also give you another perspective. Since firefox can be configured to
> use the system sqlite, we must make sure that secure delete is enabled. There
> are 2 ways this can be done:
Maybe the solution here is to remove support for system SQLite.  When I pushed to get it added I never expected I'd get this much grief.


Can I ask why everyone doesn't want secure delete?  If you care about performance, you shouldn't be using shared libraries anyway which would make this a moot issue.
Just so that the patch is not lost in the intertubes and even if this is marked as WONTFIX, here's what we're using on OpenBSD, along with ac_cv_sqlite_secure_delete=yes in CONFIGURE_ENV to trick configure to believe systemwide sqlite has secure_delete enabled by default.
security/nss not patched as we're using the systemwide one.
Two years and a half later, we're still carrying that patch and building against systemwide sqlite, and no issue at runtime. mak, as the module owner of storage now, would you reconsider sdwilsh's decision ?

My suggestion (of course, patch incoming if accepted) would be to check for SQLITE_SECURE_DELETE at configure time (as it is done now) but if it's not present, instead of erroring out set an AC_DEFINE and in that case issue the "PRAGMA secure_delete = on" at runtime in Connection::initializeInternal(). That way, the pragma would be issued only for people building against a sqlite built without secure_delete on by default, but still available at runtime. Less kittens harmed.
Flags: needinfo?(mak77)
Looking at the surrounding code, the aforementioned patch will probably be updated to use 

ExecuteSimpleSQL(NS_LITERAL_CSTRING("PRAGMA secure_delete = ON;"));

instead of ::sqlite3_exec - not sure how the return code should be handled though, MOZ_ASSERT ?

Maybe Connection::initializeClone() should also be updated to issue the secure_delete PRAGMA on cloned connections.
Attached patch secure_delete.patch (obsolete) — Splinter Review
Attachment #8373047 - Flags: review?(mak77)
(In reply to Landry Breuil (:gaston) from comment #18)
> Looking at the surrounding code, the aforementioned patch will probably be
> updated to use 
> 
> ExecuteSimpleSQL(NS_LITERAL_CSTRING("PRAGMA secure_delete = ON;"));

> instead of ::sqlite3_exec - not sure how the return code should be handled
> though, MOZ_ASSERT ?

Using the issuance of the synchronous PRAMGA as an example, maybe just cast as void and forget about it? Or else I'd imagine MOZ_ASSERT_IF(MOZ_NATIVE_SQLITE, ...) would keep things simple (or instead of MOZ_NATIVE_SQLITE, a new MOZ_* as per below).

> Maybe Connection::initializeClone() should also be updated to issue the
> secure_delete PRAGMA on cloned connections.

Just updating the list of pragmas to copy over to include secure_delete should work, yes?

(In reply to Landry Breuil (:gaston) from comment #17)
> My suggestion (of course, patch incoming if accepted) would be to check for
> SQLITE_SECURE_DELETE at configure time (as it is done now) but if it's not
> present, instead of erroring out set an AC_DEFINE and in that case issue the
> "PRAGMA secure_delete = on" at runtime in Connection::initializeInternal().
> That way, the pragma would be issued only for people building against a
> sqlite built without secure_delete on by default, but still available at
> runtime. Less kittens harmed.

I've made a patch for Firefox 26.0, applies and appears to compile fine on Gentoo Linux (with a custom ebuild whereby enabling the system-sqlite USE flag does not depend on sqlite with compile-time secure-delete). It incorporates information and ideas from previous comments. Modification may be necessary to fit best practices. The only issue left that I see is storage/test/unit/test_sqlite_secure_delete.js. Not sure what to do with that.

Note that the actual patch I used modified configure and configure.in while the attached patch does not change configure. This is because regenerating configure created a ~7000 line diff. So after applying the attached patch one must run autoconf-2.13.
Gah, sorry, used an #ifndef when I meant to use #ifdef. Fixed in this new patch. Still compiles fine.
Attachment #8373047 - Attachment is obsolete: true
Attachment #8373047 - Flags: review?(mak77)
Attachment #8373049 - Flags: review?(mak77)
I think I'd be happy to discuss this in a new bug. At the time this bug was filed there was no way to change the option at runtime, so basically no choice.

Some of the points sdwilsh expressed here are valid and I'm taking them into account, though I also think that:
1. we didn't get many requests to change our sqlite to support native sqlite in the last years. Might be cause the library is pretty much stable and current development is more towards perf and improvements than large changes. Or might be we are also using it in a less prominent way.
2. I don't think taking this option will open us to a bunch of change requests to support system sqlite and if that happens we are still in the position to decide to drop support for it and be done with it.
3. I don't think it's going to add much burden on reviewers, provided it's done properly, as close to db init as possible, so basically nothing changes, reviewers can assume any connection has it set.
4. it will change things only for a minority of users, even if something should broke (unlikely) in the future, it will be broken for a minority of users.
5. As expressed above some distributions are already working around our "protection" with custom patches, that's not better.

So, please file a new bug to use the runtime pragma and move the discussion there.

This stays wontfix cause SECURE_DELETE is mandatory, for Firefox.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mak77)
Attachment #8373049 - Attachment is obsolete: true
Attachment #8373049 - Flags: review?(mak77)
Comment on attachment 526626 [details] [diff] [review]
Set secure_delete PRAGMA by default

>$OpenBSD: patch-storage_src_mozStorageConnection_cpp,v 1.1 2011/01/14 22:49:47 landry Exp $
>set secure_delete PRAGMA on by default, instead of using internal sqlite copy
>--- storage/src/mozStorageConnection.cpp.orig	Tue Jan 11 22:23:40 2011
>+++ storage/src/mozStorageConnection.cpp	Tue Jan 11 22:26:20 2011
>@@ -383,6 +383,13 @@ Connection::initialize(nsIFile *aDatabaseFile)
>     return convertResultCode(srv);
>   }
> 
>+  srv = ::sqlite3_exec(mDBConn, "PRAGMA secure_delete = ON", NULL, NULL, NULL);
>+  if (srv != SQLITE_OK) {
>+    ::sqlite3_close(mDBConn);
>+    mDBConn = nsnull;
>+    return convertResultCode(srv);
>+  }
>+
>   // Set the synchronous PRAGMA, according to the pref
>   nsCOMPtr<nsIPrefBranch> pref(do_GetService(NS_PREFSERVICE_CONTRACTID));
>   PRInt32 synchronous = 1; // Default to NORMAL if pref not set
Attachment #526626 - Attachment is obsolete: true
(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #22)
> I think I'd be happy to discuss this in a new bug. At the time this bug was
> filed there was no way to change the option at runtime, so basically no
> choice.

I'd be glad to open a new bug. The problem is, when a solution acceptable to multiple parties was introduced (support system-sqlite while retaining secure-delete via pragma), it was rejected outright. So my hopes aren't high.

> Some of the points sdwilsh expressed here are valid and I'm taking them into
> account, though I also think that:
> 1. we didn't get many requests to change our sqlite to support native sqlite
> in the last years. Might be cause the library is pretty much stable and
> current development is more towards perf and improvements than large
> changes. Or might be we are also using it in a less prominent way.

Another possible explanation is lots of people interested in system-sqlite (Gentoo folk come to mind) saw this bug and realized that many people had already tried and failed to get support for native sqlite. Even after a bit of clamoring. If you see it's not happening, why clamor more?

> 2. I don't think taking this option will open us to a bunch of change
> requests to support system sqlite and if that happens we are still in the
> position to decide to drop support for it and be done with it.

No comment.

> 3. I don't think it's going to add much burden on reviewers, provided it's
> done properly, as close to db init as possible, so basically nothing
> changes, reviewers can assume any connection has it set.
> 4. it will change things only for a minority of users, even if something
> should broke (unlikely) in the future, it will be broken for a minority of
> users.
> 5. As expressed above some distributions are already working around our
> "protection" with custom patches, that's not better.

Agreed, agreed, and agreed.

> 
> So, please file a new bug to use the runtime pragma and move the discussion
> there.
> 
> This stays wontfix cause SECURE_DELETE is mandatory, for Firefox.

Understandable. Will file a new bug if someone else hasn't already done so. But as I said, my hopes aren't high.
Thanks mak for your detailed response, if i understand correctly we'll technically be able to "relax" the check for SECURE_DELETE at configure time, making the pragma used as a fallback at runtime conditional to the absence of SECURE_DELETE in the system-provided sqlite ? In that case, this is what attachment 8373047 [details] [diff] [review] was achieving..

if not done in some days i'll also make sure a new bug is filed with the patch for review, and contrary to the previous comment i'm pretty confident we'll manage to get something merged :)
(In reply to Michael Rowell from comment #24)
> Another possible explanation is lots of people interested in system-sqlite
> (Gentoo folk come to mind) saw this bug and realized that many people had
> already tried and failed to get support for native sqlite. Even after a bit
> of clamoring. If you see it's not happening, why clamor more?

Well, I don't think so, first of all cause not everyone will land on this bug, second cause sdwilsh was the one pushing for system sqlite in first stance, so there should be no reason to be such negative towards its support. this was just one request.
Surely each request must be evaluated very carefully (that's why I took a little bit of time to think about this request), and if some feature that is not mandatory for us becomes too costly to maintain, we *must* evaluate alternatives, no way out. I think here the cost is acceptable, in other cases might be not.

Thanks for the new bug filed.
For anyone else on the CC List interested in following, the new bug is:

https://bugzilla.mozilla.org/show_bug.cgi?id=1049920
You need to log in before you can comment on or make changes to this bug.