cope with inline PGP encrypted messages

cope with inline PGP encrypted messages

[Daniel Kahn Gillmor]

Inline PGP encrypted messages are clearly worse than PGP/MIME
structured encrypted messages.  There are no standards for how they
are formed, and they don't offer any structured metadata about how to
interpret the bytestream produced by decrypting them.

However, some other MUAs and end-user workflows may make creation of
inline PGP encrypted messages the only available option for message
encryption, and when Notmuch encounters such a message, it should make
a reasonable best-effort to render the cleartext to the user.

Due to ambiguities in interpretation of signatures on inline messages
(e.g. which parts of the message were actually signed?  what character
encoding should the bytestream be interpreted as), we continue to
ignore inline-signed messages entirely, and we do not look at the
validity of any signatures that might be found when decrypting inline
PGP encrypted messages.

We make use here of GMime's optimization function for detecting the
presence of inline PGP encrypted content, which is only found in GMime
3.0 or later.

This series is currently based n top of the "notmuch show
--decrypt=stash" series, which it needs to be able to apply cleanly.
If that series proves controversial, i could rebase this patch
manually against some earlier commit.

If you have applied this series, and you know you have some inline PGP
messages already in your message store, you can try to retroactively
reindex them with something like:

    notmuch reindex --decrypt=true BEGIN-PGP-MESSAGE and not tag:encrypted

I welcome review and feedback about this series.

  --dkg

[PATCH 1/5] crypto: prepare for decryption of inline PGP encrypted messages

[Daniel Kahn Gillmor]

Inline PGP encrypted messages are clearly worse than PGP/MIME
structured encrypted messages.  There are no standards for how they
are formed, and they don't offer any structured metadata about how to
interpret the bytestream produced by decrypting them.

However, some other MUAs and end-user workflows may make creation of
inline PGP encrypted messages the only available option for message
encryption, and when Notmuch encounters such a message, it should make
a reasonable best-effort to render the cleartext to the user.

Due to ambiguities in interpretation of signatures on inline messages
(e.g. which parts of the message were actually signed?  what character
encoding should the bytestream be interpreted as), we continue to
ignore inline-signed messages entirely, and we do not look at the
validity of any signatures that might be found when decrypting inline
PGP encrypted messages.

We make use here of GMime's optimization function for detecting the
presence of inline PGP encrypted content, which is only found in GMime
3.0 or later.

This change prepares the internal codebase for decrypting inline
encrypted messages, but does not yet actually use the capability.
---
 lib/index.cc  |  6 +++---
 mime-node.c   | 24 ++++++++++++++----------
 util/crypto.c | 35 +++++++++++++++++++++++++++--------
 util/crypto.h |  2 +-
 4 files changed, 45 insertions(+), 22 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index 22ca9ec1..f144b9fb 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -367,7 +367,7 @@ _index_content_type (notmuch_message_t *message, GMimeObject *part)
 static void
 _index_encrypted_mime_part (notmuch_message_t *message, notmuch_indexopts_t *indexopts,
 			    GMimeContentType *content_type,
-			    GMimeMultipartEncrypted *part);
+			    GMimeObject *part);
 
 /* Callback to generate terms for each mime part of a message. */
 static void
