Re: [PATCH 03/18] crypto: use stashed session-key properties for decryption, if available

[David Bremner]

Daniel Kahn Gillmor  writes:

> I also
>> don't know how motivated gmime upstream is to fix bugs in 2.6; I could
>> certainly understand if the answer was "not very".
>
> I believe the answer is "not very" -- but if there are serious bugs (i
> don't think we've talked about any of this stuff as bugs in gmime) then
> we should probably try to raise them with him.

I think Jani tried a bit to narrow it down, but didn't succeed. Part of the
problem (which I suspect is endemic to crypto issues) is that we don't
have public test cases.

>
>> There is, by the way, a function notmuch_built_with that can be used to
>> introspect the library as to what optional features it is built
>> with. It's used in notmuch_config to report back to the user about the
>> presence of optional features.
>
> Is there any naming convention for these features?  do you want me to
> add a "session-key" label with a future revision of this branch?  or are
> you asking for something else?

It could be a followup, but yeah, if there is some feature that is
sometimes compiled in, and sometimes not, then it should be listed along
with the others. For whatever reason, the existing convention is
'session_key'

This discussion does make me think there should probably be a test in
configure that sets a corresponding feature macro
HAVE_GMIME_SESSION_KEYS, in a manner similar to HAVE_XAPIAN_COMPACT
(possibly just centralizing the version comparison currently used).

d
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 03/18] crypto: use stashed session-key properties for decryption, if available

[Daniel Kahn Gillmor]

Hi Bremner--

Thanks for the review!

On Tue 2017-11-14 09:02:08 -0400, David Bremner wrote:

> Since you wrote this, I've deprecated GMime 2.6. I'm not sure that
> changes anything here, but seems worth mentioning

well, i'm happy to hear that -- i've got no problem with deprecating
GMime 2.6, and would be fine with maintaining GMime 3.0 in
stretch-backports if that would make you feel more comfortable about the
decision.

Still, I'll be kind of bummed to have to rewrite this series to strip
out the 2.6 support: i originally wrote it only with 3.0 support, and
then went back in and added 2.6 support because at the time, you didn't
want to deprecate 2.6 :( our coding cadence isn't very well synced :/

> Its a nitpick, but I don't really understand/like including = with the
> property name.  That will break, e.g. for anyone attempting to use it
> from the API.

I don't mind changing the documentation to use ``session-key`` instead
of ``session-key=``.  *shrug*

> The way the diff works out, I was pretty confused by seeing several
> "wrong" calls to _notmuch_crypto_decrypt before the actual change. It
> would be nice to telegraph that somehow, perhaps in the commit message.

sure, i can add to the commit message that _notmuch_crypto_decrypt is
growing a new parameter.

> Personally I would be fine with (and probably happier) only supporting
> new features when using gmime-3.0.  Debugging crypto related stuff is
> always hard (see recent discussion about _mime_node_create, where we
> still don't know what's wrong, and are just papering over the problem),
> and it seems worth striving for simplicity as much as possible.  I also
> don't know how motivated gmime upstream is to fix bugs in 2.6; I could
> certainly understand if the answer was "not very".

I believe the answer is "not very" -- but if there are serious bugs (i
don't think we've talked about any of this stuff as bugs in gmime) then
we should probably try to raise them with him.

> There is, by the way, a function notmuch_built_with that can be used to
> introspect the library as to what optional features it is built
> with. It's used in notmuch_config to report back to the user about the
> presence of optional features.

Is there any naming convention for these features?  do you want me to
add a "session-key" label with a future revision of this branch?  or are
you asking for something else?

--dkg
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 03/18] crypto: use stashed session-key properties for decryption, if available

[David Bremner]

Daniel Kahn Gillmor  writes:


> Note that this only works for GMime 2.6.21 and later (the session key
> interface wasn't available before then).  I don't think we're ready
> for this to be a minimum version requirement yet, so instead if you
> build against a prior version of GMime, it simply won't try to use
> stashed session keys.

Since you wrote this, I've deprecated GMime 2.6. I'm not sure that
changes anything here, but seems worth mentioning

> ---
>  doc/man7/notmuch-properties.rst | 31 +++
>  lib/index.cc|  2 +-
>  mime-node.c | 13 ++---
>  util/crypto.c   | 31 ++-
>  util/crypto.h   |  3 ++-
>  5 files changed, 74 insertions(+), 6 deletions(-)
>
> diff --git a/doc/man7/notmuch-properties.rst b/doc/man7/notmuch-properties.rst
> index 68121359..31d7a104 100644
> --- a/doc/man7/notmuch-properties.rst
> +++ b/doc/man7/notmuch-properties.rst
> @@ -74,6 +74,35 @@ of its normal activity.
>  **notmuch-config(1)**), then this property will not be set on that
>  message.
>  
> +**session-key**
> +
> +When **notmuch-show(1)** or **nomtuch-reply** encounters a message
> +with an encrypted part and ``--decrypt`` is set, if notmuch finds a
> +``session-key=`` property associated with the message, it will try
> +that stashed session key for decryption.
> +

