Hello, sorry for the delay and thanks for the patch.
On 2023/02/28 12:16:17 +0100, Philipp <philipp+open...@bureaucracy.de> wrote: > Hi > > On github someone reported an issue[0] regarding localhost MX entries. > Currently smtpd will just use the localhost relay. This leads to a > loop. Here a patch filtering localhost and localhost addresses for MX > requests. so the report is interesting. due to a typo, a mail was sent to a wrong host which happen to have "localhost" as MX. this makes smtpd loop for a while since it connects to localhost to forward the mail, and connections from localhost are per-config allowed to send mails to anyone. Rinse and repeat. after ~2 minutes (not tested myself, the actual number was provided in the github issue but seems fair) the loop detection (i.e. more than 100 Received headers) kicks in and the mail rejected. Now, to be honest I don't like the proposed patch. I'm not sure special-casing "localhost" (and its equivalent spellings) is good. We're adding code to handle a very fringe case which doesn't really have bad consequences (one envelope in the queue for a few minutes is not that of a big deal) and it's not even comprehensive. I don't see the gain. Nothing stops me now to change my MX after sending this email to match the MX of your server and cause you a similar loop when you'll reply. Or picking an ip in a similarly private blocks like in 192.168.0.0/16 as it's not that uncommon I believe to have a mx that relays mails from a LAN. However, I won't object if some other developer think this is good and wants to commit this or a variation. Since we're here, some style nits on the diff inlined below. > As next step you could implement Null-MX (rfc 7505). This could be worthwile, but wouldn't have helped at all in this case. It's something the owners of that domain should have used instead of putting "localhost" as MX. Patches to support Null-MX are welcome though. > Philipp > > [0] https://github.com/OpenSMTPD/OpenSMTPD/issues/1190 > > diff --git a/usr.sbin/smtpd/dns.c b/usr.sbin/smtpd/dns.c > index f7c6b3df..7389efec 100644 > --- a/usr.sbin/smtpd/dns.c > +++ b/usr.sbin/smtpd/dns.c > @@ -53,6 +53,7 @@ struct dns_lookup { > struct dns_session *session; > char *host; > int preference; > + int filter_localhost; > }; > > struct dns_session { > @@ -65,7 +66,7 @@ struct dns_session { > int refcount; > }; > > -static void dns_lookup_host(struct dns_session *, const char *, int); > +static void dns_lookup_host(struct dns_session *, const char *, int, int); > static void dns_dispatch_host(struct asr_result *, void *); > static void dns_dispatch_mx(struct asr_result *, void *); > static void dns_dispatch_mx_preference(struct asr_result *, void *); > @@ -139,7 +140,7 @@ dns_imsg(struct mproc *p, struct imsg *imsg) > case IMSG_MTA_DNS_HOST: > m_get_string(&m, &host); > m_end(&m); > - dns_lookup_host(s, host, -1); > + dns_lookup_host(s, host, -1, 0); > return; > > case IMSG_MTA_DNS_MX: > @@ -205,6 +206,28 @@ dns_imsg(struct mproc *p, struct imsg *imsg) > } > } > > +static int > +is_localhost(struct sockaddr *sa) > +{ > + struct sockaddr_in6 *ipv6; > + struct sockaddr_in *ipv4; > + uint32_t addr; > + > + switch (sa->sa_family) { > + case AF_INET6: > + // May check also for v6 mapped v4 addresses please don't use C++-style comments. > + ipv6 = (struct sockaddr_in6 *)sa; > + return IN6_IS_ADDR_LOOPBACK(&ipv6->sin6_addr); > + case AF_INET: > + ipv4 = (struct sockaddr_in *)sa; > + addr = ntohl(ipv4->sin_addr.s_addr); > + return ((addr >> 24) & 0xff) == 127; > + default: > + log_warnx("warn: unknown family in sockaddr"); > + } > + return 0; > +} > + > static void > dns_dispatch_host(struct asr_result *ar, void *arg) > { > @@ -215,6 +238,10 @@ dns_dispatch_host(struct asr_result *ar, void *arg) > s = lookup->session; > > for (ai = ar->ar_addrinfo; ai; ai = ai->ai_next) { > + if (lookup->filter_localhost && is_localhost(ai->ai_addr)) { > + log_warnx("warn: ignore localhost address for host %s", > lookup->host); even though is not always respected, please try to keep the lines under 80 columns.k > + continue; > + } > s->mxfound++; > m_create(s->p, IMSG_MTA_DNS_HOST, 0, 0, -1); > m_add_id(s->p, s->reqid); > @@ -280,14 +307,18 @@ dns_dispatch_mx(struct asr_result *ar, void *arg) > continue; > print_dname(rr.rr.mx.exchange, buf, sizeof(buf)); > buf[strlen(buf) - 1] = '\0'; > - dns_lookup_host(s, buf, rr.rr.mx.preference); > + if (strcasecmp("localhost", buf)==0) { nit, I'd use spaces around the comparison, like : + if (strcasecmp("localhost", buf) == 0) { > + log_warnx("ignore localhost MX-entry for domain <%s>", > lookup->host); > + continue; > + } > + dns_lookup_host(s, buf, rr.rr.mx.preference, 1); > found++; > } > free(ar->ar_data); > > /* fallback to host if no MX is found. */ > if (found == 0) > - dns_lookup_host(s, s->name, 0); > + dns_lookup_host(s, s->name, 0, 1); > } > > static void > @@ -340,7 +371,7 @@ dns_dispatch_mx_preference(struct asr_result *ar, void > *arg) > } > > static void > -dns_lookup_host(struct dns_session *s, const char *host, int preference) > +dns_lookup_host(struct dns_session *s, const char *host, int preference, int > filter_localhost) > { > struct dns_lookup *lookup; > struct addrinfo hints; > @@ -350,6 +381,7 @@ dns_lookup_host(struct dns_session *s, const char *host, > int preference) > > lookup = xcalloc(1, sizeof *lookup); > lookup->preference = preference; > + lookup->filter_localhost = filter_localhost; > lookup->host = xstrdup(host); > lookup->session = s; > s->refcount++;
