Re: avoid truncation of filtered smtpd data lines
On Wed, 21 Jun 2023 19:11:09 +0200, Omar Polo wrote:
> On 2023/06/20 14:38:37 -0600, Todd C. Miller wrote:
> > > qid = ep+1;
> > > - if ((ep = strchr(qid, '|')) == NULL)
> > > - fatalx("Missing reqid: %s", line);
> > > - ep[0] = '\0';
> > > -
> >
> > This is not a new problem but we really should set errno=0 before
> > calling strtoull() for the first time. It is possible for errno
> > to already be set to ERANGE which causes problems if strtoull()
> > returns ULLONG_MAX and there is not an error. It's fine if you
> > don't want to fix that in this commit but I do think it should get
> > fixed.
>
> if you don't mind i'll fix it in a separate commit. I've also checked
> if there were other places to adjust but this appears to be the only
> one instance.
That's perfectly fine.
> > It took me a minute to realize that this is OK, but it seems fine.
> >
> > > if (strcmp(response, "proceed") == 0) {
> > > - if (parameter != NULL)
> > > - fatalx("Unexpected parameter after proceed: %s", line);
> > > filter_protocol_next(token, reqid, 0);
> > > return;
>
> yeah it's subtle, i should have pointed it out, sorry. if "response"
> contain a parameter, strcmp() return nonzero, so the parameter check
> is not needed.
>
> The drawback is that the error messages on protocol violation are a
> bit worst. Before something like "...|proceed|foobar" would fail with
> "unexpected parameter after proceed: ", now "Invalid directive:
> ", but I don't think it's a big deal.
I noticed this and I agree that it is not a big deal. If you are
writing a filter you should know enough to debug the problem with
the amount of detail provided.
OK millert@ for the diff as-is if it was not obvious from my previous
reply.
- todd
Re: avoid truncation of filtered smtpd data lines
On 2023/06/20 14:38:37 -0600, Todd C. Miller wrote:
> > qid = ep+1;
> > - if ((ep = strchr(qid, '|')) == NULL)
> > - fatalx("Missing reqid: %s", line);
> > - ep[0] = '\0';
> > -
>
> This is not a new problem but we really should set errno=0 before
> calling strtoull() for the first time. It is possible for errno
> to already be set to ERANGE which causes problems if strtoull()
> returns ULLONG_MAX and there is not an error. It's fine if you
> don't want to fix that in this commit but I do think it should get
> fixed.
if you don't mind i'll fix it in a separate commit. I've also checked
if there were other places to adjust but this appears to be the only
one instance.
> [...]
>
> It took me a minute to realize that this is OK, but it seems fine.
>
> > if (strcmp(response, "proceed") == 0) {
> > - if (parameter != NULL)
> > - fatalx("Unexpected parameter after proceed: %s", line);
> > filter_protocol_next(token, reqid, 0);
> > return;
yeah it's subtle, i should have pointed it out, sorry. if "response"
contain a parameter, strcmp() return nonzero, so the parameter check
is not needed.
The drawback is that the error messages on protocol violation are a
bit worst. Before something like "...|proceed|foobar" would fail with
"unexpected parameter after proceed: ", now "Invalid directive:
", but I don't think it's a big deal.
> > } else if (strcmp(response, "junk") == 0) {
> > - if (parameter != NULL)
> > - fatalx("Unexpected parameter after junk: %s", line);
> > if (fs->phase == FILTER_COMMIT)
> > fatalx("filter-reponse junk after DATA");
> > filter_result_junk(reqid);
> > @@ -667,11 +648,11 @@ lka_filter_process_response(const char *name, const ch
> > if (parameter == NULL)
> > fatalx("Missing parameter: %s", line);
> >
> > - if (strcmp(response, "rewrite") == 0)
> > + if (strncmp(response, "rewrite|", 8) == 0)
> > filter_result_rewrite(reqid, parameter);
> > - else if (strcmp(response, "reject") == 0)
> > + else if (strncmp(response, "reject|", 7) == 0)
> > filter_result_reject(reqid, parameter);
> > - else if (strcmp(response, "disconnect") == 0)
> > + else if (strncmp(response, "disconnect|", 11) == 0)
> > filter_result_disconnect(reqid, parameter);
> > else
> > fatalx("Invalid directive: %s", line);
> >
diff /usr/src
commit - f47faee0d8945111b03f2db500f23a23f37892bd
path + /usr/src
blob - f0429274168cddb3532c591c6fbc352e0ff7edda
file + usr.sbin/smtpd/lka_filter.c
--- usr.sbin/smtpd/lka_filter.c
+++ usr.sbin/smtpd/lka_filter.c
@@ -605,6 +605,8 @@ lka_filter_process_response(const char *name, const ch
if ((ep = strchr(kind, '|')) == NULL)
fatalx("Missing token: %s", line);
qid = ep+1;
+
+ errno = 0;
reqid = strtoull(qid, &ep, 16);
if (qid[0] == '\0' || *ep != '|')
fatalx("Invalid reqid: %s", line);
Re: [patch] usr.sbin/smtpd filter localhost relays
Hello,
sorry for the delay and thanks for the patch.
On 2023/02/28 12:16:17 +0100, Philipp 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 spa
Re: [s...@spacehopper.org: ospf6d fib reload [Re: bgpd fix for possible crash in SE]]
On Tue, Jun 20, 2023 at 05:31:34PM +0100, Stuart Henderson wrote:
> This hasn't blown up yet... any interest?
Some comments to the kroute.c changes below. Everything else is fine.
> - Forwarded message from Stuart Henderson -
>
> From: Stuart Henderson
> Date: Fri, 26 May 2023 14:40:45 +0100
> To: tech@openbsd.org
> Subject: ospf6d fib reload [Re: bgpd fix for possible crash in SE]
> Mail-Followup-To: tech@openbsd.org
>
> On 2023/05/26 13:52, Stuart Henderson wrote:
> > I think my main issues come around LS_REFRESH_TIME intervals, when
> > there's loads of churn and "ospf6d: ospf engine" can be busy for
> > minutes at a time (not always, but very often). Don't know if that rings
> > any bells for anyone... (I am now reminded that RTM_DESYNC isn't handled
> > by ospf6d which probably doesn't help matters).
>
> Here's a first attempt at porting the fib reload/desync diffs from
> ospfd to ospf6d ... Not sure if it's good yet, but it didn't immediately
> crash and burn when I ran "ospf6ctl fib reload", at least.
>
...
> Index: ospf6d/kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/ospf6d/kroute.c,v
> retrieving revision 1.67
> diff -u -p -r1.67 kroute.c
> --- ospf6d/kroute.c 8 Mar 2023 04:43:14 - 1.67
> +++ ospf6d/kroute.c 26 May 2023 13:37:55 -
> @@ -45,16 +45,22 @@ struct {
> u_int32_t rtseq;
> pid_t pid;
> int fib_sync;
> + int fib_serial;
> u_int8_tfib_prio;
> int fd;
> - struct eventev;
> + struct eventev, reload;
Please put reload on its own line.
> u_int rdomain;
> +#define KR_RELOAD_IDLE 0
> +#define KR_RELOAD_FETCH1
> +#define KR_RELOAD_HOLD 2
> + int reload_state;
> } kr_state;
>
> struct kroute_node {
> RB_ENTRY(kroute_node)entry;
> struct kroute_node *next;
> struct krouter;
> + int serial;
> };
>
> void kr_redist_remove(struct kroute_node *, struct kroute_node *);
> @@ -90,7 +96,10 @@ void if_announce(void *);
> int send_rtmsg(int, int, struct kroute *);
> int dispatch_rtmsg(void);
> int fetchtable(void);
> -int rtmsg_process(char *, size_t);
> +int refetchtable(void);
> +int rtmsg_process(char *, size_t);
> +void kr_fib_reload_timer(int, short, void *);
> +void kr_fib_reload_arm_timer(int);
>
> RB_HEAD(kroute_tree, kroute_node)krt;
> RB_PROTOTYPE(kroute_tree, kroute_node, entry, kroute_compare)
> @@ -165,6 +174,9 @@ kr_init(int fs, u_int rdomain, int redis
> kr_dispatch_msg, NULL);
> event_add(&kr_state.ev, NULL);
>
> + kr_state.reload_state = KR_RELOAD_IDLE;
> + evtimer_set(&kr_state.reload, kr_fib_reload_timer, NULL);
> +
> return (0);
> }
>
> @@ -374,6 +386,62 @@ kr_fib_decouple(void)
> }
>
> void
> +kr_fib_reload_timer(int fd, short event, void *bula)
> +{
> + if (kr_state.reload_state == KR_RELOAD_FETCH) {
> + kr_fib_reload();
> + kr_state.reload_state = KR_RELOAD_HOLD;
> + kr_fib_reload_arm_timer(KR_RELOAD_HOLD_TIMER);
> + } else {
> + kr_state.reload_state = KR_RELOAD_IDLE;
> + }
> +}
> +
> +void
> +kr_fib_reload_arm_timer(int delay)
> +{
> + struct timeval tv;
> +
> + timerclear(&tv);
> + tv.tv_sec = delay / 1000;
> + tv.tv_usec = (delay % 1000) * 1000;
> +
> + if (evtimer_add(&kr_state.reload, &tv) == -1)
> + fatal("add_reload_timer");
> +}
> +
> +void
> +kr_fib_reload(void)
> +{
> + struct kroute_node *krn, *kr, *kn;
> +
Maybe include the:
log_info("reloading interface list and routing table");
line from ospfd/kroute.c here as well.
> + kr_state.fib_serial++;
> +
> + if (fetchifs(0) != 0 || fetchtable() != 0)
> + return;
> +
> + for (kr = RB_MIN(kroute_tree, &krt); kr != NULL; kr = krn) {
> + krn = RB_NEXT(kroute_tree, &krt, kr);
> +
> + do {
> + kn = kr->next;
> +
> + if (kr->serial != kr_state.fib_serial) {
> +
> + if (kr->r.priority == RTP_OSPF) {
Here you should use kr_state.fib_prio instead of RTP_OSPF.
> + kr->serial = kr_state.fib_serial;
> + if (send_rtmsg(kr_state.fd,
> + RTM_ADD, &kr->r) != 0)
> + break;
> + } else
> + kroute_remove(kr);
> + }
> +
> + } while ((kr = kn) != NULL);
> + }
> +}
> +
> +void
> kr_fib_update_prio(u_int8_t fib_prio)
> {
> struct k
