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, , 16);
if (qid[0] == '\0' || *ep != '|')
fatalx("Invalid reqid: %s", line);
Re: avoid truncation of filtered smtpd data lines
On Tue, 20 Jun 2023 21:38:49 +0200, Omar Polo wrote:
> Then I realized that we don't need to copy the line at all. We're
> already using strtoull to parse the number and the payload is the last
> field of the received line, so we can just advance the pointer. The
> drawback is that we now need to use a few strncmp, but I think it's
> worth it.
This seems like a good approach, minor comments inline.
- todd
> diff /usr/src
> commit - 5c586f5f5360442b12bbc4ea18ce006ea0c3d126
> path + /usr/src
> blob - a714446c26fee299f4450ff1ad40289b5b327824
> file + usr.sbin/smtpd/lka_filter.c
> --- usr.sbin/smtpd/lka_filter.c
> +++ usr.sbin/smtpd/lka_filter.c
> @@ -593,40 +593,27 @@ lka_filter_process_response(const char *name, const ch
> {
> uint64_t reqid;
> uint64_t token;
> - char buffer[LINE_MAX];
> char *ep = NULL;
> - char *kind = NULL;
> - char *qid = NULL;
> - /*char *phase = NULL;*/
> - char *response = NULL;
> - char *parameter = NULL;
> + const char *kind = NULL;
> + const char *qid = NULL;
> + const char *response = NULL;
> + const char *parameter = NULL;
> struct filter_session *fs;
>
> - (void)strlcpy(buffer, line, sizeof buffer);
> - if ((ep = strchr(buffer, '|')) == NULL)
> + kind = line;
> +
> + if ((ep = strchr(kind, '|')) == NULL)
> fatalx("Missing token: %s", line);
> - ep[0] = '\0';
> -
> - kind = buffer;
> -
> 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.
> reqid = strtoull(qid, , 16);
> - if (qid[0] == '\0' || *ep != '\0')
> + if (qid[0] == '\0' || *ep != '|')
> fatalx("Invalid reqid: %s", line);
> if (errno == ERANGE && reqid == ULLONG_MAX)
> fatal("Invalid reqid: %s", line);
>
> - qid = ep+1;
> - if ((ep = strchr(qid, '|')) == NULL)
> - fatal("Missing directive: %s", line);
> - ep[0] = '\0';
> -
> + qid = ep + 1;
> token = strtoull(qid, , 16);
> - if (qid[0] == '\0' || *ep != '\0')
> + if (qid[0] == '\0' || *ep != '|')
> fatalx("Invalid token: %s", line);
> if (errno == ERANGE && token == ULLONG_MAX)
> fatal("Invalid token: %s", line);
> @@ -637,7 +624,7 @@ lka_filter_process_response(const char *name, const ch
> if ((fs = tree_get(, reqid)) == NULL)
> return;
>
> - if (strcmp(kind, "filter-dataline") == 0) {
> + if (strncmp(kind, "filter-dataline|", 16) == 0) {
> if (fs->phase != FILTER_DATA_LINE)
> fatalx("filter-dataline out of dataline phase");
> filter_data_next(token, reqid, response);
> @@ -646,19 +633,13 @@ lka_filter_process_response(const char *name, const ch
> if (fs->phase == FILTER_DATA_LINE)
> fatalx("filter-result in dataline phase");
>
> - if ((ep = strchr(response, '|'))) {
> + if ((ep = strchr(response, '|')) != NULL)
> parameter = ep + 1;
> - ep[0] = '\0';
> - }
>
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;
> } 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);
>
avoid truncation of filtered smtpd data lines
hello tech@,
this was reported some time ago on the OpenSMTPD-portable repository[0]
[0]: https://github.com/OpenSMTPD/OpenSMTPD/pull/1192
Filters can register to the data-line event to alter the mail content.
smtpd, when parsing the filter' output it first copies the received
line in a temporary buffer. Here truncation may happen if a line of
the mail is longer than LINE_MAX (minus 50 to be exact).
The proposed diff on github bumps the local buffer to SMTP_LINE_MAX +
256, but I think it's quite wasteful to have a buffer that long on the
stack (SMTP_LINE_MAX is 65535 bytes).
A very simple fix could be:
: if (strcmp(kind, "filter-dataline") == 0) {
: if (fs->phase != FILTER_DATA_LINE)
: fatalx("filter-dataline out of dataline phase");
: - filter_data_next(token, reqid, response);
: + filter_data_next(token, reqid, line + (response - buffer));
: return;
: }
: if (fs->phase == FILTER_DATA_LINE)
since `line' contains the original line and it's guaranteed that the
fields of the reply excluding the last (variable width) fits the
buffer, it works. maybe it's too clever.
Then I realized that we don't need to copy the line at all. We're
already using strtoull to parse the number and the payload is the last
field of the received line, so we can just advance the pointer. The
drawback is that we now need to use a few strncmp, but I think it's
worth it.
disclaimer: i've run this only on localhost so far.
To reproduce, you can use an awk script like the following
#!/usr/bin/awk -f
# filter-dummy: copies the data lines as-is.
BEGIN { FS = "|" }
/^config\|ready$/ {
print "register|filter|smtp-in|data-line"
print "register|ready"
fflush()
}
"filter|smtp-in|data-line" == $1"|"$4"|"$5 {
line = substr($0, length($1$2$3$4$5$6$7) + 8)
printf "filter-dataline|%s|%s|%s\n", $6, $7, line
fflush()
}
with a configuration like:
table aliases file:/etc/mail/aliases
filter "dummy" proc-exec "/path/to/filter-dummy"
listen on lo0 filter "dummy"
action "local" mbox alias
match from local for local action "local"
and then sending a mail with a very long line:
% tr -cd '[:print:]' phase != FILTER_DATA_LINE)
fatalx("filter-dataline out of dataline phase");
filter_data_next(token, reqid, response);
@@ -646,19 +633,13 @@ lka_filter_process_response(const char *name, const ch
if (fs->phase == FILTER_DATA_LINE)
fatalx("filter-result in dataline phase");
- if ((ep = strchr(response, '|'))) {
+ if ((ep = strchr(response, '|')) != NULL)
parameter = ep + 1;
- ep[0] = '\0';
- }
if (strcmp(response, "proceed") == 0) {
- if (parameter != NULL)
- fatalx("Unexpected parameter after proceed: %s", line);
filter_protocol_next(token, reqid, 0);
return;
} 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);
