[PATCH 1/2] legacy-display: accept text/plain legacy display parts

[PATCH 1/2] legacy-display: accept text/plain legacy display parts

[Daniel Kahn Gillmor]

https://www.ietf.org/id/draft-autocrypt-lamps-protected-headers-02.html
Makes it clear that the "Legacy Display" part of an encrypted message
with protected headers can (and indeed, should) be of content-type
text/plain, though some clients still generate the Legacy Display part
as content-type text/rfc822-headers.  Notmuch should recognize the
part whichever of the two content-types it uses.

See also discussion in
https://github.com/autocrypt/protected-headers/issues/23 for why the
community of implementers is moving in the direction of text/plain.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 util/repair.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/util/repair.c b/util/repair.c
index 9fba97b7..4385d16f 100644
--- a/util/repair.c
+++ b/util/repair.c
@@ -49,8 +49,10 @@ _notmuch_crypto_payload_has_legacy_display (GMimeObject *payload)
     if (g_mime_multipart_get_count (mpayload) != 2)
 	return false;
     first = g_mime_multipart_get_part (mpayload, 0);
-    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (first),
-				       "text", "rfc822-headers"))
+    if (! (g_mime_content_type_is_type (g_mime_object_get_content_type (first),
+					"text", "plain") ||
+	   g_mime_content_type_is_type (g_mime_object_get_content_type (first),
+					"text", "rfc822-headers")))
 	return false;
     protected_header_parameter = g_mime_object_get_content_type_parameter (first, "protected-headers");
     if ((! protected_header_parameter) || strcmp (protected_header_parameter, "v1"))
-- 
2.24.0

[PATCH 2/2] legacy-display: drop tests that try to match headers in a Legacy Display part

[Daniel Kahn Gillmor]

These tests were an attempt to establish that the content of the
"Legacy Display" part is the same as the actual protected headers of
the message.  But this is more conservative than we need to be.

https://www.ietf.org/id/draft-autocrypt-lamps-protected-headers-02.html
section 5.3 makes clear that the Legacy Display part is purely
decorative, and section 5.2.1 clarifies that the detection can be done
purely by MIME structure and Content-Type alone.

Furthermore, now that we're accepting text/plain Legacy Display parts,
it's not clear the lines in the Legacy Display part should be
interpreted as needing an exact string match (e.g. "real" headers are
likely to be RFC 2047 encoded, but the text/plain Legacy Display part
probably should not be).

The concerns that motivated this test in the past were twofold: that
we might accidentally hide some information from the reader of the
message that they should have available to them, or that we could
introduce a covert channel that would be invisible to other clients.

I no longer think these are significant concerns:

 a) There will be no accidental misidentification of a Legacy Display
    part.  The identification of the Legacy Display part is
    unambiguous due to MIME structure and Content-Type.  MIME
    structure MUST be the first child part of a two-part
    multipart/mixed Cryptographic Payload. And the
    protected-headers=v1 content-type parameter must be present on
    both the cryptographic payload and the legacy display part, so no
    one would accidentally generate this structure and have it be
    accidentally matched.

 b) As for creating a covert channel, many such channels already
    exist.  For example, non-standard e-mail headers, custom MIME
    types, unusual MIME structures, etc, all make it possible to ship
    some content in a message that will be visible in some MUAs but
    not in others.  This doesn't make the situation demonstrably
    worse.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 util/repair.c | 60 ++-------------------------------------------------
 1 file changed, 2 insertions(+), 58 deletions(-)

