Protect more header fields #4227

alejandro-colomar · 2024-03-31T23:23:15Z

What does this PR do?

Protect more header fields.
Screenshots (if relevant)

--45wprh5a3u4z6yxp
Content-Type: text/plain; protected-headers=v1; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
To: [email protected]
Cc: [email protected]
Subject: test protected to
MIME-Version: 1.0

asd

--=20
<https://www.alejandro-colomar.es/>

--45wprh5a3u4z6yxp

Does this PR meet the acceptance criteria? (This is just a reminder for you,
this section can be removed if you fulfill it.)
- Documentation created/updated (you have to edit
  docs/manual.xml.head
  for that)
  
  Not yet.
- All builds and tests are passing
  
  It works, partially. It protects the headers when sending. It doesn't read the protected headers in received mails, yet.
- Added doxygen code documentation
  syntax
  
  N/A. Maybe later I need to do something about it.
- Code follows the style guide\
  
  I hope so.
What are the relevant issue numbers?
- Should To and Cc header fields be protected? #4223
- Should the In-Reply-To header field be protected? #4226

Revisions:

v15

v15 (by @flatcap ) changes:

Rebase
clang-format
Seems to also add a test

$ git range-diff devel/security..protect neomutt/main..gh/protect 
 1:  8403fe9e7 =  1:  207e6fcef ncrypt/crypt.c: Protect address lists in header fields
 2:  383f207d3 =  2:  2283b8a08 list: Add mutt_list_copy_tail()
 3:  eb8f77ee4 !  3:  61e09ca2e test/: Add tests for mutt_list_copy_tail()
    @@ test/list/mutt_list_copy_tail.c (new)
     +#define TEST_NO_MAIN
     +#include "config.h"
     +#include "acutest.h"
    -+#include <stddef.h>
     +#include <stdbool.h>
    ++#include <stddef.h>
     +#include "mutt/lib.h"
     +#include "common.h"
     +
    @@ test/list/mutt_list_copy_tail.c (new)
     +
     +  {
     +    static const char *start_names[] = { "Amy", "Beth", "Cathy", NULL };
    -+    static const char *expected_names[] = { "Amy", "Beth", "Cathy", "Zelda", "Mario", NULL };
    ++    static const char *expected_names[] = { "Amy",   "Beth",  "Cathy",
    ++                                            "Zelda", "Mario", NULL };
     +    static const char *copy_names[] = { "Zelda", "Mario", NULL };
     +    struct ListHead start = test_list_create(start_names, true);
     +    struct ListHead expected = test_list_create(expected_names, true);
 4:  ae42ee341 =  4:  e02544242 ncrypt/crypt.c: Protect References and In-Reply-To
 5:  eb3199e58 =  5:  3ab07f290 ncrypt/crypt.c: Small refactor
 6:  442a0a462 =  6:  00773c94b ncrypt/crypt.c: Read the protected header fields containing address lists
 7:  ca4cbf8db !  7:  12855dc46 list: Add mutt_list_write()
    @@ mutt/list.c: void mutt_list_copy_tail(struct ListHead *dst, const struct ListHea
      ## mutt/list.h ##
     @@
      
    - #include <stddef.h>
      #include <stdbool.h>
    + #include <stddef.h>
     +#include "buffer.h"
      #include "queue.h"
      
 8:  63ac6a413 !  8:  0cc8ce2a6 test/: Add tests for mutt_list_write()
    @@ test/list/mutt_list_write.c (new)
     +#define TEST_NO_MAIN
     +#include "config.h"
     +#include "acutest.h"
    -+#include <stddef.h>
     +#include <stdbool.h>
    ++#include <stddef.h>
     +#include "mutt/lib.h"
     +#include "common.h"
     +#include "test_common.h"
    @@ test/list/mutt_list_write.c (new)
     +    struct ListHead empty;
     +    STAILQ_INIT(&empty);
     +    struct Buffer *buf = buf_pool_get();
    ++    TEST_CHECK(mutt_list_write(NULL, buf) == 0);
    ++    TEST_CHECK(mutt_list_write(&empty, NULL) == 0);
    ++    buf_pool_release(&buf);
    ++  }
    ++
    ++  {
    ++    struct ListHead empty;
    ++    STAILQ_INIT(&empty);
    ++    struct Buffer *buf = buf_pool_get();
     +    const char expected[] = "";
     +    TEST_CHECK(mutt_list_write(&empty, buf) == strlen(expected));
     +    TEST_CHECK_STR_EQ(buf_string(buf), expected);
 9:  ec4ec416e =  9:  d4db1b919 ncrypt/crypt.c: Read the protected References and In-Reply-To header fields
10:  f52e433e1 = 10:  b7d28a25f ncrypt/crypt.c: Protect Date
11:  2e16cd692 = 11:  9a20f5e0e ncrypt/crypt.c: Read the protected Date

Merge

$ git range-diff neomutt/main..gh/protect gpg..61d32facf
 1:  207e6fcef =  1:  913a991a5 ncrypt/crypt.c: Protect address lists in header fields
 2:  2283b8a08 =  2:  25380619b list: Add mutt_list_copy_tail()
 3:  61e09ca2e =  3:  26598562c test/: Add tests for mutt_list_copy_tail()
 4:  e02544242 =  4:  13cfc6f98 ncrypt/crypt.c: Protect References and In-Reply-To
 5:  3ab07f290 =  5:  c3a197993 ncrypt/crypt.c: Small refactor
 6:  00773c94b =  6:  06f8ff5a9 ncrypt/crypt.c: Read the protected header fields containing address lists
 7:  12855dc46 =  7:  f76feba22 list: Add mutt_list_write()
 8:  0cc8ce2a6 =  8:  68e533e50 test/: Add tests for mutt_list_write()
 9:  d4db1b919 =  9:  ec02b1419 ncrypt/crypt.c: Read the protected References and In-Reply-To header fields
10:  b7d28a25f = 10:  944e06904 ncrypt/crypt.c: Protect Date
11:  9a20f5e0e = 11:  61d32facf ncrypt/crypt.c: Read the protected Date

alejandro-colomar · 2024-03-31T23:27:24Z

Protecting To and other address lists was easy. I'll have a take tomorrow on In-Reply-To.

However, reading protected fields wasn't as easy (which is why I had to cat(1) the message to test it).
My problem was that we don't have a string (const char *), but rather a struct AddressList, and so I can't call mutt_write_one_header() --as we do with Subject-- in mutt_protected_headers_handler().

Would you mind pointing to some functions I could use, or look at, @flatcap? Thanks! ;-)

flatcap · 2024-04-01T09:41:43Z

some functions I could use, or look at

All the functions for dealing with struct Address and struct AddressList live in https://github.com/neomutt/neomutt/blob/main/address/address.c

You'll probably want mutt_addrlist_equal(), but that won't cope if the lists are ordered differently.
AddressList is a linked-list, so sorting them would be a pain.

You may also need to consider "IDN".
We have mutt_addrlist_to_intl() and mutt_addrlist_to_local().

History: The address library is fairly chaotic.

Since the fork, we've:

sorted out the dependencies to create libaddress
upgraded the linked list to use TAILQ (mutt/queue.h)
upgraded the char * strings to struct Buffers
documented everything (that we know)
written some unit tests

Nobody's analysed the lifetime of an Address, so there's a lot of duplication.

alejandro-colomar · 2024-04-01T10:06:02Z

On Mon, Apr 01, 2024 at 02:42:05AM GMT, Richard Russon wrote: > some functions I could use, or look at All the functions for dealing with `struct Address` and `struct AddressList` live in https://github.com/neomutt/neomutt/blob/main/address/address.c

I'll try to find the code that prints the headers in the pager; I think it's that what I need.

You'll probably want `mutt_addrlist_equal()`, but that won't cope if the lists are ordered differently. `AddressList` is a linked-list, so sorting them would be a pain.

Nah, I decided that I don't care if they mismatch. At least for now. Maybe when I manage to print them, I change my mind.

You may also need to consider "[IDN](https://en.wikipedia.org/wiki/Internationalized_domain_name)". We have `mutt_addrlist_to_intl()` and `mutt_addrlist_to_local()`.

Maybe... This email is sent with protected In-Reply-To for the first time, BTW. Let's see if it works. :)

--- History: The address library is fairly chaotic. Since the fork, we've: - sorted out the dependencies to create `libaddress` - upgraded the linked list to use `TAILQ` (`mutt/queue.h`) - upgraded the `char *` strings to `struct Buffer`s - documented everything (that we know) - written some unit tests Nobody's analysed the lifetime of an Address, so there's a lot of duplication.

Hmmm.

…

-- <https://www.alejandro-colomar.es/>

alejandro-colomar · 2024-04-01T10:07:27Z

--xytmxyhmc47g5q3k
Content-Type: text/plain; protected-headers=v1; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
From: Alejandro Colomar <[email protected]>
To: 
        neomutt/neomutt <reply+AFA4ZOO55SYLPZBFTYNYZWGECZRG3EVBNHHIIKDVMQ@reply.github.com>
Subject: Re: [neomutt/neomutt] Protect more header fields (PR #4227)
MIME-Version: 1.0
In-Reply-To: <neomutt/neomutt/pull/4227/[email protected]>

On Mon, Apr 01, 2024 at 02:42:05AM GMT, Richard Russon wrote:
> > some functions I could use, or look at
>=20
> All the functions for dealing with `struct Address` and `struct AddressLi=
st` live in https://github.com/neomutt/neomutt/blob/main/address/address.c

I'll try to find the code that prints the headers in the pager; I think
it's that what I need.

Weee! :-)

alejandro-colomar · 2024-04-01T16:46:48Z

To and Cc protected headers being read:

[-- Begin signature information --]
Good signature from: Alejandro Colomar <[email protected]>
                aka: Alejandro Colomar <[email protected]>
                aka: Alejandro Colomar Andres <[email protected]>
            created: Mon Apr  1 15:11:40 2024
[-- End signature information --]

[-- The following data is PGP/MIME signed and encrypted --]

To: Richard Russon <[email protected]>
Subject: Re: NeoMutt, Addresses, Phishing

Hi Richard,

alejandro-colomar · 2024-04-08T12:51:58Z

Here's a test for it:

Date: Mon, 8 Apr 2024 14:48:43 +0200
From: Alejandro Colomar <[email protected]>
To: [email protected]
Cc: [email protected]
Subject: ...
Bcc: [email protected]
Message-ID: <4t5q75afgmxwr2zmpkxaxlfkonbtmx3p7fe4ck75n3xuctxn3f@ysamvmflyupg>
Reply-To: [email protected]

[-- Begin signature information --]
Good signature from: Alejandro Colomar <[email protected]>
                aka: Alejandro Colomar <[email protected]>
                aka: Alejandro Colomar Andres <[email protected]>
            created: Mon Apr  8 14:48:34 2024
[-- End signature information --]

[-- The following data is PGP/MIME signed and encrypted --]

From: Alejandro Colomar <[email protected]>
To: [email protected]
Cc: [email protected]
Reply-To: [email protected]
Subject: To protect and serve

Hi Rich,

This behavior LGTM, and I think is both necessary and sufficient to fix the two security vulnerabilities that I reported recently.

I'll now start documenting the feature.

alejandro-colomar · 2024-04-08T13:24:07Z

This behavior LGTM, and I think is both necessary and sufficient to fix the two security vulnerabilities that I reported recently.

Hmm, now I remember some thoughts I had yesterday:

This is necessary, but not sufficient. We should also make sure neomutt(1) doesn't use the non-protected fields. But I don't want to do that in this PR, since it needs some design decisions that are a bit more complex. Let's do that in a separate PR.

alejandro-colomar · 2024-04-08T16:32:30Z

v2 changes:

Add draft of feature page
Change default of $crypt_protected_headers_write to yes.
Deprecate $crypt_protected_headers_write.

$ git range-diff main gh/protect protect 
 1:  da56b5907 =  1:  da56b5907 ncrypt/crypt.c: Don't weed protected headers
 2:  b3e2fc80b =  2:  b3e2fc80b ncrypt/crypt.c: Protect address lists in header fields
 3:  fab9c841e =  3:  fab9c841e list: Add mutt_list_copy_tail()
 4:  3318db384 =  4:  3318db384 ncrypt/crypt.c: Protect In-Reply-To
 5:  adc7bdda7 =  5:  adc7bdda7 ncrypt/crypt.c: Small refactor
 6:  267be02d1 =  6:  267be02d1 ncrypt/crypt.c: Read the protected header fields containing address lists
 7:  c9a79c1a0 =  7:  c9a79c1a0 list: Add function to write lists wrapped to a buffer
 8:  a2a1eb58b =  8:  a2a1eb58b ncrypt/crypt.c: Read the protected In-Reply-To header field
 -:  --------- >  9:  5d7d86573 docs: Add feature page for protected header fields
 -:  --------- > 10:  d1bfdab13 $crypt_protected_headers_write: Change default to 'yes'
 -:  --------- > 11:  1e7fded07 $crypt_protected_headers_write: Remove variable

ncrypt/crypt.c

alejandro-colomar · 2024-04-08T21:36:59Z

v2b changes:

Fix spelling of X-Original-To

$ git range-diff main gh/protect protect 
 1:  da56b5907 =  1:  da56b5907 ncrypt/crypt.c: Don't weed protected headers
 2:  b3e2fc80b =  2:  b3e2fc80b ncrypt/crypt.c: Protect address lists in header fields
 3:  fab9c841e =  3:  fab9c841e list: Add mutt_list_copy_tail()
 4:  3318db384 =  4:  3318db384 ncrypt/crypt.c: Protect In-Reply-To
 5:  adc7bdda7 =  5:  adc7bdda7 ncrypt/crypt.c: Small refactor
 6:  267be02d1 !  6:  843e39934 ncrypt/crypt.c: Read the protected header fields containing address lists
    @@ ncrypt/crypt.c: int mutt_protected_headers_handler(struct Body *b_email, struct
     +                          wraplen, chflags, NeoMutt->sub);
     +    buf_reset(buf);
     +    mutt_addrlist_write_wrap(&b_email->mime_headers->x_original_to, buf, NULL);
    -+    mutt_write_one_header(state->fp_out, "X-Orig-To", buf_string(buf), state->prefix,
    ++    mutt_write_one_header(state->fp_out, "X-Original-To", buf_string(buf), state->prefix,
     +                          wraplen, chflags, NeoMutt->sub);
      
          if (b_email->mime_headers->subject)
 -:  --------- >  7:  406417b42 Fix misspellings of X-Original-To
 7:  c9a79c1a0 =  8:  97dbe4c35 list: Add function to write lists wrapped to a buffer
 8:  a2a1eb58b =  9:  335fbb42d ncrypt/crypt.c: Read the protected In-Reply-To header field
 9:  5d7d86573 = 10:  688ca047b docs: Add feature page for protected header fields
10:  d1bfdab13 = 11:  43b778037 $crypt_protected_headers_write: Change default to 'yes'
11:  1e7fded07 = 12:  9a0fcceb9 $crypt_protected_headers_write: Remove variable

alejandro-colomar · 2024-04-08T21:48:09Z

v2c changes:

Extend the feature page, and say the feature is always enabled (since we removed the variable that controlled it).
Reorder commits.

$ git range-diff main gh/protect protect 
 1:  da56b5907 =  1:  da56b5907 ncrypt/crypt.c: Don't weed protected headers
 2:  b3e2fc80b =  2:  b3e2fc80b ncrypt/crypt.c: Protect address lists in header fields
 3:  fab9c841e =  3:  fab9c841e list: Add mutt_list_copy_tail()
 4:  3318db384 =  4:  3318db384 ncrypt/crypt.c: Protect In-Reply-To
 5:  adc7bdda7 =  5:  adc7bdda7 ncrypt/crypt.c: Small refactor
 6:  843e39934 =  6:  843e39934 ncrypt/crypt.c: Read the protected header fields containing address lists
 7:  406417b42 =  7:  406417b42 Fix misspellings of X-Original-To
 8:  97dbe4c35 =  8:  97dbe4c35 list: Add function to write lists wrapped to a buffer
 9:  335fbb42d =  9:  335fbb42d ncrypt/crypt.c: Read the protected In-Reply-To header field
11:  43b778037 = 10:  a63efa0e2 $crypt_protected_headers_write: Change default to 'yes'
12:  9a0fcceb9 = 11:  165cc2b96 $crypt_protected_headers_write: Remove variable
10:  688ca047b ! 12:  d0010ae8a docs: Add feature page for protected header fields
    @@ docs/manual.xml.head: color progress white red
     +      <sect2 id="protected-header-fields-intro">
     +        <title>Introduction</title>
     +        <para>
    ++          To avoid tampering of several header fields,
    ++          they are included inside the protected data.
    ++          The following fields are protected:
    ++          most fields that contain address lists,
    ++          and In-Reply-To.
     +          This addresses CVE-???? and CVE-????.
     +        </para>
     +      </sect2>
    @@ docs/manual.xml.head: color progress white red
     +      <sect2 id="protected-header-fields-usage">
     +        <title>Usage</title>
     +        <para>
    -+          It's enabled by default.
    ++          It's always enabled.
     +        </para>
     +      </sect2>
     +

alejandro-colomar · 2024-05-01T12:45:12Z

v14 changes:

wfix commit message
Add tests for mutt_list_write()

$ git range-diff devel/security gh/protect protect 
 1:  4b9fa065c =  1:  4b9fa065c ncrypt/crypt.c: Protect address lists in header fields
 2:  cc9b518ee =  2:  cc9b518ee list: Add mutt_list_copy_tail()
 3:  66030fe81 =  3:  66030fe81 test/: Add tests for mutt_list_copy_tail()
 4:  0f065acc9 =  4:  0f065acc9 ncrypt/crypt.c: Protect References and In-Reply-To
 5:  9f05a19de =  5:  9f05a19de ncrypt/crypt.c: Small refactor
 6:  279f61f5f =  6:  279f61f5f ncrypt/crypt.c: Read the protected header fields containing address lists
 7:  721a6ba25 !  7:  6fc0bc5ec list: Add function to write lists to a buffer
    @@ Metadata
     Author: Alejandro Colomar (@alejandro-colomar) <[email protected]>
     
      ## Commit message ##
    -    list: Add function to write lists to a buffer
    +    list: Add mutt_list_write()
     
    -    This is similar to mutt_addrlist_write(), but simpler, and for ListHead.
    +    This function writes a list to a buffer.
    +
    +    It is similar to mutt_addrlist_write(), but simpler, and for ListHead.
     
         Link: <https://github.com/neomutt/neomutt/issues/4223>
         Link: <https://github.com/neomutt/neomutt/issues/4226>
 -:  --------- >  8:  6be5dffef test/: Add tests for mutt_list_write()
 8:  5ed3bc985 =  9:  ab39a0e50 ncrypt/crypt.c: Read the protected References and In-Reply-To header fields
 9:  7af851aeb = 10:  b672c0170 ncrypt/crypt.c: Protect Date
10:  196cd486f = 11:  7187ac760 ncrypt/crypt.c: Read the protected Date

alejandro-colomar · 2024-05-01T12:50:16Z

v14b changes:

Add missing include to test

$ git range-diff devel/security gh/protect protect 
 1:  4b9fa065c =  1:  4b9fa065c ncrypt/crypt.c: Protect address lists in header fields
 2:  cc9b518ee =  2:  cc9b518ee list: Add mutt_list_copy_tail()
 3:  66030fe81 =  3:  66030fe81 test/: Add tests for mutt_list_copy_tail()
 4:  0f065acc9 =  4:  0f065acc9 ncrypt/crypt.c: Protect References and In-Reply-To
 5:  9f05a19de =  5:  9f05a19de ncrypt/crypt.c: Small refactor
 6:  279f61f5f =  6:  279f61f5f ncrypt/crypt.c: Read the protected header fields containing address lists
 7:  6fc0bc5ec =  7:  6fc0bc5ec list: Add mutt_list_write()
 8:  6be5dffef !  8:  6c7ca0c18 test/: Add tests for mutt_list_write()
    @@ test/list/mutt_list_write.c (new)
     +#include <stdbool.h>
     +#include "mutt/lib.h"
     +#include "common.h"
    ++#include "test_common.h"
     +
     +void test_mutt_list_write(void)
     +{
 9:  ab39a0e50 =  9:  f21428e4b ncrypt/crypt.c: Read the protected References and In-Reply-To header fields
10:  b672c0170 = 10:  4c7fa7277 ncrypt/crypt.c: Protect Date
11:  7187ac760 = 11:  c8caeb850 ncrypt/crypt.c: Read the protected Date

alejandro-colomar · 2024-05-01T13:04:38Z

v14c changes:

Fix test values (I forgot to replace the 0s).

$ git range-diff devel/security gh/protect protect 
 1:  4b9fa065c =  1:  4b9fa065c ncrypt/crypt.c: Protect address lists in header fields
 2:  cc9b518ee =  2:  cc9b518ee list: Add mutt_list_copy_tail()
 3:  66030fe81 =  3:  66030fe81 test/: Add tests for mutt_list_copy_tail()
 4:  0f065acc9 =  4:  0f065acc9 ncrypt/crypt.c: Protect References and In-Reply-To
 5:  9f05a19de =  5:  9f05a19de ncrypt/crypt.c: Small refactor
 6:  279f61f5f =  6:  279f61f5f ncrypt/crypt.c: Read the protected header fields containing address lists
 7:  6fc0bc5ec =  7:  6fc0bc5ec list: Add mutt_list_write()
 8:  6c7ca0c18 !  8:  b2a3da831 test/: Add tests for mutt_list_write()
    @@ test/list/mutt_list_write.c (new)
     +    struct ListHead empty;
     +    STAILQ_INIT(&empty);
     +    struct Buffer *buf = buf_pool_get();
    -+    TEST_CHECK(mutt_list_write(&empty, buf) == 0);
    -+    TEST_CHECK_STR_EQ(buf_string(buf), "");
    ++    const char expected[] = "";
    ++    TEST_CHECK(mutt_list_write(&empty, buf) == strlen(expected));
    ++    TEST_CHECK_STR_EQ(buf_string(buf), expected);
     +    buf_pool_release(&buf);
     +  }
     +
    @@ test/list/mutt_list_write.c (new)
     +    static const char *list_names[] = { "Amy", "Beth", "Cathy", NULL };
     +    struct ListHead list = test_list_create(list_names, false);
     +    struct Buffer *buf = buf_pool_get();
    -+    TEST_CHECK(mutt_list_write(&list, buf) == 0);
    -+    TEST_CHECK_STR_EQ(buf_string(buf), "Amy Beth Cathy");
    ++    const char expected[] = "Amy Beth Cathy";
    ++    TEST_CHECK(mutt_list_write(&list, buf) == strlen(expected));
    ++    TEST_CHECK_STR_EQ(buf_string(buf), expected);
     +    buf_pool_release(&buf);
     +    mutt_list_clear(&list);
     +  }
 9:  f21428e4b =  9:  dcf8bd477 ncrypt/crypt.c: Read the protected References and In-Reply-To header fields
10:  4c7fa7277 = 10:  6e3f7afb7 ncrypt/crypt.c: Protect Date
11:  c8caeb850 = 11:  632a481b7 ncrypt/crypt.c: Read the protected Date

Protected header fields are part of the crypto message, which means the sender considers them part of the important data, and should not be carelessly weeded. If the user want to do it, allow them via this variable, but default to not weeding them. Link: <neomutt#4223> Link: <neomutt#4226> Link: <neomutt#4227> Link: <neomutt#4236> Link: <neomutt#4237> Cc: Richard Russon <[email protected]> Reviewed-by: наб <[email protected]> Cc: Pietro Cerutti <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

alejandro-colomar · 2024-05-02T10:39:49Z

v14d changes:

Rebase

$ git range-diff main..gh/protect devel/security..protect 
 1:  4b9fa065c =  1:  8403fe9e7 ncrypt/crypt.c: Protect address lists in header fields
 2:  cc9b518ee =  2:  383f207d3 list: Add mutt_list_copy_tail()
 3:  66030fe81 =  3:  eb8f77ee4 test/: Add tests for mutt_list_copy_tail()
 4:  0f065acc9 =  4:  ae42ee341 ncrypt/crypt.c: Protect References and In-Reply-To
 5:  9f05a19de =  5:  eb3199e58 ncrypt/crypt.c: Small refactor
 6:  279f61f5f =  6:  442a0a462 ncrypt/crypt.c: Read the protected header fields containing address lists
 7:  6fc0bc5ec =  7:  ca4cbf8db list: Add mutt_list_write()
 8:  b2a3da831 =  8:  63ac6a413 test/: Add tests for mutt_list_write()
 9:  dcf8bd477 =  9:  ec4ec416e ncrypt/crypt.c: Read the protected References and In-Reply-To header fields
10:  6e3f7afb7 = 10:  f52e433e1 ncrypt/crypt.c: Protect Date
11:  632a481b7 = 11:  2e16cd692 ncrypt/crypt.c: Read the protected Date

... except Bcc, for obvious reasons. There are several attacks that are possible if the To, Cc, and similar header fields are not protected. One of them is: - The metadata of a signed+encrypted message can be modified (without decrypting it), to add a malicious recipient. Subsequent replies to the thread will likely also encrypt to the malicious recipient, disclosing secret data. Read the links below for a more detailed explanation of the problem. Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

This function copies a src ListHead at the tail of a dst ListHead. This is similar to mutt_addrlist_copy(), but for ListHead. Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

Signed-off-by: Alejandro Colomar <[email protected]>

Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

Move some code out of a conditional, to add support for other protected headers in the following commits. Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

…ists Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

This function writes a list to a buffer. It is similar to mutt_addrlist_write(), but simpler, and for ListHead. Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

Signed-off-by: Alejandro Colomar <[email protected]>

…fields Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

mutt(1) has been protecting it for several years already, and it is a user-facing header field, so it is one of the fields that are recommended to be protected. It's less critical than address lists or In-Reply-To, but it's still good to do it. Signed-off-by: Alejandro Colomar <[email protected]>

Signed-off-by: Alejandro Colomar <[email protected]>

Protected header fields are part of the crypto message, which means the sender considers them part of the important data, and should not be carelessly weeded. If the user want to do it, allow them via this variable, but default to not weeding them. Link: <neomutt#4223> Link: <neomutt#4226> Link: <neomutt#4227> Link: <neomutt#4236> Link: <neomutt#4237> Cc: Richard Russon <[email protected]> Reviewed-by: наб <[email protected]> Cc: Pietro Cerutti <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

flatcap · 2024-05-08T08:08:50Z

clang-format
rebased over [devel/security]

Protected header fields are part of the crypto message, which means the sender considers them part of the important data, and should not be carelessly weeded. If the user want to do it, allow them via this variable, but default to not weeding them. Link: <neomutt#4223> Link: <neomutt#4226> Link: <neomutt#4227> Link: <neomutt#4236> Link: <neomutt#4237> Cc: Richard Russon <[email protected]> Reviewed-by: наб <[email protected]> Cc: Pietro Cerutti <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

Protected header fields are part of the crypto message, which means the sender considers them part of the important data, and should not be carelessly weeded. If the user want to do it, allow them via this variable, but default to not weeding them. Link: <#4223> Link: <#4226> Link: <#4227> Link: <#4236> Link: <#4237> Cc: Richard Russon <[email protected]> Reviewed-by: наб <[email protected]> Cc: Pietro Cerutti <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

alejandro-colomar · 2024-05-08T09:48:03Z

Thanks! I'll start using devel/security as my every-day MUA. :-)

alejandro-colomar force-pushed the protect branch 3 times, most recently from 0bed7b7 to 9053b87 Compare March 31, 2024 23:32

alejandro-colomar force-pushed the protect branch from 6b84a76 to 641584c Compare April 1, 2024 15:08

alejandro-colomar force-pushed the protect branch 2 times, most recently from a089388 to 9bcfe82 Compare April 1, 2024 19:20

flatcap mentioned this pull request Apr 5, 2024

Should To and Cc header fields be protected? #4223

Open

alejandro-colomar mentioned this pull request Apr 6, 2024

BCC leakage in encrypted mail #4234

Open

alejandro-colomar added the topic:security Security issue label Apr 6, 2024

alejandro-colomar force-pushed the protect branch 2 times, most recently from 6236c62 to a2a1eb5 Compare April 8, 2024 12:49

alejandro-colomar marked this pull request as ready for review April 8, 2024 12:50

alejandro-colomar requested a review from a team as a code owner April 8, 2024 12:50

This was linked to issues Apr 8, 2024

Should To and Cc header fields be protected? #4223

Open

Should the In-Reply-To header field be protected? #4226

Open

alejandro-colomar mentioned this pull request Apr 8, 2024

crypt_protected_headers_write should be removed, or at least default to yes #4236

Open

alejandro-colomar commented Apr 8, 2024

View reviewed changes

ncrypt/crypt.c Outdated Show resolved Hide resolved

alejandro-colomar force-pushed the protect branch from 1e7fded to 9a0fcce Compare April 8, 2024 21:36

alejandro-colomar force-pushed the protect branch from 9a0fcce to d0010ae Compare April 8, 2024 21:46

alejandro-colomar linked an issue Apr 8, 2024 that may be closed by this pull request

crypt_protected_headers_write should be removed, or at least default to yes #4236

Open

alejandro-colomar force-pushed the protect branch from 196cd48 to 7187ac7 Compare May 1, 2024 12:44

alejandro-colomar force-pushed the protect branch from 7187ac7 to c8caeb8 Compare May 1, 2024 12:49

alejandro-colomar force-pushed the protect branch from c8caeb8 to 632a481 Compare May 1, 2024 13:04

alejandro-colomar requested a review from flatcap May 1, 2024 13:06

alejandro-colomar force-pushed the protect branch from 632a481 to 2e16cd6 Compare May 2, 2024 10:39

alejandro-colomar added 11 commits May 8, 2024 08:22

test/: Add tests for mutt_list_copy_tail()

61e09ca

Signed-off-by: Alejandro Colomar <[email protected]>

ncrypt/crypt.c: Protect References and In-Reply-To

e025442

Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

ncrypt/crypt.c: Small refactor

3ab07f2

Move some code out of a conditional, to add support for other protected headers in the following commits. Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

ncrypt/crypt.c: Read the protected header fields containing address l…

00773c9

…ists Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

list: Add mutt_list_write()

12855dc

This function writes a list to a buffer. It is similar to mutt_addrlist_write(), but simpler, and for ListHead. Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

test/: Add tests for mutt_list_write()

0cc8ce2

Signed-off-by: Alejandro Colomar <[email protected]>

ncrypt/crypt.c: Read the protected References and In-Reply-To header …

d4db1b9

…fields Link: <neomutt#4223> Link: <neomutt#4226> Cc: Richard Russon <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

ncrypt/crypt.c: Read the protected Date

9a20f5e

Signed-off-by: Alejandro Colomar <[email protected]>

flatcap force-pushed the protect branch from 2e16cd6 to 9a20f5e Compare May 8, 2024 08:07

flatcap approved these changes May 8, 2024

View reviewed changes

flatcap merged commit 61d32fa into neomutt:devel/security May 8, 2024
4 of 8 checks passed

alejandro-colomar deleted the protect branch May 8, 2024 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect more header fields #4227

Protect more header fields #4227

alejandro-colomar commented Mar 31, 2024 •

edited

alejandro-colomar commented Mar 31, 2024

flatcap commented Apr 1, 2024

alejandro-colomar commented Apr 1, 2024 via email

alejandro-colomar commented Apr 1, 2024

alejandro-colomar commented Apr 1, 2024

alejandro-colomar commented Apr 8, 2024

alejandro-colomar commented Apr 8, 2024 •

edited

alejandro-colomar commented Apr 8, 2024 •

edited

alejandro-colomar commented Apr 8, 2024

alejandro-colomar commented Apr 8, 2024 •

edited

alejandro-colomar commented May 1, 2024

alejandro-colomar commented May 1, 2024

alejandro-colomar commented May 1, 2024

alejandro-colomar commented May 2, 2024

flatcap commented May 8, 2024

alejandro-colomar commented May 8, 2024

Protect more header fields #4227

Protect more header fields #4227

Conversation

alejandro-colomar commented Mar 31, 2024 • edited

alejandro-colomar commented Mar 31, 2024

flatcap commented Apr 1, 2024

alejandro-colomar commented Apr 1, 2024 via email

alejandro-colomar commented Apr 1, 2024

alejandro-colomar commented Apr 1, 2024

alejandro-colomar commented Apr 8, 2024

alejandro-colomar commented Apr 8, 2024 • edited

alejandro-colomar commented Apr 8, 2024 • edited

alejandro-colomar commented Apr 8, 2024

alejandro-colomar commented Apr 8, 2024 • edited

alejandro-colomar commented May 1, 2024

alejandro-colomar commented May 1, 2024

alejandro-colomar commented May 1, 2024

alejandro-colomar commented May 2, 2024

flatcap commented May 8, 2024

alejandro-colomar commented May 8, 2024

alejandro-colomar commented Mar 31, 2024 •

edited

alejandro-colomar commented Apr 8, 2024 •

edited

alejandro-colomar commented Apr 8, 2024 •

edited

alejandro-colomar commented Apr 8, 2024 •

edited