Safe and useful handling of "Mixed Up" mangled messages

Safe and useful handling of "Mixed Up" mangled messages

[Daniel Kahn Gillmor]

I've documented an unfortunate MTA habit over in
https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling-00#section-4.1
which i've named "Mixed Up" mangling.  In particular, popular versions
of Microsoft Exchange take a multipart/encrypted e-mail and transform
it unaccountably to multipart/mixed.

While the right thing to do long term is to get a fix for those MTAs
(which i'm also working on, elsewhere), even if we succeed in doing
that, messages that have already been mangled by them will live on in
our mailboxes forever.

What follows is a series of patches that lets a notmuch user deal with
any message mangled in this particular form in a sensible way.  In
particular, they should be able to decrypt the message correctly
despite its nonstandard structure.  This represents a deviation from
standard MIME handling procedures, but i believe it is a safe
deviation, and a useful one.

The test case included in this series should be sufficient to show the
problem specifically, but if anyone wants to receive encrypted e-mail
from me that demonstrates the problem to their personal key, i can
provide.  Please let me know by replying off-list and making sure i
know your OpenPGP key fingerprint.

If anyone has examples of other common, detectable, and repairable
message manglings, please let me know.  I'd be happy to add them to my
documentation of this sort of thing at least, and if we can fix them
up for notmuch users, even better.

All the best,

    --dkg

[PATCH 1/4] test: add test for "Mixed-Up Mime" message mangling

[Daniel Kahn Gillmor]

Some MTAs mangle e-mail messages in transit in ways that are
repairable.

Microsoft Exchange (in particular, the version running today on
Office365's mailservers) appears to mangle multipart/encrypted
messages in a way that makes them undecryptable by the recipient.

I've documented this in section 4.1 "Mixed-up encryption" of draft -00
of
https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling

Fortunately, it's possible to repair such a message, and notmuch can
do that so that a user who receives an encrypted message from a user
of office365.com can still decrypt the message.

Enigmail already knows about this particular kind of mangling.  It
describes it as "broken PGP email format probably caused by an old
Exchange server", and it tries to repair by directly changing the
message held by the user.  if this kind of repair goes wrong, the
repair process can cause data loss
(https://sourceforge.net/p/enigmail/bugs/987/, yikes).

The tests introduced here are currently broken.  In subsequent
patches, i'll introduce a non-destructive form of repair for notmuch
so that notmuch users can read mail that has been mangled in this way,
and the tests will succeed.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 test/T351-pgpmime-mangling.sh      | 28 +++++++++++++++++++++++++
 test/corpora/mangling/mixed-up.eml | 33 ++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)
 create mode 100755 test/T351-pgpmime-mangling.sh
 create mode 100644 test/corpora/mangling/mixed-up.eml

diff --git a/test/T351-pgpmime-mangling.sh b/test/T351-pgpmime-mangling.sh
new file mode 100755
index 00000000..17f94a31
--- /dev/null
+++ b/test/T351-pgpmime-mangling.sh
@@ -0,0 +1,28 @@
+#!/usr/bin/env bash
+
+test_description='PGP/MIME message mangling'
+. $(dirname "$0")/test-lib.sh || exit 1
+
+add_gnupg_home
+add_email_corpus mangling
+
+test_begin_subtest "handle 'Mixed-Up' mangled PGP/MIME message"
+test_subtest_known_broken
+output=$(notmuch show --format=json --decrypt=true id:mixed-up@mangling.notmuchmail.org)
+test_json_nodes <<<"$output" \
+                'body:[0][0][0]["body"][0]["content"][1]["content"]="The password is \"abcd1234!\", please do not tell anyone.\n"'
+
+test_begin_subtest "repaired 'Mixed-up' messages can be found with index.repaired=mixedup"
+test_subtest_known_broken
+output=$(notmuch search --output=messages property:index.repaired=mixedup)
+test_expect_equal "$output" id:mixed-up@mangling.notmuchmail.org
+
+test_begin_subtest "reindex 'Mixed-Up' mangled PGP/MIME message"
+test_expect_success 'notmuch reindex --decrypt=true id:mixed-up@mangling.notmuchmail.org'
+
+test_begin_subtest "search cleartext of 'Mixed-Up' mangled PGP/MIME message"
+test_subtest_known_broken
+output=$(notmuch search --output=messages body:password)
+test_expect_equal "$output" id:mixed-up@mangling.notmuchmail.org
+
+test_done
diff --git a/test/corpora/mangling/mixed-up.eml b/test/corpora/mangling/mixed-up.eml
new file mode 100644
index 00000000..a09f6191
--- /dev/null
+++ b/test/corpora/mangling/mixed-up.eml
@@ -0,0 +1,33 @@
+From: test_suite@notmuchmail.org
+To: test_suite@notmuchmail.org
+Subject: Here is the password
+Date: Sat, 01 Jan 2000 12:00:00 +0000
+Message-ID: <mixed-up@mangling.notmuchmail.org>
+MIME-Version: 1.0
+Content-Type: multipart/mixed; boundary="=-=-="
+
+--=-=-=
+Content-Type: text/plain; charset="us-ascii"
+Content-Transfer-Encoding: quoted-printable
+
+
+--=-=-=
+Content-Type: application/pgp-encrypted
+Content-Transfer-Encoding: base64
+
+VmVyc2lvbjogMQ0K
+
+--=-=-=
+Content-Type: application/octet-stream
+Content-Transfer-Encoding: base64
+
+LS0tLS1CRUdJTiBQR1AgTUVTU0FHRS0tLS0tDQoNCmhJd0R4RTAyM3ExVXF4WUJCQUNwNzBlN0tQ
+eTlPWWFoZUlya0x6bWhxMWxScW15NTFhTDFqQkwwSy9xTjdyZksNCkJaRUcxY1I4amVMalRGZFBL
+UExWS0pJODByN0ZnS0kweXd2V3ZsNlIxYUUxVHk1Qm5WWFQ5WHpDckVIN2ZxQ2wNClNLSzgyRXZv
+bFhUb2hBWkhVcmg2SzY2ZVFRVFRJQUMxbjdCMEE4aEVyemtnYU00K3NlTjNMbHZlelQ2VExOS00N
+CkFUcHFzRWJNMk1WckdndzBiM29Vc0dHQVBFdDJNbWpORVlzcmlLbnF3dDZkSkRaYy8vWHloamdN
+UWF5aUQ4ZGENCk4xZ1Qzb3FndS9nS0NwQlpEWXpIZjlPdFZpMlVubEZEV3k2cnJNWkxqV0RuSXY0
+dmU5UG4vcW9sd0hWanpkSjENClpmak5DNXQwejNYQURLR3JqTjl3dXRyNHFtN1NUVzFySEFYSFA2
+OFRRVHhJMHFnSktqUFhOS1dFdzZnPQ0KPXBKRzQNCi0tLS0tRU5EIFBHUCBNRVNTQUdFLS0tLS0N
+Cg==
+--=-=-=--
-- 
2.20.1

[PATCH 2/4] util/crypto: identify and repair "Mixed Up" mangled messages

[Daniel Kahn Gillmor]

This patch implements a functional identification and repair process
for "Mixed Up" MIME messages as described in
https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling-00#section-4.1

The detection test is not entirely complete, in that it does not
verify the contents of the latter two message subparts, but this is
probably safe to skip, because those two parts are unlikely to be
readable anyway, and the only part we are effectively omitting (the
first subpart) is guaranteed to be empty anyway, so its removal can be
reversed if you want to do so.  I've left FIXMEs in the code so that
anyone excited about adding these additional checks can see where to
put them in.

I'll use this functionality in the next two patches.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 util/crypto.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++
 util/crypto.h |  9 ++++++
 2 files changed, 89 insertions(+)

diff --git a/util/crypto.c b/util/crypto.c
index 9e185e03..c8c79847 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -28,6 +28,86 @@ void _notmuch_crypto_cleanup (unused(_notmuch_crypto_t *crypto))
 {
 }
 
+/* see
+ * https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling-00#section-4.1.1 */
+static bool
+_notmuch_is_mixed_up_mangled (GMimeObject *part)
+{
+    GMimeMultipart *mpart = NULL;
+    GMimeObject *first, *second, *third = NULL;
+    char *prelude_string = NULL;
+    bool prelude_is_empty;
+
+    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (part),
+				       "multipart", "mixed"))
+	return false;
+    if (! GMIME_IS_MULTIPART (part))
+	return false;
+    mpart = GMIME_MULTIPART (part);
+    if (mpart == NULL)
+	return false;
+    if (g_mime_multipart_get_count (mpart) != 3)
+	return false;
+    first = g_mime_multipart_get_part (mpart, 0);
+    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (first),
+				       "text", "plain"))
+	return false;
+    if (! GMIME_IS_TEXT_PART (first))
+	return false;
+    second = g_mime_multipart_get_part (mpart, 1);
+    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (second),
+				       "application", "pgp-encrypted"))
+	return false;
+    third = g_mime_multipart_get_part (mpart, 2);
+    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (third),
+				       "application", "octet-stream"))
+	return false;
+
+    /* Is first subpart length 0? */
+    prelude_string = g_mime_text_part_get_text (GMIME_TEXT_PART (first));
+    prelude_is_empty = ! (strcmp ("", prelude_string));
+    g_free (prelude_string);
+    if (! prelude_is_empty)
+	return false;
+
+    /* FIXME: after decoding and stripping whitespace, is second
+     * subpart just "Version: 1" ? */
+
+    /* FIXME: can we determine that third subpart is *only* PGP
+     * encrypted data?  I tried g_mime_part_get_openpgp_data () but
+     * found https://github.com/jstedfast/gmime/issues/60 */
+
+    return true;
+}
+
+
+/* see
+ * https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling-00#section-4.1.2 */
+GMimeObject*
+_notmuch_repair_mixed_up_mangled (GMimeObject *part)
+{
+    GMimeMultipart *mpart = NULL, *mpart_ret = NULL;
+    GMimeObject *ret = NULL;
+
+    if (! _notmuch_is_mixed_up_mangled (part))
+	return NULL;
+    mpart = GMIME_MULTIPART (part);
+    ret = GMIME_OBJECT (g_mime_multipart_encrypted_new ());
+    if (ret == NULL)
+	return NULL;
+    mpart_ret = GMIME_MULTIPART (ret);
+    if (mpart_ret == NULL) {
+	g_object_unref (ret);
+	return NULL;
+    }
+    g_mime_object_set_content_type_parameter (ret, "protocol", "application/pgp-encrypted");
+
+    g_mime_multipart_insert (mpart_ret, 0, g_mime_multipart_get_part (mpart, 1));
+    g_mime_multipart_insert (mpart_ret, 1, g_mime_multipart_get_part (mpart, 2));
+    return ret;
+}
+
+
 GMimeObject *
 _notmuch_crypto_decrypt (bool *attempted,
 			 notmuch_decryption_policy_t decrypt,
diff --git a/util/crypto.h b/util/crypto.h
index fdbb5da5..09c0ebcb 100644
--- a/util/crypto.h
+++ b/util/crypto.h
@@ -22,6 +22,15 @@ _notmuch_crypto_decrypt (bool *attempted,
 			 GMimeDecryptResult **decrypt_result,
 			 GError **err);
 
+/* Detecting and repairing "Mixed-Up MIME mangling". see
+ * https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling-00#section-4.1
+ * If this returns NULL, the message was probably not "Mixed up".  If
+ * it returns non-NULL, then there is a newly-allocated MIME part that
+ * represents the repaired version.  The caller is responsible for
+ * ensuring that any returned object is freed with g_object_unref. */
+GMimeObject*
+_notmuch_repair_mixed_up_mangled (GMimeObject *part);
+
 void
 _notmuch_crypto_cleanup (_notmuch_crypto_t *crypto);
 
-- 
2.20.1

Re: [PATCH 2/4] util/crypto: identify and repair "Mixed Up" mangled messages

[Rollins, Jameson]

On Tue, May 28 2019, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> This patch implements a functional identification and repair process
> for "Mixed Up" MIME messages as described in
> https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling-00#section-4.1
>
> The detection test is not entirely complete, in that it does not
> verify the contents of the latter two message subparts, but this is
> probably safe to skip, because those two parts are unlikely to be
> readable anyway, and the only part we are effectively omitting (the
> first subpart) is guaranteed to be empty anyway, so its removal can be
> reversed if you want to do so.  I've left FIXMEs in the code so that
> anyone excited about adding these additional checks can see where to
> put them in.
>
> I'll use this functionality in the next two patches.
>
> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> ---
>  util/crypto.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  util/crypto.h |  9 ++++++
>  2 files changed, 89 insertions(+)

I understand that this fix is for multipart/encrypted messages, but I'm
not sure I would call the repair function itself a "crypto function".
Given that I can imagine more repair functions in the future, would it
make sense to break them out into their own library?

jamie.

Re: [PATCH 2/4] util/crypto: identify and repair "Mixed Up" mangled messages

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

On Thu 2019-05-30 02:18:57 +0000, Rollins, Jameson wrote:
> I understand that this fix is for multipart/encrypted messages, but I'm
> not sure I would call the repair function itself a "crypto function".
> Given that I can imagine more repair functions in the future, would it
> make sense to break them out into their own library?

I agree that this technically isn't a "crypto function", and as such
might not belong where i've put it.  What would you think about
util/repair.{c,h} or util/demangling.{c,h} ?

      --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 2/4] util/crypto: identify and repair "Mixed Up" mangled messages

