445164 - Cookies not securely deleted from cookies.sqlite

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Alec Moskvin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

After clearing private data, the information may still remain in the .sqlite file although it can no longer be accessed by Firefox and SQLite.

Reproducible: Sometimes

Steps to Reproduce:
1. Make sure you haven't cleared your data in a long time (a few weeks)
2. Close Firefox.
3. Back up your data in your profile folder.
4. Start Firefox and go to Tools > Clear Private Data.
5. Check all the boxes and press the "Clear Private Data Now" button.
6. Open a .sqlite file (cookies.sqlite, for example) with a text editor.
Actual Results:  
The sqlite file is still the same size, and except for some changes in the beginning, it is identical. Personal data can can be easily read, since it is in plain text.

Expected Results:  
No traces of the user's data should be left. If a user clears private data, there must be a reason for doing so, so it should not be so easily recoverable.

A solution could be to run VACUUM after the data is cleared.

This has been tested on Linux and Windows XP.

Comment 1

[Jo Hermans]

We are already using SQLITE_SECURE_DELETE (see bug 405925). Maybe you're confusing the history data (that is cleared with Tools > Clear Private Data) with the bookmark data, which is stored in the same database.

Note that VACUUM only shrinks the file, but doesn't really help your privacy, because the old data can still be found on your hard disk (although much more difficult to recover). We're not doing a VACUUM right now, since it's very slow. But there's lots of bugs open about it, mainly to shrink the databases.

Comment 2

[Alec Moskvin]

Jo, the file that I am looking at right now is cookies.sqlite. I have a copy from before clearing and after, and, as I said, they both contain my data. I would attach them here, but I won't for obvious reasons. (And it does not easily reproduce since the db needs to be large enough.)

I also checked the other files and it seems they all get cleared correctly. I might try this on some other systems some time, but if someone else could confirm that it's just cookies or that it sometimes happens to other databases too, that would be great.

I know that VACUUM is pretty slow, but considering that the database is empty, it shouldn't be too bad (unless it's Places). I also know that after a VACUUM, the data still remains on the hard drive, but I'm not really concerned about it since it's not as easy to recover it as opening a file with Notepad.

Comment 3

[Dotan Cohen]

I cannot reproduce this bug on Ubuntu Linux 8.04 with Firefox 3.0.1. Although I tried having visited only 5 websites and the OP mentions that the database must be 'large enough'.

@Alec: How large is large enough? Please create a new profile and try to reproduce the bug by visiting websites that you do not mind posting the cookies for.

Additionally, do you have any recovery/restore software on your computer? Are you certain that the profile your are looking at is in fact the same profile that you ran Clear Private Data on?

Might the data not being erased be leftovers from a Firefox 2 profile? If so then the issue is still serious, but the fix is much easier.

Comment 4

[Alec Moskvin]

Dotan, Five sites certainly won't do. As I said above, "large enough" is when you've used that profile for a few weeks. Unfortunately, I can not avoid visiting secure sites for such a long period of time, and I do not have the time to wander aimlessly all day to fill up the db...

Hence, I'm hoping that someone will try this with their *primary* profile. If you back up that folder, clear the data, exit Firefox, check the results, and replace the data (as I wrote in the reproduction steps) you won't lose any data.

Any external software affecting this is out of the question, since, as I've said, I've tested this on different machines with diffrent operating systems.

The data being from the previous version may be a possibility, since I've used Firefox before version 3; however, it is unlikely since previous versions did not use SQLite.

Again, if someone could try the steps I posted in the report on a profile that has been used for at least a few weeks, and confirm that this really happens, that would be great!

Comment 5

[Dotan Cohen]

That should be you, Alec. Make a new, clean profile with no extensions, themes, or other modifications and do your regular browsing from it. Use your standard profile only for secure logins that you do not want to post. After a few weeks, test and tell us the result.

Note that I tried on my regular profile in addition to the new profile that I tested. In neither could I reproduce this bug.

Your problem may be related to an extension, or a corrupted profile, or a computer misconfiguration, or even malware on your system. Reproduce the bug on a clean profile first, and we will take it from there. I will continue trying to reproduce the bug as well, but don't count on me alone to find the bug. Help us find it.

Comment 6

[Mumia W.]

