Reviewing an Integer Overflow in MIT Kerberos

Reviewing a known Kerberos vulnerability

I completed this secure-programming review in February 2024 using MIT Kerberos 1.16 and the public record for CVE-2018-5709. I was analysing a known vulnerability, not claiming its original discovery.

The assessment brief placed the work in a fictional energy-sector company that used Kerberos and had no dedicated application-security role. I approached it as a development-team problem: explain the weakness to engineers, recommend a review and testing process, trace one published CVE through the code, and show where the invalid state should be rejected.

Kerberos is central to authentication in many Active Directory environments, so a parser defect in its database tooling is more than an isolated C mistake. The exercise was to follow the data through the source, identify the unsafe conversion, and connect the code-level fix to a repeatable secure-development practice.

The vulnerable component was not the normal ticket exchange itself. It was the database dump loader used by administrative tooling. That distinction shaped the threat model: the input was a structured file handled by a privileged maintenance path, and the parser treated its numeric fields as valid before proving that they fitted the types used internally.

The integer conversion

The vulnerable path handled a Kerberos database dump. In src/include/kdb.h, the krb5_db_entry structure stored n_key_data as a signed 16-bit integer. In src/kadmin/dbutil/dump.c, the process_k5beta7_princ function read several values from the dump into unsigned integers, including a 32-bit value represented by u4.

The relevant structure member was:

krb5_int16 n_key_data;

The parser declared the input fields as ordinary unsigned integers and populated them with fscanf():

unsigned int u1, u2, u3, u4, u5;

nread = fscanf(
    filep,
    "%u\t%u\t%u\t%u\t%u\t",
    &u1, &u2, &u3, &u4, &u5
);

The critical assignment moved u4 into n_key_data. Without checking the range first, a dump value larger than the signed 16-bit maximum could not be represented correctly in the destination. The value could wrap or be truncated, leaving later allocation and processing code working from a different size than the input supplied.

For the types used in this branch, the significant boundary is INT16_MAX, or 32,767. The input variable can represent values far above that boundary, whereas n_key_data cannot. A value of 32,768 is therefore syntactically valid for %u but invalid for this structure member. The parser has to reject it before the assignment; checking only that fscanf() accepted an unsigned number is insufficient.

That is the CWE-190 pattern in this case: the source and destination integer types have different ranges, and untrusted file data crosses the boundary without validation.

Integer overflow is distinct from writing a long string directly beyond a character buffer. The immediate fault is numeric: a valid 32-bit value becomes a different 16-bit value. The memory-safety risk appears when that corrupted count controls allocation, indexing, copying, or loop bounds.

This is also why the defect is easy to describe imprecisely as a “buffer size” problem. The first failed invariant is not the physical size of a character array. It is that a record count read from the file must have one consistent, representable value wherever the parser stores and consumes it.

Tracing the effect through the parser

The assignment only makes sense when followed into the surrounding parser. n_key_data describes how many key-data records belong to a database entry. It therefore influences allocation and subsequent processing rather than remaining an unused bookkeeping value.

A secure review has to follow that relationship. Looking only for a visibly unsafe memory copy would miss the earlier type conversion that makes the later state inconsistent. The same principle applies to lengths, counts, offsets, and allocation sizes throughout C and C++ code.

The dump file was also being treated as trusted input. Database utilities are often run by administrators, but that does not make every file they ingest trustworthy or structurally valid. Import and recovery tools need the same boundary checks as network-facing parsers.

The source trail crossed two files and several assumptions:

dump field -> unsigned 32-bit u4 -> signed 16-bit n_key_data
           -> allocation and key-data parsing

That cross-file relationship explains why a simple search for copying functions would not find the defect. The dangerous state is created at the conversion boundary and consumed later.

My review treated the count as an invariant shared across the whole operation:

declared records in file
  = representable count in krb5_db_entry
  = number of allocated krb5_key_data elements
  = maximum number of records parsed into that allocation

If any stage uses a different interpretation, the parser can allocate for one count and iterate according to another. The same review pattern applies to packet lengths, image dimensions, archive entry counts, protocol offsets, and multiplication used to calculate allocation size.

The dump reader also has more failure cases than the overflow alone. It has to confirm that the expected number of fields were read, reject malformed separators or missing values, handle zero deliberately, prevent multiplication overflow when deriving byte sizes, check allocation failure, stop cleanly when the file ends early, and release partially constructed entries on every error path. The CVE fix addresses the type boundary at the point where it becomes unsafe; the surrounding parser still needs those wider invariants.

Bounds checking before allocation

The mitigation I reviewed rejected u4 when it exceeded INT16_MAX before assigning it to the structure. It then attempted allocation only for a non-zero, accepted count and checked whether calloc returned NULL.

if (u4 > INT16_MAX) {
    load_err(fname, *linenop, _("invalid key_data size"));
    goto fail;
}

if (u4 && (kp = calloc(u4, sizeof(krb5_key_data))) == NULL)
    goto fail;

That order is important:

1. validate the input against the destination type
2. reject values the program cannot represent
3. allocate from the validated count
4. handle allocation failure before processing records

The check is narrow, but it restores an invariant the rest of the parser expects: the key-data count stored in the structure is the same count used to size and process the associated allocation.

The failure path is part of the fix. load_err() associates the invalid size with the source filename and line number, then control moves to the common cleanup path. Silently clamping the value would be weaker because the loader would continue with a database entry different from the one represented in the input. Rejecting the record preserves both memory safety and data integrity.

Using calloc() also does two jobs: it allocates space for u4 elements and initialises the resulting storage to zero. It does not validate that u4 is meaningful for the destination structure, so allocation cannot replace the explicit range check. In other parsers, a review should also verify that count * sizeof(element) cannot overflow the allocator’s size type.

Manual review and automated analysis

The wider submission compared static review, dynamic testing, and fuzzing as complementary software-security techniques. For this CVE, manual data-flow review was the most useful part because the source and destination types were defined in different files and the security effect appeared later in processing.

A compiler warning, static analyser, or targeted fuzz harness can help identify this class of failure, but each needs enough build and input context. Flawfinder, discussed in the submission, is a source-code scanner rather than a fuzzer; it can flag risky C functions and support review, but a multi-file type-and-data-flow defect may require deeper analysis. Fuzzing is the dynamic part: execute the built parser repeatedly with generated or mutated dump records and observe failures.

A useful test would exercise dump values around the type boundary and assert that invalid counts fail cleanly instead of reaching allocation or record parsing.

For this field, a focused boundary set would include:

Input class	Expected result
zero	accepted as no key-data records
small valid count	allocation and parsing continue
INT16_MAX	accepted if the remaining file is structurally valid
INT16_MAX + 1	rejected before conversion
maximum unsigned value	rejected before conversion
missing or malformed field	parser reports an input error

The test needs a structurally valid surrounding dump record. Random bytes alone may never reach the conversion because the parser rejects an earlier field.

I would begin with a minimal valid beta-7 principal record and make the key-data count the fuzzed field. Seeds would cover zero, one record, several records, the exact signed boundary, and malformed records. Mutations could then alter the count, truncate subsequent key-data fields, duplicate separators, or make the declared and available record counts disagree. That gives the fuzzer a realistic route to the target branch.

The runtime build should enable integer, address, and undefined-behaviour sanitisation where the compiler supports them. A crash or sanitizer finding needs the exact input, build revision, stack trace, and parser configuration retained so that the defect can be reproduced. Once fixed, that input becomes a regression test.

In a development pipeline I would combine:

• warnings for narrowing conversions and signedness changes
• static analysis on parser and allocation paths
• unit tests at integer boundaries
• fuzzing of the database-dump grammar
• review rules for counts, lengths, and offsets crossing type boundaries

Putting the review into a development process

The wider secure-programming submission considered code review and fuzzing together. Code review can follow types and invariants across files, while fuzzing can repeatedly test the compiled parser with malformed and boundary-case records.

I would place the checks at several points:

• a pull-request rule flags implicit narrowing in security-sensitive parsers
• unit tests cover each integer boundary and failure path
• a grammar-aware fuzz target drives the database-dump loader
• sanitised builds catch memory errors reached during fuzzing
• CI stores the crashing input as a regression test
• reviewers confirm that allocation, loop, and copy counts come from the same validated value

Those checks can be staged without making every build equally expensive. Compiler warnings and focused unit tests belong in each pull request. Static analysis can run on changed code or nightly, depending on cost. A short fuzzing budget can run in CI, while longer campaigns execute separately and report newly discovered inputs back to the development team.

The review checklist should ask concrete questions:

• Is every external length or count validated before conversion?
• Is the destination type wide enough for all accepted values?
• Are signed and unsigned comparisons deliberate?
• Can addition or multiplication overflow before allocation?
• Do allocation, loop, and copy operations use the same validated count?
• Does malformed input reach one cleanup path without leaving partial state?
• Is the rejected boundary represented by an automated test?

This turns one CVE into a reusable review pattern. Any external count that changes type before controlling memory deserves the same trace.

Secure-development takeaway

The review gave me a concrete example of why input validation is not only string sanitisation. An integer can be syntactically valid and still be unsafe for the type or operation that receives it.

It also connected a local implementation detail to system behaviour. Kerberos authentication may be the visible service, but maintenance tools, import formats, and administrative parsers are part of the same security boundary. Reviewing those paths requires attention to type width, signedness, allocation, error handling, and the assumptions carried between functions.

The assignment also sharpened the difference between finding a known vulnerable line and building a prevention process. The bounds check resolves this specific conversion. The reusable work is defining the invariant, testing both sides of the boundary, instrumenting the compiled parser, retaining failing inputs, and reviewing every place where untrusted numeric data controls memory.

This page reflects the MIT Kerberos 1.16 code and public CVE material I reviewed in 2024. It is a secure-code review of that historical branch, not a statement about current Kerberos releases.