[Rollins, Jameson]

On Thu, May 30 2019, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> I agree that this technically isn't a "crypto function", and as such
> might not belong where i've put it.  What would you think about
> util/repair.{c,h} or util/demangling.{c,h} ?

I guess "repair" is maybe clearer?  No strong opinion.

jamie.

[PATCH 3/4] index: repair "Mixed Up" messages before indexing.

[Daniel Kahn Gillmor]

When encountering a message that has been mangled in the "mixed up"
way by an intermediate MTA, notmuch should instead repair it and index
the repaired form.

When it does this, it also associates the index.repaired=mixedup
property with the message.  If a problem is found with this repair
process, or an improved repair process is proposed later, this should
make it easy for people to reindex the relevant message.  The property
will also hopefully make it easier to diagnose this particular problem
in the future.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 doc/man7/notmuch-properties.rst | 13 +++++++++++++
 lib/index.cc                    | 22 +++++++++++++++++-----
 test/T351-pgpmime-mangling.sh   |  2 --
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/doc/man7/notmuch-properties.rst b/doc/man7/notmuch-properties.rst
index 802e6763..31de576e 100644
--- a/doc/man7/notmuch-properties.rst
+++ b/doc/man7/notmuch-properties.rst
@@ -109,6 +109,19 @@ of its normal activity.
     example, an AES-128 key might be stashed in a notmuch property as:
     ``session-key=7:14B16AF65536C28AF209828DFE34C9E0``.
 
