On 10/4/19 9:36 AM, Gilles Chehade wrote:
> 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 ?
>
This is similar to the diff I send a few months ago, but still doesn't
fix the case when someone sends a standalone '\n' as line-ending.
I'd prefer if we go with reverting (=my previous diff) until we have
something that definitively fixes things. If people send broken mails
they get broken signatures until that time.
