Re: Making pglister work with exim 4.96+

On Tue, Jun 18, 2024 at 10:19 AM Stefan Kaltenbrunner <
stefan(at)kaltenbrunner(dot)cc> wrote:

> On 17.06.24 22:59, Célestin Matte wrote:
> >> We have been briefly discussing that very issue last year and the
> >> consensus was basically going the environment variable route (which
> >> can also be implemented on older exim installs) - the above list is
> >> only the "default" set of environment variables available and we can
> >> add more.
> >
> > But using environment variable is just working around the problem by
> > evading the security mechanism. Documentation still warns about being
> > careful [1]. And given that exim keeps extending tainting to more
> > places, it's possible this solution will break in the future.
>
> personally I doubt that environment variable passing will ever be part
> of tainting - in effect that would mean that the environment feature
> would have to be dropped entirely and that would break compatibility
> with myriads of CLI tools expecting at least the default set of variables.
>

Yeah, and I don't see why they would? The reason they do the taint marking
in variables used in commands and filenames is that it would be a potential
venue for attackers to inject things. No such vulnerability exists with
environment variables. Obviously the receiving code, whether a shellscript
or a python program or a c program or whatever, can have injection
vulnerabilities of it's own, but the passing values layer (which is what
Exim is responsible for there) does not.

> > I think I found a good, yet hacky, workaround: using a pgsql lookup to
> > insert the values directly into the database. This way, we avoid passing
> > dangerous data through a shell, and we can escape them using
> > ${quote_pgsql}. Using event_action, I can execute this at the right time
> > (after delivery).
> > My current solution is something like this:
> > command = /pglister_path/web/pglister/bin/python
> > /pglister_path/bin/inject.py -d $local_part_data(at)$domain_data -m
> > $message_id -s ''
> > event_action = ${if eq {msg:delivery}{$event_name} {${lookup
> > pgsql{update incoming_mail set sender='${quote_pgsql:$sender_address}'
> > where messageid='${quote_pgsql:$message_id}'; notify incoming}} {}}}
> > and removing the "notify incoming" in inject.py.
> >
> > This still requires tweaking and adding the bounce case, but I think
> > it's a good start and tests are working so far.
> >
> > [1] :
> >
> https://www.exim.org/exim-html-current/doc/html/spec_html/ch-the_pipe_transport.html
> point 4
>
>
> I don't think we should use the event infrastructure for this - I have
> limited faith into the robustness of the event infrastructure for a use
> case like that and I envision various race conditions in failure cases
> especially during concurrent operations and failure cases.
>
> The msg:delivery event fires after delivery and is kinda independent on
> the (more or less) atomic operation of the inject.py script.
> In production we will have multiple inject.pl running in parallel and
> one delivery process might send the NOTIFY before the other even reaches
> the update.
> It also adds additional complexity for maintenance operations because a
> failure in running the event_action(say a database restart as part of a
> server reboot) AFTER the script ran might result in strange states.
>
>
Yeah, this seems extremely fragile. Concurrent delivery is a common thing,
and not the only potential problem I bet. The proper fix surely is to make
invoke.py work properly.

And the above doesn't actually solve the problem does it? It still requires
passing the message-id which is a tainted variable?

//Magnus