+**index.repaired**
+
+    Some mail transport agents mangle messages in transit in ways that
+    are both detectable and reversible.  If notmuch encounters such a
+    mangling during indexing, it will try to index the repaired form
+    of the message (while still leaving the message on disk
+    untouched).  If successful, it will use the ``index.repaired``
+    property to note the kind of mangling that was repaired.
+    Currently, only one form of repairable mangling is detected and
+    repaired, which is denoted with ``index.repaired=mixedup``.  See
+    https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling
+    for more information.
+
 SEE ALSO
 ========
 
diff --git a/lib/index.cc b/lib/index.cc
index 1fd9e67e..44a42deb 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -385,11 +385,20 @@ _index_mime_part (notmuch_message_t *message,
     GMimeContentType *content_type;
     char *body;
     const char *charset;
+    GMimeObject *repaired_part = NULL;
 
     if (! part) {
 	_notmuch_database_log (notmuch_message_get_database (message),
 			      "Warning: Not indexing empty mime part.\n");
-	return;
+	goto DONE;
+    }
+
+    repaired_part = _notmuch_repair_mixed_up_mangled (part);
+    if (repaired_part) {
+	/* This was likely "Mixed Up" in transit!  We will instead use
+	 * the more likely-to-be-correct variant. */
+	notmuch_message_add_property (message, "index.repaired", "mixedup");
+	part = repaired_part;
     }
 
     _index_content_type (message, part);
@@ -441,7 +450,7 @@ _index_mime_part (notmuch_message_t *message,
 				       notmuch_status_to_string (status));
 	    _index_mime_part (message, indexopts, child, msg_crypto);
 	}
