Skip to content

parse_tz: reject implausibly large timecnt/typecnt/charcnt header fields#174

Open
94xhn wants to merge 2 commits into
derickr:masterfrom
94xhn:fix-parse-tz-header-count-overflow
Open

parse_tz: reject implausibly large timecnt/typecnt/charcnt header fields#174
94xhn wants to merge 2 commits into
derickr:masterfrom
94xhn:fix-parse-tz-header-count-overflow

Conversation

@94xhn

@94xhn 94xhn commented Jul 18, 2026

Copy link
Copy Markdown

Problem

`read_32bit_header()` and `read_64bit_header()` in `parse_tz.c` read the `timecnt`/`typecnt`/`charcnt` fields directly from the TZif file header and pass them, unvalidated, down the call chain to `timelib_malloc()`/`memcpy()` in `read_64bit_transitions()` and `read_64bit_types()`.

There is no check that these counts are plausible, and no check against the actual remaining length of the input buffer -- the real on-disk file length is known at index-build time via `fstat()` in `read_tzfile()` (`parse_zoneinfo.c`), but it's only used to size the shared data blob and is not threaded through to the parser.

A corrupted or truncated tzfile (e.g. a system zoneinfo file cut off mid-write, or any other malformed TZif input reaching `timelib_parse_tzfile()`) that claims a huge `timecnt` causes `memcpy()` to read far past the end of the actual buffer -- a heap out-of-bounds read that can crash the process or leak adjacent heap memory into the parsed timezone data.

Fix

Add a single sanity bound (`TIMELIB_TZINFO_MAX_COUNT = 1000000`) on `timecnt`/`typecnt`/`charcnt` in both header readers, following the same pattern as the existing "transitions do not increase" sanity check. Both readers now return an `int` error code (previously `void`), and `timelib_parse_tzfile()` bails out early with a new `TIMELIB_ERROR_CORRUPT_HEADER_COUNTS` error if the bound is exceeded, instead of trusting the header values.

Real-world TZif files are nowhere near this bound -- I checked the bundled IANA tzdata for the zones with historically the most transitions (Europe/London: 159, America/New_York: 175, Pacific/Apia: 26, Europe/Moscow: 78, Africa/Casablanca: 72, ...) and all are under 200, three to four orders of magnitude below the new limit.

Verification

Built a standalone reproduction: a crafted 88-byte TZif buffer with `timecnt=10000000`.

  • Against the current code: `timelib_parse_tzfile()` segfaults (confirmed with gcc + gdb backtrace landing in the `memcpy` inside `read_64bit_transitions()`).
  • Against this patch: the same input is cleanly rejected with `error_code == TIMELIB_ERROR_CORRUPT_HEADER_COUNTS`, no crash.
  • Regression: parsed all zones in the bundled `timezonedb.h` used in the check above via `timelib_builtin_db()` -- all succeed with `error_code == TIMELIB_ERROR_NO_ERROR` and correct `timecnt`/`typecnt`/`charcnt`, confirming the new bound doesn't reject any legitimate data.

