After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 659500 - Prevent the unencrypted database from hitting the disk
Prevent the unencrypted database from hitting the disk
Status: RESOLVED FIXED
Product: almanah
Classification: Other
Component: Encryption
unspecified
Other All
: Normal normal
: ---
Assigned To: diary-maint
diary-maint
Depends on:
Blocks:
 
 
Reported: 2011-09-19 18:11 UTC by Philip Withnall
Modified: 2015-06-19 15:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
VFS: Included the GSettings backend in the VFS (10.53 KB, patch)
2015-06-10 12:16 UTC, Álvaro Peña
committed Details | Review
Tests: First VFS tests (12.99 KB, patch)
2015-06-10 12:16 UTC, Álvaro Peña
committed Details | Review
Tests: Removed "gtest" prefix in VFS tests (955 bytes, patch)
2015-06-19 14:58 UTC, Álvaro Peña
committed Details | Review

Description Philip Withnall 2011-09-19 18:11:13 UTC
At the moment, we decrypt the database into a new file (on disk) and open it using SQLite. This is horrendous: apart from all the security issues with the file remaining in the clear if Almanah crashes, and the problems with synchronisation between the two database files (encrypted and unencrypted), the code for it is horrible.

(Note: it was written this way because SQLite could only open database connections to files when Almanah was originally written.)