-	return;
+	goto DONE;
     }
 
     if (GMIME_IS_MESSAGE_PART (part)) {
@@ -451,14 +460,14 @@ _index_mime_part (notmuch_message_t *message,
 
 	_index_mime_part (message, indexopts, g_mime_message_get_mime_part (mime_message), msg_crypto);
 
-	return;
+	goto DONE;
     }
 
     if (! (GMIME_IS_PART (part))) {
 	_notmuch_database_log (notmuch_message_get_database (message),
 			      "Warning: Not indexing unknown mime part: %s.\n",
 			      g_type_name (G_OBJECT_TYPE (part)));
-	return;
+	goto DONE;
     }
 
     disposition = g_mime_object_get_content_disposition (part);
@@ -473,7 +482,7 @@ _index_mime_part (notmuch_message_t *message,
 
 	/* XXX: Would be nice to call out to something here to parse
 	 * the attachment into text and then index that. */
-	return;
+	goto DONE;
     }
 
     byte_array = g_byte_array_new ();
@@ -519,6 +528,9 @@ _index_mime_part (notmuch_message_t *message,
 
 	free (body);
     }
+ DONE:
+    if (repaired_part)
+	g_object_unref (repaired_part);
 }
 
 /* descend (if desired) into the cleartext part of an encrypted MIME
diff --git a/test/T351-pgpmime-mangling.sh b/test/T351-pgpmime-mangling.sh
index 17f94a31..a20066e4 100755
--- a/test/T351-pgpmime-mangling.sh
+++ b/test/T351-pgpmime-mangling.sh
@@ -13,7 +13,6 @@ test_json_nodes <<<"$output" \
                 'body:[0][0][0]["body"][0]["content"][1]["content"]="The password is \"abcd1234!\", please do not tell anyone.\n"'
 
 test_begin_subtest "repaired 'Mixed-up' messages can be found with index.repaired=mixedup"
-test_subtest_known_broken
 output=$(notmuch search --output=messages property:index.repaired=mixedup)
 test_expect_equal "$output" id:mixed-up@mangling.notmuchmail.org
 
@@ -21,7 +20,6 @@ test_begin_subtest "reindex 'Mixed-Up' mangled PGP/MIME message"
 test_expect_success 'notmuch reindex --decrypt=true id:mixed-up@mangling.notmuchmail.org'
 
 test_begin_subtest "search cleartext of 'Mixed-Up' mangled PGP/MIME message"
-test_subtest_known_broken
 output=$(notmuch search --output=messages body:password)
 test_expect_equal "$output" id:mixed-up@mangling.notmuchmail.org
 
-- 
2.20.1

[PATCH 4/4] cli/show: show repaired form of "Mixed Up" mangled messages

[Daniel Kahn Gillmor]

When showing a message that has been mangled in transit by an MTA in
the "Mixed up" way, "notmuch show" should instead show the repaired
form of the message.

Tracking the repaired GMimeObject for the lifetime of the mime_node so
that it is cleaned up properly is probably the trickiest part of this
patch, but the choices here are based on the idea that the
mime_node_context is the memory manager for the whole mime_node tree
in the first place, so new GMimeObject tree created on-the-fly during
message parsing should be disposed of in the same place.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 mime-node.c                   | 22 ++++++++++++++++++++++
 test/T351-pgpmime-mangling.sh |  1 -
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/mime-node.c b/mime-node.c
index 3492bcd0..cafe1537 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -36,6 +36,9 @@ typedef struct mime_node_context {
     GMimeMessage *mime_message;
     _notmuch_message_crypto_t *msg_crypto;
 
+    /* repaired/unmangled parts that will need to be cleaned up */
+    GSList *repaired_parts;
+
     /* Context provided by the caller. */
     _notmuch_crypto_t *crypto;
 } mime_node_context_t;
