[PATCH 4/4] lib-mail: o_stream_dot_sendv() - Do not send unguarded '.' after bare...

Gbp-Pq: Name 0004-lib-mail-o_stream_dot_sendv-Do-not-send-unguarded-.-.patch

diff --git a/src/lib-mail/ostream-dot.c b/src/lib-mail/ostream-dot.c
index 81930a0c57cef2f579b162e8bd42b93c58ef7b01..51c8bcf8545d08aec762e7d4c441fe0a47a9a2bd 100644 (file)
--- a/src/lib-mail/ostream-dot.c
+++ b/src/lib-mail/ostream-dot.c
@@ -71,10 +71,12 @@ o_stream_dot_close(struct iostream_private *stream, bool close_parent)
                o_stream_close(dstream->ostream.parent);
 }
 
+#define ADD_MAX 3
 enum o_stream_dot_sendv_add {
        ADD_NONE,
        ADD_CR,
        ADD_DOT,
+       ADD_LF_DOT,
 };
 
 static ssize_t
@@ -111,14 +113,14 @@ o_stream_dot_sendv(struct ostream_private *stream,
 
                p = data;
                pend = CONST_PTR_OFFSET(data, size);
-               for (; p < pend && ((size_t)(p - data) + 2) <= max_bytes; p++) {
+               for (; p < pend && ((size_t)(p - data) + ADD_MAX) <= max_bytes; p++) {
                        enum o_stream_dot_sendv_add add = ADD_NONE;
 
                        size = pend - p;
                        switch (dstream->state) {
                        /* none */
                        case STREAM_STATE_NONE: {
-                               size_t maxlen = I_MIN(size, max_bytes - ((size_t)(p - data) + 2));
+                               size_t maxlen = I_MIN(size, max_bytes - ((size_t)(p - data) + ADD_MAX));
                                p = CONST_PTR_OFFSET(p, i_memcspn(p, maxlen, "\r\n", 2));
                                i_assert(p <= pend);
                                if (p == pend) {
@@ -145,6 +147,9 @@ o_stream_dot_sendv(struct ostream_private *stream,
                                case '\n':
                                        dstream->state = STREAM_STATE_CRLF;
                                        break;
+                               case '.':
+                                       add = ADD_LF_DOT;
+                                       /* fall through */
                                default:
                                        dstream->state = STREAM_STATE_NONE;
                                        break;
@@ -186,9 +191,31 @@ o_stream_dot_sendv(struct ostream_private *stream,
                                        max_bytes -= chunk;
                                        sent += chunk;
                                }
-                               /* insert byte (substitute one with pair) */
                                data++;
 
+                               /* Apply the modifications required according to RFC 5321 to the
+                                  stream :
+
+                                  ADD_DOT    - Apply standard SMTP dot-stuffing
+
+                                  ADD_CR     - Convert invalid lone LFs to CRLF to comply with
+                                               DATA line termination requirements, and avoid
+                                               potentially emitting bare "\n." sequences (same
+                                               threat as below for "\r." sequences).
+
+                                  ADD_LF_DOT - Guard against invalid "\r." sequences.
+                                               A downstream MTA that incorrectly handles bare CRs
+                                               as line terminators could interpret a '.' at the
+                                               start of the next line as end-of-DATA. This would
+                                               allow any following bytes to be processed as SMTP
+                                               commands.
+
+                                  ADD_CR and ADD_LF_DOT make the istream-dot round-trip lossy,
+                                  as these introduce bytes not present in the original input.
+                                  We accept this tradeoff, since preserving exact byte fidelity
+                                  for malformed input is less important than eliminating an
+                                  injection vector. */
+
                                switch(add) {
                                case ADD_DOT:
                                        iovn.iov_base = "..";
@@ -198,12 +225,17 @@ o_stream_dot_sendv(struct ostream_private *stream,
                                        iovn.iov_base = "\r\n";
                                        iovn.iov_len = 2;
                                        break;
+                               case ADD_LF_DOT:
+                                       iovn.iov_base = "\n..";
+                                       iovn.iov_len = 3;
+                                       break;
                                default:
                                        i_unreached();
                                }
 
                                array_push_back(&iov_arr, &iovn);
-                               i_assert(max_bytes >= iovn.iov_len);
+                               i_assert(iovn.iov_len <= ADD_MAX);
+                               i_assert(iovn.iov_len <= max_bytes);
                                max_bytes -= iovn.iov_len;
                                added += iovn.iov_len - 1;
                                sent++;

diff --git a/src/lib-mail/test-ostream-dot.c b/src/lib-mail/test-ostream-dot.c
index 6a1813d2469e5480a60fdafee374d07141f8990f..4e1b2978a9c618a89cb8911cd46d570f8bdbf112 100644 (file)
--- a/src/lib-mail/test-ostream-dot.c
+++ b/src/lib-mail/test-ostream-dot.c
@@ -57,6 +57,9 @@ static void test_ostream_dot(void)
                { "foo\n.\n", "foo\r\n..\r\n.\r\n" },
                { ".foo\r\n.\r\nfoo\r\n", "..foo\r\n..\r\nfoo\r\n.\r\n" },
                { ".foo\n.\nfoo\n", "..foo\r\n..\r\nfoo\r\n.\r\n" },
+               { ".", "..\r\n.\r\n" },
+               { "\r.", "\r\n..\r\n.\r\n" },
+               { "\r\r.", "\r\r\n..\r\n.\r\n" },
                { "\r\n", "\r\n.\r\n" },
                { "\n", "\r\n.\r\n" },
                { "", "\r\n.\r\n" },
@@ -92,7 +95,7 @@ static void test_ostream_dot_parent_almost_full(void)
        test_end();
 }
 
-static void test_ostream_dot_parent_exact_fit(void)
+static void test_ostream_dot_parent_max_bytes_boundary(void)
 {
        buffer_t *output_data;
        struct ostream *test_output, *output;
@@ -100,8 +103,8 @@ static void test_ostream_dot_parent_exact_fit(void)
 
        test_begin("dot ostream parent exact fit");
        output_data = t_buffer_create(1024);
-       test_output = test_ostream_create_nonblocking(output_data, 2);
-       test_ostream_set_max_output_size(test_output, 2);
+       test_output = test_ostream_create_nonblocking(output_data, 3);
+       test_ostream_set_max_output_size(test_output, 3);
 
        output = o_stream_create_dot(test_output, FALSE);
        ret = o_stream_send(output, ".", 1);
@@ -114,12 +117,72 @@ static void test_ostream_dot_parent_exact_fit(void)
        test_end();
 }
 
+/* STATE_CR must persist across sendv calls so that a bare CR ending one send
+   followed by '.' starting the next still triggers ADD_LF_DOT.
+   A stateless per-call implementation would emit the '.' unguarded and let a
+   downstream MTA that treats bare CR as EOL interpret it as end-of-DATA. */
+static void test_ostream_dot_cr_across_sendv(void)
+{
+       buffer_t *output_data;
+       struct ostream *test_output, *output;
+       ssize_t ret;
+       const char *expected = "\r\n..\r\n.\r\n";
+
+       test_begin("dot ostream CR across sendv calls");
+       output_data = t_buffer_create(1024);
+       test_output = o_stream_create_buffer(output_data);
+
+       output = o_stream_create_dot(test_output, FALSE);
+       ret = o_stream_send(output, "\r", 1);
+       test_assert(ret == 1);
+       ret = o_stream_send(output, ".", 1);
+       test_assert(ret == 1);
+       test_assert(o_stream_finish(output) > 0);
+
+       o_stream_unref(&output);
+       o_stream_unref(&test_output);
+
+       test_assert_ucmp(str_len(output_data), ==, strlen(expected));
+       test_assert_memcmp(str_c(output_data), str_len(output_data),
+       expected, strlen(expected));
+       test_end();
+}
+
+/* Off-by-one regression guard for the bare-CR + '.' injection.
+   Input "\r." places '.' at offset 1; the ADD_LF_DOT pair injects 3 bytes
+   ("\n.."). The loop guard check becomes (1 + 3) <= max_bytes, so max_bytes==4
+   is the exact-fit case. */
+static void test_ostream_dot_cr_dot_exact_fit(void)
+{
+       buffer_t *output_data;
+       struct ostream *test_output, *output;
+       ssize_t ret;
+
+       test_begin("dot ostream CR+dot exact fit");
+       output_data = t_buffer_create(1024);
+       test_output = test_ostream_create_nonblocking(output_data, 4);
+       test_ostream_set_max_output_size(test_output, 4);
+
+       output = o_stream_create_dot(test_output, FALSE);
+       ret = o_stream_send(output, "\r.", 2);
+       test_assert(ret == 2);
+       test_assert_ucmp(output_data->used, ==, 4);
+       test_assert_memcmp(output_data->data, output_data->used,
+       "\r\n..", 4);
+
+       o_stream_unref(&output);
+       o_stream_unref(&test_output);
+       test_end();
+}
+
 int main(void)
 {
        static void (*const test_functions[])(void) = {
                test_ostream_dot,
                test_ostream_dot_parent_almost_full,
-               test_ostream_dot_parent_exact_fit,
+               test_ostream_dot_parent_max_bytes_boundary,
+               test_ostream_dot_cr_across_sendv,
+               test_ostream_dot_cr_dot_exact_fit,
                NULL
        };
        return test_run(test_functions);