Happy to adjust the bound value or take a different validation approach (e.g. checking against the actual buffer end, if you'd rather thread that through) if you'd prefer.

read_32bit_header() and read_64bit_header() take the timecnt/typecnt/
charcnt fields straight from the TZif file header and use them, further
down the call chain, as sizes for timelib_malloc()/memcpy() in
read_64bit_transitions() and read_64bit_types() -- with no check that
these counts are plausible for a real timezone file, and no check
against the actual remaining length of the input buffer (which is not
tracked past index-build time in parse_zoneinfo.c).

A corrupted or truncated tzfile -- e.g. a system zoneinfo file cut off
mid-write, or any other source of malformed TZif input -- that claims
a huge timecnt (or typecnt/charcnt) causes memcpy() to read far past
the end of the actual buffer, a heap out-of-bounds read that can crash
the process or leak adjacent heap memory into the parsed timezone data.

Add a single sanity bound (1,000,000) on these three fields in both
header readers, matching the pattern already used by the transitions
sanity check ("transitions do not increase"): reject the file with a
new TIMELIB_ERROR_CORRUPT_HEADER_COUNTS error code instead of using the
values. Real-world TZif files are nowhere near this bound -- the
largest transition counts in the bundled IANA tzdata are under 200
(verified against Europe/London, America/New_York, Pacific/Apia,
Europe/Moscow, and others) -- so this cannot reject any legitimate
file while still stopping the demonstrated out-of-bounds read.

Verified with a standalone reproduction: a crafted 88-byte TZif buffer
with timecnt=10000000 segfaults timelib_parse_tzfile() on the
unpatched code, and is cleanly rejected with
TIMELIB_ERROR_CORRUPT_HEADER_COUNTS after this change, with zero
regressions against the bundled real-world timezone data.

@derickr derickr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this patch. There indeed could be some extra checks in the parser.

Ideally, it's made totally safe from read errors, but that's not going to be trivial with the current implementation (especially because we don't record the length of reach tz file in the built-in database in tzdatabase.h). Also ideally, I would have a fuzzer here, but that's even more of a large task.

I do have some requests for changes here, as the limits can be tightened much further than your suggestion of 100000.

Comment thread parse_tz.c Outdated
* abbreviation bytes; a header claiming otherwise is corrupt or malicious
* input, and must be rejected before its counts are used for allocation
* and copying sizes. */
#define TIMELIB_TZINFO_MAX_COUNT 1000000

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This define should probably move to timelib_private.h, and the values can be restricted further.

The original IANA tzdb code, has the following for the count numbers (in tzfile.h:

#ifndef TZ_MAX_TIMES
/* The following limit applies to localtime.c; zic has no such limit.
   The limit must be at least 310 for Asia/Hebron with 'zic -b fat'.  */
# define TZ_MAX_TIMES 2000
#endif /* !defined TZ_MAX_TIMES */

#ifndef TZ_MAX_TYPES
/* This must be at least 18 for Europe/Vilnius with 'zic -b fat'.  */
# define TZ_MAX_TYPES 256 /* Limited to 256 by Internet RFC 9636.  */
#endif /* !defined TZ_MAX_TYPES */

#ifndef TZ_MAX_CHARS
/* This must be at least 40 for America/Anchorage.  */
# define TZ_MAX_CHARS 256   /* Maximum number of abbreviation characters */
                /* (limited to 256 by Internet RFC 9636) */
#endif /* !defined TZ_MAX_CHARS */

It would be safe to use these values therefore.

Comment thread timelib.c
"The embedded POSIX string is not valid",
"The embedded POSIX string is empty"
"The embedded POSIX string is empty",
"Corrupt tzfile: The header contains an implausibly large count field"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the three different fields how have different limits, the error messages ought to be three separate ones too, one for each of the fields.

Comment thread timelib.h
#define TIMELIB_ERROR_SLIM_FILE 0x07 /* Warns if the file is SLIM, but we can't read it */
#define TIMELIB_ERROR_CORRUPT_POSIX_STRING 0x08
#define TIMELIB_ERROR_EMPTY_POSIX_STRING 0x09 /* Warns if the POSIX string is empty, but still produces results */
#define TIMELIB_ERROR_CORRUPT_HEADER_COUNTS 0x0A

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you add the two other error codes (and change the name for this one), can you add a comment for each of them too?

- typecnt is capped at 256, the hard architectural ceiling: each transition's
  type is stored as a single byte index (see skip_32bit_transitions()/
  skip_64bit_transitions()), so no valid file can ever reference a type
  beyond that.
- timecnt and charcnt don't have a format-derived bound, so they're capped
  at 10000 and 2000 respectively based on scanning every file in the IANA
  tzdata distribution (both v1 and v2/64-bit header counts): the real-world
  worst case is 310 transitions (Asia/Hebron) and 40 abbreviation bytes
  (America/Anchorage), so these limits leave generous headroom for future
  rule changes while rejecting anything implausible.
@94xhn

94xhn commented Jul 23, 2026

Copy link
Copy Markdown
Author

Thanks for the feedback — you're right, 100000 (well, actually it was 1000000, but same point) was way looser than reality warrants. I tightened it in 8e42d51:

  • typecnt is now capped at 256, which isn't a guess — it's the hard ceiling the format itself imposes. Each transition's type is a single-byte index into the types array (see skip_32bit_transitions()/skip_64bit_transitions()), so no valid file could ever have more than 256 types; anything above that is unambiguously corrupt.
  • timecnt and charcnt don't have a format-derived bound like that, so I scanned every file in the IANA tzdata distribution (all 499 zones, both the v1 and v2/64-bit header counts) to see what real data actually looks like. Worst case: 310 transitions (Asia/Hebron) and 40 abbreviation bytes (America/Anchorage). I set the caps at 10000 and 2000 — generous headroom for decades of future rule changes, but nowhere near '100000'.

I also re-ran the scan against the new caps to confirm zero real zoneinfo files would be wrongly rejected, and re-verified the corrupt-header rejection path still works for the actual bug (the original crafted count that started this PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants