On 2023/06/20 14:38:37 -0600, Todd C. Miller <[email protected]> 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: <line>", now "Invalid directive:
<line>", 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);
