On Fri, Oct 04, 2019 at 07:35:39AM +0200, Martijn van Duren wrote: > On 10/3/19 9:05 PM, Eric Faurot wrote: > > On Thu, Sep 19, 2019 at 05:48:17PM +0000, [email protected] wrote: > > > >>> To me, the only real problem with '\r' is at the end of lines. It's > >>> confusing > >>> since you never really know whether it's part of the content or the > >>> protocol. > >>> > >>> So I suggest that we strip all '\r' found at the end of a line, > >>> and retain the others. > >>> > >> > >> I'm not sure the only problem is at the end of lines, but I don't think > >> any solution that's graceful to bad clients will be correct so I'm okay > >> with your suggestion. > >> > > > > Here is a diff for that. > > Note that it strips the '\r' on all input, not just DATA. > > > > Eric. > > > > 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 3 Oct 2019 18:55:52 -0000 > > @@ -1109,14 +1109,9 @@ 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; > > - } > > + /* Strip trailing '\r' */ > > + while (len && line[len - 1] == '\r') > > + line[--len] = '\0'; > > > > /* Message body */ > > eom = 0; > > > See my previous mail. > This will break existing dkim signatures on mails which have trailing > '\r'. Even though clients must not send those, they apparently do. >
I agree with you for the most part but let's also put perspective here. This week, on my machines, out of 15k incoming and 14k outgoing e-mails only 2 mails had '\r' in the incoming path, 0 on the outgoing path, the 2 mails were spam. Not saying the '\r' issue doesn't need to be adressed but just pointing that it is a margin case and that the "quick fix" we want at this point in the release cycle must not take risks of regressions for everyone to cope with that margin case. > Do we > want to invalidate them as well? If that's the case, just remove this > check, because if the old signatures don't matter than the new signature > from filter-dkimsign (and others) won't matter as well. > I agree, both solutions break DKIM for trailing '\r' e-mails, but if we go that path I prefer that we handle this upfront rather than on filter returns. > Seriously, these solutions are like trying to fit a too large carpet, > 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. -- Gilles Chehade @poolpOrg https://www.poolp.org patreon: https://www.patreon.com/gilles
