From: Omar Polo Subject: Re: [PATCH] relax 553 ORCPT address syntax error (was Re: EMails to "ORCPT=rfc822;user@example.co To: tech@openbsd.org Cc: Tassilo Philipp Date: Tue, 02 Jan 2024 14:08:12 +0100 friendly three-weekly bump :) On 2023/12/11 20:56:08 +0100, Omar Polo wrote: > [moving to tech@] > > This seems to be a long-standing issue in opensmtpd. There seem to be > (at least) one fairly popular application that uses a, upon a first > look, odd ORCPT parameter: > > > >>> On Fri, Oct 28, 2022 at 04:16:36PM +0200, Tim Kuijsten wrote: > > >>>> I have refined and more thoroughly tested a previous patch that > > >>>> relaxes the ORCPT address check. > > >>>> > > >>>> Over the years mail has been rejected by senders that use RCPT > > >>>> TO commands like: > > >>>> RCPT TO: > > >>>> ORCPT=rfc822;groupwise-info@example.com:0:0 or RCPT > > >>>> TO: > > >>>> ORCPT=rfc822;groupwise-info@example.com:0:0 > > >>>> NOTIFY=SUCCESS,FAILURE > > >>>> > > >>>> In the above the domain part of the ORCPT address resolves to > > >>>> example.com:0:0 which is rejected by smtpd with the message: > > >>>> smtpd[20797]: 1a3a396cd4c57d05 smtp failed-command command="RCPT > > >>>> TO: > > >>>> ORCPT=rfc822;groupwise-info@example.com:0:0 > > >>>> NOTIFY=SUCCESS,FAILURE" result="553 ORCPT address syntax error" > > >>>> > > >>>> [...] > > Quoting RFC 3461, the ORCPT parameter is defined as (ΒΆ 4.2) > > orcpt-parameter = "ORCPT=" original-recipient-address > original-recipient-address = addr-type ";" xtext > addr-type = atom > > The "addr-type" portion MUST be an IANA-registered electronic mail > address-type (as defined in [3]), while the "xtext" portion contains > an encoded representation of the original recipient address using the > rules in section 5 of this document. > > [...] > > The "addr-type" portion of the original-recipient-address is used to > indicate the "type" of the address which appears in the ORCPT > parameter value. However, the address associated with the ORCPT > keyword is NOT constrained to conform to the syntax rules for that > "addr-type". > > I'd like to emphasize the last sentence, which is the one I missed > upon the first couple of look at this RFC: even if addr-type is > rfc822, the xtext part (once decoded) is not constrained to conform to > the syntax rules of rfc822. > > That is, this groupwise thingy is actually confirming to the rfc when > using a param of "rfc822;groupwise-info@example.com:0:0". > > Honesty, I fail to understand the meaning of the addr-type if any > gargabe is allowed. > > The "once decoded" part is important since the RFC allows for ANY > character to be encoded, so rfc822;+45+78+62+6d+70+6c+65+40... > (i.e. example@...) is still a valid ORCPT param. > > Tassilo' diff (which is a slightly modified version of the one > originally sent in the bug report by Tim) is available here > > https://www.mail-archive.com/misc@opensmtpd.org/msg06054.html > > and it relaxes the checks but IMHO it doesn't address the underlying > issue: we expect a valid rfc822 address where it's not mandatory. > > I think we should just keep the ORCPT address as on opaque string > (modulo some validation) without trying to parse it. > > I still have to test the diff below in a real-world scenario, but I'm > sending it out so other can hopefully give it a spin. Other than Tassilo (thanks!) I've got this running on my instance too. > (and I couldn't resist splitting a long line in the process.) ok? diff /home/op/w/smtpd commit - 45a7eac758c7b1e9c4f16ab2dfcb25672b49aad9 path + /home/op/w/smtpd blob - c9611beb48feab47602b766061a7546c375160a8 file + envelope.c --- envelope.c +++ envelope.c @@ -443,7 +443,8 @@ ascii_load_field(const char *field, struct envelope *e return ascii_load_uint8(&ep->dsn_notify, buf); if (strcasecmp("dsn-orcpt", field) == 0) - return ascii_load_mailaddr(&ep->dsn_orcpt, buf); + return ascii_load_string(ep->dsn_orcpt, buf, + sizeof(ep->dsn_orcpt)); if (strcasecmp("dsn-ret", field) == 0) return ascii_load_dsn_ret(&ep->dsn_ret, buf); @@ -703,11 +704,8 @@ ascii_dump_field(const char *field, const struct envel if (strcasecmp(field, "dsn-ret") == 0) return ascii_dump_dsn_ret(ep->dsn_ret, buf, len); - if (strcasecmp(field, "dsn-orcpt") == 0) { - if (ep->dsn_orcpt.user[0] && ep->dsn_orcpt.domain[0]) - return ascii_dump_mailaddr(&ep->dsn_orcpt, buf, len); - return 1; - } + if (strcasecmp(field, "dsn-orcpt") == 0) + return ascii_dump_string(ep->dsn_orcpt, buf, len); if (strcasecmp(field, "dsn-envid") == 0) return ascii_dump_string(ep->dsn_envid, buf, len); blob - f0bb42c53ffb8f98012d542209bb777755ecd1ae file + mta.c --- mta.c +++ mta.c @@ -809,11 +809,8 @@ mta_handle_envelope(struct envelope *evp, const char * if (strcmp(buf, e->dest)) e->rcpt = xstrdup(buf); e->task = task; - if (evp->dsn_orcpt.user[0] && evp->dsn_orcpt.domain[0]) { - (void)snprintf(buf, sizeof buf, "%s@%s", - evp->dsn_orcpt.user, evp->dsn_orcpt.domain); - e->dsn_orcpt = xstrdup(buf); - } + if (evp->dsn_orcpt[0] != '\0') + e->dsn_orcpt = xstrdup(evp->dsn_orcpt); (void)strlcpy(e->dsn_envid, evp->dsn_envid, sizeof e->dsn_envid); e->dsn_notify = evp->dsn_notify; blob - 160b41d30b838d8cdd6be0db6ae9f3a6ac05a3dd file + mta_session.c --- mta_session.c +++ mta_session.c @@ -809,7 +809,7 @@ again: e->dest, e->dsn_notify ? " NOTIFY=" : "", e->dsn_notify ? dsn_strnotify(e->dsn_notify) : "", - e->dsn_orcpt ? " ORCPT=rfc822;" : "", + e->dsn_orcpt ? " ORCPT=" : "", e->dsn_orcpt ? e->dsn_orcpt : ""); } else mta_send(s, "RCPT TO:<%s>", e->dest); blob - 7848ece404a05d36d61525cb965b1a5550a9c048 file + smtp_session.c --- smtp_session.c +++ smtp_session.c @@ -2471,16 +2471,15 @@ smtp_tx_rcpt_to(struct smtp_tx *tx, const char *line) " combined with other options"); return; } - } else if (ADVERTISE_EXT_DSN(tx->session) && strncasecmp(opt, "ORCPT=", 6) == 0) { + } else if (ADVERTISE_EXT_DSN(tx->session) && + strncasecmp(opt, "ORCPT=", 6) == 0) { + size_t len = sizeof(tx->evp.dsn_orcpt); + opt += 6; - if (strncasecmp(opt, "rfc822;", 7) == 0) - opt += 7; - - if (!text_to_mailaddr(&tx->evp.dsn_orcpt, opt) || - !valid_localpart(tx->evp.dsn_orcpt.user) || - (strlen(tx->evp.dsn_orcpt.domain) != 0 && - !valid_domainpart(tx->evp.dsn_orcpt.domain))) { + if ((p = strchr(opt, ';')) == NULL || + !valid_xtext(p + 1) || + strlcpy(opt, tx->evp.dsn_orcpt, len) >= len) { smtp_reply(tx->session, "553 ORCPT address syntax error"); return; blob - 25090c3acf9980c0fbe22784a5242dae64c8ec53 file + smtpd.h --- smtpd.h +++ smtpd.h @@ -467,6 +467,7 @@ struct maddrmap { #define DSN_NEVER 0x08 #define DSN_ENVID_LEN 100 +#define DSN_ORCPT_LEN 500 #define SMTPD_ENVELOPE_VERSION 3 struct envelope { @@ -507,7 +508,7 @@ struct envelope { time_t nexttry; time_t lastbounce; - struct mailaddr dsn_orcpt; + char dsn_orcpt[DSN_ORCPT_LEN+1]; char dsn_envid[DSN_ENVID_LEN+1]; uint8_t dsn_notify; enum dsn_ret dsn_ret; @@ -1703,6 +1704,7 @@ int valid_localpart(const char *); int valid_domainpart(const char *); int valid_domainname(const char *); int valid_smtp_response(const char *); +int valid_xtext(const char *); int secure_file(int, char *, char *, uid_t, int); int lowercase(char *, const char *, size_t); void xlowercase(char *, const char *, size_t); blob - feb663cc61ec2572b57741fd694926f0c26967c7 file + util.c --- util.c +++ util.c @@ -531,6 +531,30 @@ valid_smtp_response(const char *s) } int +valid_xtext(const char *s) +{ + for (; *s != '\0'; ++s) { + if (*s < '!' || *s > '~' || *s == '=') + return 0; + + if (*s != '+') + continue; + + s++; + if (!isdigit((unsigned char)*s) && + !(*s >= 'A' && *s <= 'F')) + return 0; + + s++; + if (!isdigit((unsigned char)*s) && + !(*s >= 'A' && *s <= 'F')) + return 0; + } + + return 1; +} + +int secure_file(int fd, char *path, char *userdir, uid_t uid, int mayread) { char buf[PATH_MAX];