On Fri, Oct 04, 2019 at 08:43:28AM +0200, Martijn van Duren wrote:
> >> annoying bumps will just be moved around. If we *really* want to fix
> >> this we need to make it fit within the specifications:
> >>
> >> [...]
> >>
> >> This means stop opportunistic scanning for '\r' in iobuf!
> >>
> >
> > Sure but fixing iobuf is not a two liner and it affects virtually all of
> > the daemon and at this point we're looking for stability in the code, so
> > unless eric@ or you can come up with a diff that's trivial and that will
> > not affect any code paths beyond smtp client and filter getlines(), I'll
> > prefer a degraded margin case than taking the risk of a regression.
> >
> > I don't know iobuf enough to dive into this in a rush but if you managed
> > to pull a iobuf_getline_lf() that's fully isolated from rest of iobuf so
> > we could use that one in smtp_session.c and lka_proc.c, I'd be fine too,
> > and it would solve your concern.
> >
> >
> This whole ordeal was originally triggered by filter-dkimsign returning
> invalid signatures because additional \r were stripped. This is an
> uncommon usecase.
> The "fix" at the time was disallowing random '\r', which seemed to work
> for some time, but now it turns out that it breaks semarie's printer.
>
> If we don't want to rush anything and keep as many systems running as
> possible in a similar way we did before I propose we just revert the
> '\r' forbidden diff and accept that signatures are broken for clients
> that don't send valid mails until we fix this in a proper way.
>
> Doing it this way will keep things best effort running with minimal
> code that could cause confusion once we start fixing this.
>
> Index: smtp_session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> retrieving revision 1.414
> diff -u -p -r1.414 smtp_session.c
> --- smtp_session.c 3 Oct 2019 05:08:21 -0000 1.414
> +++ smtp_session.c 4 Oct 2019 06:41:58 -0000
> @@ -1109,15 +1109,6 @@ smtp_io(struct io *io, int evt, void *ar
> if (line == NULL)
> return;
>
> - if (strchr(line, '\r')) {
> - s->flags |= SF_BADINPUT;
> - smtp_reply(s, "500 %s <CR> is only allowed before <LF>",
> - esc_code(ESC_STATUS_PERMFAIL, ESC_OTHER_STATUS));
> - smtp_enter_state(s, STATE_QUIT);
> - io_set_write(io);
> - return;
> - }
> -
> /* Message body */
> eom = 0;
> if (s->state == STATE_BODY) {
I'm fine with that.
Alternatively, I took a look at what it would take to write an
iobuf_getline_nostrip() function which would be called _only_ in the
filtering code path.
The diff is not too invasive:
- introduce a iobuf_getline_nostrip() function so we don't touch the
iobuf_getline() function (it's used in the MTA layer too and we're
not getting anywhere close that), we can factor post release.
- the client input is read with iobuf_getline() which strips \r, but
the indirection through filters use iobuf_getline_nostrip(), which
will let the '\r', effectively stopping opportunistic strip as you
suggest.
Setup that don't have filters will go _exactly_ through old behavior
whereas setups with filters will have all \r preserved, keeping DKIM
ok.
I'm running with that diff right now.
I'm ok with either one of committing a degraded DKIM for trailing-\r
or going towards that diff.
comments ?
diff --git a/smtpd/iobuf.c b/smtpd/iobuf.c
index f5d8b20a..78a83cc6 100644
--- a/smtpd/iobuf.c
+++ b/smtpd/iobuf.c
@@ -184,6 +184,34 @@ iobuf_getline(struct iobuf *iobuf, size_t *rlen)
return (NULL);
}
+char *
+iobuf_getline_nostrip(struct iobuf *iobuf, size_t *rlen)
+{
+ char *buf;
+ size_t len, i;
+
+ buf = iobuf_data(iobuf);
+ len = iobuf_len(iobuf);
+
+ for (i = 0; i + 1 <= len; i++)
+ if (buf[i] == '\n') {
+ /* Note: the returned address points into the iobuf
+ * buffer. We NUL-end it for convenience, and discard
+ * the data from the iobuf, so that the caller doesn't
+ * have to do it. The data remains "valid" as long
+ * as the iobuf does not overwrite it, that is until
+ * the next call to iobuf_normalize() or iobuf_extend().
+ */
+ iobuf_drop(iobuf, i + 1);
+ buf[i] = '\0';
+ if (rlen)
+ *rlen = i;
+ return (buf);
+ }
+
+ return (NULL);
+}
+
void
iobuf_normalize(struct iobuf *io)
{
diff --git a/smtpd/iobuf.h b/smtpd/iobuf.h
index c454d0a1..e6e8023c 100644
--- a/smtpd/iobuf.h
+++ b/smtpd/iobuf.h
@@ -52,6 +52,7 @@ size_t iobuf_len(struct iobuf *);
size_t iobuf_left(struct iobuf *);
char *iobuf_data(struct iobuf *);
char *iobuf_getline(struct iobuf *, size_t *);
+char *iobuf_getline_nostrip(struct iobuf *, size_t *);
ssize_t iobuf_read(struct iobuf *, int);
ssize_t iobuf_read_tls(struct iobuf *, void *);
diff --git a/smtpd/ioev.c b/smtpd/ioev.c
index d36210fd..4b3733de 100644
--- a/smtpd/ioev.c
+++ b/smtpd/ioev.c
@@ -506,6 +506,12 @@ io_getline(struct io *io, size_t *sz)
return iobuf_getline(&io->iobuf, sz);
}
+char *
+io_getline_nostrip(struct io *io, size_t *sz)
+{
+ return iobuf_getline_nostrip(&io->iobuf, sz);
+}
+
void
io_drop(struct io *io, size_t sz)
{
diff --git a/smtpd/ioev.h b/smtpd/ioev.h
index f155a7fc..857f7a20 100644
--- a/smtpd/ioev.h
+++ b/smtpd/ioev.h
@@ -67,4 +67,5 @@ size_t io_queued(struct io *);
void* io_data(struct io *);
size_t io_datalen(struct io *);
char* io_getline(struct io *, size_t *);
+char* io_getline_nostrip(struct io *, size_t *);
void io_drop(struct io *, size_t);
diff --git a/smtpd/lka_filter.c b/smtpd/lka_filter.c
index 695ce4c3..1a7c6c39 100644
--- a/smtpd/lka_filter.c
+++ b/smtpd/lka_filter.c
@@ -399,7 +399,7 @@ filter_session_io(struct io *io, int evt, void *arg)
switch (evt) {
case IO_DATAIN:
nextline:
- line = io_getline(fs->io, &len);
+ line = io_getline_nostrip(fs->io, &len);
/* No complete line received */
if (line == NULL)
return;
@@ -653,7 +653,7 @@ filter_data_internal(struct filter_session *fs, uint64_t
token, uint64_t reqid,
/* no filter_entry, we either had none or reached end of chain */
if (filter_entry == NULL) {
- io_printf(fs->io, "%s\r\n", line);
+ io_printf(fs->io, "%s\n", line);
return;
}
diff --git a/smtpd/lka_proc.c b/smtpd/lka_proc.c
index f90d006f..c44a081f 100644
--- a/smtpd/lka_proc.c
+++ b/smtpd/lka_proc.c
@@ -152,7 +152,7 @@ processor_io(struct io *io, int evt, void *arg)
switch (evt) {
case IO_DATAIN:
- while ((line = io_getline(io, &len)) != NULL) {
+ while ((line = io_getline_nostrip(io, &len)) != NULL) {
if (strncmp("register|", line, 9) == 0) {
processor_register(name, line);
continue;
@@ -182,7 +182,7 @@ processor_errfd(struct io *io, int evt, void *arg)
switch (evt) {
case IO_DATAIN:
- while ((line = io_getline(io, &len)) != NULL)
+ while ((line = io_getline_nostrip(io, &len)) != NULL)
log_warnx("%s: %s", name, line);
}
}
diff --git a/smtpd/smtp_session.c b/smtpd/smtp_session.c
index 95db9099..2e38f5f7 100644
--- a/smtpd/smtp_session.c
+++ b/smtpd/smtp_session.c
@@ -1109,15 +1109,6 @@ smtp_io(struct io *io, int evt, void *arg)
if (line == NULL)
return;
- if (strchr(line, '\r')) {
- s->flags |= SF_BADINPUT;
- smtp_reply(s, "500 %s <CR> is only allowed before <LF>",
- esc_code(ESC_STATUS_PERMFAIL, ESC_OTHER_STATUS));
- smtp_enter_state(s, STATE_QUIT);
- io_set_write(io);
- return;
- }
-
/* Message body */
eom = 0;
if (s->state == STATE_BODY) {
@@ -2792,7 +2783,7 @@ filter_session_io(struct io *io, int evt, void *arg)
switch (evt) {
case IO_DATAIN:
nextline:
- line = io_getline(tx->filter, &len);
+ line = io_getline_nostrip(tx->filter, &len);
/* No complete line received */
if (line == NULL)
return;
--
Gilles Chehade @poolpOrg
https://www.poolp.org patreon: https://www.patreon.com/gilles