I'm sorry, but I was also unable to reproduce this bug with Firefox 3.0.1 (Ubuntu i386); however, I tried it on only one site: www.redhat.com . I visited www.redhat.com, they gave me a cookie. I used "strings cookies.sqlite" to view the cookie. I closed and restarted FX and went through the procedure to clear private data, and upon examing cookies.sqlite, I saw there were no cookies. Note, I have FX set to keep cookies until they expire.

Alex, are there any conditions under which you are _unable_ to reproduce this problem? In particular, are you willing to test it with www.redhat.com?

Comment 9

[Daniel Veditz [:dveditz]]

I'm going to confirm that _something_ is happening based on dupes

Comment 10

[(no longer active)]

Shawn, do you know if VACUUM could do something useful here?

Comment 11

[Shawn Wilsher :sdwilsh]

a VACUUM would reduce the size, but we compile with SQLITE_SECURE_DELETE which zeros out data upon deletion.  If someone can come up with a test case that shows this doesn't happen, we either have a bug in our code, or there is a bug with SQLite.

Comment 12

[Scott Wolchok]

I've reproed this on Ubuntu jaunty's "Shiretoko", which it claims is Firefox 3.5.3. Detailed steps to repro:

1) Think of a site you haven't visited before, like redhat.com. grep for redhat in places.sqlite to confirm it's not in there.


2) Visit redhat.com. grep redhat places.sqlite and note that favicon.ico shows up (twice, when I do it):

swolchok@starman:~/.mozilla/firefox-3.5/2vw172yj.default$ strings places.sqlite | grep redhat
http://www.redhat.com/favicon.ico                                                            
http://www.redhat.com/favicon.ico 


3) Tools/Clear Recent History and clear everything, with all the boxes checked. grep for redhat in places.sqlite and note that it still shows up (and shows up *more*, probably because a transaction got flushed to disk):

swolchok@starman:~/.mozilla/firefox-3.5/2vw172yj.default$ strings places.sqlite | grep redhat
http://www.redhat.com/favicon.ico
http://www.redhat.com/favicon.ico
http://redhat.com/redhat.commoc.tahder.
http://www.redhat.com/redhat.com | The World's Open Source Leadermoc.tahder.www.
http://www.redhat.com/!
http://redhat.com/!
http://redhat.com/!
http://www.redhat.com/!
http://www.redhat.com/redhat.com | The World's Open Source Leadermoc.tahder.www.
http://redhat.com/redhat.commoc.tahder.


4) Note that vacumming manually corrects this issue:

swolchok@starman:~/.mozilla/firefox-3.5/2vw172yj.default$ cp places.sqlite places.sqlite.bak
swolchok@starman:~/.mozilla/firefox-3.5/2vw172yj.default$ sqlite3 places.sqlite
SQLite version 3.6.10
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> vacuum;
sqlite> .quit
swolchok@starman:~/.mozilla/firefox-3.5/2vw172yj.default$ strings places.sqlite | grep redhat
swolchok@starman:~/.mozilla/firefox-3.5/2vw172yj.default$

Looks like the solution using SQLITE_SECURE_DELETE way back from Bug 328140 is not working.

Comment 13

[(no longer active)]

Shawn, is this a known sqlite bug?  Maybe we should try vacuuming manually anyway?

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> I've reproed this on Ubuntu jaunty's "Shiretoko", which it claims is Firefox
> 3.5.3. Detailed steps to repro:
Well, Ubuntu ships their own version of Firefox and it uses the system SQLite.  I'm betting they don't compile with SQLITE_SECURE_DELETE, which is all sorts of sadness.

Not sure who to get in touch with them to get them to do that.  I should add a test in our test suite to make sure that the library in question is compiled with SQLITE_SECURE_DELETE, not that any distros run our test suite to my knowledge before shipping with system SQLite.