Its a nitpick, but I don't really understand/like including = with the
property name.  That will break, e.g. for anyone attempting to use it
from the API.

> -clear = _notmuch_crypto_decrypt (crypto_ctx, encrypted_data, NULL, );
> +clear = _notmuch_crypto_decrypt (message, crypto_ctx, encrypted_data, 
> NULL, );

The way the diff works out, I was pretty confused by seeing several
"wrong" calls to _notmuch_crypto_decrypt before the actual change. It
would be nice to telegraph that somehow, perhaps in the commit message.

>  {
>  GMimeObject *ret = NULL;
>  
> +/* the versions of notmuch that can support session key decryption */
> +#if (GMIME_MAJOR_VERSION >= 3 || (GMIME_MAJOR_VERSION == 2 && 
> GMIME_MINOR_VERSION == 6 && GMIME_MICRO_VERSION >= 21))

Personally I would be fine with (and probably happier) only supporting
new features when using gmime-3.0.  Debugging crypto related stuff is
always hard (see recent discussion about _mime_node_create, where we
still don't know what's wrong, and are just papering over the problem),
and it seems worth striving for simplicity as much as possible.  I also
don't know how motivated gmime upstream is to fix bugs in 2.6; I could
certainly understand if the answer was "not very".

There is, by the way, a function notmuch_built_with that can be used to
introspect the library as to what optional features it is built
with. It's used in notmuch_config to report back to the user about the
presence of optional features.
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 03/18] crypto: use stashed session-key properties for decryption, if available

[Daniel Kahn Gillmor]

On Wed 2017-10-25 02:51:48 -0400, Daniel Kahn Gillmor wrote:
> diff --git a/util/crypto.c b/util/crypto.c
> index 087536ec..e014db5d 100644
> --- a/util/crypto.c
> +++ b/util/crypto.c
> @@ -140,13 +140,42 @@ void _notmuch_crypto_cleanup (unused(_notmuch_crypto_t 
> *crypto))
>  #endif
>  
>  GMimeObject *
> -_notmuch_crypto_decrypt (g_mime_3_unused(GMimeCryptoContext* crypto_ctx),
> +_notmuch_crypto_decrypt (notmuch_message_t *message,
> +  g_mime_3_unused(GMimeCryptoContext* crypto_ctx),
>GMimeMultipartEncrypted *part,
>GMimeDecryptResult **decrypt_result,
>GError **err)
>  {
>  GMimeObject *ret = NULL;
>  
> +/* the versions of notmuch that can support session key decryption */
> +#if (GMIME_MAJOR_VERSION >= 3 || (GMIME_MAJOR_VERSION == 2 && 
> GMIME_MINOR_VERSION == 6 && GMIME_MICRO_VERSION >= 21))
> +if (message) {
> + notmuch_message_properties_t *list = NULL;
> +
> + for (list = notmuch_message_get_properties (message, "session-key", 
> TRUE);
> +  notmuch_message_properties_valid (list); 
> notmuch_message_properties_move_to_next (list)) {
> +#if (GMIME_MAJOR_VERSION < 3)
> + ret = g_mime_multipart_encrypted_decrypt_session (part,
> +   crypto_ctx,
> +   
> notmuch_message_properties_value (list),
> +   decrypt_result, 
> err);
> +#else
> + ret = g_mime_multipart_encrypted_decrypt (part,
> +   GMIME_DECRYPT_NONE,
> +   
> notmuch_message_properties_value (list),
> +   decrypt_result, err);
> +#endif
> + if (ret)
> + break;
> + }
> + if (list)
> + notmuch_message_properties_destroy (list);
> + if (ret)
> + return ret;
> +}
> +#endif
> +
>  #if (GMIME_MAJOR_VERSION < 3)
>  ret = g_mime_multipart_encrypted_decrypt(part, crypto_ctx,
>decrypt_result, err);

In the change above, i realized that we might accidentally clobber the
GError of any intermediate failed decryption attempt, which would
produce a GLib warning to stderr.

In my revised/updated series ("session-keys" on
https://gitlab.com/dkg/notmuch), i clear err (if present) before each
attempted decryption.  This effectively throws away all errors except
for the last one, but i think that's the right thing to do -- we'll try
whatever we can for decrypting, but if the final decryption fails,
that's the error we'd want reported back anyway.

   --dkg the self-reviewer :)


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 03/18] crypto: use stashed session-key properties for decryption, if available

