reject out-of-range transition times in TimeZoneInfo::Load#352
Conversation
Extreme times slip past the sentinel guards and overflow the reverse-conversion arithmetic in MakeSkipped()/MakeRepeated().
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
devbww
left a comment
There was a problem hiding this comment.
Looks good (although I'm not sure that avoiding overflow with "hostile" zoneinfo files is a big concern).
That said, I don't believe the "first-half" and "second-half" no-op transitions that we conditionally add, and as mentioned in the PR description, have anything to do with this issue. So, I'd update the description to something more like the comment you added.
|
Right, the conditional sentinels aren't the culprit; an extreme stored transition overflows on its own once it becomes the bound in MakeTime(). Reworded the description along the lines of the code comment. Agreed it's a narrow concern; I hit it while poking at the same untrusted-file surface as #345/#349. |
OK, looks good. Thanks. Note, however, that the "first half" and "second half" no-op transitions that we add when there aren't existing ones, are not "sentinels". They don't form a boundary. They are only there so that the difference between two adjacent transitions is always representable. The only mention of "sentinel" in the code concerns zic-added transitions, which were done away with in tz2018f. At some point we can remove those allowances from |
|
Fair point on the terminology. Reworded the comment (and the description) to refer to the no-op transition times instead, so "sentinel" won't outlive that NextTransition()/PrevTransition() cleanup. |
A valid zoneinfo file keeps its transition times far from the int64 limits, but a crafted TZif blob can place one at the extremes. When that transition becomes the bounding transition during the reverse conversions in MakeTime(), MakeSkipped()/MakeRepeated() compute tr.unix_time - 1 and tr.unix_time +/- a sub-day civil delta, overflowing the signed 64-bit time (UBSan flags it at both ends, from a transition at INT64_MIN and one at INT64_MAX).
Bound the decoded transition times to +/-(1<<59), the times the loader already uses for the no-op transitions it adds, and reject anything outside. Real zoneinfo transitions live many orders of magnitude inside that range, so valid input is untouched, and checking at decode time keeps every downstream conversion representable without spreading guards across the lookup hot path.