(In reply to comment #13)
> Shawn, is this a known sqlite bug?  Maybe we should try vacuuming manually
> anyway?
SQLite has test coverage on this, and knows that we depend on it working.

Comment 15

[Scott Wolchok]

(In reply to comment #14)
> (In reply to comment #12)
> > I've reproed this on Ubuntu jaunty's "Shiretoko", which it claims is Firefox
> > 3.5.3. Detailed steps to repro:
> Well, Ubuntu ships their own version of Firefox and it uses the system SQLite. 
> I'm betting they don't compile with SQLITE_SECURE_DELETE, which is all sorts of
> sadness.
> 
> Not sure who to get in touch with them to get them to do that.  I should add a
> test in our test suite to make sure that the library in question is compiled
> with SQLITE_SECURE_DELETE, not that any distros run our test suite to my
> knowledge before shipping with system SQLite.

What about vacuuming only if SQLite is not compiled with SQLITE_SECURE_DELETE? This seems more likely to be secure than relying on distros to negatively impact performance of every use of SQLite in exchange for browser security.

Also, it would be extra super nice if it were possible to avoid unnecessarily hitting disk with possibly sensitive information if Clear Recent History was invoked before the data got flushed to disk, but that level of control might not be exposed by SQLite.

Comment 16

[Reed Loden [:reed]]

(In reply to comment #14)
> Well, Ubuntu ships their own version of Firefox and it uses the system SQLite. 
> I'm betting they don't compile with SQLITE_SECURE_DELETE, which is all sorts of
> sadness.
> 
> Not sure who to get in touch with them to get them to do that.

You can always ping me about Ubuntu-specific issues, and I'll contact downstream to work with them.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> What about vacuuming only if SQLite is not compiled with SQLITE_SECURE_DELETE?
> This seems more likely to be secure than relying on distros to negatively
> impact performance of every use of SQLite in exchange for browser security.
Vacuuming is an expensive operation.  Distros need to get the changes they make to Firefox approved in order to call it Firefox (although you have Shiretoko, so all bets are off).  There are parts of Firefox that depend on SQLITE_SECURE_DELETE, so if they want to use system SQLite, they need to compile it that way.

> Also, it would be extra super nice if it were possible to avoid unnecessarily
> hitting disk with possibly sensitive information if Clear Recent History was
> invoked before the data got flushed to disk, but that level of control might
> not be exposed by SQLite.
Data tends to get flushed to disk pretty immediately.

(In reply to comment #16)
> You can always ping me about Ubuntu-specific issues, and I'll contact
> downstream to work with them.
I saw you cc'd and figured you'd chime in.  Otherwise, you would have been cc'd.  Thanks reed!

Comment 18

[Reed Loden [:reed]]

I filed the Ubuntu-specific issue downstream as https://bugs.launchpad.net/firefox/+bug/457791.

Comment 19

[Reed Loden [:reed]]

glandium, if Debian is using system sqlite, you'll want to add -DSQLITE_SECURE_DELETE=1 to get this same issue fixed on your end.

Comment 20

[Josh Triplett]

Title updated to reflect that this bug solely relates to cookies.sqlite, since comment #2 says the other files get properly cleared.

Comment 21

[Josh Triplett]

I can reproduce this issue quickly and easily on my system.

Steps to reproduce:

1) Either start from a new profile, or do a "Clear Recent History" with
   time range "Everything" and at least the "Cookies" box checked
   followed by running "sqlite3 cookies.sqlite vacuum".

2) Run "strings cookies.sqlite | grep -i google", and observe that no
   results appear.

3) Open Firefox, and visit google.com.  Close Firefox.

4) Run "strings cookies.sqlite | grep -i google", and observe that some
   results appear, as expected.

5) Open Firefox.  Do a "Clear Recent History" with time range
   "Everything" and at least the "Cookies" box checked.  Close
   Firefox.

6) Run "strings cookies.sqlite | grep -i google", and observe that the
   results from step 4 still appear, despite having cleared cookies.

Comment 22

[Shawn Wilsher :sdwilsh]

This looks to be basically fixed upstream (reed is ensuring that).  What version and distribution of linux are you using?

Comment 23

[Josh Triplett]

