Ticket History #8914: Invalid negative record length in keytab file

What about something like this? It keeps the spirit of the second approach without changing the file calls. When reading for the next record, use two fseek calls to skip past these odd records. Ignore them as well when looking for a record to overwrite. It's not a complete patch as it has no tests - and I still need to look for any other places this logic may be lurking.

If nothing else it stops the infinite loop.

--- /krb5-1.18.2.orig/src/lib/krb5/keytab/kt_file.c 2020-05-22 00:21:40.000000000 +0000
+++ /krb5-1.18.2/src/lib/krb5/keytab/kt_file.c 2020-06-15 11:02:30.107034312 +0000
@@ -921,6 +921,16 @@
size = ntohl(size);

if (size < 0) {
+ /* -INT32_MIN == INT32_MIN */
+
+ if (size == INT32_MIN) {
+ if (fseek(KTFILEP(id), 1, SEEK_CUR)) {
+ return errno;
+ }
+
+ size++;
+ }
+
if (fseek(KTFILEP(id), -size, SEEK_CUR)) {
return errno;
}
@@ -1352,6 +1362,14 @@
*size_needed = size;
break;
} else {
+ /* -INT32_MIN == INT32_MIN; skip these unusual records */
+ if (size == INT32_MIN) {
+ if (fseek(fp, 1, SEEK_CUR)) {
+ return errno;
+ }
+
+ size = -(size + 1);
+ }
if (fseek(fp, size, SEEK_CUR))
return errno;
}

On 6/12/20, 18:38, "Greg Hudson via RT" <rt@krbdev.mit.edu> wrote:

External Email - Use Caution

There are two reasonable ways to react to a length value of -2^31: throw an
error because the size of the purported hole is absurdly large, or skip forward
2^31 bytes. (The value is absurd because an entry is only a little bit longer
than the marshalled principal name, and a principal name shouldn't marshal to
anywhere near 2^31 bytes.)

To throw an error we can simply add a comparison to INT32_MIN.

Handling the value as stated would require more work. If we could assume that
long is 64 bits, it would be fairly easy; we'd just have to adjust the
marshalling code so that the variable size is of type long instead of int32_t.
But we can't assume that long is 64 bits, even in the steady state (long is
still 32 bits on 64-bit Windows), so we'd have to abandon stdio and use POSIX
I/O. That almost certainly isn't worth it.

Here's a patch to fail fast with a format error. It's not much code but still protects against this unlikely edge case.

--- /krb5-1.18.2.orig/src/lib/krb5/keytab/kt_file.c 2020-05-22 00:21:40.000000000 +0000
+++ /krb5-1.18.2/src/lib/krb5/keytab/kt_file.c 2020-07-01 19:16:42.000000000 +0000
@@ -921,6 +921,9 @@
size = ntohl(size);

if (size < 0) {
+ if (size == INT32_MIN)
+ return KRB5_KT_FORMAT;
+
if (fseek(KTFILEP(id), -size, SEEK_CUR)) {
return errno;
}
@@ -1347,6 +1350,8 @@
return errno;
} else if (size < 0) {
/* Empty record; use if it's big enough, seek past otherwise. */
+ if (size == INT32_MIN)
+ return KRB5_KT_FORMAT;
size = -size;
if (size >= *size_needed) {
*size_needed = size;
--- /krb5-1.18.2.orig/src/tests/t_keytab.py 2020-05-22 00:21:40.000000000 +0000
+++ /krb5-1.18.2/src/tests/t_keytab.py 2020-07-03 00:58:00.000000000 +0000
@@ -185,5 +185,13 @@
test_addent(realm, 'exp', '-f')
test_addent(realm, 'pexp', '-f')

+# Test for proper INT32_MIN record length handling.
+mark('invalid record length')
+f = open(realm.keytab, 'wb')
+f.write(b'\x05\x02\x80\x00\x00\x00')
+f.close()
+msg = 'Bad format in keytab while scanning keytab'
+realm.run([klist, '-k'], expected_code=1, expected_msg=msg)
+
success('Keytab-related tests')
success('Keytab-related tests')