[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 at 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 at notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch