[krbdev.mit.edu #6475] Adding keys to malformed keytabs can infinitely extend the file

[Roland C. Dowdeswell via RT]

I have notice that in some conditions, krb5_ktfileint_find_slot() will
get into an infinite loop extending a keytab until it fills the disk.

To recreate this error condition, append a few zeros to the end of an
existing keytab, e.g.:

	$ perl -e 'printf("%c%c%c%c", 0, 0, 0, 0)' >> /tmp/keytab
	$ perl -e 'printf("%c%c%c%c", 0, 0, 0, 0)' >> /tmp/keytab
	$ perl -e 'printf("%c%c%c%c", 0, 0, 0, 0)' >> /tmp/keytab
	$ perl -e 'printf("%c%c%c%c", 0, 0, 0, 0)' >> /tmp/keytab

And then attempt to add a key to it.

I attach a patch which addresses the issue and simplifies the code a
bit.  It could certainly be simplified a little more, of course...

I have only yet run cursory testing on the patch and it appears to
address the issue.

--
    Roland Dowdeswell                      http://Imrryr.ORG/~elric/

Index: krb5/keytab/kt_file.c
===================================================================
RCS file: /ms/dev/kerberos/mitkrb5/cvs-dirs/mitkrb5-1.4/mitkrb5/src/lib/krb5/keytab/kt_file.c,v
retrieving revision 1.1.1.1
diff -u -u -r1.1.1.1 kt_file.c
--- krb5/keytab/kt_file.c	28 Mar 2005 21:43:35 -0000	1.1.1.1
+++ krb5/keytab/kt_file.c	22 Apr 2009 03:43:50 -0000
@@ -1604,11 +1604,8 @@
 krb5_ktfileint_find_slot(krb5_context context, krb5_keytab id, krb5_int32 *size_needed, krb5_int32 *commit_point)
 {
     krb5_int32      size;
-    krb5_int32      remainder;
     krb5_int32      zero_point;
     krb5_kt_vno     kt_vno;
-    krb5_boolean    found = FALSE;
-    char            iobuf[BUFSIZ];
 
     KTCHECKLOCK(id);
     /*
@@ -1621,7 +1618,7 @@
         return KRB5_KT_IOERR;
     }
 
-    while (!found) {
+    for (;;) {
         *commit_point = ftell(KTFILEP(id));
         if (!xfread(&size, sizeof(size), 1, KTFILEP(id))) {
             /*
@@ -1632,86 +1629,62 @@
             /* fseek to synchronise buffered I/O on the key table. */
 	    /* XXX Without the weird setbuf hack, can we nuke this now?  */
             if (fseek(KTFILEP(id), 0L, SEEK_CUR) < 0)
-            {
                 return errno;
-            }
-	    
-#ifdef notdef
-	    /* We don't have to do this because htonl(0) == 0 */
-	    if (KTVERSION(id) != KRB5_KT_VNO_1)
-		    size = htonl(size);
-#endif
-	    
-            if (!xfwrite(&size, sizeof(size), 1, KTFILEP(id))) {
+
+            if (!xfwrite(&size, sizeof(size), 1, KTFILEP(id)))
                 return KRB5_KT_IOERR;
-            }
-            found = TRUE;
+	    break;
         }
 
 	if (KTVERSION(id) != KRB5_KT_VNO_1)
 		size = ntohl(size);
 
-        if (size > 0) {
-            if (fseek(KTFILEP(id), size, SEEK_CUR)) {
-                return errno;
-            }
-        } else if (!found) {
-            size = -size;
-            if (size >= *size_needed) {
-                *size_needed = size;
-                found = TRUE;	
-            } else if (size > 0) {
-                /*
-                 * The current hole is not large enough, so skip it
-                 */
-                if (fseek(KTFILEP(id), size, SEEK_CUR)) {
-                    return errno;
-                }
-            } else {
-
-                 /* fseek to synchronise buffered I/O on the key table. */
-
-                 if (fseek(KTFILEP(id), 0L, SEEK_CUR) < 0)
-                 {
-                     return errno;
-                 }
-
-                /*
-                 * Found the end of the file (marked by a 0 length buffer)
-                 * Make sure we zero any trailing data.
-                 */
-                zero_point = ftell(KTFILEP(id));
-                while ((size = xfread(iobuf, 1, sizeof(iobuf), KTFILEP(id)))) {
-                    if (size != sizeof(iobuf)) {
-                        remainder = size % sizeof(krb5_int32);
-                        if (remainder) {
-                            size += sizeof(krb5_int32) - remainder;
-                        }
-                    }
-
-                    if (fseek(KTFILEP(id), 0L, SEEK_CUR) < 0)
-                    {
-                        return errno;
-                    }
-
-                    memset(iobuf, 0, (size_t) size);
-                    xfwrite(iobuf, 1, (size_t) size, KTFILEP(id));
-		    fflush(KTFILEP(id));
-                    if (feof(KTFILEP(id))) {
-                        break;
-                    }
-
-                    if (fseek(KTFILEP(id), 0L, SEEK_CUR) < 0)
-                    {
-                        return errno;
-                    }
-
-                }
-                if (fseek(KTFILEP(id), zero_point, SEEK_SET)) {
-                    return errno;
-                }
-            }
-        }
+	/* Positive size indicates full, negative indicates empty. */
+
+	if (-size >= *size_needed) {
+	    /* We found a slot which is large enough, return it */
+	    *size_needed = size;
+	    break;
+	}
+
+	/* fseek to synchronise buffered I/O on the key table. */
+	if (fseek(KTFILEP(id), 0L, SEEK_CUR) < 0)
+	     return errno;
+
+	if (size != 0) {
+	    /* Hole is either full or too small, skip it... */
+	    if (fseek(KTFILEP(id), size>0?size:-size, SEEK_CUR) < 0)
+		 return errno;
+	    continue;
+	}
+
+	/*
+	 * Found the end of the file (marked by a 0 length buffer)
+	 * Make sure we zero enough space to contain both our new
+	 * key and include sizeof(krb5_int32) of zero's afterwards
+	 * just in case there are additional extra bytes further on
+	 * in the file...
+	 */
+
+	zero_point = ftell(KTFILEP(id));
+	while (size < (*size_needed + sizeof(krb5_int32))) {
+	    size_t	bufsiz;
+	    char    iobuf[BUFSIZ];
+
+	    bufsiz = (*size_needed + sizeof(krb5_int32));
+	    if (bufsiz > sizeof(iobuf))
+		bufsiz = sizeof(iobuf);
+
+	    memset(iobuf, 0, (size_t) size);
+	    xfwrite(iobuf, 1, bufsiz, KTFILEP(id));
+	    fflush(KTFILEP(id));
+	    if (fseek(KTFILEP(id), 0L, SEEK_CUR) < 0)
+		return errno;
+	    size += bufsiz;
+	}
+	if (fseek(KTFILEP(id), zero_point, SEEK_SET))
+	    return errno;
+	break;
     }
 
     return 0;