Hi David, David Sommerseth wrote: > On 16/04/10 11:35, Gert Doering wrote: >> Hi, > >> On Fri, Apr 16, 2010 at 11:16:32AM +0200, David Sommerseth wrote: >>> I'll look more into this, as the only advantage is that if open() with >>> O_EXCL|O_CREAT fails if the file exists, it should be used instead. >> Unfortunately, this won't help against symlink attacks directed to >> non-existant files (like "-> /etc/nologin"). > > That's right, this could create a local DoS. I'm going to have a more > careful look at test_file() afterwards. Considering to make it use > stat() instead of just trying to open the file for reading.
As Davide already pointed out, the problem is taken care of by open().
So ... there should be no need for additional checking after a
successful open() with O_CREAT|O_EXCL.
(BTW, I thought creat() took a flags parameter, but it only takes a mode
param. My mistake. So you're correct in wanting to use open() instead of
creat().)
To let the code speak, here's my suggestion (not compile-tested):
> do
> {
> uint8_t rndbytes[16];
> const char *rndstr;
>
> ++attempts;
> mutex_lock_static (L_CREATE_TEMP);
> ++counter;
> mutex_unlock_static (L_CREATE_TEMP);
>
> prng_bytes (rndbytes, sizeof rndbytes);
> rndstr = format_hex_ex (rndbytes, sizeof rndbytes, 40, 0, NULL, gc);
> buf_printf (&fname, PACKAGE "_%s_%s.tmp", prefix, rndstr);
>
> retfname = gen_path (directory, BSTR (&fname), gc);
> if (!retfname)
> {
> msg (M_FATAL, "Failed to create temporary filename and path");
> return NULL;
> }
>
> /* Atomically create the file. Errors out if the file already
> exists. */
> fd = open (retfname, O_CREAT | O_EXCL | O_WRONLY, S_IRUSR | S_IWUSR);
> if (fd != -1)
> {
> close (fd);
> return retfname;
> }
> else if (fd == -1 && errno != EEXIST)
> {
> /* Something else went wrong, no need to retry. */
> struct gc_arena gcerr = gc_new ();
> msg (M_FATAL, "Could not create temporary file '%s': %s",
> retfname, strerror_ts (errno, &gcerr));
> gc_free (&gcerr);
> return NULL;
> }
> }
> while (attempts < 6);
>
> msg (M_FATAL, "Failed to create temporary file after %i attempts", attempts);
> return NULL;
(Regarding the code style: The above uses what I had thought would be
the current style, but as I've only read parts of the OpenVPN code, I'm
likely to be wrong. Are there any code-parts / newer code-parts which
should be considered reference material?)
> I've dived into the kernel code to see what it *really* does (when the
> man page are so unclear), and it should behave as those other Unices
> does as well. So, O_EXCL do make sense to avoid overwriting existing
> files if it is a symlink to an existing file.
IMHO the Linux man-page ist quite clear (I'm using version 3.24):
> O_EXCL Ensure that this call creates the file: if this flag is speci‐
> fied in conjunction with O_CREAT, and pathname already exists,
> then open() will fail. The behavior of O_EXCL is undefined if
> O_CREAT is not specified.
>
> When these two flags are specified, symbolic links are not fol‐
> lowed: if pathname is a symbolic link, then open() fails regard‐
> less of where the symbolic link points to.
>
> O_EXCL is only supported on NFS when using NFSv3 or later on
> kernel 2.6 or later. In environments where NFS O_EXCL support
> is not provided, programs that rely on it for performing locking
> tasks will contain a race condition. Portable programs that
> want to perform atomic file locking using a lockfile, and need
> to avoid reliance on NFS support for O_EXCL, can create a unique
> file on the same file system (e.g., incorporating hostname and
> PID), and use link(2) to make a link to the lockfile. If
> link(2) returns 0, the lock is successful. Otherwise, use
> stat(2) on the unique file to check if its link count has
> increased to 2, in which case the lock is also successful.
Using open with O_CREAT|O_EXCL:
- makes the atomic guarantee, that the file was created by us (or
the function call fails)
- does not follow symlinks (if a symlink exists, it fails)
- works over NFS for NFS implementations that support O_EXCL. (This
should apply to all current Linux NFS implementations.)
So, anything remaining that you'd like cleared up?
> Btw ... When diving into the kernel code, I stumbled upon this comment
> in fs/namei.c:1872:
>
> /* Does someone understand code flow here? Or it is only
> * me so stupid? Anathema to whoever designed this non-sense
> * with "intent.open".
> */
>
> Thought that one was worth sharing ;-)
:-D
Cheers
Fabian
signature.asc
Description: OpenPGP digital signature
