Mailing List Archive

On 15/08/2022 10:21, Martin D Kealey via Exim-dev wrote:
> Even though lstat alone would work (proving the existence of the parent
> directory, the ability to traverse it, and the existence of the target
> filename), dsearch_open calls exim_opendir, and then immediately closes it,
> and then fails because the directory lacks 'r' permission. Natch!
>
> The comment above dsearch_open says "We open the directory to test whether
> it exists and whether it is searchable", but that's not actually true,
> because "readable" and "searchable" are two different things.

Seems a reasonable request for change; please open an item
at https://bugs.exim.org/

> Is there any reason why dsearch_open shouldn't simply be an empty function
> that always succeeds?

The error message that it can return. Admittedly, we should probably modify
that to something like "cannot find dir %s" if we just lstat rather than
exim_opendir(). And we'll need an explicit taint check, like exim_opendir() does.
--
Cheers,
Jeremy

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

As noted earlier, dsearch current requires both read- and exec-permission
on the containing directory, but should really only require exec-permission.

I've created https://bugs.exim.org/show_bug.cgi?id=2916 relating to this,
and a potential solution as https://github.com/Exim/exim/pull/87

(I appreciate that github pull requests are not the preferred method of
submitting patches, so please advise if there is a more preferred method.)

In addition to the directory read permission issue, my patch also rounds
out the dsearch functionality in several other ways:
1. It uses fstatat where supported, to reduce the syscall count and avoid
race conditions;
2. it performs explicit taint checking before anything else, so that it's
not possible to infer filename (non)existence from varying error messages;
3. It allows you to check for the existence of a sentinel name within a
directory without using that name in the result (new "ret=dir" option).
4. It allows you to require a symlink. Initially I wrote just that, and
then I got bored and generalized the filetype matching process into a small
bit vector, so that it becomes possible to intentionally match on *any*
file type; you can even say "filter=tty" to require S_ISCHR(st_mode) if
you're getting info from a device over a USB serial link. A small set of
reusable library functions has been created to support name mapping for the
S_IFxxx macros.

I've yet to add test coverage. I've done manual testing using «
path/to/new/exim -be » but I'm unclear how to integrate those into the
existing test suite. Advice on that would be appreciated.

I've updated ChangeLog, NewStuff, README.UPDATING, and spec.xfpt, but I'm
not sure I've quite hit the mark on all those so feedback would be
appreciated.

-Martin

PS: TL;DR - look ma, no indenting ... AAARGHHH!

Having worked on all that, I've come to the conclusion that right-shifted
curly braces are b*%#*y disorientating but eventually manageable, whereas
un-indented function bodies are a perpetual gift from Beelzebub: they make
it impossible to see quickly which declarations are at file or auto scope,
and thoroughly obscure the starts and ends of functions. I tried running
the code through "indent", which did make it MUCH easier to read and edit,
but then it turned out to be impossible to convince indent to reverse the
process. If anyone has a magical ".indent.pro" that matches Exim's source
code layout, please post a copy. Failing that, could consideration be given
to choosing some sane settings for "indent" and then running the entire
codebase through it? Then everyone can edit the code in their preferred
layout, and then run it through "indent" to standardize the layout before
submitting patches.
--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##