@@ -52,9 +55,20 @@ _mime_node_context_free (mime_node_context_t *res)
     if (res->stream)
 	g_object_unref (res->stream);
 
+    if (res->repaired_parts)
+	g_slist_free_full (res->repaired_parts, g_object_unref);
+
     return 0;
 }
 
+/* keep track of objects that need to be destroyed when the mime node
+   context goes away. */
+static void
+_mime_node_context_track_repaired_part (mime_node_context_t *ctx, GMimeObject *part) {
+    if (part)
+	ctx->repaired_parts = g_slist_prepend (ctx->repaired_parts, part);
+}
+
 const _notmuch_message_crypto_t*
 mime_node_get_message_crypto_status (mime_node_t *node)
 {
@@ -286,6 +300,14 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
     if (GMIME_IS_PART (part)) {
 	node->nchildren = 0;
     } else if (GMIME_IS_MULTIPART (part)) {
+	GMimeObject *repaired_part = _notmuch_repair_mixed_up_mangled (part);
+	if (repaired_part) {
+	    /* This was likely "Mixed Up" in transit!  We replace it
+	     * with the more likely-to-be-correct variant. */
+	    _mime_node_context_track_repaired_part (node->ctx, repaired_part);
+	    part = repaired_part;
+	    node->part = part;
+	}
 	node->nchildren = g_mime_multipart_get_count (GMIME_MULTIPART (part));
     } else if (GMIME_IS_MESSAGE_PART (part)) {
 	/* Promote part to an envelope and open it */
diff --git a/test/T351-pgpmime-mangling.sh b/test/T351-pgpmime-mangling.sh
index a20066e4..a613fe3e 100755
--- a/test/T351-pgpmime-mangling.sh
+++ b/test/T351-pgpmime-mangling.sh
@@ -7,7 +7,6 @@ add_gnupg_home
 add_email_corpus mangling
 
 test_begin_subtest "handle 'Mixed-Up' mangled PGP/MIME message"
-test_subtest_known_broken
 output=$(notmuch show --format=json --decrypt=true id:mixed-up@mangling.notmuchmail.org)
 test_json_nodes <<<"$output" \
                 'body:[0][0][0]["body"][0]["content"][1]["content"]="The password is \"abcd1234!\", please do not tell anyone.\n"'
-- 
2.20.1

Re: [PATCH 4/4] cli/show: show repaired form of "Mixed Up" mangled messages

[Rollins, Jameson]

On Tue, May 28 2019, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> When showing a message that has been mangled in transit by an MTA in
> the "Mixed up" way, "notmuch show" should instead show the repaired
> form of the message.

Given that this fix is in mime-node.c it will apply to reply as well,
not just show, yes?

Re: [PATCH 4/4] cli/show: show repaired form of "Mixed Up" mangled messages

[Rollins, Jameson]

On Wed, May 29 2019, Jameson Graef Rollins <jrollins@caltech.edu> wrote:
> On Tue, May 28 2019, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
>> When showing a message that has been mangled in transit by an MTA in
>> the "Mixed up" way, "notmuch show" should instead show the repaired
>> form of the message.
>
> Given that this fix is in mime-node.c it will apply to reply as well,
> not just show, yes?

Not that we really need to bother changing the commit log for that...
just checking.

Re: [PATCH 4/4] cli/show: show repaired form of "Mixed Up" mangled messages

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]

On Thu 2019-05-30 02:09:47 +0000, Rollins, Jameson wrote:
> On Wed, May 29 2019, Jameson Graef Rollins <jrollins@caltech.edu> wrote:
>> On Tue, May 28 2019, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
>>> When showing a message that has been mangled in transit by an MTA in
>>> the "Mixed up" way, "notmuch show" should instead show the repaired
>>> form of the message.
>>
>> Given that this fix is in mime-node.c it will apply to reply as well,
>> not just show, yes?
>
> Not that we really need to bother changing the commit log for that...
> just checking.

Yes, it does fix notmuch reply as well.  Do you think that we should
augment the test suite to include 'notmuch reply' too?

       --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 4/4] cli/show: show repaired form of "Mixed Up" mangled messages

