Re: [libvirt] PATCH: 10/25: Remove use of strerror()
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
...
> Incidentally, virFileMakePath will have to go, eventually. Not
only
> is it recursive in the number of components of the name being created,
> but allocating PATH_MAX for each stack frame is very wasteful, and might
> make it easy to blow the stack/DoS -- if there's a way to make libvirtd
> call it with an abusive argument. Worst still, it creates directories
> with mkdir(path, 0777). No virFileMakePath caller is careful to chmod
> _all_ of the possibly-just-created directories, and even if they all were,
> there'd still be a race.
I'm not sure about the mkdir 0777 mode - it will be modulated by the
process umask which will typically be 002 (regular user) or 022 (root).
It may be worth checking the callers of this to see if this will be a
problem or not.
True, but creating leading directories with too-lenient
permissions is perhaps too harsh a penalty for running libvirtd
with an overly-permissive umask. Maybe we should just make
libvirtd examine the current umask and if it's too permissive,
just bail out. But even that might not work, since when
I'm running it as non-root it's impossible to know if I want
directories to be group-writable or not, in general.
As for the stack/recursion issue, the impl is fairly naive. It can
be
trivially re-written to do one single strdup() of the incoming path,
and then implement the whole thing using iteration instead of recursion.
Surprisingly, doing it _right_ is highly non-trivial.
Luckily, there's code in gnulib to do this. I think it's even
thread-safe. Though it's GPL'd, we should be able to fix that, if needed.
Finally, the malicious user supplied data issue - reasonably sure we
only use directories based off getpwuid / /etc/libvirt/libvirtd.conf
and don't allow user to supply arbitrary paths, but again worth checking
the callers.
I glanced through the callers, and they look ok, since
all names are from compiled-in constants or are read
from config files. This seems to be the sole exception:
src/storage_backend_fs.c: if (virFileMakePath(pool->def->target.path) < 0) {