Skip Menu |
 

Date: Fri, 12 Jun 2020 18:52:52 +0000
To: "krb5-bugs@mit.edu" <krb5-bugs@mit.edu>
From: "Joshua Neuheisel" <jneuheisel@stsci.edu>
Subject: Invalid negative record length in keytab file

I’ve found an unlikely case where keytab record lengths produce unexpected behavior. The following snippet of code, when run in bash, will create a 2GB file called bad.keytab. When this file is read (read_kt) using ktutil, the process will enter an infinite loop:

(echo -ne '\05\02\0200\0\0\010'; dd if=/dev/zero count=21474 bs=100000; dd if=/dev/zero count=1 bs=83640; echo -ne '\0200\0\0\0') >bad.keytab

 

The reason is the code in krb5_ktfileint_internal_read_entry (kt_file.c:924 in krb5-1.18.2.tar.gz) assumes that the 2s compliment of a negative 32bit integer is always positive. This is not true for (int32_t) 0x80000000.

 

Given the wording of https://web.mit.edu/kerberos/krb5-1.18/doc/formats/keytab_file_format.html, it’s not clear to me what the expected behavior should be, although an infinite loop is definitely undesirable. I’d be happy to help supply a patch if it’s clear what the expected behavior should be.

 

Thanks,

Joshua Neuheisel

 

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.
 
Subject: Re: [krbdev.mit.edu #8914] Invalid negative record length in keytab file
Date: Tue, 16 Jun 2020 01:06:38 +0000
From: "Joshua Neuheisel" <jneuheisel@stsci.edu>
To: "rt@krbdev.mit.edu" <rt@krbdev.mit.edu>
Download (untitled) / with headers
text/plain 2.4KiB
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.



I think it would be better to just throw an error; that's a lot of code to spend on an edge case.
 
Date: Fri, 3 Jul 2020 01:18:24 +0000
From: "Joshua Neuheisel" <jneuheisel@stsci.edu>
Subject: Re: [krbdev.mit.edu #8914] Invalid negative record length in keytab file
To: "rt@krbdev.mit.edu" <rt@krbdev.mit.edu>
Download (untitled) / with headers
text/plain 1.4KiB
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')

Subject: git commit
From: ghudson@mit.edu

Avoid backward seeks when reading keytab files

When considering or bypassing an empty record in a keytab file, check
for a lenth of INT32_MIN. Otherwise we could perform a backwards
seek, as the inverse of INT32_MIN is still negative.

[ghudson@mit.edu: adjusted comments; wrote commit message]

https://github.com/krb5/krb5/commit/99f7ad2831a01f264c07eed42a0a3a9336b86184
Author: Joshua Neuheisel <jneuheisel@stsci.edu>
Committer: Greg Hudson <ghudson@mit.edu>
Commit: 99f7ad2831a01f264c07eed42a0a3a9336b86184
Branch: master
src/lib/krb5/keytab/kt_file.c | 4 ++++
src/tests/t_keytab.py | 9 ++++++++-
2 files changed, 12 insertions(+), 1 deletions(-)