Hi David, David Sommerseth schrieb: > As promised in the meeting today, a patch for hardening > create_temp_filename().
Great! :)
> I've added more checks to what create_temp_filename() returns where it
> is called in addition, to make it even safer.
> + 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;
> + }
> + } while ( test_file (retfname) && (attempts < 6) );
> +
> + if( attempts > 5 ) {
> + msg (M_FATAL, "Failed to create temporary file after %i attempts",
> attempts);
> + return NULL;
> + }
> +
> + /* Create the tempfile to avoid a race condition */
> + fd = creat (retfname, S_IRUSR | S_IWUSR);
> + if( fd == -1 ) {
You'll probably want to add O_EXCL to creat()'s mode flags:
| 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.
Otherwise, there would be a race-condition between test_file() and
creat() and someone could create a symlink under you feet.
And as the important "does it exist?" test thereby moves to creat(), I
would like to suggest that you move the creat() call into the loop. The
exit condition would then be "successful creat()" or "max attempts".
Cheers
Fabian
PS: Is there some kind of code style guide for OpenVPN? I notice that
you're using a different style than I expected to see...
signature.asc
Description: OpenPGP digital signature