[Daniel Kahn Gillmor]

When doing any decryption, if the notmuch database knows of any
session keys associated with the message in question, try them before
defaulting to using default symmetric crypto.

The primary advantage this offers is a significant speedup when
rendering large encrypted threads ("notmuch show") if session keys
happen to be cached.

Additionally, it permits message composition without access to
asymmetric secret keys ("notmuch reply"); and it permits recovering a
cleartext index when reindexing after a "notmuch restore" for those
messages that already have a session key stored.

This does nothing if no session keys are stashed in the database,
which is fine.  Actually stashing session keys in the database will
come as a subsequent patch.

Note that this only works for GMime 2.6.21 and later (the session key
interface wasn't available before then).  I don't think we're ready
for this to be a minimum version requirement yet, so instead if you
build against a prior version of GMime, it simply won't try to use
stashed session keys.
---
 doc/man7/notmuch-properties.rst | 31 +++
 lib/index.cc|  2 +-
 mime-node.c | 13 ++---
 util/crypto.c   | 31 ++-
 util/crypto.h   |  3 ++-
 5 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/doc/man7/notmuch-properties.rst b/doc/man7/notmuch-properties.rst
index 68121359..31d7a104 100644
--- a/doc/man7/notmuch-properties.rst
+++ b/doc/man7/notmuch-properties.rst
@@ -74,6 +74,35 @@ of its normal activity.
 **notmuch-config(1)**), then this property will not be set on that
 message.
 
+**session-key**
+
+When **notmuch-show(1)** or **nomtuch-reply** encounters a message
+with an encrypted part and ``--decrypt`` is set, if notmuch finds a
+``session-key=`` property associated with the message, it will try
+that stashed session key for decryption.
+
+Using a stashed session key with "notmuch show" will speed up
+rendering of long encrypted threads.  It also allows the user to
+destroy the secret part of any expired encryption-capable subkey
+while still being able to read any retained messages for which
+they have stashed the session key.  This enables truly deletable
+e-mail, since (once the session key and asymmetric subkey are both
+destroyed) there are no keys left that can be used to decrypt any
+copy of the original message previously stored by an adversary.
+
+However, access to the stashed session key for an encrypted message
+permits full byte-for-byte reconstruction of the cleartext
+message.  This includes attachments, cryptographic signatures, and
+other material that cannot be reconstructed from the index alone.
+
+The session key should be in the ASCII text form produced by
+GnuPG.  For OpenPGP, that consists of a decimal representation of
+the hash algorithm used (identified by number from RFC 4880,
+e.g. 9 means AES-256) followed by a colon, followed by a
+hexidecimal representation of the algorithm-specific key.  For
+example, an AES-128 key might be stashed in a notmuch property as:
+``session-key=7:14B16AF65536C28AF209828DFE34C9E0``.
+
 SEE ALSO
 
 
@@ -83,5 +112,7 @@ SEE ALSO
 **notmuch-insert(1)**,
 **notmuch-new(1)**,
 **notmuch-reindex(1)**,
+**notmuch-reply(1)**,
 **notmuch-restore(1)**,
+**notmuch-show(1)**,
 ***notmuch-search-terms(7)**
diff --git a/lib/index.cc b/lib/index.cc
index 19d03456..6eb60f30 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -548,7 +548,7 @@ _index_encrypted_mime_part (notmuch_message_t *message,
}
 }
 #endif
-clear = _notmuch_crypto_decrypt (crypto_ctx, encrypted_data, NULL, );
+clear = _notmuch_crypto_decrypt (message, crypto_ctx, encrypted_data, 
NULL, );
 if (err) {
_notmuch_database_log (notmuch, "Failed to decrypt during indexing. 
(%d:%d) [%s]\n",
   err->domain, err->code, err->message);
diff --git a/mime-node.c b/mime-node.c
index 7f116531..09170bfd 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -198,9 +198,16 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject 
*part,
 GMimeDecryptResult *decrypt_result = NULL;
 GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
 
-node->decrypt_attempted = true;
-if (! node->decrypted_child)
-   node->decrypted_child = _notmuch_crypto_decrypt (cryptoctx, 
encrypteddata, _result, );
+if (! node->decrypted_child) {
+   mime_node_t *parent;
+   for (parent = node; parent; parent = parent->parent)
+   if (parent->envelope_file)
+   break;
+
+   node->decrypt_attempted = true;
+   node->decrypted_child = _notmuch_crypto_decrypt (parent ? 
parent->envelope_file : NULL,
+cryptoctx, 
encrypteddata, _result, );
+}
 if (!