(In reply to comment #22)
> This looks to be basically fixed upstream (reed is ensuring that).  What
> version and distribution of linux are you using?

I originally discovered the problem while running Debian's package of 3.5.5.

However, I then reproduced the problem using Mozilla's Firefox 3.5.5 package for Linux, which presumably uses its own bundled libsqlite3.so.

Comment 24

[Shawn Wilsher :sdwilsh]

Attachment: test v1.0

Test to ensure SQLITE_SECURE_DELETE=1 works as advertised as promised in comment 14.  This also fails if you don't compile with SQLITE_SECURE_DELETE=1 (tested locally be removing that define in the Makefile for SQLite).

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> However, I then reproduced the problem using Mozilla's Firefox 3.5.5 package
> for Linux, which presumably uses its own bundled libsqlite3.so.
Ehsan - this looks like it may be a private browsing bug assuming my test also passes on Linux, or maybe it's a cookie bug.

Comment 26

[(no longer active)]

(In reply to comment #25)
> (In reply to comment #23)
> > However, I then reproduced the problem using Mozilla's Firefox 3.5.5 package
> > for Linux, which presumably uses its own bundled libsqlite3.so.
> Ehsan - this looks like it may be a private browsing bug assuming my test also
> passes on Linux, or maybe it's a cookie bug.

This is the code responsible for deleting the cookies:

<http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#134>

For the case of deleting everything, it simply calls nsICookieManager::RemoveAll, which boils down to: <http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/src/nsCookieService.cpp#862>.

This code looks pretty sane to me.

The thing I'm not sure about is whether your test actually represents the cookie service's db queries precisely.  Dan can probably comment on that.

Comment 27

[[e-mail]]

(In reply to comment #21)
> I can reproduce this issue quickly and easily on my system.

Based on the fact that you know Firefox is writing to your cookies.sqlite successfully, database corruption is not involved. And nsCookieService::RemoveAll() has always been pretty simple - if it has a database connection, it deletes everything in the table. The only way this can fail is if there is no connection, i.e. if you're in PB mode. But you're not, because you've observed the browser writing to the file.

Therefore, this must be a secure delete issue. (To prove this, run 'sqlite3' on your database file, do a 'SELECT * FROM moz_cookies', and observe zero results.)

I assume we're talking about 1.9.1, 1.9.2, or trunk here.

Comment 28

[Shawn Wilsher :sdwilsh]

It shouldn't be the secure delete issue from one of our own builds though.

Comment 29

[[e-mail]]

Let's see what Josh says about the sqlite3 results, since that'll end the debate one way or the other...

Comment 30

[Josh Triplett]

Yes, I explicitly confirmed that the database's moz_cookies table has no rows in it, despite the data visible in cookies.sqlite.  This does indeed represent the secure deletion issue, since issuing an explicit "sqlite3 cookies.sqlite vacuum" makes the data go away.

I specifically confirmed that the issue occurs with a Mozilla Firefox 3.5.5 build downloaded from Mozilla.

Comment 31

[Shawn Wilsher :sdwilsh]

(In reply to comment #30)
> I specifically confirmed that the issue occurs with a Mozilla Firefox 3.5.5
> build downloaded from Mozilla.
In that build, can you please go to about:buildconfig and paste the contents of the section titled "Configure arguments" in the bug please?  Also, can we get the useragent string please?

Comment 32

[Josh Triplett]

(In reply to comment #31)
> (In reply to comment #30)
> > I specifically confirmed that the issue occurs with a Mozilla Firefox 3.5.5
> > build downloaded from Mozilla.
> In that build, can you please go to about:buildconfig and paste the contents of
> the section titled "Configure arguments" in the bug please?  Also, can we get
> the useragent string please?

Sure.  about:buildconfig says:

Configure arguments
--enable-application=browser --enable-optimize --enable-update-channel=release --enable-update-packaging --disable-debug --disable-tests --enable-official-branding

User-Agent:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5

And, for completeness, since someone mentioned the compiler option -DSQLITE_SECURE_DELETE=1, the compiler versions and flags:

gcc version 4.1.2 20061011 (Red Hat 4.1.1-29) 	-Wall -W -Wno-unused -Wpointer-arith -Wcast-align -W -Wno-long-long -pedantic -gstabs+ -fno-strict-aliasing -pthread -pipe -DNDEBUG -DTRIMMED -Os -freorder-blocks -fno-reorder-functions -finline-limit=50

gcc version 4.1.2 20061011 (Red Hat 4.1.1-29) 	-fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -gstabs+ -fno-strict-aliasing -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -Os -freorder-blocks -fno-reorder-functions -finline-limit=50 

Source says "Built from http://hg.mozilla.org/releases/mozilla-1.9.1/rev/57f71400f4cf"

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #32)
hrm, well, I'm stumped.  If somebody on Linux who sees this issue wants to apply my test to their tree and see how it goes, that'd be awesome.  To make it even more look cookie service, you can add these two lines after the call to openDatabase:
db.executeSimpleSQL("PRAGMA synchronous = OFF");
db.executeSimpleSQL("PRAGMA locking_mode = EXCLUSIVE");

Also, you should change the call to openDatabase to openUnsharedDatabase.  At that point, the database is setup exactly like the cookie database

> And, for completeness, since someone mentioned the compiler option
> -DSQLITE_SECURE_DELETE=1, the compiler versions and flags:
Note that it's a module define, so you won't see it in that list anyway.  It is defined here:
http://mxr.mozilla.org/mozilla1.9.1/source/db/sqlite3/src/Makefile.in#95

Comment 34

[Reed Loden [:reed]]

I can't reproduce this with the directions in comment #21.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091112 Minefield/3.7a1pre

It's possible it only affects 1.9.1 or x86_64 machines using i686 builds or something... I'll try testing 1.9.1 later tonight.

Comment 35

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #21)
> I can reproduce this issue quickly and easily on my system.
> 
> Steps to reproduce:
> 
> 1) Either start from a new profile, or do a "Clear Recent History" with
>    time range "Everything" and at least the "Cookies" box checked
>    followed by running "sqlite3 cookies.sqlite vacuum".
> 
> 2) Run "strings cookies.sqlite | grep -i google", and observe that no
>    results appear.
> 
> 3) Open Firefox, and visit google.com.  Close Firefox.
> 
> 4) Run "strings cookies.sqlite | grep -i google", and observe that some
>    results appear, as expected.
> 
> 5) Open Firefox.  Do a "Clear Recent History" with time range
>    "Everything" and at least the "Cookies" box checked.  Close
>    Firefox.
> 
> 6) Run "strings cookies.sqlite | grep -i google", and observe that the
>    results from step 4 still appear, despite having cleared cookies.

Have you also tested this with some other domain than google.com?
The problem is that if between steps 5 and 6 there were some hidden connections to Google (eg. related with so-called "safebrowsing"), then the google cookie "magically" reappears.

Here are steps to reproduce (I haven't reported it as a separate bug, as it is part of bug 368255, I guess -- and the real fix of this bug is apparently not wanted - see comments and history of that bug):
1) run Firefox on default settings (in particular make sure that you have one of the "safebrowsing" related options checked in UI, ie. either "Block reported web forgeries" or "Block reported attack sites", or both (as is default))
2) open cookie manager, you should see cookie from google.com (if not - visit google.com or other Google-related site)
3) close all tabs, go to about:blank (just to be sure...)
4) remove all cookies or only cookie from google.com
5) close and reopen cookie manager, just to make sure that google cookie is gone
6) having only about:blank loaded wait at most half an hour (don't use browser at all during this time, just keep its window opened (minimized))
7) check cookie manager again - you'll notice google.com cookie again - it was silently set during hidden "safebrowsing" related connections.