diff --git a/util/repair.c b/util/repair.c
index 4385d16f..6c13601d 100644
--- a/util/repair.c
+++ b/util/repair.c
@@ -27,13 +27,7 @@ _notmuch_crypto_payload_has_legacy_display (GMimeObject *payload)
 {
     GMimeMultipart *mpayload;
     const char *protected_header_parameter;
-    GMimeTextPart *legacy_display;
-    char *legacy_display_header_text = NULL;
-    GMimeStream *stream = NULL;
-    GMimeParser *parser = NULL;
-    GMimeObject *legacy_header_object = NULL, *first;
-    GMimeHeaderList *legacy_display_headers = NULL, *protected_headers = NULL;
-    bool ret = false;
+    GMimeObject *first;
 
     if (! g_mime_content_type_is_type (g_mime_object_get_content_type (payload),
 				       "multipart", "mixed"))
@@ -60,57 +54,7 @@ _notmuch_crypto_payload_has_legacy_display (GMimeObject *payload)
     if (! GMIME_IS_TEXT_PART (first))
 	return false;
 
-    /* ensure that the headers in the first part all match the values
-     * found in the payload's own protected headers!  if they don't,
-     * we should not treat this as a valid "legacy-display" part.
-     *
-     * Crafting a GMimeHeaderList object from the content of the
-     * text/rfc822-headers part is pretty clumsy; we should probably
-     * push something into GMime that makes this a one-shot
-     * operation. */
-    if ((protected_headers = g_mime_object_get_header_list (payload), protected_headers) &&
-	(legacy_display = GMIME_TEXT_PART (first), legacy_display) &&
-	(legacy_display_header_text = g_mime_text_part_get_text (legacy_display), legacy_display_header_text) &&
-	(stream = g_mime_stream_mem_new_with_buffer (legacy_display_header_text, strlen (legacy_display_header_text)), stream) &&
-	(g_mime_stream_write (stream, "\r\n\r\n", 4) == 4) &&
-	(g_mime_stream_seek (stream, 0, GMIME_STREAM_SEEK_SET) == 0) &&
-	(parser = g_mime_parser_new_with_stream (stream), parser) &&
-	(legacy_header_object = g_mime_parser_construct_part (parser, NULL), legacy_header_object) &&
-	(legacy_display_headers = g_mime_object_get_header_list (legacy_header_object), legacy_display_headers)) {
-	/* walk through legacy_display_headers, comparing them against
-	 * their values in the protected_headers: */
-	ret = true;
-	for (int i = 0; i < g_mime_header_list_get_count (legacy_display_headers); i++) {
-	    GMimeHeader *dh = g_mime_header_list_get_header_at (legacy_display_headers, i);
-	    if (dh == NULL) {
-		ret = false;
-		goto DONE;
-	    }
-	    GMimeHeader *ph = g_mime_header_list_get_header (protected_headers, g_mime_header_get_name (dh));
-	    if (ph == NULL) {
-		ret = false;
-		goto DONE;
-	    }
-	    const char *dhv = g_mime_header_get_value (dh);
-	    const char *phv = g_mime_header_get_value (ph);
-	    if (dhv == NULL || phv == NULL || strcmp (dhv, phv)) {
-		ret = false;
-		goto DONE;
-	    }
-	}
-    }
-
- DONE:
-    if (legacy_display_header_text)
-	g_free (legacy_display_header_text);
-    if (stream)
-	g_object_unref (stream);
-    if (parser)
-	g_object_unref (parser);
-    if (legacy_header_object)
-	g_object_unref (legacy_header_object);
-
-    return ret;
+    return true;
 }
 
 GMimeObject *
-- 
2.24.0

Re: [PATCH 1/2] legacy-display: accept text/plain legacy display parts

[David Edmondson]

On Monday, 2019-12-23 at 12:39:26 -05, Daniel Kahn Gillmor wrote:

> https://www.ietf.org/id/draft-autocrypt-lamps-protected-headers-02.html
> Makes it clear that the "Legacy Display" part of an encrypted message
> with protected headers can (and indeed, should) be of content-type
> text/plain, though some clients still generate the Legacy Display part
> as content-type text/rfc822-headers.  Notmuch should recognize the
> part whichever of the two content-types it uses.
>
> See also discussion in
> https://github.com/autocrypt/protected-headers/issues/23 for why the
> community of implementers is moving in the direction of text/plain.
>
> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> ---
>  util/repair.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/util/repair.c b/util/repair.c
> index 9fba97b7..4385d16f 100644
> --- a/util/repair.c
> +++ b/util/repair.c
> @@ -49,8 +49,10 @@ _notmuch_crypto_payload_has_legacy_display (GMimeObject *payload)
>      if (g_mime_multipart_get_count (mpayload) != 2)
>  	return false;
>      first = g_mime_multipart_get_part (mpayload, 0);
> -    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (first),
> -				       "text", "rfc822-headers"))
> +    if (! (g_mime_content_type_is_type (g_mime_object_get_content_type (first),
> +					"text", "plain") ||
> +	   g_mime_content_type_is_type (g_mime_object_get_content_type (first),
> +					"text", "rfc822-headers")))

