[Dnsmasq-discuss] [PATCH] Validate the tftp root directory option

Daniel Veillard veillard at redhat.com
Tue Jun 12 16:10:48 BST 2012


On Tue, Jun 12, 2012 at 03:14:33PM +0100, Simon Kelley wrote:
> On 12/06/12 04:57, Daniel Veillard wrote:
> >   Hi Simon,
> > 
> > in response to the following bug
> >   https://bugzilla.redhat.com/show_bug.cgi?id=824214
> 
> I can't access that bug, even after I registered with the buzilla.
> 
> > I made the small patch below checking the tftp root directory.
> > The only downside I could see would be if using a symlink for
> > the patch given on the command line as S_ISDIR() will not catch
> > this, I'm not sure it's worth adding readlink() complexity.
> > Also I wasn't sure the patch should reindent the full block.
> > 
> >   Any opinions ?
> 
> Two thinks come to mind.
> 
> 1) The test should be done after dnsmasq drop privs. There's no point
> checking directory access as root when you're going to do it for real as
> a non-privileged user. By the time priv drop has occurred, fatal errors
> are a bit more complex than just calling die(), but are possible.

  To be honnest we were more worrying about crafted CLI arguments given
and not really trying to check accessing the directory for real. In the
end dnsmasq would serve file out of that directory so the directory in
itself is just a small precondition of proper working of the
functionality. Here we would just do the simplest check and as soon as
possible. Then a variety of errors may still happen at runtime.
  My patch was trying to be relatively simple though clearly limited :-)

> 2) Would opendir() be a better test? That solves the symlink problem.

  Yes that's closer to the actual need. I could make a new patch doing
this instead if you prefer,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/



More information about the Dnsmasq-discuss mailing list