Security issue: a stack buffer overflow that a script in a safe interpreter can trigger through the clock alias, on trunk and in every 9.x release. It also affects any application that passes a time zone from outside data to clock format. 8.6 is not affected.
ClockFmtToken_TimeZone_Proc reserves room for 7 characters for %z (FrmResultAllocate(dateFmt, 7), tclClockFmt.c:2931), which is enough for +hhmmss. The hours are printed by Clock_itoaw with a minimum width of 2 but no maximum, so an offset of 100 hours or more prints more digits than were reserved, and the terminating NUL and possibly some digits land past the end of the result buffer. The result buffer is the 80-byte array resMem on the stack of TclClockFormat for every call whose output fits in 80 bytes, and a heap block otherwise.
All PoCs run in a plain tclsh.
Fixed offset. The fixed-offset pattern in clock.tcl (^([-+])(\d\d)(?::?(\d\d)(?::?(\d\d))?)?) does not range-check minutes or seconds, so +99:99:99 is accepted as 100 hours, 40 minutes and 39 seconds and printed as +1004039. With 72 bytes of output before it, the NUL lands one byte past resMem:
clock format 0 -timezone +99:99:99 -format "[string repeat a 72]%z"
ASan on trunk:
WRITE of size 1 ... overflows variable 'resMem' (line 3399)
#0 Clock_itoaw tclClockFmt.c:230
#1 ClockFmtToken_TimeZone_Proc tclClockFmt.c:2940
#2 TclClockFormat tclClockFmt.c:3504
SUMMARY: AddressSanitizer: stack-buffer-overflow tclClockFmt.c:230 in Clock_itoaw
The same on 9.1.0 (line 230) and 9.0.4 (line 234). POSIX rules such as ABC-99:99:99 give the same offset. Two-digit fields limit this path to exactly one NUL byte. With more output before %z, so that the buffer has been moved to the heap, the same byte lands one byte past the heap block (heap-buffer-overflow, "0 bytes after 260-byte region" with -format "[string repeat a 100]%Y[string repeat b 148]%z"). In that case TclClockFormat also writes the terminator past the block a second time (line 3559) and keeps the block as the string of the result.
Offsets from TZData. The offset of a time zone table can be any 32-bit value. For a table set by a script, the largest offset prints as +5965231407, 11 characters plus the NUL against 8 reserved, so up to four bytes of digits and the NUL are written past the buffer:
clock format 0 -gmt 1
set ::tcl::clock::TZData(:big) {{-9223372036854775808 2000000000 0 BIG}}
clock format 0 -timezone :big -format "[string repeat a 72]%z"
SUMMARY: AddressSanitizer: stack-buffer-overflow tclClockFmt.c:230 in Clock_itoaw
Same lines as item 1 on 9.1.0 and 9.0.4. An offset of -2147483648 additionally hits undefined behaviour in z = -z (line 2928, 2930 on 9.0.4):
clock format 0 -gmt 1
set ::tcl::clock::TZData(:min) {{-9223372036854775808 -2147483648 0 MIN}}
clock format 0 -timezone :min -format %z
tclClockFmt.c:2928:10: runtime error: negation of -2147483648 cannot be represented in type 'int'
Zoneinfo files carry their offsets as 32-bit values too; whether clock.tcl checks them on loading was not looked at.
From a safe interpreter, item 1 works unchanged, since clock there is an alias to the parent's clock, which runs ClockFormat in the parent:
interp eval [interp create -safe] {
clock format 0 -timezone +99:99:99 -format "[string repeat a 72]%z"
}
This gives the same ASan report as item 1, through TclAliasObjCmd, on trunk, 9.1.0 and 9.0.4, and with ::safe::interpCreate as well.
Effect without sanitisers. On macOS arm64 the stack protector's canary sits directly after resMem, so the overwritten byte is in the canary and the process aborts with "stack buffer overflow" (__stack_chk_fail). All three PoCs above abort the stock tclsh 9.1.0 from Homebrew this way. A safe child can therefore take down the whole process, including the parent and every other interpreter in it. Where the canary's lowest byte is already zero (glibc) or there is no stack protector, a single NUL may go unnoticed or overwrite whatever the compiler placed after resMem. The multi-byte overflow of item 2 goes further than that.
8.6 is not affected: there clock format is Tcl code (FormatNumericTimeZone) and prints +1004039 without trouble. The C format engine and this reservation came to the release line with the clock rewrite in 9.0 (TIP 688), from the clock speed-up branch of 2017 and 2018.
Fix suggestion.
- In
ClockFmtToken_TimeZone_Proc, reserve room for the longest possible output: sign, up to 6 digits of hours for a 32-bit offset, 2 of minutes and 2 of seconds, that is FrmResultAllocate(dateFmt, 11). Alternatively compute the width of the hours first and reserve exactly what is needed.
- Negate the offset without signed overflow, for example in an unsigned or 64-bit variable.
- Independently, in
clock.tcl, reject fixed offsets and POSIX offsets with minutes or seconds of 60 or more (and possibly hours above 24), so that +99:99:99 becomes a "time zone not found" error instead of a 100-hour offset. This changes behaviour that 8.6 shares, so it may be a separate decision; the C change alone fixes the overflow.
The attached patch against trunk does the first two: it makes the offset a Tcl_WideInt before negating it and reserves 11 characters. It adds tests clock-72.1 to clock-72.4 (items 1 and 2, stack and heap buffer, largest and smallest offset). Without the fix, they make ASan and UBSan report the overflows and the negation; with it, they pass, and clock.test passes in full. The C part applies unchanged to 9.1.0 and 9.0.4. The clock.tcl check is not in the patch.
