Skip Menu |
 

Subject: library double-free with an empty keytab
When the Kerberos library opens an empty keytab, it recognizes an
immediate EOF as an error condition and closes the keytab, but it then
doesn't set the error return. The calling function therefore doesn't
recognize this as an error, tries to search in the keytab file, sees the
EOF again, and then closes it again. The second close causes a double
free. This patch fixes this by setting an error when the keytab file is
empty.

Patch from Steve Langasek.
Download empty-keytab
application/octet-stream 568B

Message body not shown because it is not plain text.

Download (untitled) / with headers
text/plain 3.6KiB
From krb5-bugs-incoming-bounces@PCH.mit.edu Sat Apr 22 22:09:00 2006
Received: from pch.mit.edu (PCH.MIT.EDU [18.7.21.90]) by krbdev.mit.edu (8.9.3p2) with ESMTP
id WAA25446; Sat, 22 Apr 2006 22:09:00 -0400 (EDT)
Received: from pch.mit.edu (pch.mit.edu [127.0.0.1])
by pch.mit.edu (8.13.6/8.12.8) with ESMTP id k3N28PMF027605
for <krb5-send-pr@krbdev.mit.edu>; Sat, 22 Apr 2006 22:08:25 -0400
Received: from pacific-carrier-annex.mit.edu (PACIFIC-CARRIER-ANNEX.MIT.EDU
[18.7.21.83])
by pch.mit.edu (8.13.6/8.12.8) with ESMTP id k3LDd5J5005237
for <krb5-bugs-incoming@PCH.mit.edu>; Fri, 21 Apr 2006 09:39:05 -0400
Received: from farside.sncag.com ([217.111.56.2])
by pacific-carrier-annex.mit.edu (8.13.6/8.9.2) with ESMTP id
k3LDd7NM021001
for <krb5-bugs@mit.edu>; Fri, 21 Apr 2006 09:39:07 -0400 (EDT)
Received: from farside.sncag.com (localhost [127.0.0.1])
by farside.sncag.com (8.13.4/8.13.4/Debian-3sarge1) with ESMTP id
k3LDd6Y1015047
for <krb5-bugs@mit.edu>; Fri, 21 Apr 2006 15:39:06 +0200
Received: (from rw@localhost)
by farside.sncag.com (8.13.4/8.13.4/Submit) id k3LDd6GD015044;
Fri, 21 Apr 2006 15:39:06 +0200
Date: Fri, 21 Apr 2006 15:39:06 +0200
From: Rainer Weikusat <rainer.weikusat@sncag.com>
Message-Id: <200604211339.k3LDd6GD015044@farside.sncag.com>
To: krb5-bugs@mit.edu
Subject: Incorrect error check in src/lib/krb5/keytab/kt_file.c
X-send-pr-version: 3.99
X-Spam-Score: -2.599
X-Spam-Flag: NO
X-Scanned-By: MIMEDefang 2.42
X-Mailman-Approved-At: Sat, 22 Apr 2006 22:08:24 -0400
X-BeenThere: krb5-bugs-incoming@mailman.mit.edu
X-Mailman-Version: 2.1.6
Precedence: list
Reply-To: rainer.weikusat@sncag.com
Sender: krb5-bugs-incoming-bounces@PCH.mit.edu
Errors-To: krb5-bugs-incoming-bounces@PCH.mit.edu


Show quoted text
>Submitter-Id: net
>Originator: Rainer Weikusat
>Organization: SNC AG
>Confidential: no
>Synopsis: EOF mistakenly interpreted as error causes re-use of closed stream
>Category: krb5-libs
>Class: sw-bug
>Release: 1.4.3
>Environment:

System: Linux farside 2.6.16.9 #3 Wed Apr 19 11:30:29 CEST 2006 i686 GNU/Linux
Architecture: i686

Show quoted text
>Description:
The file mentioned in the subject contains the following code section,
which is supposed to deal with read errors occuring during an attempted
kvno read from an existing keytab file:

if (!xfread(&kt_vno, sizeof(kt_vno), 1, KTFILEP(id))) {
if (feof(KTFILEP(id))) kerror = KRB5_KT_END;
else kerror = errno;

(void) krb5_unlock_file(context, fileno(KTFILEP(id)));
(void) fclose(KTFILEP(id));
return kerror;
}

