Ticket History #560: BUG: possible lib/krb4/tf_util.c race condition

From schwim@whatmore.Stanford.EDU Wed Mar 18 20:38:36 1998
Received: from MIT.EDU (PACIFIC-CARRIER-ANNEX.MIT.EDU [18.69.0.28]) by rt-11.MIT.EDU (8.7.5/8.7.3) with SMTP id UAA24963 for <bugs@RT-11.MIT.EDU>; Wed, 18 Mar 1998 20:38:35 -0500
Received: from whatmore.Stanford.EDU by MIT.EDU with SMTP
id AA27003; Wed, 18 Mar 98 20:39:08 EST
Received: (from schwim@localhost)
by whatmore.Stanford.EDU (8.8.8/8.8.8) id RAA04137;
Wed, 18 Mar 1998 17:38:33 -0800 (PST)
Message-Id: <199803190138.RAA04137@whatmore.Stanford.EDU>
Date: Wed, 18 Mar 1998 17:38:33 -0800 (PST)
From: Larry Schwimmer <schwim@whatmore.Stanford.EDU>
Cc: krb5-bugs@MIT.EDU, schwim@leland.Stanford.EDU
In-Reply-To: <199803172350.PAA29584@whatmore.Stanford.EDU> from "Larry Schwimmer" at Mar 17, 98 03:50:32 pm
Subject: Re: BUG: possible lib/krb4/tf_util.c race condition


State-Changed-From-To: open-closed
State-Changed-By: mdh
State-Changed-When: Thu Jul 9 18:14:18 1998
State-Changed-Why:

Superseded by PR 566.

Responsible-Changed-From-To: gnats-admin->mdh
Responsible-Changed-By: mdh
Responsible-Changed-When: Thu Jul 9 18:27:04 1998
Responsible-Changed-Why:

I'll take this.

You (Larry Schwimmer) write:

Sample patch included at the end of the note. It's basically
the same for the krb4 and krb5 distributions.

yours,
Larry Schwimmer
schwim@leland.stanford.edu
Leland Systems Group


--- lib/krb4/tf_util.c.orig Fri Feb 6 19:44:22 1998
+++ lib/krb4/tf_util.c Wed Mar 18 17:31:55 1998
@@ -278,10 +278,26 @@
#endif /* TKT_SHMEM */

