Re: mail/exim and printf("%n")

Jeremie Courreges-Anglas <jca@wxcvbn.org> wrote:

> On Fri, Sep 03 2021, Renaud Allard <renaud@allard.it> wrote:
> > On 9/2/21 11:38 PM, Jeremie Courreges-Anglas wrote:
> >> On Thu, Sep 02 2021, Jeremie Courreges-Anglas <jca@wxcvbn.org> wrote:
> >>> On Thu, Sep 02 2021, "Theo de Raadt" <deraadt@openbsd.org> wrote:
> >>>> Jeremie Courreges-Anglas <jca@wxcvbn.org> wrote:
> >>>>
> >>>>>
> >>>>>
> >>>>> exim apparently uses printf("%n"), which is currently forbidden (libc
> >>>>> calls abort(3)).
> >>>>>
> >>>>> I don't want us to fix all the %n uses in the ports tree, but instead
> >>>>> wait for user reports. Though for some software like exim it makes
> >>>>> sense to help users avoid such a hard crash.
> >>>>>
> >>>>> The diff below doesn't pretend to fix all uses of %n in the exim source.
> >>>>> There may be others that can't be flagged by the compiler (support for
> >>>>> that hesn't been committed yet) because of indirections through wrapper
> >>>>> functions.
> >>>>> +--- src/acl.c.orig
> >>>>> ++++ src/acl.c
> >>>>> +@@ -2906,10 +2906,12 @@ for (; cb; cb = cb->next)
> >>>>> +
> >>>>> + HDEBUG(D_acl)
> >>>>> + {
> >>>>> +- int lhswidth = 0;
> >>>>> +- debug_printf_indent("check %s%s %n",
> >>>>> ++ uschar buf[256];
> >>>>> ++ int lhswidth = snprintf(CS buf, sizeof buf, "check %s%s ",
> >>>>> + (!conditions[cb->type].is_modifier && cb->u.negated)? "!":"",
> >>>>> +- conditions[cb->type].name, &lhswidth);
> >>>>> ++ conditions[cb->type].name);
> >>>>> ++ if (lhswidth == -1) lhswidth = 0;
> >>>>> ++ debug_printf_indent("%s");
> >>>>
> >>>> Doesn't this %s need an argument buf?
> >>>
> >>> Urkh, indeed, thanks. New diff below.
> >> Theo pointed out another problem in one of the two changes:
> >>
> >>> + uschar * s = string_sprintf("Return-path: <%.*s>\n%n",
> >> Here I forgot to actually drop the %n. clang still accepted it,
> >> probably because this function isn't marked as "printf-like".
> >> I'm not proposing another diff right now because I'm busy with other
> >> stuff, I'll get back to this when I have a clear mind.
> >>
> >
> > Actually, it seems there is more than one:
> > src/transport.c: uschar * s = string_sprintf("Return-path: <%.*s>\n%n",
> > src/rewrite.c: new = string_sprintf("%.*s%s%n%s",
>
> If I read correctly the source code, the formatting by string_sprintf
> and debug_printf_indent seem to be implemented by code in src/string.c,
> not by our libc formatting code. Therefore exim shouldn't reach the
> fatal abort(3) in our libc, therefore I'm dropping the whole diff.
> I suggest that you verify this claim by testing test exim on a -current
> system where printf("%n") aborts.

wow. shades of heartbleed.