SQLite seems to have grown a VFS layer since then (or I've only just noticed it), which should allow us to implement this in a much nicer way:
 • http://sqlite.org/c3ref/vfs.htmlhttp://sqlite.org/vfs.htmlhttp://sqlite.org/c3ref/open.html

We could use our own sqlite3_vfs implementation which decrypts the encrypted database into (non-pageable) memory, and ensure that it never reaches the disk. Such a VFS layer could also allow us to pack other (non-SQLite-database) objects into the encrypted file, such as audio and video recordings (see: bug #659494).

For bonus points, the VFS layer could use an encryption system which permits random access, so we don't have to have the entire unencrypted database in memory at once. This would probably mean switching from asymmetric to symmetric encryption, though perhaps using a randomly generated symmetric key which is protected by the user's PGP key as before.
Comment 1 Álvaro Peña 2014-12-31 10:42:54 UTC
I have done a lot of work about the SQLite VFS in the Git master branch.

The work has been splitted in several steps:

1. Use the SQLite "demovfs"[1] as a base.
2. Move all the encryption/decryption stuff into the VFS.
3. Remove the conditional compiling (always requiring gpgme) to reduce the code complexity.
4. Create a callback based data buffer for gpgme[2] allowing the decryption into a non-pageable memory with GCR library[3].

[1] http://www.sqlite.org/src/doc/trunk/src/test_demovfs.c
[2] https://www.gnupg.org/documentation/manuals/gpgme/Callback-Based-Data-Buffers.html#Callback-Based-Data-Buffers
[3] https://developer.gnome.org/gcr/stable/gcr-Non-pageable-Memory.html

At the moment, the code seems to work in the same way that before the VFS. Some testing by third party could be useful.

Remains:

1. Use g_open, g_close (portability).
2. Remove unnecessary functions.
3. Clean up the code (code style).

It would be great to implement unit test cases, but looks difficult to manage the UI interaction during decryption.
Comment 2 Álvaro Peña 2014-12-31 10:47:46 UTC
Some toughs: I think that we need to resolve the security user experience before change to a symmetric encryption or to integrate data like photos or video, because I have found a lot of users that have problems understanding the encryption key stuff.

There is a bug about this (https://bugzilla.gnome.org/show_bug.cgi?id=676829).
Comment 3 Philip Withnall 2015-01-06 00:40:28 UTC
Great work (though I haven’t had a chance to review the code in detail)! You should blog about this once you’re happy to call it finished.

(In reply to comment #1)
> It would be great to implement unit test cases, but looks difficult to manage
> the UI interaction during decryption.

Should be possible by using a hard-coded key for the unit tests. You would need to abstract out get_encryption_key() so that the key is a property of the AlmanahSQLiteVFS object, so you can then instantiate it with a hard-coded key and temporary database file in the unit tests.

(In reply to comment #2)
> Some toughs: I think that we need to resolve the security user experience
> before change to a symmetric encryption or to integrate data like photos or
> video, because I have found a lot of users that have problems understanding the
> encryption key stuff.
> 
> There is a bug about this (https://bugzilla.gnome.org/show_bug.cgi?id=676829).

Yup. Let’s keep that in bug #676829 though.
Comment 4 Álvaro Peña 2015-03-02 19:30:20 UTC
(In reply to Philip Withnall from comment #3)
> Great work (though I haven’t had a chance to review the code in detail)! You
> should blog about this once you’re happy to call it finished.

Thanks Philip! I think that is finished, remaining just some unit tests.

> (In reply to comment #1)
> > It would be great to implement unit test cases, but looks difficult to manage
> > the UI interaction during decryption.
> 
> Should be possible by using a hard-coded key for the unit tests. You would
> need to abstract out get_encryption_key() so that the key is a property of
> the AlmanahSQLiteVFS object, so you can then instantiate it with a
> hard-coded key and temporary database file in the unit tests.

I think that something like that would be possible with a SQLite URI parameter, like "almanah:/path/to/the/diary.db?key:PGP_KEY_ID", then the get_encryption_key function turns unnecessary, just for the test of course.

Then is possible to do a good test, but not the best... I would have a way to mock GSettings, something like that would be great... Do you know a way to do something like that?
Comment 5 Philip Withnall 2015-03-05 21:26:04 UTC
(In reply to Álvaro Peña from comment #4)
> (In reply to Philip Withnall from comment #3)
> > Should be possible by using a hard-coded key for the unit tests. You would
> > need to abstract out get_encryption_key() so that the key is a property of
> > the AlmanahSQLiteVFS object, so you can then instantiate it with a
> > hard-coded key and temporary database file in the unit tests.
> 
> I think that something like that would be possible with a SQLite URI
> parameter, like "almanah:/path/to/the/diary.db?key:PGP_KEY_ID", then the
> get_encryption_key function turns unnecessary, just for the test of course.

That’s basically a non-type-safe way of passing the key around as a struct member.

I suggest something like the following:
 1. Pass the key (as a string) to almanah_vfs_init().
 2. It passes the key to sqlite3_almanah_vfs().
 3. sqlite3_almanah_vfs() stores a copy of the key in the sqlite3_vfs.pAppData field.
 4. almanah_vfs_open() reads the key from pVfs->pAppData and stores that pointer in a new field in the AlmanahSQLiteVFS. All the almanah_vfs_io_*() methods have access to that struct, so can replace calls to get_encryption_key() with a read from it.

The unit tests can then call almanah_vfs_init("some-hardcoded-key"), while the main code can call almanah_vfs_init(get_encryption_key()).

Do you think that will work?
Comment 6 Álvaro Peña 2015-03-07 19:51:28 UTC
(In reply to Philip Withnall from comment #5)
> (In reply to Álvaro Peña from comment #4)
> > (In reply to Philip Withnall from comment #3)
> > > Should be possible by using a hard-coded key for the unit tests. You would
> > > need to abstract out get_encryption_key() so that the key is a property of
> > > the AlmanahSQLiteVFS object, so you can then instantiate it with a
> > > hard-coded key and temporary database file in the unit tests.
> > 
> > I think that something like that would be possible with a SQLite URI
> > parameter, like "almanah:/path/to/the/diary.db?key:PGP_KEY_ID", then the
> > get_encryption_key function turns unnecessary, just for the test of course.
> 
> That’s basically a non-type-safe way of passing the key around as a struct
> member.
> 
> I suggest something like the following:
>  1. Pass the key (as a string) to almanah_vfs_init().
>  2. It passes the key to sqlite3_almanah_vfs().
>  3. sqlite3_almanah_vfs() stores a copy of the key in the
> sqlite3_vfs.pAppData field.
>  4. almanah_vfs_open() reads the key from pVfs->pAppData and stores that
> pointer in a new field in the AlmanahSQLiteVFS. All the almanah_vfs_io_*()
> methods have access to that struct, so can replace calls to
> get_encryption_key() with a read from it.
> 
> The unit tests can then call almanah_vfs_init("some-hardcoded-key"), while
> the main code can call almanah_vfs_init(get_encryption_key()).
> 
> Do you think that will work?

The main reason of "get_encryption_key" is the user possibility to change the encryption key after the execution of almana_vfs_init. This can take place in three scenarios:

1. The user want to encrypt his plain database picking an encryption key.
2. The user change the encryption key at execution time, from one to another.
3. The user returns to a plain database.

So I think that your suggestion is valid just for testing, but not for the main code.

I'm thinking in a conditional compiling for get_encryption_key, with a fixed code just for the test...

BTW I really wish something like that https://cmocka.org/ (just talking about mock objects)
Comment 7 Philip Withnall 2015-03-09 22:01:33 UTC
(In reply to Álvaro Peña from comment #6)
> So I think that your suggestion is valid just for testing, but not for the
> main code.

You’re right, I was being stupid. How about: instead of passing in the encryption key, pass in a GSettings* object, the same way as I described. This means almanah_vfs_io_*() methods can call g_settings_get_string(pVfs->pAppData, "encryption-key").

This essentially moves the GSettings object construction outside the VFS layer.

Then, you can use:
 1. g_settings_new("org.gnome.almanah") for production code.
 2. g_settings_new_with_backend("org.gnome.almanah", mock_gsettings_backend) for testing code.

The mock_gsettings_backend would be an instance of a custom implementation of GSettingsBackend which returns testing keys.

> I'm thinking in a conditional compiling for get_encryption_key, with a fixed
> code just for the test...

Yuck!
Comment 8 Álvaro Peña 2015-03-10 07:55:50 UTC
(In reply to Philip Withnall from comment #7)
> (In reply to Álvaro Peña from comment #6)
> > So I think that your suggestion is valid just for testing, but not for the
> > main code.
> 
> You’re right, I was being stupid. How about: instead of passing in the
> encryption key, pass in a GSettings* object, the same way as I described.
> This means almanah_vfs_io_*() methods can call
> g_settings_get_string(pVfs->pAppData, "encryption-key").

No sir, you have not been stupid, you are really helpful.

> 
> This essentially moves the GSettings object construction outside the VFS
> layer.
> 
> Then, you can use:
>  1. g_settings_new("org.gnome.almanah") for production code.
>  2. g_settings_new_with_backend("org.gnome.almanah", mock_gsettings_backend)
> for testing code.
> 
> The mock_gsettings_backend would be an instance of a custom implementation
> of GSettingsBackend which returns testing keys.

This is a really cool solution! Thanks for the idea!

> > I'm thinking in a conditional compiling for get_encryption_key, with a fixed
> > code just for the test...
> 
> Yuck!

Yes, yuck!
Comment 9 Álvaro Peña 2015-06-10 12:16:04 UTC
Created attachment 304960 [details] [review]
VFS: Included the GSettings backend in the VFS

The GSettings backend has been added to the VFS to allow the
creation of tests. The VFS uses the GSettings backend to retrieve
the encryption key fingerprint (if selected)
Comment 10 Álvaro Peña 2015-06-10 12:16:09 UTC
Created attachment 304961 [details] [review]
Tests: First VFS tests

There are three tests to test the VFS:

 1. "plain": just open and close the DB
 2. "data": open db, insert some data, close db and then ensuring
    the data reopening
 3. "encrypted:" The same as "data", but encrypting the database.
    To do it, we create a tests keys in order to encrypt and sign
    the DB.
    This is something to improve, because the keys creation takes
    a long time, and the GPGme backend request for confirmation
    when we delete the keys from the public ring.
Comment 11 Álvaro Peña 2015-06-10 12:18:57 UTC
(In reply to Álvaro Peña from comment #10)
>     This is something to improve, because the keys creation takes
>     a long time, and the GPGme backend request for confirmation
>     when we delete the keys from the public ring.

The encryption test is something to improve it this two ways, but this bug has been taken a lot of my time, so I prefer to open a new bug with the task of improve the tests, and continue with the new release process.
Comment 12 Álvaro Peña 2015-06-11 06:56:04 UTC
Attachment 304960 [details] pushed as 61a38bc - VFS: Included the GSettings backend in the VFS
Attachment 304961 [details] pushed as 30cbdb9 - Tests: First VFS tests
Comment 13 Philip Withnall 2015-06-11 08:19:05 UTC
Review of attachment 304960 [details] [review]:

A bit of post-hoc review. :-)

::: src/storage-manager.c
@@ +206,3 @@
 			break;
+		case PROP_SETTINGS:
+			g_clear_object (&priv->settings);

No need for this — g_set_object() does it automatically.

::: src/vfs.c
@@ +1252,3 @@
+	sqlite3_vfs *almanah_vfs;
+
+	almanah_vfs = (sqlite3_vfs *) g_new0(sqlite3_vfs, 1);

Won’t this leak?
Comment 14 Philip Withnall 2015-06-11 08:26:10 UTC
Review of attachment 304961 [details] [review]:

A few post-hoc nits. Otherwise, this looks fantastic! :-D

Please blog about this!

::: tests/Makefile.am
@@ +14,3 @@
+	../src/vfs.h \
+	../src/vfs.c \
+	gtests_vfs.c

I would name this just ‘vfs’ (i.e. vfs_SOURCES). There’s no need to namespace it or include the test framework name in the test name.

::: tests/gtests_vfs.c
@@ +29,3 @@
+#include "../src/vfs.h"
+
+#define ALMANAH_TEST_VFS_DATABASE "/tmp/almanah_tests.db"

This should really be in a directory created at runtime using g_dir_make_tmp(), otherwise the tests cannot easily be parallelised, and fragments left over from one test might interfere with the next test run.

@@ +46,3 @@
+
+	settings_backend = g_settings_backend_get_default ();
+	g_object_unref (settings_backend);

What are these two lines for?
Comment 15 Álvaro Peña 2015-06-14 18:38:03 UTC
(In reply to Philip Withnall from comment #13)
> Review of attachment 304960 [details] [review] [review]:
> 
> A bit of post-hoc review. :-)
> 
> ::: src/storage-manager.c
> @@ +206,3 @@
>  			break;
> +		case PROP_SETTINGS:
> +			g_clear_object (&priv->settings);
> 
> No need for this — g_set_object() does it automatically.

Yes (must read the docs from time to time)

> 
> ::: src/vfs.c
> @@ +1252,3 @@
> +	sqlite3_vfs *almanah_vfs;
> +
> +	almanah_vfs = (sqlite3_vfs *) g_new0(sqlite3_vfs, 1);
> 
> Won’t this leak?

Yes, I have forgotten this thing. It is required to append a "finish" function for the VFS. Thanks!
Comment 16 Álvaro Peña 2015-06-15 08:38:37 UTC
(In reply to Philip Withnall from comment #14)
> Review of attachment 304961 [details] [review] [review]:
> 
> A few post-hoc nits. Otherwise, this looks fantastic! :-D
> 
> Please blog about this!
> 
> ::: tests/Makefile.am
> @@ +14,3 @@
> +	../src/vfs.h \
> +	../src/vfs.c \
> +	gtests_vfs.c
> 
> I would name this just ‘vfs’ (i.e. vfs_SOURCES). There’s no need to
> namespace it or include the test framework name in the test name.

Are you suggesting to turn the VFS in something not related to Almanah? As a generic VFS?

> 
> ::: tests/gtests_vfs.c
> @@ +29,3 @@
> +#include "../src/vfs.h"
> +
> +#define ALMANAH_TEST_VFS_DATABASE "/tmp/almanah_tests.db"
> 
> This should really be in a directory created at runtime using
> g_dir_make_tmp(), otherwise the tests cannot easily be parallelised, and
> fragments left over from one test might interfere with the next test run.

Looks good.

> 
> @@ +46,3 @@
> +
> +	settings_backend = g_settings_backend_get_default ();
> +	g_object_unref (settings_backend);
> 
> What are these two lines for?

You can remove it and watch the result in the next lines (g_memory_settings_backend_new):

GLib-GIO-WARNING **: Tried to implement non-registered extension point gsettings-backend

And you get a coredump in g_memory_settings_backend_new  so I guess that the "g_settings_backend_get_default" initializes something required... Looks like a bug or something misdocumented...
Comment 17 Philip Withnall 2015-06-15 12:15:00 UTC
(In reply to Álvaro Peña from comment #16)
> (In reply to Philip Withnall from comment #14)
> > Review of attachment 304961 [details] [review] [review] [review]:
> > 
> > A few post-hoc nits. Otherwise, this looks fantastic! :-D
> > 
> > Please blog about this!
> > 
> > ::: tests/Makefile.am
> > @@ +14,3 @@
> > +	../src/vfs.h \
> > +	../src/vfs.c \
> > +	gtests_vfs.c
> > 
> > I would name this just ‘vfs’ (i.e. vfs_SOURCES). There’s no need to
> > namespace it or include the test framework name in the test name.
> 
> Are you suggesting to turn the VFS in something not related to Almanah? As a
> generic VFS?

Not really — just saying that it seems unnecessary to put ‘gtests’ in the filename. If you add more tests, would you also prefix them with ‘gtests’? Then you would end up with a lot of repetition.

> > @@ +46,3 @@
> > +
> > +	settings_backend = g_settings_backend_get_default ();
> > +	g_object_unref (settings_backend);
> > 
> > What are these two lines for?
> 
> You can remove it and watch the result in the next lines
> (g_memory_settings_backend_new):
> 
> GLib-GIO-WARNING **: Tried to implement non-registered extension point
> gsettings-backend
> 
> And you get a coredump in g_memory_settings_backend_new  so I guess that the
> "g_settings_backend_get_default" initializes something required... Looks
> like a bug or something misdocumented...

Indeed. Unfortunately, this looks like the only way to get this to work. :-(
I can’t see any relevant GLib bug reports — I would fix this by exposing g_io_modules_ensure_loaded() publicly, or calling it before any g_io_extension_point_register() call inside GLib.
Comment 18 Álvaro Peña 2015-06-19 14:58:17 UTC
Created attachment 305703 [details] [review]
Tests: Removed "gtest" prefix in VFS tests
Comment 19 Álvaro Peña 2015-06-19 15:00:34 UTC
(In reply to Philip Withnall from comment #17)
> (In reply to Álvaro Peña from comment #16)
> > (In reply to Philip Withnall from comment #14)
> > > Review of attachment 304961 [details] [review] [review] [review] [review]:
> > > 
> > > A few post-hoc nits. Otherwise, this looks fantastic! :-D
> > > 
> > > Please blog about this!
> > > 
> > > ::: tests/Makefile.am
> > > @@ +14,3 @@
> > > +	../src/vfs.h \
> > > +	../src/vfs.c \
> > > +	gtests_vfs.c
> > > 
> > > I would name this just ‘vfs’ (i.e. vfs_SOURCES). There’s no need to
> > > namespace it or include the test framework name in the test name.
> > 
> > Are you suggesting to turn the VFS in something not related to Almanah? As a
> > generic VFS?
> 
> Not really — just saying that it seems unnecessary to put ‘gtests’ in the
> filename. If you add more tests, would you also prefix them with ‘gtests’?
> Then you would end up with a lot of repetition.

OK, now I understand you. Patch added.

> 
> > > @@ +46,3 @@
> > > +
> > > +	settings_backend = g_settings_backend_get_default ();
> > > +	g_object_unref (settings_backend);
> > > 
> > > What are these two lines for?
> > 
> > You can remove it and watch the result in the next lines
> > (g_memory_settings_backend_new):
> > 
> > GLib-GIO-WARNING **: Tried to implement non-registered extension point
> > gsettings-backend
> > 
> > And you get a coredump in g_memory_settings_backend_new  so I guess that the
> > "g_settings_backend_get_default" initializes something required... Looks
> > like a bug or something misdocumented...
> 
> Indeed. Unfortunately, this looks like the only way to get this to work. :-(
> I can’t see any relevant GLib bug reports — I would fix this by exposing
> g_io_modules_ensure_loaded() publicly, or calling it before any
> g_io_extension_point_register() call inside GLib.

Great!
Comment 20 Álvaro Peña 2015-06-19 15:01:27 UTC
Attachment 305703 [details] pushed as f3d3feb - Tests: Removed "gtest" prefix in VFS tests