[Rollins, Jameson]

On Thu, May 30 2019, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> Yes, it does fix notmuch reply as well.  Do you think that we should
> augment the test suite to include 'notmuch reply' too?

I mean, more tests don't hurt, but I don't see it as critical.  I think
it could be as simple as (please verify):

test_begin_subtest "reply 'Mixed-Up' mangled PGP/MIME message"
output=$(notmuch reply --format=json --decrypt id:mixed-up@mangling.notmuchmail.org)
test_json_nodes <<<"$output" \
                'body:["original"]["body"][0]["content"][1]["content"]="The password is \"abcd1234!\", please do not tell anyone.\n"'

Re: Safe and useful handling of "Mixed Up" mangled messages

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 367 bytes --]

On Tue 2019-05-28 18:54:48 -0400, Daniel Kahn Gillmor wrote:
> The test case included in this series should be sufficient to show the
> problem specifically

I forgot to mention: this test case makes use of the test_json_nodes
functionality introduced in 03/17 of the protected header series.

So please only consider this after that patch has been merged.

   --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: Safe and useful handling of "Mixed Up" mangled messages

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

On Tue 2019-05-28 18:58:22 -0400, Daniel Kahn Gillmor wrote:
> I forgot to mention: this test case makes use of the test_json_nodes
> functionality introduced in 03/17 of the protected header series.
>
> So please only consider this after that patch has been merged.