Comment 36

[Bartłomiej Brzozowiec (BartZilla)]

Just to be clear what I mean by "open cookie manager": choose Edit -> Preferences... (Linux) or Tools -> Options (Windows), then "Privacy" tab, then choose link "remove individual cookies". (Instructions for trunk (3.7), but I guess 3.5/3.6 are identical.)

Comment 37

[Josh Triplett]

(In reply to comment #35)
> (In reply to comment #21)
> > I can reproduce this issue quickly and easily on my system.
> > 
> > Steps to reproduce:
> > 
> > 1) Either start from a new profile, or do a "Clear Recent History" with
> >    time range "Everything" and at least the "Cookies" box checked
> >    followed by running "sqlite3 cookies.sqlite vacuum".
> > 
> > 2) Run "strings cookies.sqlite | grep -i google", and observe that no
> >    results appear.
> > 
> > 3) Open Firefox, and visit google.com.  Close Firefox.
> > 
> > 4) Run "strings cookies.sqlite | grep -i google", and observe that some
> >    results appear, as expected.
> > 
> > 5) Open Firefox.  Do a "Clear Recent History" with time range
> >    "Everything" and at least the "Cookies" box checked.  Close
> >    Firefox.
> > 
> > 6) Run "strings cookies.sqlite | grep -i google", and observe that the
> >    results from step 4 still appear, despite having cleared cookies.
> 
> Have you also tested this with some other domain than google.com?
> The problem is that if between steps 5 and 6 there were some hidden connections
> to Google (eg. related with so-called "safebrowsing"), then the google cookie
> "magically" reappears.

