Skip Menu |
 

Subject: krb5_rc_io_creat should use mkstemp

The following code in krb5_rc_io_creat() should be replaced with mkstemp():

if (asprintf(&d->fn, "%s%skrb5_RC%daaa",
dir, PATH_SEPARATOR, (int) UNIQUE) < 0) {
d->fn = NULL;
return KRB5_RC_IO_MALLOC;
}
c = d->fn + strlen(d->fn) - 3;
while ((d->fd = THREEPARAMOPEN(d->fn, O_WRONLY | O_CREAT | O_TRUNC |
O_EXCL | O_BINARY, 0600)) == -1) {
if ((c[2]++) == 'z') {
c[2] = 'a';
if ((c[1]++) == 'z') {
c[1] = 'a';
if ((c[0]++) == 'z')
break; /* sigh */
}
}
}
mkstemp() does not take file mode flags, so to get the correct file
permissions on the file, we need to either use umask() (not
thread-friendly) or fchmod().
From: tlyu@mit.edu
Subject: SVN Commit

Make krb5_rc_io_creat() use mkstemp.


Commit By: tlyu



Revision: 20537
Changed Files:
_U trunk/
U trunk/src/lib/krb5/rcache/rc_io.c
From: Ken Raeburn <raeburn@MIT.EDU>
To: rt-comment@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #6002] krb5_rc_io_creat should use mkstemp
Date: Thu, 17 Jul 2008 20:47:44 -0400
RT-Send-Cc:
On Jul 17, 2008, at 11:02, Tom Yu via RT wrote:
Show quoted text
> mkstemp() does not take file mode flags, so to get the correct file
> permissions on the file, we need to either use umask() (not
> thread-friendly) or fchmod().

With fchmod, we would have a race condition where some other party
could open the file after it was created but before the fchmod call.
In the normal UNIX model, fchmod does not revoke access to an already
opened file.
From: tlyu@mit.edu
Subject: SVN Commit

Revert due to potential file modes race condition.


Commit By: tlyu



Revision: 20538
Changed Files:
_U trunk/
U trunk/src/lib/krb5/rcache/rc_io.c
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #6002] krb5_rc_io_creat should use mkstemp
From: Tom Yu <tlyu@MIT.EDU>
Date: Thu, 17 Jul 2008 23:08:47 -0400
RT-Send-Cc:
"Ken Raeburn via RT" <rt-comment@krbdev.mit.edu> writes:

Show quoted text
> With fchmod, we would have a race condition where some other party
> could open the file after it was created but before the fchmod call.
> In the normal UNIX model, fchmod does not revoke access to an already
> opened file.

Ok, so this is a case where using mkstemp() is clearly less safe.
What should we do? tmpnam() and open(O_CREAT|O_EXCL)? Some
development environments are evolving toward warning about uses of
mktemp(), which is similar to tmpnam(), so they may also flag uses of
tmpnam().

We could use umask(), but while we could lock around it, we could not
guarantee that the application would not call umask() outside of our
locks.

I'm going to revert this change for now.
From: Ken Raeburn <raeburn@MIT.EDU>
To: rt-comment@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #6002] krb5_rc_io_creat should use mkstemp
Date: Thu, 17 Jul 2008 23:23:55 -0400
RT-Send-Cc:
On Jul 17, 2008, at 23:09, Tom Yu via RT wrote:
Show quoted text
> Ok, so this is a case where using mkstemp() is clearly less safe.
> What should we do? tmpnam() and open(O_CREAT|O_EXCL)? Some
> development environments are evolving toward warning about uses of
> mktemp(), which is similar to tmpnam(), so they may also flag uses of
> tmpnam().

Coming up with names not already taken isn't all that hard, it just
requires setting up a loop and having a reasonably large space of
names to work through. With a large enough namespace and a halfway
decent PRNG, we ought to be able to find an unused name in one or two
tries, actually: dir + "/krb5_RC" + base64(random).

Do we have this sort of thing happening elsewhere, such that a utility
function mkstemp_mode_0600 would help?

Ken
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #6002] krb5_rc_io_creat should use mkstemp
From: Tom Yu <tlyu@MIT.EDU>
Date: Thu, 17 Jul 2008 23:53:05 -0400
RT-Send-Cc:
"Ken Raeburn via RT" <rt-comment@krbdev.mit.edu> writes:

Show quoted text
> Coming up with names not already taken isn't all that hard, it just
> requires setting up a loop and having a reasonably large space of
> names to work through. With a large enough namespace and a halfway
> decent PRNG, we ought to be able to find an unused name in one or two
> tries, actually: dir + "/krb5_RC" + base64(random).

We already have a mkstemp() replacement in the tree, it seems, but
only use it when the system does not already have mkstemp().

Show quoted text
> Do we have this sort of thing happening elsewhere, such that a utility
> function mkstemp_mode_0600 would help?

It looks like recent BSD-derived implementations of mkstemp() use mode
0600, but POSIX does not guarantee this. We could call mkstemp() and
then fstat() to make sure we got the modes we expect, and if we get
modes we do not expect, fall back on something more irritating. (Or
do autoconf run-time tests to see if mkstemp() is sane, but I'd rather
not do run-time tests.)
From: Ken Raeburn <raeburn@MIT.EDU>
To: rt-comment@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #6002] krb5_rc_io_creat should use mkstemp
Date: Fri, 18 Jul 2008 01:39:07 -0400
RT-Send-Cc:
On Jul 17, 2008, at 23:53, Tom Yu via RT wrote:
Show quoted text
> It looks like recent BSD-derived implementations of mkstemp() use mode
> 0600, but POSIX does not guarantee this. We could call mkstemp() and
> then fstat() to make sure we got the modes we expect, and if we get
> modes we do not expect, fall back on something more irritating. (Or
> do autoconf run-time tests to see if mkstemp() is sane, but I'd rather
> not do run-time tests.)

If we need to have the fallback code anyways, what's the benefit in
trying mkstemp+fstat?

Ken
From: tlyu@mit.edu
Subject: SVN Commit

Use mkstemp(), and fstat() the file to make sure that the mkstemp()
implementation is setting sane file modes.


Commit By: tlyu



Revision: 20543
Changed Files:
_U trunk/
U trunk/src/lib/krb5/rcache/rc_io.c
Show quoted text
> Revision: 20543
> U trunk/src/lib/krb5/rcache/rc_io.c

It looks to me like, if strdup fails, the file is left open (which is probably okay if the caller then uses krb5_rc_close to dispose
of the handle, but may cause a file and file descriptor leak if the caller tries krb5_rc_io_creat again), and d->fn is a dangling
pointer (which could be freed again by krb5_rc_io_close).
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #6002] krb5_rc_io_creat should use mkstemp
From: Tom Yu <tlyu@MIT.EDU>
Date: Fri, 25 Jul 2008 15:54:36 -0400
RT-Send-Cc:
"Ken Raeburn via RT" <rt-comment@krbdev.mit.edu> writes:

Show quoted text
>> Revision: 20543
>> U trunk/src/lib/krb5/rcache/rc_io.c
>
> It looks to me like, if strdup fails, the file is left open (which
> is probably okay if the caller then uses krb5_rc_close to dispose of
> the handle, but may cause a file and file descriptor leak if the
> caller tries krb5_rc_io_creat again), and d->fn is a dangling
> pointer (which could be freed again by krb5_rc_io_close).

That looks like a pre-existing bug. You could open a new ticket for it.
Okay, opened #6054.