It has been merged, so this series is good to go.  Please review!

I'm running this series today, and it is definitely useful.  It'd be
great to get it into 0.29 if we can.

   --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: Safe and useful handling of "Mixed Up" mangled messages

[Rollins, Jameson]

On Wed, May 29 2019, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> On Tue 2019-05-28 18:58:22 -0400, Daniel Kahn Gillmor wrote:
>> I forgot to mention: this test case makes use of the test_json_nodes
>> functionality introduced in 03/17 of the protected header series.
>>
>> So please only consider this after that patch has been merged.
>
> It has been merged, so this series is good to go.  Please review!
>
> I'm running this series today, and it is definitely useful.  It'd be
> great to get it into 0.29 if we can.

As someone who's messages have been mangled in this way, I very much
appreciate this fix.  Thank you dkg!

Daniel has documented this particular mangling very well in his IETF
draft [0], and this patch follows the recommendation there for fixing
messages.  The way he handles the repair seems reasonable to me (modulo
a couple minor comments in reply).  I suppose the index.repaired
property is a good way to indicate that a repair has happened.  Given
that we can have multiple properties with the same key, we will be able
to indicate other repairs in the same way, which is good.

All tests pass for me with these patches.

jamie.

[0] https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling-00

Re: Safe and useful handling of "Mixed Up" mangled messages

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]

On Thu 2019-05-30 02:21:02 +0000, Rollins, Jameson wrote:
> The way he handles the repair seems reasonable to me (modulo
> a couple minor comments in reply).

I've responded to jamie's review here with v2 of this series, which can
be found at id:20190530172707.10378-1-dkg@fifthhorseman.net.

If anyone else wants to review, please review that revision.

Regards,

   --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]