Fix keytab parser panics and signed record-length framing (#1)#3
Merged
Conversation
The parser indexed fixed offsets and sliced variable-length fields without validating buffer lengths, ignored the error returned by every sub-parser, read the per-record length as unsigned, and advanced to the next record by the number of bytes consumed rather than by the record length. As a result it panicked on truncated input and on hole records, and mis-parsed valid files that contain holes or trailing padding. Add bounds checks before every read, propagate sub-parser errors, treat the record length as a signed int32 (negative = zero-filled hole, 0 = end of file), and advance by the record length field. Fixes #1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked Issue
Closes #1Root Cause
The record-parsing read path made two flawed assumptions. First, it trusted the input to be well-formed: every fixed-offset index (
data[0:4],data[0:2],data[0]) and every variable-length slice (data[:c.Length],data[:k.Size]) ran without checking that enough bytes remained, and theerrorreturned by each sub-parser was discarded. Second, it misread the record framing: the per-record length, which the keytab format defines as a signed 32-bit value, was read asuint32, and the next-record offset was computed from the bytes the entry fields happened to consume (data[entry.RawBytesSize:]) instead of from the record length. A negative length (a zero-filled hole) therefore became a huge unsigned size and triggered a slice-bounds panic, and any padding between the parsed fields and the record boundary desynchronized parsing.Fix Description
CountedOctetString.FromBytes,KeyBlock.FromBytes, andKeytabEntry.FromBytesnow validatelen(data)before each read and return a descriptive error instead of panicking, and they propagate the errors returned by the sub-parsers they call.Keytab.FromBytesreads the per-record length as a signedint32and interprets it per the file format: a positive value is a record of that size, a negative value is a zero-filled hole that is skipped, and0marks end-of-file. It advances by the record length field rather than by consumed bytes, and validates the header and each record length against the remaining buffer.How Verified
go test ./...passes, including new tests.keytab describe -f example.ktstill parses all three entries.head -c 50 example.kt > truncated.kt && keytab describe -f truncated.ktnow printsError parsing keytab file: keytab entry at offset 2 claims 62 bytes but only 48 remaininstead of panicking.Test Coverage
src/keytab/Keytab_parsing_test.go:Test_Keytab_TruncatedInputReturnsError— parsing every truncation prefix of a valid file never panics.Test_Keytab_HoleIsSkipped— a negative-length hole record is skipped and the following entry parses.Test_Keytab_EndOfFileMarker— a zero-length record terminates parsing and trailing bytes are ignored.Scope of Change
src/keytab/Keytab.go,src/keytab/KeytabEntry.go,src/keytab/KeyBlock.go,src/keytab/CountedOctetString.go,src/keytab/Keytab_parsing_test.goRisk and Rollout
Local to the parse path; output of
ToBytes/serialization is unchanged. Safe to merge directly.