Yes, I've confirmed it with domains other than google.com.  And, I also confirmed when I saw the data in cookies.sqlite that the moz_cookies table had no rows, thus indicating no cookies present; the data appearing in cookies.sqlite thus represents random garbage not deleted from the file.

Comment 38

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

Attachment: test v1.0 with requested review changes

While it would not surprise me if the sqlite people have committed to always storing plaintext in the clear, it would also not surprise me if they went and did some kind of crazy string compression.  I suggest (and have made the change) to make sure we actually see the string in the file before declaring victory that it is no longer in the file.  r=asuth with this change.  If you qimport please be sure to restore the authorship of the patch to yourself; I'm not entirely sure how to get qdiff to export that information so it doesn't get lost.

Comment 39

[Shawn Wilsher :sdwilsh]

Attachment: C configure test

This is a simple C file we can run in configure too.  This is from drh on the SQLite team.  He also found that there are certain cases where the data is securely deleted, and this will be fixed in 3.6.21.

Comment 40

[(no longer active)]

Attachment: configure patch - first try

I have very little experience with autoconf syntax, so I hope that I did this correctly.  I tried running the configure script, and it ran fine, but then again I'm not using the system library -- I'm not quite sure how to test this patch...

Comment 41

[(no longer active)]

Comment on attachment 414125 [details] [diff] [review]
configure patch - first try

This is not good, running configure with --enable-system-sqlite says:

checking for SQLITE_SECURE_DELETE support in system sqlite... (cached) no

