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 _______________________________________________ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch