MITKRB5-SA-2007-005: kadmind vulnerable to buffer overflow

[Lee Hinman]

On Jun 27, 2007, at 3:27 PM, Mike Friedman wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Wed, 27 Jun 2007 at 13:28 (-0500), Lee Hinman wrote:
>
>> Peter,
>>
>> Just a little suggestion on your patch.  Calling error_message  
>> (ret.code) when ret.code == 0 may cause your output to be  
>> something like "Unknown error: 0".  It will depend on what your  
>> libc does when you call sterror(0).  Previously it would print out  
>> "success".  The change below restores that behavior.
>
> Lee,
>
> I guess you're referring to Russ Allbery's patch.
>
> Maybe I'm missing something, but I don't see your proposed change;  
> what you included in your email seems to be just Russ's patch as-is.

Sorry, not sure what I did.  Hopefully this will come through OK.

>
> Are you saying that 'error_message(ret.code)' should be replaced  
> with something else, because the test for (ret.code == 0) is not  
> always reliable as an indicator of success?  If so, what should be  
> used instead?

ret.code==0 is a reliable indicator for success but in one place  
ret.code is being passed to error_message without first checking to  
see if ret.code==0 (so a success).   And error_message doesn't know  
how to handle an error code of 0, so it passes it on to libc, and  
sterror may or may not know what to do with an error code of 0.

So after applying Russ Allbery's patch, my suggestion is:

diff -Naur src.orig/kadmin/server/server_stubs.c src.patched/kadmin/ 
server/server_stubs.c
--- src.orig/kadmin/server/server_stubs.c	2007-06-27  
19:44:39.000000000 -0500
+++ src.patched/kadmin/server/server_stubs.c	2007-06-27  
19:45:47.000000000 -0500
@@ -583,7 +583,8 @@
                           "%.*s%s to %.*s%s, %s, "
                           "client=%.*s%s, service=%.*s%s, addr=%s",
                           tlen1, prime_arg1, tdots1,
-                         tlen2, prime_arg2, tdots2, error_message 
(ret.code),
+                         tlen2, prime_arg2, tdots2,
+			 ((ret.code==0) ? "success" : error_message(ret.code)),
                           clen, client_name.value, cdots,
                           slen, service_name.value, sdots,
                           inet_ntoa(rqstp->rq_xprt- 
 >xp_raddr.sin_addr));

So if ret.code==0 use "success" for the sting otherwise call  
error_message with ret.code as the argument and use the return value  
for the string.

--
Lee