Patch looks good, though I would quite like there to be a comment in the
code. Something simple like:

/* text/rfc822-headers was replaced by text/plain as the MIME type for
“Legacy Display” parts - we allow either. */

>  	return false;
>      protected_header_parameter = g_mime_object_get_content_type_parameter (first, "protected-headers");
>      if ((! protected_header_parameter) || strcmp (protected_header_parameter, "v1"))
> -- 
> 2.24.0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

dme.
-- 
It's alright, we told you what to dream.

Re: [PATCH 1/2] legacy-display: accept text/plain legacy display parts

[Daniel Kahn Gillmor]

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

On Tue 2019-12-24 10:59:09 +0000, David Edmondson wrote:
> Patch looks good, though I would quite like there to be a comment in the
> code. Something simple like:
>
> /* text/rfc822-headers was replaced by text/plain as the MIME type for
> “Legacy Display” parts - we allow either. */

I have no objection to having this comment in the code (though i think
i'd replace U+201C LEFT DOUBLE QUOTATION MARK and U+201D RIGHT DOUBLE
QUOTATION MARK with U+0022 QUOTATION MARK.  i don't think we have any
non-ASCII text in our C source at the moment, and this doesn't seem like
the right place to introduce it.


Since the code will be read in the future, not the past, i think the
right comment might be:

 /* Early implementations that generated "Legacy Display" parts used
    Content-Type: text/rfc822-headers, but text/plain is more widely
    rendered, so it is now the standard choice.  We accept either as a
    Legacy Display part. */

Bremner, if you are considering a merge, and want to inject this comment
yourself, that's fine with me.  let me know if you want me to send
another revision instead.

        --dkg

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

Re: [PATCH 1/2] legacy-display: accept text/plain legacy display parts

[David Bremner]

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

> Bremner, if you are considering a merge, and want to inject this comment
> yourself, that's fine with me.  let me know if you want me to send
> another revision instead.
>
>         --dkg

another revision is probably best.

d

[PATCH v2] legacy-display: accept text/plain legacy display parts

[Daniel Kahn Gillmor]

https://www.ietf.org/id/draft-autocrypt-lamps-protected-headers-02.html
Makes it clear that the "Legacy Display" part of an encrypted message
with protected headers can (and indeed, should) be of content-type
text/plain, though some clients still generate the Legacy Display part
as content-type text/rfc822-headers.  Notmuch should recognize the
part whichever of the two content-types it uses.

See also discussion in
https://github.com/autocrypt/protected-headers/issues/23 for why the
community of implementers is moving in the direction of text/plain.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 util/repair.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/util/repair.c b/util/repair.c
index 9fba97b7..f5cbb14b 100644
--- a/util/repair.c
+++ b/util/repair.c
@@ -49,8 +49,14 @@ _notmuch_crypto_payload_has_legacy_display (GMimeObject *payload)
     if (g_mime_multipart_get_count (mpayload) != 2)
 	return false;
     first = g_mime_multipart_get_part (mpayload, 0);
-    if (! g_mime_content_type_is_type (g_mime_object_get_content_type (first),
-				       "text", "rfc822-headers"))
+    /* Early implementations that generated "Legacy Display" parts used
+       Content-Type: text/rfc822-headers, but text/plain is more widely
+       rendered, so it is now the standard choice.  We accept either as a
+       Legacy Display part. */
+    if (! (g_mime_content_type_is_type (g_mime_object_get_content_type (first),
+					"text", "plain") ||
+	   g_mime_content_type_is_type (g_mime_object_get_content_type (first),
+					"text", "rfc822-headers")))
 	return false;
     protected_header_parameter = g_mime_object_get_content_type_parameter (first, "protected-headers");
     if ((! protected_header_parameter) || strcmp (protected_header_parameter, "v1"))
-- 
2.24.0

Re: [PATCH 1/2] legacy-display: accept text/plain legacy display parts

[Daniel Kahn Gillmor]

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

On Wed 2019-12-25 06:23:57 +0900, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> Bremner, if you are considering a merge, and want to inject this comment
>> yourself, that's fine with me.  let me know if you want me to send
>> another revision instead.
>
> another revision is probably best.

done, i've sent id:20191224220544.392201-1-dkg@fifthhorseman.net to this
thread as a substituted, and updated the notmuch::* tags appropriately
(i hope).

thanks for taking the time to review and consider merging!

       --dkg

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

Re: [PATCH 1/2] legacy-display: accept text/plain legacy display parts

[David Edmondson]

On Tuesday, 2019-12-24 at 10:54:46 -05, Daniel Kahn Gillmor wrote:

> On Tue 2019-12-24 10:59:09 +0000, David Edmondson wrote:
>> Patch looks good, though I would quite like there to be a comment in the
>> code. Something simple like:
>>
>> /* text/rfc822-headers was replaced by text/plain as the MIME type for
>> “Legacy Display” parts - we allow either. */
>
> I have no objection to having this comment in the code (though i think
> i'd replace U+201C LEFT DOUBLE QUOTATION MARK and U+201D RIGHT DOUBLE
> QUOTATION MARK with U+0022 QUOTATION MARK.  i don't think we have any
> non-ASCII text in our C source at the moment, and this doesn't seem like
> the right place to introduce it.

Agreed.

> Since the code will be read in the future, not the past, i think the
> right comment might be:
>
>  /* Early implementations that generated "Legacy Display" parts used
>     Content-Type: text/rfc822-headers, but text/plain is more widely
>     rendered, so it is now the standard choice.  We accept either as a
>     Legacy Display part. */

Looks good to me, thank you.

> Bremner, if you are considering a merge, and want to inject this comment
> yourself, that's fine with me.  let me know if you want me to send
> another revision instead.
>
>         --dkg

dme.
-- 
And the sign said: long haired freaky people need not apply.

Re: [PATCH 2/2] legacy-display: drop tests that try to match headers in a Legacy Display part

[Tomi Ollila]

On Mon, Dec 23 2019, Daniel Kahn Gillmor wrote:

> These tests were an attempt to establish that the content of the
> "Legacy Display" part is the same as the actual protected headers of
> the message.  But this is more conservative than we need to be.
>
> https://www.ietf.org/id/draft-autocrypt-lamps-protected-headers-02.html
> section 5.3 makes clear that the Legacy Display part is purely
> decorative, and section 5.2.1 clarifies that the detection can be done
> purely by MIME structure and Content-Type alone.
>
> Furthermore, now that we're accepting text/plain Legacy Display parts,
> it's not clear the lines in the Legacy Display part should be
> interpreted as needing an exact string match (e.g. "real" headers are
> likely to be RFC 2047 encoded, but the text/plain Legacy Display part
> probably should not be).
>
> The concerns that motivated this test in the past were twofold: that
> we might accidentally hide some information from the reader of the
> message that they should have available to them, or that we could
> introduce a covert channel that would be invisible to other clients.
>
> I no longer think these are significant concerns:
>
>  a) There will be no accidental misidentification of a Legacy Display
>     part.  The identification of the Legacy Display part is
>     unambiguous due to MIME structure and Content-Type.  MIME
>     structure MUST be the first child part of a two-part
>     multipart/mixed Cryptographic Payload. And the
>     protected-headers=v1 content-type parameter must be present on
>     both the cryptographic payload and the legacy display part, so no
>     one would accidentally generate this structure and have it be
>     accidentally matched.
>
>  b) As for creating a covert channel, many such channels already
>     exist.  For example, non-standard e-mail headers, custom MIME
>     types, unusual MIME structures, etc, all make it possible to ship
>     some content in a message that will be visible in some MUAs but
>     not in others.  This doesn't make the situation demonstrably
>     worse.