and then passes.  :(

I don't know what I'm doing wrong.  Ted, do you have any idea?

Comment 42

[(no longer active)]

sdwilsh's test landed on trunk: <http://hg.mozilla.org/mozilla-central/rev/fc05a42babe2>

Comment 43

[(no longer active)]

Shawn, is this test also wanted on 1.9.2 (and 1.9.1)?

Comment 44

[Shawn Wilsher :sdwilsh]

(In reply to comment #43)
> Shawn, is this test also wanted on 1.9.2 (and 1.9.1)?
Yes, it can land across the board.

Comment 45

[D. Richard Hipp]

FYI:  The defect in the SQLITE_SECURE_DELETE implementation of SQLite turned
out to be obscure - much more obscure than I originally thought and reported
to Shawn.  The only way that deleted content would fail to be
overwritten with zeros is when all of the content for an entire table fit on a 
single database page and the entire table was deleted using "DELETE FROM table"
with no WHERE clause.  In other words, very small tables, deleted in total all
at once, might not get overwritten.  But as far as we can see now, content is 
correctly zeroed-out in all other cases.  The defect will be fixed in SQLite
3.6.21.

Comment 46

[(no longer active)]

Landed the test on 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cc03e11b9e72

Comment 47

[(no longer active)]

Landed the test on 1.9.1 as well: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/37d0f274bafc

Comment 48

[(not currently active) Ted Mielczarek]

We really don't want to add AC_TRY_RUN checks, since they don't work in cross-compile situations. There's no way to test this without running code?

Comment 49

[Shawn Wilsher :sdwilsh]

(In reply to comment #48)
> We really don't want to add AC_TRY_RUN checks, since they don't work in
> cross-compile situations. There's no way to test this without running code?
Sadly, no.

Comment 50

[(not currently active) Ted Mielczarek]

Comment on attachment 414125 [details] [diff] [review]
configure patch - first try

I think you have at least two problems here, that I can see.
One--configure.in is made up of M4 macros, which use square brackets [] as quoting characters. Thus, you can't use them in your program text as-is, they'll be removed. There is a function called 'changequote' that you can use to work around this. It's a little convoluted, but you can see an example in our configure.in in an mmap test, it's under the heading "See if mmap sees writes". (I'd link it, but I'm writing this on a plane.)

2) You probably need to get SQLITE_CFLAGS into CFLAGS and SQLITE_LIBS into LDFLAGS in order to compile and link the test program. You can find numerous examples in configure.in of tests that do this. They generally save the old values in variables like _SAVE_CFLAGS and _SAVE_LDFLAGS, run the test, then restore the old values from those variables.

I'm not going to review the actual test program, since I have no idea how that works anyway.

Comment 51

[Josh Triplett]

(In reply to comment #50)
> (From update of attachment 414125 [details] [diff] [review])
> I think you have at least two problems here, that I can see.
> One--configure.in is made up of M4 macros, which use square brackets [] as
> quoting characters. Thus, you can't use them in your program text as-is,
> they'll be removed. There is a function called 'changequote' that you can use
> to work around this. It's a little convoluted, but you can see an example in
> our configure.in in an mmap test, it's under the heading "See if mmap sees
> writes". (I'd link it, but I'm writing this on a plane.)

Mixing changequote and auto* represents a really bad idea in almost all cases.  Automake provides a mechanism to write [ and ]: 

http://www.gnu.org/software/automake/manual/autoconf/Quadrigraphs.html#Quadrigraphs

Comment 52

[(not currently active) Ted Mielczarek]

Wow, that looks...insane.

Comment 53

[(no longer active)]

Attachment: configure patch

(In reply to comment #50)
> (From update of attachment 414125 [details] [diff] [review])
> I think you have at least two problems here, that I can see.
> One--configure.in is made up of M4 macros, which use square brackets [] as
> quoting characters. Thus, you can't use them in your program text as-is,
> they'll be removed. There is a function called 'changequote' that you can use
> to work around this. It's a little convoluted, but you can see an example in
> our configure.in in an mmap test, it's under the heading "See if mmap sees
> writes". (I'd link it, but I'm writing this on a plane.)

I solved this problem in the least controversial fassion: getting rid of the square brackets inside the C source (yes, a malloc and pointer arithmetic)!  So, there is no longer any square brackets in the program to worry about.  :-)

> 2) You probably need to get SQLITE_CFLAGS into CFLAGS and SQLITE_LIBS into
> LDFLAGS in order to compile and link the test program. You can find numerous
> examples in configure.in of tests that do this. They generally save the old
> values in variables like _SAVE_CFLAGS and _SAVE_LDFLAGS, run the test, then
> restore the old values from those variables.

Done.  (I assume you meant integrating SQLITE_LIBS into LIBS, not LDFLAGS, is that correct?)

Also, I added an actual check to make sure that the executed program finishes successfully (using AC_MSG_ERROR).

I think the patch is correct now.  Testing this patch with --enable-system-sqlite on my Mac machine now aborts the configure process with an error, which is the expected behavior.

Comment 54

[(not currently active) Ted Mielczarek]

Comment on attachment 416474 [details] [diff] [review]
configure patch

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -6132,16 +6132,85 @@ MOZ_NATIVE_SQLITE=1,
> MOZ_NATIVE_SQLITE= )
> 
> if test -z "$MOZ_NATIVE_SQLITE"
> then
>     SQLITE_CFLAGS=
>     SQLITE_LIBS='$(call EXPAND_LIBNAME_PATH,sqlite3,$(DIST)/lib)'
> else
>     PKG_CHECK_MODULES(SQLITE, sqlite3 >= $SQLITE_VERSION)
>+    dnl ***********************************
>+    dnl *** SQLITE_SECURE_DELETE checks ***
>+    dnl ***********************************

Nit: we seem to use dnl ============= to denote sections in configure.in more than *******.

>+    AC_CACHE_VAL(ac_cv_sqlite_secure_delete,[
>+        AC_TRY_RUN([
>+            #include "sqlite3.h"
>+            #include <stdio.h>
>+            #include <assert.h>
>+
>+            int main(int argc, char **argv){
>+              sqlite3 *db;
>+              sqlite3_uint64 r;
>+              char *zFilename;
>+              FILE *in;
>+              int i;
>+              int rc;
>+              char *zBuf;
>+
>+              zBuf = malloc(1024*3*sizeof(char));
>+              assert( zBuf );
>+              rc = sqlite3_open(":memory:", &db);
>+              assert( rc==SQLITE_OK );
>+              sqlite3_close(db);
>+              sqlite3_randomness(sizeof(r), &r);
>+              zFilename = sqlite3_mprintf("test_db_%llu.sqlite", r);
>+              rc = sqlite3_open(zFilename, &db);
>+              assert( rc==SQLITE_OK );
>+              rc = sqlite3_exec(db, 
>+                "BEGIN;"
>+                "CREATE TABLE t1(x);"
>+                "INSERT INTO t1 VALUES(zeroblob(1000)||'abcdefghijklmnopqrstuvwxyz');"
>+                "COMMIT;"
>+                "DELETE FROM t1;",
>+                0, 0, 0
>+              );
>+              assert( rc==SQLITE_OK );
>+              sqlite3_close(db);
>+              in = fopen(zFilename, "r");
>+              assert( in!=0 );
>+              rc = fread(zBuf, 1, sizeof(zBuf), in);
>+              assert( rc==sizeof(zBuf) );
>+              fclose(in);
>+              unlink(zFilename);
>+              free( zBuf );
>+              for(i=0; i<sizeof(zBuf)-11; i++){
>+                if( *(zBuf+i)=='h' && memcmp(zBuf+i, "hijklmnopq", 10)==0 ){
>+                  return 1;
>+                }
>+              }
>+              return 0;
>+            }],
>+            ac_cv_sqlite_secure_delete=yes,
>+            ac_cv_sqlite_secure_delete=no,
>+            ac_cv_sqlite_secure_delete=no
>+        )
>+    ])
>+    AC_MSG_RESULT($ac_cv_sqlite_secure_delete)
>+    CFLAGS="$_SAVE_CFALGS"

You have a typo here.

r=me with those fixed.

Comment 55

[(no longer active)]

Landed on trunk with the typos addressed:

http://hg.mozilla.org/mozilla-central/rev/06dd18a36470

Shawn, do we also want this patch on 1.9.{1,2}?

Comment 56

[Shawn Wilsher :sdwilsh]

(In reply to comment #55)
> Shawn, do we also want this patch on 1.9.{1,2}?
I think that is a good idea, yeah (at least 1.9.2)

Comment 57

[Micah Gersten]

(In reply to comment #55)
> Landed on trunk with the typos addressed:
> 
> http://hg.mozilla.org/mozilla-central/rev/06dd18a36470
> 
> Shawn, do we also want this patch on 1.9.{1,2}?

This test is now failing with sqlite 3.6.21  This is probably due to a fix in that version:
The SQLITE_SECURE_DELETE  compile-time option fixed to make sure that content is deleted even when the truncate optimization applies. 

I see there's already Bug 530667 to upgrade to 3.6.21

Comment 58

[(no longer active)]

(In reply to comment #57)
> (In reply to comment #55)
> > Landed on trunk with the typos addressed:
> > 
> > http://hg.mozilla.org/mozilla-central/rev/06dd18a36470
> > 
> > Shawn, do we also want this patch on 1.9.{1,2}?
> 
> This test is now failing with sqlite 3.6.21  This is probably due to a fix in
> that version:
> The SQLITE_SECURE_DELETE  compile-time option fixed to make sure that content
> is deleted even when the truncate optimization applies. 

Shawn, have you tested the configure test with sqlite 3.6.21?

Comment 59

[Shawn Wilsher :sdwilsh]

(In reply to comment #58)
> Shawn, have you tested the configure test with sqlite 3.6.21?
No, but we plan on landing with 3.6.22 next anyway.

Comment 60

[(no longer active)]

So, is the problem in comment 57 solved with 3.6.22?

Comment 61

[Shawn Wilsher :sdwilsh]

I believe so

Comment 62

[Serge Gautherie (:sgautherie)]

(In reply to comment #55)
> http://hg.mozilla.org/mozilla-central/rev/06dd18a36470

Ftr, this "should" be copied to comm-central, but m-c check should be enough ;-)

Comment 63

[Brandon Sterne (:bsterne)]

Comment on attachment 416474 [details] [diff] [review]
configure patch

We missed 1.9.2.2.  Moving approval request forward.

Comment 64

[Daniel Veditz [:dveditz]]

Comment on attachment 416474 [details] [diff] [review]
configure patch

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.

Comment 65

[(no longer active)]

Comment on attachment 416474 [details] [diff] [review]
configure patch

This is just a configure script check to make sure that the system sqlite library has been compiled with SQLITE_SECURE_DELETE, and we bail out of configure if the check fails.  This is mostly intended for distros which use system sqlite library, and should not result in any code change in Firefox.

Comment 66

[LegNeato]

Comment on attachment 416474 [details] [diff] [review]
configure patch

a=LegNeato for 1.9.2. Please land this on mozilla-1.9.2 default

Comment 67

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ce3178faa33b