[Openvas-devel] Race condition in openvassd

[Tim Brown]

On Wednesday 07 October 2009 06:45:09 Felix Wolfsteller wrote:
> On Tuesday 06 October 2009 23:52:25 Tim Brown wrote:
> > Whilst doing some janitor work on the OpenVAS code base I spotted a
> > potential race condition.
> >
> > openvas-scanner/openvassd/utils.c contains a function temp_file_name()
> > which contains a loop that recusively constructs random filenames under
> > OPENVASSD_STATEDIR/tmp and then checks whether they exist by attempting
> > to open them with O_RDONLY.  When open returns a file descriptor >= 0,
> > the filename is returned.
> >
> > This function is called from openvas-scanner/openvassd/ntp_11.c by the
> > ntp_11_recv_file() function.  This function opens the filename returned
> > with O_CREAT|O_WRONLY|O_TRUNC and then writes to it.
> >
> > There exists a time of check, time of use race condition which might be
> > exploited to overwrite arbitrary files.
> >
> > Thoughts before I start to clean it up?
>
> I would say go ahead.
>
> > ATTACHED_FILE still seems to be supported by OTP 1.0, no?
>
> Yes, its needed for credentials assignment (comes in form of a file) and
> for nvts that require a "File" preference.

Hmm, this is actually a lot more bothersome that I first thought....the 
lifetime of the files seems to be quite long.  After generating the filename 
it writes to it and then does daft things like:

files_add_translation (globals, origname, localname);

Using mkstemp alone won't be enough although it did solve the time of check 
time of write which inititially caught my eye :(.

Seems we need to cache the filename we generate, the device ID and the inode 
and check this on each subsequent read but it seems like a real nasty 
solution.  Either that or hold the contents in memory and bin the whole write 
to disk element entirely.

Suggestions?

Tim
-- 
Tim Brown
<mailto:timb at openvas.org>
<http://www.openvas.org/>