Ticket History #6475: Adding keys to malformed keytabs can infinitely extend the file

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;

In krb5_ktfileint_find_slot, don't continue the loop when we find a
final zero-length buffer. This is a minimal fix intended to be pulled
up to the 1.7 branch; a code cleanup commit will follow.


https://github.com/krb5/krb5/commit/c395ff699651ce5ea1ef2fd106fc191937ac7e31
Commit By: ghudson
Revision: 22278
Changed Files:
U trunk/src/lib/krb5/keytab/kt_file.c

Simplify and shorten krb5_ktfileint_find_slot, and properly handle the
commit_point output parameter.


https://github.com/krb5/krb5/commit/582aabf279c16a3a55cc44c958130ada29e46e41
Commit By: ghudson
Revision: 22279
Changed Files:
U trunk/src/lib/krb5/keytab/kt_file.c

pull up r22278 from trunk

------------------------------------------------------------------------
r22278 | ghudson | 2009-04-24 15:49:54 -0400 (Fri, 24 Apr 2009) | 9 lines
Changed paths:
M /trunk/src/lib/krb5/keytab/kt_file.c

ticket: 6475
status: open
tags: pullup
target_version: 1.7

In krb5_ktfileint_find_slot, don't continue the loop when we find a
final zero-length buffer. This is a minimal fix intended to be pulled
up to the 1.7 branch; a code cleanup commit will follow.

https://github.com/krb5/krb5/commit/7b96dd5250657f261c41cef3f304247eeaed4333
Commit By: tlyu
Revision: 22328
Changed Files:
U branches/krb5-1-7/src/lib/krb5/keytab/kt_file.c