Looks good to me, and removal of 58 (56) lines of code looks good too...

Tomi

>
> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> ---
>  util/repair.c | 60 ++-------------------------------------------------
>  1 file changed, 2 insertions(+), 58 deletions(-)
>
> diff --git a/util/repair.c b/util/repair.c
> index 4385d16f..6c13601d 100644
> --- a/util/repair.c
> +++ b/util/repair.c
> @@ -27,13 +27,7 @@ _notmuch_crypto_payload_has_legacy_display (GMimeObject *payload)
>  {
>      GMimeMultipart *mpayload;
>      const char *protected_header_parameter;
> -    GMimeTextPart *legacy_display;
> -    char *legacy_display_header_text = NULL;
> -    GMimeStream *stream = NULL;
> -    GMimeParser *parser = NULL;
> -    GMimeObject *legacy_header_object = NULL, *first;
> -    GMimeHeaderList *legacy_display_headers = NULL, *protected_headers = NULL;
> -    bool ret = false;
> +    GMimeObject *first;
>  
>      if (! g_mime_content_type_is_type (g_mime_object_get_content_type (payload),
>  				       "multipart", "mixed"))
> @@ -60,57 +54,7 @@ _notmuch_crypto_payload_has_legacy_display (GMimeObject *payload)
>      if (! GMIME_IS_TEXT_PART (first))
>  	return false;
>  
> -    /* ensure that the headers in the first part all match the values
> -     * found in the payload's own protected headers!  if they don't,
> -     * we should not treat this as a valid "legacy-display" part.
> -     *
> -     * Crafting a GMimeHeaderList object from the content of the
> -     * text/rfc822-headers part is pretty clumsy; we should probably
> -     * push something into GMime that makes this a one-shot
> -     * operation. */
> -    if ((protected_headers = g_mime_object_get_header_list (payload), protected_headers) &&
> -	(legacy_display = GMIME_TEXT_PART (first), legacy_display) &&
> -	(legacy_display_header_text = g_mime_text_part_get_text (legacy_display), legacy_display_header_text) &&
> -	(stream = g_mime_stream_mem_new_with_buffer (legacy_display_header_text, strlen (legacy_display_header_text)), stream) &&
> -	(g_mime_stream_write (stream, "\r\n\r\n", 4) == 4) &&
> -	(g_mime_stream_seek (stream, 0, GMIME_STREAM_SEEK_SET) == 0) &&
> -	(parser = g_mime_parser_new_with_stream (stream), parser) &&
> -	(legacy_header_object = g_mime_parser_construct_part (parser, NULL), legacy_header_object) &&
> -	(legacy_display_headers = g_mime_object_get_header_list (legacy_header_object), legacy_display_headers)) {
> -	/* walk through legacy_display_headers, comparing them against
> -	 * their values in the protected_headers: */
> -	ret = true;
> -	for (int i = 0; i < g_mime_header_list_get_count (legacy_display_headers); i++) {
> -	    GMimeHeader *dh = g_mime_header_list_get_header_at (legacy_display_headers, i);
> -	    if (dh == NULL) {
> -		ret = false;
> -		goto DONE;
> -	    }
> -	    GMimeHeader *ph = g_mime_header_list_get_header (protected_headers, g_mime_header_get_name (dh));
> -	    if (ph == NULL) {
> -		ret = false;
> -		goto DONE;
> -	    }
> -	    const char *dhv = g_mime_header_get_value (dh);
> -	    const char *phv = g_mime_header_get_value (ph);
> -	    if (dhv == NULL || phv == NULL || strcmp (dhv, phv)) {
> -		ret = false;
> -		goto DONE;
> -	    }
> -	}
> -    }
> -
> - DONE:
> -    if (legacy_display_header_text)
> -	g_free (legacy_display_header_text);
> -    if (stream)
> -	g_object_unref (stream);
> -    if (parser)
> -	g_object_unref (parser);
> -    if (legacy_header_object)
> -	g_object_unref (legacy_header_object);
> -
> -    return ret;
> +    return true;
>  }
>  
>  GMimeObject *
> -- 
> 2.24.0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/2] legacy-display: drop tests that try to match headers in a Legacy Display part

[David Bremner]

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

> These tests were an attempt to establish that the content of the
> "Legacy Display" part is the same as the actual protected headers of
> the message.  But this is more conservative than we need to be.

pushed.

d

Re: [PATCH v2] legacy-display: accept text/plain legacy display parts

[David Bremner]

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

> https://www.ietf.org/id/draft-autocrypt-lamps-protected-headers-02.html
> Makes it clear that the "Legacy Display" part of an encrypted message
> with protected headers can (and indeed, should) be of content-type
> text/plain, though some clients still generate the Legacy Display part
> as content-type text/rfc822-headers.  Notmuch should recognize the
> part whichever of the two content-types it uses.

pushed,

d