This is incorrect, because xfread (fread) can return zero if the file exists
and is empty, with errno also being zero (because no error ocurred), which
will lead to the stream being closed without an error indication passed up
to the caller (which, in my case, will proceed with calling fseek on the
closed stream, returning KRB5_KT_END as EINVAL-in-disguise and finally
crashing inside malloc while trying to format an error message to be
printed describing this error (add codepath)).
Show quoted text
>How-To-Repeat:
Call krb5_kt_add_entry w/ a keytab id refering to a file that exists
and is empty and try to print an error message via (Linux/Gnu) vsyslog
afterwards.
Show quoted text
>Fix:
--- src/lib/krb5/keytab/kt_file.c 19 Mar 2006 14:42:00 -0000 1.1.1.1
+++ src/lib/krb5/keytab/kt_file.c 21 Apr 2006 13:14:34 -0000 1.2
@@ -1107,7 +1107,9 @@
} else {
/* gotta verify it instead... */
if (!xfread(&kt_vno, sizeof(kt_vno), 1, KTFILEP(id))) {
- kerror = errno;
+ if (feof(KTFILEP(id))) kerror = KRB5_KT_END;
+ else kerror = errno;
+
(void) krb5_unlock_file(context, fileno(KTFILEP(id)));
(void) fclose(KTFILEP(id));
return kerror;
From: Russ Allbery <rra@stanford.edu>
To: rt@krbdev.mit.edu
Cc: Rainer Weikusat <rainer.weikusat@sncag.com>
Subject: Re: [krbdev.mit.edu #3685] EOF mistakenly interpreted as error causes re-use of closed stream
Date: Sat, 22 Apr 2006 19:47:35 -0700
RT-Send-Cc:
Download (untitled) / with headers
text/plain 1.3KiB
The RT System itself via RT <rt-comment@krbdev.mit.edu> writes:

Show quoted text
> The file mentioned in the subject contains the following code
> section, which is supposed to deal with read errors occuring
> during an attempted kvno read from an existing keytab file:

Show quoted text
> if (!xfread(&kt_vno, sizeof(kt_vno), 1, KTFILEP(id))) {
> if (feof(KTFILEP(id))) kerror = KRB5_KT_END;
> else kerror = errno;

Show quoted text
> (void) krb5_unlock_file(context, fileno(KTFILEP(id)));
> (void) fclose(KTFILEP(id));
> return kerror;
> }

Show quoted text
> This is incorrect, because xfread (fread) can return zero if the
> file exists and is empty, with errno also being zero (because no
> error ocurred), which will lead to the stream being closed without
> an error indication passed up to the caller (which, in my case,
> will proceed with calling fseek on the closed stream, returning
> KRB5_KT_END as EINVAL-in-disguise and finally crashing inside
> malloc while trying to format an error message to be printed
> describing this error (add codepath)).

Yup, this is RT #3549, which I think includes a slightly better patch
courtesy of Steve Langasek. This is one of the 19 patches that we're
carrying in the Debian package at the moment that have also been submitted
to RT.

--
Russ Allbery (rra@stanford.edu) <http://www.eyrie.org/~eagle/>
To: Russ Allbery <rra@stanford.edu>
Cc: rt@krbdev.mit.edu, Rainer Weikusat <rweikusat@sncag.com>
Subject: Re: [krbdev.mit.edu #3685] EOF mistakenly interpreted as error causes re-use of closed stream
From: Rainer Weikusat <rainer.weikusat@sncag.com>
Date: Sun, 23 Apr 2006 13:39:05 +0200
RT-Send-Cc:
Download (untitled) / with headers
text/plain 1.5KiB
Russ Allbery <rra@stanford.edu> writes:
Show quoted text
> The RT System itself via RT <rt-comment@krbdev.mit.edu> writes:
>
>> The file mentioned in the subject contains the following code
>> section, which is supposed to deal with read errors occuring
>> during an attempted kvno read from an existing keytab file:
>
>> if (!xfread(&kt_vno, sizeof(kt_vno), 1, KTFILEP(id))) {
>> if (feof(KTFILEP(id))) kerror = KRB5_KT_END;
>> else kerror = errno;
>
>> (void) krb5_unlock_file(context, fileno(KTFILEP(id)));
>> (void) fclose(KTFILEP(id));
>> return kerror;
>> }
>
>> This is incorrect, because xfread (fread) can return zero if the
>> file exists and is empty, with errno also being zero (because no
>> error ocurred), which will lead to the stream being closed without
>> an error indication passed up to the caller (which, in my case,
>> will proceed with calling fseek on the closed stream, returning
>> KRB5_KT_END as EINVAL-in-disguise and finally crashing inside
>> malloc while trying to format an error message to be printed
>> describing this error (add codepath)).
>
> Yup, this is RT #3549, which I think includes a slightly better patch
> courtesy of Steve Langasek.

Presumably, because it returns a totally random synthetic system error
that bears absolutely no relation to the condition that caused it?
(keytab w/o kvno)

Do me a favor and *DO NOT* send me any of this "Hey, I know this guy
but who an earth are you"-nonsense again, ok?
From: Russ Allbery <rra@stanford.edu>
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #3685] EOF mistakenly interpreted as error causes re-use of closed stream
Date: Sun, 23 Apr 2006 11:39:31 -0700
RT-Send-Cc:
Download (untitled) / with headers
text/plain 1.2KiB
rainer weikusat@sncag com via RT <rt-comment@krbdev.mit.edu> writes:
Show quoted text
> Russ Allbery <rra@stanford.edu> writes:

Show quoted text
>> Yup, this is RT #3549, which I think includes a slightly better patch
>> courtesy of Steve Langasek.

Show quoted text
> Presumably, because it returns a totally random synthetic system error
> that bears absolutely no relation to the condition that caused it?
> (keytab w/o kvno)

Well, no, more because I think it's cleaner to detect EOF from the
function return than explicitly call feof. It may well be that your
choice of error code is better; that's a very good point.

Show quoted text
> Do me a favor and *DO NOT* send me any of this "Hey, I know this guy but
> who an earth are you"-nonsense again, ok?

It would be useful to be able to have a conversation about the merits of
an approach without this sort of reaction. I was not intending to be
offensive; I was intending to point out for the Kerberos developers that
this should probably be merged with the other bug report for the same
issue and was advocating a patch that I thought was cleaner.

If you disagree and have good reason, as it sounds like you do, then by
all means we should arrive at the best possible patch!

--
Russ Allbery (rra@stanford.edu) <http://www.eyrie.org/~eagle/>
From: Russ Allbery <rra@stanford.edu>
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #3685] EOF mistakenly interpreted as error causes re-use of closed stream
Date: Sun, 23 Apr 2006 11:44:31 -0700
RT-Send-Cc:
Russ Allbery via RT <rt-comment@krbdev.mit.edu> writes:
Show quoted text
> rainer weikusat@sncag com via RT <rt-comment@krbdev.mit.edu> writes:

Show quoted text
>> Presumably, because it returns a totally random synthetic system error
>> that bears absolutely no relation to the condition that caused it?
>> (keytab w/o kvno)

Show quoted text
> Well, no, more because I think it's cleaner to detect EOF from the
> function return than explicitly call feof. It may well be that your
> choice of error code is better; that's a very good point.

And actually, now that I look at this more closely, I do see why you did
that. Advocacy withdrawn; I think your approach is better since it isn't
subject to possible garbage in errno before this code runs.

I'll update the patch in Debian to use the same approach.

--
Russ Allbery (rra@stanford.edu) <http://www.eyrie.org/~eagle/>
From: Russ Allbery <rra@stanford.edu>
To: rt-comment@krbdev.mit.edu
Subject: [krbdev.mit.edu #3549] Better patch
Date: Sun, 23 Apr 2006 12:23:48 -0700
RT-Send-Cc:
Download (untitled) / with headers
text/plain 1.2KiB
Here is a better patch, almost identical to the patch by Rainer Weikusat
in RT #3685. I use KRB5_KEYTAB_BADVNO as the return for this case,
similar to the code immediately below, rather than using KRB5_KT_END. I
can see arguments for either.

Committed to the Debian source tree, although not in any uploaded version
yet.

% touch keytab
% klist -k keytab
Keytab name: FILE:keytab
KVNO Principal
---- --------------------------------------------------------------------------
*** glibc detected *** double free or corruption (!prev): 0x0804d6a8 ***
Abort
% env LD_LIBRARY_PATH=libkrb53/usr/lib klist -k keytab
Keytab name: FILE:keytab
klist: Unsupported key table format version number while starting keytab scan


--- krb5-1.4.3.orig/src/lib/krb5/keytab/kt_file.c
+++ krb5-1.4.3/src/lib/krb5/keytab/kt_file.c
@@ -1107,7 +1107,10 @@
} else {
/* gotta verify it instead... */
if (!xfread(&kt_vno, sizeof(kt_vno), 1, KTFILEP(id))) {
- kerror = errno;
+ if (feof(KTFILEP(id)))
+ kerror = KRB5_KEYTAB_BADVNO;
+ else
+ kerror = errno;
(void) krb5_unlock_file(context, fileno(KTFILEP(id)));
(void) fclose(KTFILEP(id));
return kerror;

--
Russ Allbery (rra@stanford.edu) <http://www.eyrie.org/~eagle/>
From: Russ Allbery <rra@stanford.edu>
Subject: CVS Commit
Prevent a library double-free and crash when a keytab is zero-length.
Based on a patch from Rainer Weikusat.

Commit By: rra



Revision: 18120
Changed Files:
U trunk/src/lib/krb5/keytab/kt_file.c