avoid signed overflow computing start offset in AndroidZoneInfoSource#356
Conversation
| if (start < 0 || length < 0) break; | ||
| ebuf[40] = '\0'; // ensure zone name is NUL terminated | ||
| if (strcmp(name.c_str() + pos, ebuf) == 0) { | ||
| if (fseek(fp.get(), static_cast<long>(start), SEEK_SET) != 0) break; |
There was a problem hiding this comment.
Should we then also be checking that the newly-widened start fits into a long?
There was a problem hiding this comment.
Good point, done. long is still 32 bits on LLP64 targets, so the cast at the fseek() could truncate a large start; the guard now also rejects entries with start > std::numeric_limits<long>::max(). Where long is 64 bits the check folds away, and gcc/clang stay quiet about it under the CI's -Wall/-Wextra.
| static_cast<std::int_fast64_t>(data_offset) + Decode32(ebuf + 40); | ||
| const std::int_fast32_t length = Decode32(ebuf + 44); | ||
| if (start < 0 || length < 0) break; | ||
| if (start < 0 || start > std::numeric_limits<long>::max() || length < 0) |
There was a problem hiding this comment.
It looks like somewhere along the way (2017), we failed to add #include <limits> to this file, so we might as well do it now.
There was a problem hiding this comment.
Done, added to the include block.
| const std::int_fast32_t length = Decode32(ebuf + 44); | ||
| if (start < 0 || length < 0) break; | ||
| if (start < 0 || start > std::numeric_limits<long>::max() || length < 0) | ||
| break; // fseek() takes a long |
There was a problem hiding this comment.
This new comment only applies to one of the conditionals, so I'd either just remove the comment, or separate the "fseek() takes a long" conditional into its own statement.
There was a problem hiding this comment.
Went with the split. The long-range check is now its own statement with the comment on it, and the original start/length check reads the same as before the PR.
| if (fread(ebuf, 1, sizeof(ebuf), fp.get()) != sizeof(ebuf)) break; | ||
| const std::int_fast32_t start = data_offset + Decode32(ebuf + 40); | ||
| const std::int_fast64_t start = | ||
| static_cast<std::int_fast64_t>(data_offset) + Decode32(ebuf + 40); |
There was a problem hiding this comment.
Following an idea from #360, we're better off using brace initialization instead of static_cast for a widening integer conversion. That is, std::int_fast64_t{data_offset}.
There was a problem hiding this comment.
Done, switched to std::int_fast64_t{data_offset}. Nice property that it'd fail to compile if the conversion ever became narrowing.
devbww
left a comment
devbww
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks.
We'll have to wait for a Google owner to initiate the tests.
4 participants
AndroidZoneInfoSource::Open decodes each index entry's absolute data offset as
data_offset + Decode32(ebuf + 40), where both operands come from a loaded, untrusted tzdata database.std::int_fast32_tis 32 bits on Bionic, macOS, and Windows, so a crafted index withdata_offsetand an entry offset nearINT32_MAXoverflows the addition before thestart < 0guard ever runs (UBSan flags2147483647 + 2147483646).Do the sum in
int_fast64_tbefore the range check, the same way the TZif reader inLoad()guards its decoded values. Real tzdata offsets stay well underINT32_MAX, so valid zones are unaffected; only the hostile case changes, and it now fails the bounds check cleanly instead of relying on undefined behavior.