[prev in list] [next in list] [prev in thread] [next in thread] 

List:       exim-users
Subject:    Re: [exim] De-tainting with ${sg} expansion
From:       Jamie Barnes via Exim-users <exim-users () exim ! org>
Date:       2020-07-31 2:06:03
Message-ID: b7348ba4-d291-4e77-aefd-47c6d35c0d99 () www ! fastmail ! com
[Download RAW message or body]

(Apologies if this doesn't thread properly -- I am writing via email and reading via \
Lurker...)

On 27/07/2020 19:45, Jamie Barnes via Exim-users wrote:
> > I've been avoiding check_local_user (since it tries to chdir into home \
> > directories that the exim user has no access to), so I don't think I have access \
> > to $local_part_data (as nothing populates it).

On 2020-07-28 20:25 +100, Jeremy Harris via Exim-users wrote:
> Not so. Any lookup done by a local_parts= condition on a router also sets it.

That's nice to know, thanks, but I'm not doing any lookup in a `local_parts` router \
directive either.  I'm literally storing everything that comes in, in a \
domain/localpart folder tree.

> You didn't show the routers calling these transports...
> Transports are only run when called by a router...

There were a lot of routers, with a lot of logic in between, and I didn't expect \
everyone to attempt to parse it all.  In summary, when I was putting it all together \
originally, I short-circuited all of that routing logic for mail sent locally (e.g. \
by daemons to the root user), so I have this one first...

store_local_user:
  driver                        = accept
  condition                     = ${if bool{$acl_c_islocal}}
  transport                     = local_delivery
  log_as_local
  no_more

...and then, as a fallback to catch all the places where logic hasn't routed \
something, the last router is...

unhandled:
  driver                        = accept
  transport                     = local_unhandled
  verify                        = false
  no_more

> > local_delivery:
> > driver                        = appendfile
> > file                          = ${if or{{bool{$acl_m_localdiscard}}           \
> > {bool{${lookup{$local_part}           \
> > lsearch{/etc/passwd}    \
> > {no}                    \
> > {yes}}}}}               \
> > {/dev/null}                              \
> > {/var/spool/mail/${sg{$local_part}{BADFILECHARS}{_}}}}
> 
> The discard invoked by the /dev/null choice could be done by a router (redirect, to \
> :blackhole:). Doing that would simplify this to
> 
> file = /var/spool/mail/${sg{$local_part}{BADFILECHARS}{_}}
> 
> ... and then we can detaint easily with a lookup rather than the ${sg}
> 
> file = /var/spool/mail/${lookup{$local_part}lsearch,ret=key{/etc/passwd}}
> 
> (whether you need any further cleaning depends on your policies on username \
> creation).

To be honest, despite the documentation, I've never really understood the redirect \
router.  If there is any reasonable gain to be had from switching this logic to \
redirect, I might re-evaluate it. 

> Depending on the router calling this transport, you might be able to do the
> lookup there, populating $local_part_data for use here.
> 
> > user                          = ${if or{{bool{$acl_m_localdiscard}}           \
> > {eqi {$local_part}{root}}             \
> > {bool{${lookup{$local_part}           \
> > lsearch{/etc/passwd}   \
> > {no}                   \
> > {yes}}}}}              \
> > {mail}                                   \
> > {$local_part}}
> 
> This looks like it uses the same lookup or value as the file= option.

It is.  The irony is that local mail is the least of my worries here, but Exim wasn't \
originally the server's default system mail handler and this was mostly an attempt to \
get that facility back when we switched away from Postfix.  It's just that the local \
mail was clogging up the queue (defers, because of the Tainted errors).

> local_unhandled:
> driver                        = appendfile
> create_directory              = yes
> directory                     = /var/spool/exim/unhandled/\
> ${sg{$domain}{BADFILECHARS}{_}}/\
> ${sg{$local_part}{BADFILECHARS}{_}}/\
> $tod_logfile
> 
> This transport is much harder to deal with. Currently you have to use
> one of the gross hacks presented as a get-out-of-jail-free, to
> obtain untainted strings for that path. I hate them, but there were
> always going to be ways for shooting oneself in the foot.
> 
> I fully expect them to be cargo-culted, probably via serverfault.
> 
> Hint: Use a lookup which matches anything, and returns the key.

I missed the fact that Exim-users is moderated, so after not seeing my email in \
Lurker, I went back over the documentation after posting and came up with adding the \
below to the above routers: 

set = r_fs_local_part=${lookup{${sg{$local_part}{BADFILECHARS}{_}}}\
                        lsearch*,ret=key{$config_dir/detaint}}
set = r_fs_domain=${lookup{${sg{$domain}{BADFILECHARS}{_}}}\
                    lsearch*,ret=key{$config_dir/detaint}}
(where /detaint is a file containing `*`)

I suppose I could use a macro, such as...

SAFELOCALPART = ${lookup{${sg{$local_part}{BADFILECHARS}{_}}} \
lsearch*,ret=key{$config_dir/detaint}} SAFEDOMAIN = \
${lookup{${sg{$domain}{BADFILECHARS}{_}}} lsearch*,ret=key{$config_dir/detaint}}

...if I find myself needing it more often?

I assume this is one of the gross hacks you are referring to (it certainly felt like \
one to me, when I wrote it), but it appears to be working as expected.  If there is \
something dangerous I'm missing with the above approach (assuming I trust my \
BADFILECHARS regex macro), I would appreciate learning it.

> I'm not happy with the situation either - but I've not been able to
> think of a principled way of dealing with it. I do not regard the
> above hack as principled because there are zero limits on it.
> 
> > Is ${sg} not a suitable expansion to de-taint $local_part or $domain?
> 
> No - because there are no limits on it, and I see no way for Exim
> to interpret the replacement pattern in a $(sg) to infer useful limits.

Do you really have to infer useful limits, rather than implementing a sane default \
and just documenting the risk?  There's always going to be someone who has a \
different definition of "useful".  

I guess I originally came at this from a Perl perspective, where a similar "taint" \
concept exists.  They also suffer their concerns with dangerous patterns (and the \
possibility of a generic detainting regex pattern, such as `s/(.+)/$1/g`) but, at the \
end of the day, they document the risk and then trust their end-users to do the right \
thing regarding their patterns.  

I appreciate Exim is not Perl but, since someone is always going to attempt to work \
around this, maybe ${sg} offers a less-gross hack than a hard-to-parse/reason-about \
${lookup} with an external file dependency.  
> Possibly some form of operator which had the semantics of
> "anything below this given path is fair game" would work. But it
> could not be a general de-tainting string expansion, because taint
> not only prevents use as a file path but also prevents further
> expansion.

Maybe just an operator ${safe:$...}, which is a shortcut for \
${sg{...}{PREDEFINED_REGEX}{_}}...?  I've been using `[A-Za-z0-9._-]`, but that's \
just because I know where I'm using it and why (...he says!)

> -- 
> Cheers,
> Jeremy 

Thanks for your help.

-- 
## List details at https://lists.exim.org/mailman/listinfo/exim-users
## Exim details at http://www.exim.org/
## Please use the Wiki with this list - http://wiki.exim.org/


[prev in list] [next in list] [prev in thread] [next in thread]