On 10/4/19 8:23 AM, Gilles Chehade wrote:
> 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.
>
>
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) {
