search

alexandrebelloni / tzdump

2 stars 5 forks source link

PR #10 faulty #11

Open kurahaupo opened 4 months ago

kurahaupo commented 4 months ago

Supersedes issue #7

The patch provided in #10 silences various warnings but fixes neither the underlying buffer-overrun vulnerability with strncat, nor the lack of a portable xxprintf format specifier for time_t.

The standard C strncat function is widely regarded as so badly flawed as to be useless, because its limit parameter does not reflect the overall size of the target buffer, but rather the space remaining in that buffer after the length of its current content is taken into account. Even when provided with a correct limit value, its safeguard is to truncate the string, resulting in an incorrect value which is useless for the purpose of this program.

To be fair, the limits provided are very generous, so one needs to go to extreme lengths to broach them, but this command demonstrates how the stack can be corrupted, causing an abort when it attempts to return from dumptzdata (back to main):

z=/./././.  # length 8
z=$z$z$z$z  # length 32
z=$z$z$z$z  # length 128
z=$z$z$z$z  # length 512
z=$z$z$z$z  # length 2048 
./tzdump -p /usr/share/zoneinfo$z .$z$z$z$z/PST8PDT

More broadly, return values that indicate potential errors should be checked, not simply cast away with (void).

alexandrebelloni commented 2 months ago

I don't think the code is worse after #10 so I won't revert but I'll take a proper fix on top of it.