@@ -421,7 +421,7 @@ _index_mime_part (notmuch_message_t *message,
 		if (i == GMIME_MULTIPART_ENCRYPTED_CONTENT) {
 		    _index_encrypted_mime_part(message, indexopts,
 					       content_type,
-					       GMIME_MULTIPART_ENCRYPTED (part));
+					       part);
 		} else {
 		    if (i != GMIME_MULTIPART_ENCRYPTED_VERSION) {
 			_notmuch_database_log (notmuch_message_get_database (message),
@@ -518,7 +518,7 @@ static void
 _index_encrypted_mime_part (notmuch_message_t *message,
 			    notmuch_indexopts_t *indexopts,
 			    g_mime_3_unused(GMimeContentType *content_type),
-			    GMimeMultipartEncrypted *encrypted_data)
+			    GMimeObject *encrypted_data)
 {
     notmuch_status_t status;
     GError *err = NULL;
diff --git a/mime-node.c b/mime-node.c
index 75b79f98..973133d9 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -196,10 +196,10 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part,
 {
     GError *err = NULL;
     GMimeDecryptResult *decrypt_result = NULL;
-    GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
     notmuch_message_t *message = NULL;
 
-    if (! node->decrypted_child) {
+    if (GMIME_IS_PART (part) || /* must be inline */
+	(GMIME_IS_MULTIPART_ENCRYPTED (part) && ! node->decrypted_child)) {
 	for (mime_node_t *parent = node; parent; parent = parent->parent)
 	    if (parent->envelope_file) {
 		message = parent->envelope_file;
@@ -209,7 +209,7 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part,
 	node->decrypted_child = _notmuch_crypto_decrypt (&node->decrypt_attempted,
 							 node->ctx->crypto->decrypt,
 							 message,
-							 cryptoctx, encrypteddata, &decrypt_result, &err);
+							 cryptoctx, part, &decrypt_result, &err);
     }
     if (! node->decrypted_child) {
 	fprintf (stderr, "Failed to decrypt part: %s\n",
@@ -217,15 +217,19 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part,
 	goto DONE;
     }
 
-    node->decrypt_success = true;
-    node->verify_attempted = true;
 
     if (decrypt_result) {
-	/* This may be NULL if the part is not signed. */
-	node->sig_list = g_mime_decrypt_result_get_signatures (decrypt_result);
-	if (node->sig_list) {
-	    g_object_ref (node->sig_list);
-	    set_signature_list_destructor (node);
+	node->decrypt_success = true;
+	if (GMIME_IS_MULTIPART_ENCRYPTED (part)) {
+	    /* Only check signatures on PGP/MIME messages, not inline
+	       messages. To understand why, see
+	       https://dkg.fifthhorseman.net/notes/inline-pgp-harmful/ */
+	    node->verify_attempted = true;
+	    node->sig_list = g_mime_decrypt_result_get_signatures (decrypt_result);
+	    if (node->sig_list) {
+		g_object_ref (node->sig_list);
+		set_signature_list_destructor (node);
+	    }
 	}
 
 #if HAVE_GMIME_SESSION_KEYS
diff --git a/util/crypto.c b/util/crypto.c
index 9d3b6dad..7965ab8e 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -144,7 +144,7 @@ _notmuch_crypto_decrypt (bool *attempted,
 			 notmuch_decryption_policy_t decrypt,
 			 notmuch_message_t *message,
 			 g_mime_3_unused(GMimeCryptoContext* crypto_ctx),
-			 GMimeMultipartEncrypted *part,
+			 GMimeObject *part,
 			 GMimeDecryptResult **decrypt_result,
 			 GError **err)
 {
@@ -166,15 +166,26 @@ _notmuch_crypto_decrypt (bool *attempted,
 	    if (attempted)
 		*attempted = true;
 #if (GMIME_MAJOR_VERSION < 3)
-	    ret = g_mime_multipart_encrypted_decrypt_session (part,
+	    ret = g_mime_multipart_encrypted_decrypt_session (GMIME_MULTIPART_ENCRYPTED (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);
+	    if (GMIME_IS_MULTIPART_ENCRYPTED (part)) {
+		ret = g_mime_multipart_encrypted_decrypt (GMIME_MULTIPART_ENCRYPTED (part),
+							  GMIME_DECRYPT_NONE,
+							  notmuch_message_properties_value (list),
+							  decrypt_result, err);
+	    } else if (GMIME_IS_PART (part) && g_mime_part_get_openpgp_data (GMIME_PART (part)) == GMIME_OPENPGP_DATA_ENCRYPTED) {
+		*decrypt_result = g_mime_part_openpgp_decrypt (GMIME_PART (part),
+							       GMIME_DECRYPT_NONE,
+							       notmuch_message_properties_value (list),
+							       err);
+		if (decrypt_result) {
+		    ret = part;
+		    g_object_ref (ret);
+		}
+	    }
 #endif
 	    if (ret)
 		break;
@@ -214,8 +225,16 @@ _notmuch_crypto_decrypt (bool *attempted,
     GMimeDecryptFlags flags = GMIME_DECRYPT_NONE;
     if (decrypt == NOTMUCH_DECRYPT_TRUE && decrypt_result)
 	flags |= GMIME_DECRYPT_EXPORT_SESSION_KEY;
-    ret = g_mime_multipart_encrypted_decrypt(part, flags, NULL,
-					     decrypt_result, err);
+    if (GMIME_IS_MULTIPART_ENCRYPTED (part)) {
+	ret = g_mime_multipart_encrypted_decrypt(GMIME_MULTIPART_ENCRYPTED (part), flags, NULL,
+						 decrypt_result, err);
+    } else if (GMIME_IS_PART (part) && g_mime_part_get_openpgp_data (GMIME_PART (part)) == GMIME_OPENPGP_DATA_ENCRYPTED) {
+	*decrypt_result = g_mime_part_openpgp_decrypt (GMIME_PART (part), flags, NULL, err);
+	if (decrypt_result) {
+	    ret = part;
+	    g_object_ref (ret);
+	}
+    }
 #endif
     return ret;
 }
diff --git a/util/crypto.h b/util/crypto.h
index c384601c..d9e4a1c7 100644
--- a/util/crypto.h
+++ b/util/crypto.h
@@ -20,7 +20,7 @@ _notmuch_crypto_decrypt (bool *attempted,
 			 notmuch_decryption_policy_t decrypt,
 			 notmuch_message_t *message,
 			 GMimeCryptoContext* crypto_ctx,
-			 GMimeMultipartEncrypted *part,
+			 GMimeObject *part,
 			 GMimeDecryptResult **decrypt_result,
 			 GError **err);
 
-- 
2.15.1

[PATCH 2/5] cli/{show, reply}: try to decrypt inline PGP encrypted messages

[Daniel Kahn Gillmor]

We try this only for leaf parts that are explicitly marked as
Content-Type: text/*, since we don't want to accidentally match on any
other weird part that happens to contain the magic string, or on the
payload child of a multipart/encrypted part.

Of course, this only works for GMime 3.0 and later, because of how
we're detecting the presence of the OpenPGP inline encrypted blob.
---
 mime-node.c                        |  4 ++
 test/T359-inline-pgp-decryption.sh | 97 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100755 test/T359-inline-pgp-decryption.sh

diff --git a/mime-node.c b/mime-node.c
index 973133d9..3c94bb62 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -325,6 +325,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 	} else {
 	    node_verify (node, part, cryptoctx);
 	}
+#if (GMIME_MAJOR_VERSION >= 3)
+    } else if (GMIME_IS_TEXT_PART (part) && g_mime_part_get_openpgp_data (GMIME_PART (part)) == GMIME_OPENPGP_DATA_ENCRYPTED) {
+	node_decrypt_and_verify (node, part, cryptoctx);
+#endif
     }
 
     return node;
diff --git a/test/T359-inline-pgp-decryption.sh b/test/T359-inline-pgp-decryption.sh
new file mode 100755
index 00000000..c0db8eaf
--- /dev/null
+++ b/test/T359-inline-pgp-decryption.sh
@@ -0,0 +1,97 @@
+#!/usr/bin/env bash
+
+test_description='Decryption of inline PGP messages'
+. $(dirname "$0")/test-lib.sh || exit 1
+
+##################################################
+
+add_gnupg_home
+
+test_begin_subtest "Adding inline PGP encrypted message"
+mkdir -p "$MAIL_DIR/cur"
+cat <<EOF > "$MAIL_DIR/cur/inline-pgp-encrypted.eml"
+Message-Id: inline-pgp-encrypted@testsuite.notmuchmail.org
+Content-Type: text/plain
+Subject: inline PGP encrypted message
+Date: Sat, 01 Jan 2000 12:00:00 +0000
+From: test_suite@notmuchmail.org
+To: test_suite@notmuchmail.org
+
+$(echo "this is the sekrit message" | gpg --no-tty --batch --quiet --trust-model=always --encrypt --armor --recipient test_suite@notmuchmail.org)
+EOF
+test_expect_success 'notmuch new'
+
+test_begin_subtest "inline PGP decryption, --format=json"
+test_subtest_broken_gmime_2
+output=$(notmuch show --format=json --decrypt=true id:inline-pgp-encrypted@testsuite.notmuchmail.org \
+    | notmuch_json_show_sanitize)
+expected='
+ [[[{"body": [{
+ "content": "this is the sekrit message\n",
+ "content-type": "text/plain",
+ "encstatus": [{"status": "good" }],
+ "id": 1
+ }],
+ "date_relative": "2000-01-01",
+ "excluded": false,
+ "filename": ["YYYYY"],
+ "headers": {
+   "Date": "Sat, 01 Jan 2000 12:00:00 +0000",
+   "From": "test_suite@notmuchmail.org",
+   "Subject": "inline PGP encrypted message",
+   "To": "test_suite@notmuchmail.org"
+  },
+ "id": "XXXXX",
+ "match": true,
+ "tags": ["inbox", "unread"],
+ "timestamp": 946728000
+ },
+ []]]]'
+
+test_expect_equal_json \
+    "$output" \
+    "$expected"
+
+test_begin_subtest "inline PGP decryption for reply"
+test_subtest_broken_gmime_2
+output=$(notmuch reply --format=json --decrypt=true id:inline-pgp-encrypted@testsuite.notmuchmail.org \
+    | notmuch_json_show_sanitize)
+expected='
+ {"original": {"body": [{
+ "content": "this is the sekrit message\n",
+ "content-type": "text/plain",
+ "encstatus": [{"status": "good" }],
+ "id": 1
+ }],
+ "date_relative": "2000-01-01",
+ "excluded": false,
+ "filename": ["YYYYY"],
+ "headers": {
+   "Date": "Sat, 01 Jan 2000 12:00:00 +0000",
+   "From": "test_suite@notmuchmail.org",
+   "Subject": "inline PGP encrypted message",
+   "To": "test_suite@notmuchmail.org"
+  },
+ "id": "XXXXX",
+ "match": false,
+ "tags": ["inbox", "unread"],
+ "timestamp": 946728000
+ },
+ "reply-headers": {
+   "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
+   "In-reply-to": "<inline-pgp-encrypted@testsuite.notmuchmail.org>",
+   "References": "<inline-pgp-encrypted@testsuite.notmuchmail.org>",
+   "Subject": "Re: inline PGP encrypted message"
+ }
+}'
+
+test_expect_equal_json \
+    "$output" \
+    "$expected"
+
+test_begin_subtest "searching for cleartext of inline PGP encrypted message should fail"
+output=$(notmuch search 'sekrit')
+expected=''
+test_expect_equal "$output" "$expected"
+
+test_done
-- 
2.15.1

[PATCH 3/5] index: tag text parts with inline PGP encryption as "encrypted"

[Daniel Kahn Gillmor]

Assuming we have GMime 3.0 (which has efficient detection of inline
PGP encrypted blobs) we should be able to mark those messages with the
same tag that we mark PGP/MIME and S/MIME encrypted messages.
---
 lib/index.cc                       | 6 ++++++
 test/T359-inline-pgp-decryption.sh | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index f144b9fb..e03f5230 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -468,6 +468,12 @@ _index_mime_part (notmuch_message_t *message,
 	return;
     }
 
+#if (GMIME_MAJOR_VERSION >= 3)
+    if (GMIME_IS_TEXT_PART (part) && g_mime_part_get_openpgp_data (GMIME_PART (part)) == GMIME_OPENPGP_DATA_ENCRYPTED) {
+	_notmuch_message_add_term (message, "tag", "encrypted");
+    }
+#endif
+
     byte_array = g_byte_array_new ();
 
     stream = g_mime_stream_mem_new_with_byte_array (byte_array);
diff --git a/test/T359-inline-pgp-decryption.sh b/test/T359-inline-pgp-decryption.sh
index c0db8eaf..314ca786 100755
--- a/test/T359-inline-pgp-decryption.sh
+++ b/test/T359-inline-pgp-decryption.sh
@@ -43,7 +43,7 @@ expected='
   },
  "id": "XXXXX",
  "match": true,
- "tags": ["inbox", "unread"],
+ "tags": ["encrypted", "inbox", "unread"],
  "timestamp": 946728000
  },
  []]]]'
@@ -74,7 +74,7 @@ expected='
   },
  "id": "XXXXX",
  "match": false,
- "tags": ["inbox", "unread"],
+ "tags": ["encrypted", "inbox", "unread"],
  "timestamp": 946728000
  },
  "reply-headers": {
-- 
2.15.1

[PATCH 4/5] index: _index_encrypted_mime_part returns success or failure

[Daniel Kahn Gillmor]

This change prepares us to know whether or not
_index_encrypted_mime_part succeeded or not on a given MIME part.

We don't currently make use of the information, but we will in
subsequent changes.
---
 lib/index.cc | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index e03f5230..29ede685 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -364,7 +364,7 @@ _index_content_type (notmuch_message_t *message, GMimeObject *part)
     }
 }
 
-static void
+static bool
 _index_encrypted_mime_part (notmuch_message_t *message, notmuch_indexopts_t *indexopts,
 			    GMimeContentType *content_type,
 			    GMimeObject *part);
@@ -419,6 +419,8 @@ _index_mime_part (notmuch_message_t *message,
 		_index_content_type (message,
 				     g_mime_multipart_get_part (multipart, i));
 		if (i == GMIME_MULTIPART_ENCRYPTED_CONTENT) {
+		    /* deliberately ignore return value here: if it fails to decrypt,
+		       we have nothing else to try */
 		    _index_encrypted_mime_part(message, indexopts,
 					       content_type,
 					       part);
@@ -519,8 +521,9 @@ _index_mime_part (notmuch_message_t *message,
 }
 
 /* descend (if desired) into the cleartext part of an encrypted MIME
- * part while indexing. */
-static void
+ * part while indexing.  Returns true if there was a successful
+ * decryption, false if there was not.*/
+static bool
 _index_encrypted_mime_part (notmuch_message_t *message,
 			    notmuch_indexopts_t *indexopts,
 			    g_mime_3_unused(GMimeContentType *content_type),
@@ -532,7 +535,7 @@ _index_encrypted_mime_part (notmuch_message_t *message,
     GMimeObject *clear = NULL;
 
     if (!indexopts || (notmuch_indexopts_get_decrypt_policy (indexopts) == NOTMUCH_DECRYPT_FALSE))
-	return;
+	return false;
 
     notmuch = notmuch_message_get_database (message);
 
@@ -550,7 +553,7 @@ _index_encrypted_mime_part (notmuch_message_t *message,
 	    if (status)
 		_notmuch_database_log_append (notmuch, "failed to add index.decryption "
 					      "property (%d)\n", status);
-	    return;
+	    return false;
 	}
     }
 #endif
@@ -560,7 +563,7 @@ _index_encrypted_mime_part (notmuch_message_t *message,
     clear = _notmuch_crypto_decrypt (&attempted, notmuch_indexopts_get_decrypt_policy (indexopts),
 				     message, crypto_ctx, encrypted_data, get_sk ? &decrypt_result : NULL, &err);
     if (!attempted)
-	return;
+	return false;
     if (err || !clear) {
 	if (decrypt_result)
 	    g_object_unref (decrypt_result);
@@ -576,7 +579,7 @@ _index_encrypted_mime_part (notmuch_message_t *message,
 	if (status)
 	    _notmuch_database_log_append (notmuch, "failed to add index.decryption "
 					  "property (%d)\n", status);
-	return;
+	return false;
     }
     if (decrypt_result) {
 #if HAVE_GMIME_SESSION_KEYS
@@ -597,7 +600,7 @@ _index_encrypted_mime_part (notmuch_message_t *message,
     if (status)
 	_notmuch_database_log (notmuch, "failed to add index.decryption "
 			       "property (%d)\n", status);
-
+    return true;
 }
 
 notmuch_status_t
-- 
2.15.1

[PATCH 5/5] index: try indexing the cleartext of inline PGP encrypted text parts

[Daniel Kahn Gillmor]

Assuming that we're using GMime 3.0 or later, and the user has asked
for decryption of some sort, we should go ahead and index the
cleartext.
---
 lib/index.cc                       | 7 +++++++
 test/T359-inline-pgp-decryption.sh | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/lib/index.cc b/lib/index.cc
index 29ede685..49f7cfbf 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -473,6 +473,13 @@ _index_mime_part (notmuch_message_t *message,
 #if (GMIME_MAJOR_VERSION >= 3)
     if (GMIME_IS_TEXT_PART (part) && g_mime_part_get_openpgp_data (GMIME_PART (part)) == GMIME_OPENPGP_DATA_ENCRYPTED) {
 	_notmuch_message_add_term (message, "tag", "encrypted");
+	if (_index_encrypted_mime_part(message, indexopts,
+				       content_type,
+				       part))
+	    return;
+	/* if decryption on inline PGP encrypted message fails, we
+	 * should still fall through and try indexing the MIME part
+	 * anyway (this is what we did before inline PGP decryption) */
     }
 #endif
 
diff --git a/test/T359-inline-pgp-decryption.sh b/test/T359-inline-pgp-decryption.sh
index 314ca786..66b85d5b 100755
--- a/test/T359-inline-pgp-decryption.sh
+++ b/test/T359-inline-pgp-decryption.sh
@@ -94,4 +94,11 @@ output=$(notmuch search 'sekrit')
 expected=''
 test_expect_equal "$output" "$expected"
 
+test_begin_subtest "reindexing cleartext of inline PGP encrypted message should succeed"
+test_subtest_broken_gmime_2
+notmuch reindex --decrypt=true id:inline-pgp-encrypted@testsuite.notmuchmail.org
+output=$(notmuch search 'sekrit')
+expected='thread:0000000000000001   2000-01-01 [1/1] test_suite@notmuchmail.org; inline PGP encrypted message (encrypted inbox unread)'
+test_expect_equal "$output" "$expected"
+
 test_done
-- 
2.15.1

Re: [PATCH 1/5] crypto: prepare for decryption of inline PGP encrypted messages

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> We make use here of GMime's optimization function for detecting the
> presence of inline PGP encrypted content, which is only found in GMime
> 3.0 or later.

We nominally support gmime-2.6 still. Maybe we shouldn't anymore, but
that's a different discussion.  Does this series compile and fail
gracefully with gmime 2.6?

d

Re: [PATCH 1/5] crypto: prepare for decryption of inline PGP encrypted messages

[David Bremner]

David Bremner <david@tethera.net> writes:

> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> We make use here of GMime's optimization function for detecting the
>> presence of inline PGP encrypted content, which is only found in GMime
>> 3.0 or later.
>
> We nominally support gmime-2.6 still. Maybe we shouldn't anymore, but
> that's a different discussion.  Does this series compile and fail
> gracefully with gmime 2.6?
>

Looking at the rest of the series, it looks like it at least tries
to. So maybe it's just the commit message of the first commit that is
confusing, since the commit has nothing that looks like it handles gmime
2.6.

Re: [PATCH 1/5] crypto: prepare for decryption of inline PGP encrypted messages

[Daniel Kahn Gillmor]

On Mon 2018-04-30 08:42:24 -0300, David Bremner wrote:
> David Bremner <david@tethera.net> writes:
>
>> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>>
>>> We make use here of GMime's optimization function for detecting the
>>> presence of inline PGP encrypted content, which is only found in GMime
>>> 3.0 or later.
>>
>> We nominally support gmime-2.6 still. Maybe we shouldn't anymore, but
>> that's a different discussion.  Does this series compile and fail
>> gracefully with gmime 2.6?
>
> Looking at the rest of the series, it looks like it at least tries
> to. So maybe it's just the commit message of the first commit that is
> confusing, since the commit has nothing that looks like it handles gmime
> 2.6.

I think the point was that this is making use of features only found in
gmime 3.0.  so if you build against 2.6, the functionality is absent,
but it shouldn't cause breakage.

    --dkg

Re: [PATCH 1/5] crypto: prepare for decryption of inline PGP encrypted messages

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> Inline PGP encrypted messages are clearly worse than PGP/MIME
> structured encrypted messages.  There are no standards for how they
> are formed, and they don't offer any structured metadata about how to
> interpret the bytestream produced by decrypting them.
>
> However, some other MUAs and end-user workflows may make creation of
> inline PGP encrypted messages the only available option for message
> encryption, and when Notmuch encounters such a message, it should make
> a reasonable best-effort to render the cleartext to the user.
>
> Due to ambiguities in interpretation of signatures on inline messages
> (e.g. which parts of the message were actually signed?  what character
> encoding should the bytestream be interpreted as), we continue to
> ignore inline-signed messages entirely, and we do not look at the
> validity of any signatures that might be found when decrypting inline
> PGP encrypted messages.
>
> We make use here of GMime's optimization function for detecting the
> presence of inline PGP encrypted content, which is only found in GMime
> 3.0 or later.

I already objected to "here", since that doesn't happen in this commit.
>
> This change prepares the internal codebase for decrypting inline
> encrypted messages, but does not yet actually use the capability.

The ratio of backstory to "what is going on here" is a little high.
Perhaps moving the last few lines to the top would help.

> ---

> +    if (GMIME_IS_PART (part) || /* must be inline */
For some reason it wasn't obvious that you meant "inline PGP" where you
wrote "inline"

>  #if (GMIME_MAJOR_VERSION < 3)
> -	    ret = g_mime_multipart_encrypted_decrypt_session (part,
> +	    ret = g_mime_multipart_encrypted_decrypt_session (GMIME_MULTIPART_ENCRYPTED (part),
>  							      crypto_ctx,
>  							      notmuch_message_properties_value (list),
>  							      decrypt_result, err);

that lo

>  #else
> -	    ret = g_mime_multipart_encrypted_decrypt (part,
> -						      GMIME_DECRYPT_NONE,
> -						      notmuch_message_properties_value (list),
> -						      decrypt_result, err);
> +	    if (GMIME_IS_MULTIPART_ENCRYPTED (part)) {
> +		ret = g_mime_multipart_encrypted_decrypt (GMIME_MULTIPART_ENCRYPTED (part),
> +							  GMIME_DECRYPT_NONE,
> +							  notmuch_message_properties_value (list),
> +							  decrypt_result, err);
> +	    } else if (GMIME_IS_PART (part) &&
> g_mime_part_get_openpgp_data (GMIME_PART (part)) ==
> GMIME_OPENPGP_DATA_ENCRYPTED) {

Some of these lines are getting pretty long. devel/STYLE suggests 70 or
80 columns

>  		break;
> @@ -214,8 +225,16 @@ _notmuch_crypto_decrypt (bool *attempted,
>      GMimeDecryptFlags flags = GMIME_DECRYPT_NONE;
>      if (decrypt == NOTMUCH_DECRYPT_TRUE && decrypt_result)
>  	flags |= GMIME_DECRYPT_EXPORT_SESSION_KEY;
> -    ret = g_mime_multipart_encrypted_decrypt(part, flags, NULL,
> -					     decrypt_result, err);
> +    if (GMIME_IS_MULTIPART_ENCRYPTED (part)) {
> +	ret = g_mime_multipart_encrypted_decrypt(GMIME_MULTIPART_ENCRYPTED (part), flags, NULL,
> +						 decrypt_result, err);
> +    } else if (GMIME_IS_PART (part) && g_mime_part_get_openpgp_data (GMIME_PART (part)) == GMIME_OPENPGP_DATA_ENCRYPTED) {
> +	*decrypt_result = g_mime_part_openpgp_decrypt (GMIME_PART (part), flags, NULL, err);
> +	if (decrypt_result) {
> +	    ret = part;
> +	    g_object_ref (ret);
> +	}
> +    }
>  #endif

This looks like somewhat duplicated code. Did you try a little static function?

Re: cope with inline PGP encrypted messages

[Daniel Kahn Gillmor]

On Tue 2017-12-12 01:15:48 -0500, Daniel Kahn Gillmor wrote:
> Inline PGP encrypted messages are clearly worse than PGP/MIME
> structured encrypted messages.  There are no standards for how they
> are formed, and they don't offer any structured metadata about how to
> interpret the bytestream produced by decrypting them.
>
> However, some other MUAs and end-user workflows may make creation of
> inline PGP encrypted messages the only available option for message
> encryption, and when Notmuch encounters such a message, it should make
> a reasonable best-effort to render the cleartext to the user.

Jamie Rollins points out that I need to think more about some of the
security implications of this patch series, so i'd prefer to withdraw it
from consideration for notmuch at the moment.  i'd say it's a WIP but
really not ready for general consumption.  Not sure how to best
represent that in nmbug -- but for now i've removed
notmuch::needs-review and added notmuch::wip.  bremner, let me know if
you think i should have done something different.

I do think that we need to come up with *some* way of letting people
read messages with inline PGP encrypted chunks in them safely.
Otherwise, notmuch users will resort to dirty tricks (because they want
to read the mail), and those dirty tricks will possibly be worse than
anything we come up with.

But higher-priority issues are drawing my attention right now, and i
don't want this series to distract from them.

      --dkg

Re: cope with inline PGP encrypted messages

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> Not sure how to best
> represent that in nmbug -- but for now i've removed
> notmuch::needs-review and added notmuch::wip.  bremner, let me know if
> you think i should have done something different.

I also marked the other two patches in the series as WIP; feel free to
remind me they've already been reviewed if/when the whole series is
resubmitted.

d

Re: cope with inline PGP encrypted messages

[Daniel Kahn Gillmor]

On Thu 2018-05-10 09:39:32 -0300, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> Not sure how to best
>> represent that in nmbug -- but for now i've removed
>> notmuch::needs-review and added notmuch::wip.  bremner, let me know if
>> you think i should have done something different.
>
> I also marked the other two patches in the series as WIP; feel free to
> remind me they've already been reviewed if/when the whole series is
> resubmitted.

i think you marked two patches from a different series (the "notmuch
show --decrypt=stash" series) as WIP.  For the record, that series is
not the same as this inline PGP series!

I've gone ahead and pushed a v2 of the "notmuch show --decrypt=stash"
series, and removed the notmuch::wip tag from the v1 patches, so i think
there's nothing to clean up now.  just wanted to make it clear that i am
still pursuing "notmuch show --decrypt=stash" (i think it's ready for
merge actually!) even as i take "inline PGP encryption" back to the shop
for repairs.

    --dkg