if (wflag) {
- fd = open(tf_name, O_RDWR, 0600);
+ fd = open(tf_name, O_RDWR|O_CREAT|O_EXCL, 0600);
if (fd < 0) {
return TKT_FIL_ACC;
}
+ if (fstat(fd, &stat_buf) < 0) {
+ (void) close(fd);
+ fd = -1;
+ switch (errno) {
+ case ENOENT:
+ return NO_TKT_FIL;
+ default:
+ return TKT_FIL_ACC;
+ }
+ }
+ if ((stat_buf.st_uid != me && me != 0) ||
+ ((stat_buf.st_mode & S_IFMT) != S_IFREG)) {
+ (void) close(fd);
+ fd = -1;
+ return TKT_FIL_ACC;
+ }
if (flock(fd, LOCK_EX | LOCK_NB) < 0) {
sleep(TF_LCK_RETRY);
if (flock(fd, LOCK_EX | LOCK_NB) < 0) {
@@ -297,8 +313,24 @@
* for read-only operations and locked for shared access.
*/

- fd = open(tf_name, O_RDONLY, 0600);
+ fd = open(tf_name, O_RDONLY|O_NONBLOCK, 0600);
if (fd < 0) {
+ return TKT_FIL_ACC;
+ }
+ if (fstat(fd, &stat_buf) < 0) {
+ (void) close(fd);
+ fd = -1;
+ switch (errno) {
+ case ENOENT:
+ return NO_TKT_FIL;
+ default:
+ return TKT_FIL_ACC;
+ }
+ }
+ if ((stat_buf.st_uid != me && me != 0) ||
+ ((stat_buf.st_mode & S_IFMT) != S_IFREG)) {
+ (void) close(fd);
+ fd = -1;
return TKT_FIL_ACC;
}
if (flock(fd, LOCK_SH | LOCK_NB) < 0) {

From schwim@whatmore.Stanford.EDU Wed Mar 18 22:27:50 1998
Received: from MIT.EDU (PACIFIC-CARRIER-ANNEX.MIT.EDU [18.69.0.28]) by rt-11.MIT.EDU (8.7.5/8.7.3) with SMTP id WAA25422 for <bugs@RT-11.MIT.EDU>; Wed, 18 Mar 1998 22:27:49 -0500
Received: from whatmore.Stanford.EDU by MIT.EDU with SMTP
id AA14576; Wed, 18 Mar 98 22:28:21 EST
Received: (from schwim@localhost)
by whatmore.Stanford.EDU (8.8.8/8.8.8) id TAA04743;
Wed, 18 Mar 1998 19:27:47 -0800 (PST)
Message-Id: <199803190327.TAA04743@whatmore.Stanford.EDU>
Date: Wed, 18 Mar 1998 19:27:46 -0800 (PST)
From: Larry Schwimmer <schwim@whatmore.Stanford.EDU>
Cc: krb5-bugs@MIT.EDU, schwim@leland.Stanford.EDU
In-Reply-To: <199803190138.RAA04137@whatmore.Stanford.EDU> from "Larry Schwimmer" at Mar 18, 98 05:38:33 pm
Subject: Re: BUG: possible lib/krb4/tf_util.c race condition


State-Changed-From-To: open-closed
State-Changed-By: mdh
State-Changed-When: Thu Jul 9 18:21:28 1998
State-Changed-Why:

Superseded by PR 567.

Responsible-Changed-From-To: gnats-admin->mdh
Responsible-Changed-By: mdh
Responsible-Changed-When: Thu Jul 9 18:29:27 1998
Responsible-Changed-Why:

I'll take this.

You (Larry Schwimmer) write:

Sorry, let try that one more time.
Since the file exists, O_EXCL is not an option for creation.
So, open then fstat. I did an lstat after that to check that the file
opened is the one that the code thinks it opened. I'm not positive
that is needed, but it shouldn't hurt.
I haven't dealt with the shared memory code in this patch; the
fopen call probably also needs a check on the file.

yours,
Larry Schwimmer
schwim@leland.stanford.edu
Leland Systems Group

--- lib/krb4/tf_util.c.orig Fri Feb 6 19:44:22 1998
+++ lib/krb4/tf_util.c Wed Mar 18 19:20:11 1998
@@ -184,7 +184,7 @@
{
int wflag;
uid_t me, getuid();
- struct stat stat_buf;
+ struct stat stat_buf, stat_buffd;
#ifdef TKT_SHMEM
char shmidname[MAXPATHLEN];
FILE *sfp;
@@ -207,17 +207,7 @@
if (tf_name == 0)
tf_name = tkt_string();

- if (lstat(tf_name, &stat_buf) < 0)
- switch (errno) {
- case ENOENT:
- return NO_TKT_FIL;
- default:
- return TKT_FIL_ACC;
- }
me = getuid();
- if ((stat_buf.st_uid != me && me != 0) ||
- ((stat_buf.st_mode & S_IFMT) != S_IFREG))
- return TKT_FIL_ACC;
#ifdef TKT_SHMEM
(void) strcpy(shmidname, tf_name);
(void) strcat(shmidname, ".shm");
@@ -280,6 +270,40 @@
if (wflag) {
fd = open(tf_name, O_RDWR, 0600);
if (fd < 0) {
+ return TKT_FIL_ACC;
+ }
+ /* Check the ownership on the file opened */
+ if (fstat(fd, &stat_buffd) < 0) {
+ (void) close(fd);
+ fd = -1;
+ switch (errno) {
+ case ENOENT:
+ return NO_TKT_FIL;
+ default:
+ return TKT_FIL_ACC;
+ }
+ }
+ if ((stat_buffd.st_uid != me && me != 0) ||
+ ((stat_buffd.st_mode & S_IFMT) != S_IFREG)) {
+ (void) close(fd);
+ fd = -1;
+ return TKT_FIL_ACC;
+ }
+ /* Check to see that the file opened is the correct one */
+ if (lstat(tf_name, &stat_buf) < 0) {
+ (void) close(fd);
+ fd = -1;
+ switch (errno) {
+ case ENOENT:
+ return NO_TKT_FIL;
+ default:
+ return TKT_FIL_ACC;
+ }
+ }
+ if ((stat_buf.st_ino != stat_buffd.st_ino) ||
+ (stat_buf.st_dev != stat_buffd.st_dev)) {
+ (void) close(fd);
+ fd = -1;
return TKT_FIL_ACC;
}
if (flock(fd, LOCK_EX | LOCK_NB) < 0) {
@@ -297,10 +321,45 @@
* for read-only operations and locked for shared access.
*/

- fd = open(tf_name, O_RDONLY, 0600);
+ fd = open(tf_name, O_RDONLY|O_NONBLOCK, 0600);
if (fd < 0) {
return TKT_FIL_ACC;
}
+ /* Check the ownership on the file opened */
+ if (fstat(fd, &stat_buf) < 0) {
+ (void) close(fd);
+ fd = -1;
+ switch (errno) {
+ case ENOENT:
+ return NO_TKT_FIL;
+ default:
+ return TKT_FIL_ACC;
+ }
+ }
+ if ((stat_buf.st_uid != me && me != 0) ||
+ ((stat_buf.st_mode & S_IFMT) != S_IFREG)) {
+ (void) close(fd);
+ fd = -1;
+ return TKT_FIL_ACC;
+ }
+ /* Check to see that the file opened is the correct one */
+ if (lstat(tf_name, &stat_buf) < 0) {
+ (void) close(fd);
+ fd = -1;
+ switch (errno) {
+ case ENOENT:
+ return NO_TKT_FIL;
+ default:
+ return TKT_FIL_ACC;
+ }
+ }
+ if ((stat_buf.st_ino != stat_buffd.st_ino) ||
+ (stat_buf.st_dev != stat_buffd.st_dev)) {
+ (void) close(fd);
+ fd = -1;
+ return TKT_FIL_ACC;
+ }
+
if (flock(fd, LOCK_SH | LOCK_NB) < 0) {
sleep(TF_LCK_RETRY);
if (flock(fd, LOCK_SH | LOCK_NB) < 0) {

From schwim@whatmore.Stanford.EDU Tue Mar 17 18:50:36 1998
Received: from MIT.EDU (SOUTH-STATION-ANNEX.MIT.EDU [18.72.1.2]) by rt-11.MIT.EDU (8.7.5/8.7.3) with SMTP id SAA18401 for <bugs@RT-11.MIT.EDU>; Tue, 17 Mar 1998 18:50:35 -0500
Received: from whatmore.Stanford.EDU by MIT.EDU with SMTP
id AA23732; Tue, 17 Mar 98 18:50:33 EST
Received: (from schwim@localhost)
by whatmore.Stanford.EDU (8.8.8/8.8.8) id PAA29584;
Tue, 17 Mar 1998 15:50:32 -0800 (PST)
Message-Id: <199803172350.PAA29584@whatmore.Stanford.EDU>
Date: Tue, 17 Mar 1998 15:50:32 -0800 (PST)
From: Larry Schwimmer <schwim@whatmore.Stanford.EDU>
To: krb5-bugs@MIT.EDU
Cc: schwim@leland.Stanford.EDU
Subject: BUG: possible lib/krb4/tf_util.c race condition


State-Changed-From-To: open-closed
State-Changed-By: mdh
State-Changed-When: Thu Jul 9 19:50:27 1998
State-Changed-Why:

Patch checked in, modified slightly from Larry's. A similar patch was made on
the shared libraries code, which is largely untested but bascially identical.

Responsible-Changed-From-To: gnats-admin->mdh
Responsible-Changed-By: mdh
Responsible-Changed-When: Thu Jul 9 19:51:57 1998
Responsible-Changed-Why:

Taken and solved.

Submitter-Id: net
Originator: Larry Schwimmer
Confidential: no
Synopsis: tf_init has a /tmp race condition
Severity: serious
Priority: medium
Category: krb5-libs
Class: sw-bug
Release: 1.0.5
Environment: All
Description:

tf_init uses lstat to check the permissions on the ticket
file. Since lstat+open is not atomic, a race condition exists. Since
the open call only specifies O_RDWR and the call may be made by root
for a regular user, it is a potential root-level exploit for code
using the krb4 compatibility library.

How-To-Repeat:

Read the code.

Fix:

Replace

lstat + open(O_RDWR)

with

open(O_RDWR|O_CREAT|O_EXCL) + fstat

for the write call and

open(O_RDONLY|O_NONBLOCK) + fstat.

for the read call. This eliminates the race condition since the file
descriptor checked by fstat is the file that was created or read.
This eliminates the symlink problem by using O_CREAT|O_EXCL. This
eliminates the blocking problem (as a named pipe could do) by
specifying O_NONBLOCK.

yours,
Larry Schwimmer
schwim@leland.stanford.edu
Leland Systems Group

From schwim@whatmore.Stanford.EDU Thu Mar 19 08:10:10 1998
Received: from MIT.EDU (SOUTH-STATION-ANNEX.MIT.EDU [18.72.1.2]) by rt-11.MIT.EDU (8.7.5/8.7.3) with SMTP id IAA27987 for <bugs@RT-11.MIT.EDU>; Thu, 19 Mar 1998 08:10:09 -0500
Received: from whatmore.Stanford.EDU by MIT.EDU with SMTP
id AA27729; Thu, 19 Mar 98 08:09:59 EST
Received: (from schwim@localhost)
by whatmore.Stanford.EDU (8.8.8/8.8.8) id FAA06007;
Thu, 19 Mar 1998 05:09:52 -0800 (PST)
Message-Id: <199803191309.FAA06007@whatmore.Stanford.EDU>
Date: Thu, 19 Mar 1998 05:09:51 -0800 (PST)
From: Larry Schwimmer <schwim@whatmore.Stanford.EDU>
Cc: krb5-bugs@MIT.EDU, schwim@leland.Stanford.EDU
In-Reply-To: <199803190327.TAA04743@whatmore.Stanford.EDU> from "Larry Schwimmer" at Mar 18, 98 07:27:46 pm
Subject: Re: BUG: possible lib/krb4/tf_util.c race condition


Responsible-Changed-From-To: gnats-admin->mdh
Responsible-Changed-By: mdh
Responsible-Changed-When: Thu Jul 9 19:55:37 1998
Responsible-Changed-Why:

Taken and solved.

State-Changed-From-To: open-closed
State-Changed-By: mdh
State-Changed-When: Thu Jul 9 19:55:51 1998
State-Changed-Why:

Fixed. See pr 560.

You (Larry Schwimmer) write:

*sigh* Third time's the charm. (I used the wrong variable
for the read code in the previous patch.)
I've tested this against reading and writing of tickets.

yours,
Larry


--- lib/krb4/tf_util.c.orig Fri Feb 6 19:44:22 1998
+++ lib/krb4/tf_util.c Thu Mar 19 03:37:22 1998
@@ -184,7 +184,7 @@
{
int wflag;
uid_t me, getuid();
- struct stat stat_buf;
+ struct stat stat_buf, stat_buffd;
#ifdef TKT_SHMEM
char shmidname[MAXPATHLEN];
FILE *sfp;
@@ -207,17 +207,7 @@
if (tf_name == 0)
tf_name = tkt_string();

- if (lstat(tf_name, &stat_buf) < 0)
- switch (errno) {
- case ENOENT:
- return NO_TKT_FIL;
- default:
- return TKT_FIL_ACC;
- }
me = getuid();
- if ((stat_buf.st_uid != me && me != 0) ||
- ((stat_buf.st_mode & S_IFMT) != S_IFREG))
- return TKT_FIL_ACC;
#ifdef TKT_SHMEM
(void) strcpy(shmidname, tf_name);
(void) strcat(shmidname, ".shm");
@@ -280,6 +270,40 @@
if (wflag) {
fd = open(tf_name, O_RDWR, 0600);
if (fd < 0) {
+ return TKT_FIL_ACC;
+ }
+ /* Check the ownership on the file opened */
+ if (fstat(fd, &stat_buffd) < 0) {
+ (void) close(fd);
+ fd = -1;
+ switch (errno) {
+ case ENOENT:
+ return NO_TKT_FIL;
+ default:
+ return TKT_FIL_ACC;
+ }
+ }
+ if ((stat_buffd.st_uid != me && me != 0) ||
+ ((stat_buffd.st_mode & S_IFMT) != S_IFREG)) {
+ (void) close(fd);
+ fd = -1;
+ return TKT_FIL_ACC;
+ }
+ /* Check to see that the file opened is the correct one */
+ if (lstat(tf_name, &stat_buf) < 0) {
+ (void) close(fd);
+ fd = -1;
+ switch (errno) {
+ case ENOENT:
+ return NO_TKT_FIL;
+ default:
+ return TKT_FIL_ACC;
+ }
+ }
+ if ((stat_buf.st_ino != stat_buffd.st_ino) ||
+ (stat_buf.st_dev != stat_buffd.st_dev)) {
+ (void) close(fd);
+ fd = -1;
return TKT_FIL_ACC;
}
if (flock(fd, LOCK_EX | LOCK_NB) < 0) {
@@ -297,10 +321,45 @@
* for read-only operations and locked for shared access.
*/

- fd = open(tf_name, O_RDONLY, 0600);
+ fd = open(tf_name, O_RDONLY|O_NONBLOCK, 0600);
if (fd < 0) {
return TKT_FIL_ACC;
}
+ /* Check the ownership on the file opened */
+ if (fstat(fd, &stat_buffd) < 0) {
+ (void) close(fd);
+ fd = -1;
+ switch (errno) {
+ case ENOENT:
+ return NO_TKT_FIL;
+ default:
+ return TKT_FIL_ACC;
+ }
+ }
+ if ((stat_buffd.st_uid != me && me != 0) ||
+ ((stat_buffd.st_mode & S_IFMT) != S_IFREG)) {
+ (void) close(fd);
+ fd = -1;
+ return TKT_FIL_ACC;
+ }
+ /* Check to see that the file opened is the correct one */
+ if (lstat(tf_name, &stat_buf) < 0) {
+ (void) close(fd);
+ fd = -1;
+ switch (errno) {
+ case ENOENT:
+ return NO_TKT_FIL;
+ default:
+ return TKT_FIL_ACC;
+ }
+ }
+ if ((stat_buf.st_ino != stat_buffd.st_ino) ||
+ (stat_buf.st_dev != stat_buffd.st_dev)) {
+ (void) close(fd);
+ fd = -1;
+ return TKT_FIL_ACC;
+ }
+
if (flock(fd, LOCK_SH | LOCK_NB) < 0) {
sleep(TF_LCK_RETRY);
if (flock(fd, LOCK_SH | LOCK_NB) < 0) {