RT RT/krbdev.mit.edu: Ticket #3549 library double-free with an empty keytab Signed in as guest.
[Logout]

[Home] [Search] [Configuration]

[Display] [History] [Basics] [Dates] [People] [Links] [Jumbo]

 
 

 The Basics  
Id
3549
Status
resolved
Worked
0 min
Priority
0/0
Queue
krb5
 

 Keyword Selections  
Component
  • krb5-libs
Tags
Version_reported
  • 1.4.3
Version_Fixed
  • 1.5
Target_Version
 

 Relationships  
Depends on:
Depended on by:
Parents:
Children:

Refers to:
Referred to by:
 
 Dates  
Created: Wed Mar 22 23:30:34 2006
Starts: Not set
Started: Tue Jun 13 10:14:30 2006
Last Contact: Tue Jun 13 10:14:36 2006
Due: Not set
Updated: Tue May 20 14:59:50 2008 by guest
 

 People  
Owner
 rra
Requestors
 rra@debian.org, rainer.weikusat@sncag.com
Cc
 
AdminCc
 
 

 More about rra@debian.org  
Comments about this user:
No comment entered about this user
This user's 25 highest priority tickets:
 
 More about rainer.weikusat@sncag.com  
Comments about this user:
No comment entered about this user
This user's 25 highest priority tickets:
 

History   Display mode: [Brief headers] [Full headers]
      Wed Mar 22 23:30:35 2006  guest - Ticket created    
     
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 (untitled) 458b
     
 
Download empty-keytab 568b
      Sat Apr 22 22:09:03 2006  RT_System - Ticket 3685: Ticket created    
     
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


>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

>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)).
>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.
>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;


Download (untitled) 3.6k
      Sat Apr 22 22:09:06 2006  RT_System - Ticket 3685: Component krb5-libs added    
      Sat Apr 22 22:47:45 2006  rra@stanford.edu - Ticket 3685: Correspondence added    
     
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: 

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.  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/>


Download (untitled) 1.3k
      Sun Apr 23 07:39:46 2006  rainer.weikusat@sncag.com - Ticket 3685: Correspondence added    
     
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: 

Russ Allbery <rra@stanford.edu> writes:
> 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?


Download (untitled) 1.5k
      Sun Apr 23 14:39:35 2006  rra@stanford.edu - Ticket 3685: Correspondence added    
     
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: 

rainer weikusat@sncag com via RT <rt-comment@krbdev.mit.edu> writes:
> Russ Allbery <rra@stanford.edu> writes:

>> 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)

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.

> 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/>


Download (untitled) 1.2k
      Sun Apr 23 14:44:34 2006  rra@stanford.edu - Ticket 3685: Correspondence added    
     
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:
> rainer weikusat@sncag com via RT <rt-comment@krbdev.mit.edu> writes:

>> 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.

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/>


Download (untitled) 841b
      Sun Apr 23 15:23:51 2006  rra@stanford.edu - Comments added    
     
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: 

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/>


Download (untitled) 1.2k
      Tue Jun 13 10:08:33 2006  rra - Ticket 3685: Ticket 3685 MergedInto ticket 3549.    
      Tue Jun 13 10:14:30 2006  rra - Status changed from new to resolved    
      Tue Jun 13 10:14:32 2006  rra - Version_reported 1.4.3 added    
      Tue Jun 13 10:14:32 2006  rra - Given to rra    
      Tue Jun 13 10:14:33 2006  rra - Component krb5-libs added    
      Tue Jun 13 10:14:33 2006  rra - Correspondence added    
     
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


Download (untitled) 199b
      Mon Jun 19 21:33:44 2006  tlyu - Version_Fixed 1.5 added