David Zambonini
2017-Nov-01 17:34 UTC
Bug: lmtp proxy does not quote local parts with spaces
Hi again, I've not heard anything further regarding this bug, so I've had a look at the code. To restate the bug in a more precise way: LMTP in dovecot treats external RFC822 email addresses in the envelope recipient and internal usernames as almost identical/interchangeable. This is incorrect and leads to issues when attempting to use director as an LMTP proxy to proxy to recipients with quoted-local parts, as it is issuing invalid email addresses at the LMTP protocol level (it strips quotes from the local part and then does not add them again when proxying). It's also causing issues with director username hashing. I've created a "bugfix" patch to indicate what I mean, it appears to solve the issue, although I do not think it is anywhere near a production ready change. 1. The first problem is that dovecot is not performing a full decode on the envelope recipient address when creating the username, leading to escaped characters left in escaped form, and is not treating it consistently, choosing to either strip the surrounding quotes or not depending on whether or not it contains an @. I've fixed this by changing the code in lmtp_unescape_address() [src/lmtp/commands.c] to use rfc822_parse_quoted_string(). 2. This leads to the second problem where the username becomes ambiguous if the local-part contains an @ (regardless of whether or not the first fix is applied or not). I've worked around this by using strrchr() instead of strchr() on the username string to split the domain out in mail_user_hash() [src/lib-mail/mail-user-hash.c] and message_detail_address_parse() [src/lib-mail/message-address.c], although likely I've missed some place this change should be made. 3. The third problem is then re-encoding the username in the envelope recipient when proxying, which was not done at all. I've added a function lmtp_client_rfc822_escape_address(), which is similar to str_append_maybe_escape() to escape the address at protocol time in lmtp_client_send_rcpts() [src/lib-smtp/lmtp-client.c], although I suspect it should be done earlier, this is just a working proof. The other reason I don't believe this patch is production quality is that I have not examined any interaction between these changes and sieve's use of the envelope recipient. I'm hoping that a developer can chip in here? (hint!) (Apologies for top posting) On 30/10/2017 13:18, David Zambonini wrote:> On 26/10/2017 19:33, David Zambonini wrote: >> On 26/10/2017 18:38, Alexander Dalloz wrote: >>> Am 26.10.2017 um 12:20 schrieb David Zambonini: >>>> >>>> There seems to be a bug with RFC822 processing in ltmp proxying that >>>> doesn't >>>> quote local parts that, for example, contain spaces. >>> >>> Newer related RFCs are RFC 5321 and 5322. >> >> Typo, meant to say RFC2822, which they still supercede, not that the >> local-part spec has changed. :) >> >>> >>> [ ... ] >>> >>>> MAIL FROM:<test at testdomain.com>\r\n >>>> RCPT TO:<deemzed.uk+Junk E-mail>\r\n >>>> >>>> 501 5.5.4 Invalid.parameters\r\n >>> >>> That recipient address is totally invalid. It is neither just a local >>> part without a domain, nor a plussed address destination. >>> >>> Check your setup with i.e. >>> >>> RCPT TO:<"Junk E-mail"@deemzed.uk> >>> >>> or >>> >>> RCPT TO:<"test+Junk E-mail"@deemzed.uk> >> >> Apologies, I was attempting to cut the config down at the time the dump >> was taken. Correcting (I can provide config privately, but not share to >> list), I still get: >> >> MAIL FROM:<test at testdomain.com>\r\n >> RCPT TO:<"deemzed.uk+Junk E-mail"@mailbox.localhost>\r\n >> DATA\r\n >> (etc) >> .\r\n >> >> 501 5.5.4 Invalid parameters\r\n >> >> QUIT\r\n >> >> from director -> dovecot LMTP network dump: >> >> I could have a look at >> starting to get a fix together tomorrow with an aim to providing a pull >> request, if it turns out there are no side-effects to treating >> lmtp_rcpt.address like this and you'd like an example of what I mean. > > My apologies for not adding your address on my initial response, Alexander - not > sure if you noticed what I replied with or not. > > Nope, this isn't going to happen. I'm not familiar with the dovecot internals > but lmtp uses just the address string in the form of "full address with quotes > stripped from local part but otherwise not decoded" and nothing else throughout, > which touches on quite a bit of code. It makes it indeterminate and not always > possible to reassemble the original, it's a bit of a trainwreck. > > The sanest option to me seems to me to be to store a decoded local part and > domain in addition to the detail in mail_recipient, and keeping a now properly > rfc822 encoded address in sync with it. However, this would cause a deviation > from existing behaviour for the full original user (the quotes would be seen). > > I'm between a rock and a hard place here - at the very least I'd like this bug > to be officially recognised.-- David Zambonini -------------- next part -------------- --- dovecot-2.2.33.2.original/src/lib-mail/mail-user-hash.c 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2/src/lib-mail/mail-user-hash.c 2017-10-31 16:21:20.424866755 +0000 @@ -33,9 +33,13 @@ tab = t_malloc(sizeof(static_tab)); memcpy(tab, static_tab, sizeof(static_tab)); tab[0].value = username; - tab[1].value = t_strcut(username, '@'); - tab[2].value = strchr(username, '@'); - if (tab[2].value != NULL) tab[2].value++; + tab[2].value = strrchr(username, '@'); + if (tab[2].value != NULL) { + tab[1].value = t_strndup(username, tab[2].value - username); + tab[2].value++; + } else { + tab[1].value = username; + } var_expand(str, format, tab); md5_get_digest(str_data(str), str_len(str), md5); --- dovecot-2.2.33.2.original/src/lib-mail/message-address.c 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2/src/lib-mail/message-address.c 2017-10-31 10:12:50.185866755 +0000 @@ -540,7 +540,7 @@ if (*delimiter_string == '\0') return; - domain = strchr(address, '@'); + domain = strrchr(address, '@'); p = strstr(address, delimiter_string); if (p != NULL && (domain == NULL || p < domain)) { /* user+detail at domain */ --- dovecot-2.2.33.2.original/src/lib-smtp/Makefile.am 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2/src/lib-smtp/Makefile.am 2017-11-01 16:05:07.393866755 +0000 @@ -2,7 +2,8 @@ AM_CPPFLAGS = \ -I$(top_srcdir)/src/lib \ - -I$(top_srcdir)/src/lib-dns + -I$(top_srcdir)/src/lib-dns \ + -I$(top_srcdir)/src/lib/mail libsmtp_la_SOURCES = \ lmtp-client.c --- dovecot-2.2.33.2.original/src/lib-smtp/lmtp-client.c 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2/src/lib-smtp/lmtp-client.c 2017-11-01 16:09:28.384866755 +0000 @@ -8,6 +8,7 @@ #include "ostream.h" #include "str.h" #include "dns-lookup.h" +#include "rfc822-parser.h" #include "lmtp-client.h" #include <ctype.h> @@ -778,6 +779,73 @@ client->data_header = p_strdup(client->pool, str); } +/* similar to (private) str_append_maybe_escape() in lib-mail */ +static void lmtp_client_rfc822_escape_address(string_t *dest_r, const char *str) +{ + const char *p; + const char *domain; + + /* no strrchrnul */ + domain = strrchr(str, '@'); + if (domain == NULL) { + domain = str; + while (*domain != '\0') + domain++; + } + + /* see if we need to quote local part */ + for (p=str; p < domain; p++) { + if (!IS_ATEXT(*p)) + break; + } + + /* local part is atext */ + if (p == domain) { + str_append(dest_r, str); + return; + } + + /* need to quote the local part */ + str_append_c(dest_r, '"'); + + /* see if we need to escape */ + for (p=str; p < domain; p++) { + if (strchr("\t\"\r\n\\", *p)) + break; + } + + if (p == domain) { + /* only quote */ + str_append_data(dest_r, str, (size_t)(p - str)); + } else { + /* escape local-part */ + for (p=str; p < domain; p++) { + switch (*p) { + case '\t': + str_append(dest_r, "\\t"); + break; + case '"': + str_append(dest_r, "\\\""); + break; + case '\r': + str_append(dest_r, "\\r"); + break; + case '\n': + str_append(dest_r, "\\n"); + break; + case '\\': + str_append(dest_r, "\\\\"); + break; + default: + str_append_c(dest_r, *p); + } + } + } + + str_append_c(dest_r, '"'); + str_append(dest_r, domain); +} + static void lmtp_append_xtext(string_t *dest, const char *str) { unsigned int i; @@ -800,7 +868,9 @@ rcpt = array_get(&client->recipients, &count); for (i = client->rcpt_next_send_idx; i < count; i++) { str_truncate(str, 0); - str_printfa(str, "RCPT TO:<%s>", rcpt[i].address); + str_append(str, "RCPT TO:<"); + lmtp_client_rfc822_escape_address(str, rcpt[i].address); + str_append_c(str, '>'); if (rcpt->params.dsn_orcpt != NULL) { str_append(str, " ORCPT="); lmtp_append_xtext(str, rcpt->params.dsn_orcpt); --- dovecot-2.2.33.2.original/src/lmtp/commands.c 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2/src/lmtp/commands.c 2017-11-01 13:07:23.614866755 +0000 @@ -441,34 +441,18 @@ static const char *lmtp_unescape_address(const char *name) { + struct rfc822_parser_context parser; string_t *str; - const char *p; if (*name != '"') return name; - /* quoted-string local-part. drop the quotes unless there's a - '@' character inside or there's an error. */ + /* decode quoted-string local-part */ str = t_str_new(128); - for (p = name+1; *p != '"'; p++) { - if (*p == '\0') - return name; - if (*p == '\\') { - if (p[1] == '\0') { - /* error */ - return name; - } - p++; - } - if (*p == '@') - return name; - str_append_c(str, *p); - } - p++; - if (*p != '@' && *p != '\0') - return name; + rfc822_parser_init(&parser, (const unsigned char*)name, strlen(name), NULL); + rfc822_parse_quoted_string(&parser, str); + str_append(str, (const char*)parser.data); - str_append(str, p); return str_c(str); }
Stephan Bosch
2017-Nov-03 11:48 UTC
Bug: lmtp proxy does not quote local parts with spaces
Hi, Sorry, we're in a bit of a v2.3 merge frenzy. Much of the LMTP code will be replaced in v2.3, but I'll give the? older code a look as well. This can take a while though. Regards, Stephan. Op 1-11-2017 om 18:34 schreef David Zambonini:> Hi again, > > I've not heard anything further regarding this bug, so I've had a look at the code. > > To restate the bug in a more precise way: LMTP in dovecot treats external RFC822 > email addresses in the envelope recipient and internal usernames as almost > identical/interchangeable. This is incorrect and leads to issues when attempting > to use director as an LMTP proxy to proxy to recipients with quoted-local parts, > as it is issuing invalid email addresses at the LMTP protocol level (it strips > quotes from the local part and then does not add them again when proxying). It's > also causing issues with director username hashing. > > I've created a "bugfix" patch to indicate what I mean, it appears to solve the > issue, although I do not think it is anywhere near a production ready change. > > 1. The first problem is that dovecot is not performing a full decode on the > envelope recipient address when creating the username, leading to escaped > characters left in escaped form, and is not treating it consistently, choosing > to either strip the surrounding quotes or not depending on whether or not it > contains an @. I've fixed this by changing the code in lmtp_unescape_address() > [src/lmtp/commands.c] to use rfc822_parse_quoted_string(). > > 2. This leads to the second problem where the username becomes ambiguous if the > local-part contains an @ (regardless of whether or not the first fix is applied > or not). I've worked around this by using strrchr() instead of strchr() on the > username string to split the domain out in mail_user_hash() > [src/lib-mail/mail-user-hash.c] and message_detail_address_parse() > [src/lib-mail/message-address.c], although likely I've missed some place this > change should be made. > > 3. The third problem is then re-encoding the username in the envelope recipient > when proxying, which was not done at all. I've added a function > lmtp_client_rfc822_escape_address(), which is similar to > str_append_maybe_escape() to escape the address at protocol time in > lmtp_client_send_rcpts() [src/lib-smtp/lmtp-client.c], although I suspect it > should be done earlier, this is just a working proof. > > The other reason I don't believe this patch is production quality is that I have > not examined any interaction between these changes and sieve's use of the > envelope recipient. I'm hoping that a developer can chip in here? (hint!) > > (Apologies for top posting) > > On 30/10/2017 13:18, David Zambonini wrote: >> On 26/10/2017 19:33, David Zambonini wrote: >>> On 26/10/2017 18:38, Alexander Dalloz wrote: >>>> Am 26.10.2017 um 12:20 schrieb David Zambonini: >>>>> There seems to be a bug with RFC822 processing in ltmp proxying that >>>>> doesn't >>>>> quote local parts that, for example, contain spaces. >>>> Newer related RFCs are RFC 5321 and 5322. >>> Typo, meant to say RFC2822, which they still supercede, not that the >>> local-part spec has changed. :) >>> >>>> [ ... ] >>>> >>>>> MAIL FROM:<test at testdomain.com>\r\n >>>>> RCPT TO:<deemzed.uk+Junk E-mail>\r\n >>>>> >>>>> 501 5.5.4 Invalid.parameters\r\n >>>> That recipient address is totally invalid. It is neither just a local >>>> part without a domain, nor a plussed address destination. >>>> >>>> Check your setup with i.e. >>>> >>>> RCPT TO:<"Junk E-mail"@deemzed.uk> >>>> >>>> or >>>> >>>> RCPT TO:<"test+Junk E-mail"@deemzed.uk> >>> Apologies, I was attempting to cut the config down at the time the dump >>> was taken. Correcting (I can provide config privately, but not share to >>> list), I still get: >>> >>> MAIL FROM:<test at testdomain.com>\r\n >>> RCPT TO:<"deemzed.uk+Junk E-mail"@mailbox.localhost>\r\n >>> DATA\r\n >>> (etc) >>> .\r\n >>> >>> 501 5.5.4 Invalid parameters\r\n >>> >>> QUIT\r\n >>> >>> from director -> dovecot LMTP network dump: >>> >>> I could have a look at >>> starting to get a fix together tomorrow with an aim to providing a pull >>> request, if it turns out there are no side-effects to treating >>> lmtp_rcpt.address like this and you'd like an example of what I mean. >> My apologies for not adding your address on my initial response, Alexander - not >> sure if you noticed what I replied with or not. >> >> Nope, this isn't going to happen. I'm not familiar with the dovecot internals >> but lmtp uses just the address string in the form of "full address with quotes >> stripped from local part but otherwise not decoded" and nothing else throughout, >> which touches on quite a bit of code. It makes it indeterminate and not always >> possible to reassemble the original, it's a bit of a trainwreck. >> >> The sanest option to me seems to me to be to store a decoded local part and >> domain in addition to the detail in mail_recipient, and keeping a now properly >> rfc822 encoded address in sync with it. However, this would cause a deviation >> from existing behaviour for the full original user (the quotes would be seen). >> >> I'm between a rock and a hard place here - at the very least I'd like this bug >> to be officially recognised.
David Zambonini
2017-Nov-03 14:25 UTC
Bug: lmtp proxy does not quote local parts with spaces
On 03/11/2017 11:48, Stephan Bosch wrote:> Hi, > > Sorry, we're in a bit of a v2.3 merge frenzy. Much of the LMTP code will be > replaced in v2.3, but I'll give the? older code a look as well. > > This can take a while though.Thank you very much for getting back to me, I can appreciate it can get hectic, and I don't wish to appear ungrateful, I wholeheartedly endorse/recommend dovecot and the company I work for does use paid for OX elsewhere. For my own part, the platform I manage is > 300,000 mailboxes and dovecot performs incredibly well. I came up with some much smaller patches that accomplish the same thing in v2.2 using built-in functions and pushing the re-encoding slightly further up the call stack - address/username being interchangeable over most of the lmtp code makes significant changes problematic, so I thought it best not to try a rework. Looking at gitub, though, I don't see any significant changes in behaviour as far as the problem I'm seeing goes, which is worrying. What I'll do is leave the patches here for reference, and pick this up again after the v2.3 release. If you do have time for a further response, I could also provide them as pull requests against current on github if you'd like to request that. 1. Cut on the final instead of initial @ when splitting user/domain parts in LMTP, this can fix some issues where localpart contains a quoted @: dovecot-2.2.33.2-reverse-domaincut.patch 2. Fully decode local part on receipt in LMTP, and re-encode when proxying. This fixes the issue where quoted local quotes are stripped on proxy, preventing successful proxying, and some director hashing problems (exposes str_append_maybe_escape in message-address.h, some logging is still inconsistent, though, but would require a major rework): dovecot-2.2.33.2-quoted-local-proxy.patch -- David Zambonini -------------- next part -------------- --- dovecot-2.2.33.2/src/lib-mail/message-address.c 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2.quoted-local-proxy/src/lib-mail/message-address.c 2017-11-02 12:21:57.572866755 +0000 @@ -34,7 +34,7 @@ } /* quote with "" and escape all '\', '"' and "'" characters if need */ -static void str_append_maybe_escape(string_t *dest, const char *cstr, bool escape_dot) +void str_append_maybe_escape(string_t *dest, const char *cstr, bool escape_dot) { const char *p; --- dovecot-2.2.33.2/src/lib-mail/message-address.h 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2.quoted-local-proxy/src/lib-mail/message-address.h 2017-11-02 13:22:45.093866755 +0000 @@ -39,4 +39,7 @@ const char *address, const char **username_r, const char **detail_r); +/* quote with "" and escape all '\', '"' and "'" characters if need */ +void str_append_maybe_escape(string_t *dest, const char *cstr, bool escape_dot); + #endif --- dovecot-2.2.33.2/src/lmtp/commands.c 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2.quoted-local-proxy/src/lmtp/commands.c 2017-11-02 13:50:25.794866755 +0000 @@ -441,34 +441,18 @@ static const char *lmtp_unescape_address(const char *name) { + struct rfc822_parser_context parser; string_t *str; - const char *p; if (*name != '"') return name; - /* quoted-string local-part. drop the quotes unless there's a - '@' character inside or there's an error. */ + /* decode quoted-string local-part */ str = t_str_new(128); - for (p = name+1; *p != '"'; p++) { - if (*p == '\0') - return name; - if (*p == '\\') { - if (p[1] == '\0') { - /* error */ - return name; - } - p++; - } - if (*p == '@') - return name; - str_append_c(str, *p); - } - p++; - if (*p != '@' && *p != '\0') - return name; + rfc822_parser_init(&parser, (const unsigned char*)name, strlen(name), NULL); + rfc822_parse_quoted_string(&parser, str); - str_append(str, p); + str_append(str, (const char*)parser.data); return str_c(str); } --- dovecot-2.2.33.2/src/lmtp/lmtp-proxy.c 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2.quoted-local-proxy/src/lmtp/lmtp-proxy.c 2017-11-02 13:49:41.154866755 +0000 @@ -8,6 +8,7 @@ #include "ostream.h" #include "str.h" #include "time-util.h" +#include "message-address.h" #include "lmtp-client.h" #include "lmtp-proxy.h" @@ -288,6 +289,24 @@ lmtp_proxy_try_finish(conn->proxy); } +static char *lmtp_proxy_escape_address(pool_t pool, const char *address) { + const char *domain; + string_t *dest; + + domain = strrchr(address, '@'); + dest = str_new(pool, 128); + + if (domain == NULL) { + str_append_maybe_escape(dest, address, FALSE); + } else { + const char *local_part = t_strdup_until(address, domain); + str_append_maybe_escape(dest, local_part, FALSE); + str_append(dest, domain); + } + + return str_free_without_data(&dest); +} + int lmtp_proxy_add_rcpt(struct lmtp_proxy *proxy, const char *address, const struct lmtp_proxy_rcpt_settings *set) { @@ -301,10 +320,10 @@ rcpt = p_new(proxy->pool, struct lmtp_proxy_recipient, 1); rcpt->idx = array_count(&proxy->rcpt_to); rcpt->conn = conn; - rcpt->address = p_strdup(proxy->pool, address); + rcpt->address = lmtp_proxy_escape_address(proxy->pool, address); array_append(&proxy->rcpt_to, &rcpt, 1); - lmtp_client_add_rcpt_params(conn->client, address, &set->params, + lmtp_client_add_rcpt_params(conn->client, rcpt->address, &set->params, lmtp_proxy_conn_rcpt_to, lmtp_proxy_conn_data, rcpt); return 0; -------------- next part -------------- --- dovecot-2.2.33.2/src/lib-mail/mail-user-hash.c 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2.reverse-domaincut/src/lib-mail/mail-user-hash.c 2017-11-02 16:04:03.724866755 +0000 @@ -33,9 +33,13 @@ tab = t_malloc(sizeof(static_tab)); memcpy(tab, static_tab, sizeof(static_tab)); tab[0].value = username; - tab[1].value = t_strcut(username, '@'); - tab[2].value = strchr(username, '@'); - if (tab[2].value != NULL) tab[2].value++; + tab[2].value = strrchr(username, '@'); + if (tab[2].value != NULL) { + tab[1].value = t_strdup_until(username, tab[2].value); + tab[2].value++; + } else { + tab[1].value = username; + } var_expand(str, format, tab); md5_get_digest(str_data(str), str_len(str), md5); --- dovecot-2.2.33.2/src/lib-mail/message-address.c 2017-10-05 18:10:44.000000000 +0100 +++ dovecot-2.2.33.2.reverse-domaincut/src/lib-mail/message-address.c 2017-11-02 16:00:30.926866755 +0000 @@ -540,7 +540,7 @@ if (*delimiter_string == '\0') return; - domain = strchr(address, '@'); + domain = strrchr(address, '@'); p = strstr(address, delimiter_string); if (p != NULL && (domain == NULL || p < domain)) { /* user+detail at domain */