Re: possible buffer overflow in tkcanvas?

[Ethan A M. <sf...@us...>]

On Saturday, 13 February, 2016 02:47:43 Jun T. wrote:
> With the CVS HEAD, clang gives the following warning (and suggestion):
> 
> ../term/tkcanvas.trm:694:33: warning: the value of the size argument in 'strncat' is too large, might lead to a buffer overflow [-Wstrncat-size]
>                 strncat(tmp_dashpattern, buf, sizeof(tmp_dashpattern) - strlen(tmp_dashpattern));


Thanks.  Good idea to run it through clang.
The clang I have here (3.5.2) picks up two more errors in tkcanvas
in addition to the ones you saw.  One is harmless, but this one

../term/tkcanvas.trm:683:27: warning: comparison of array 'custom_dash_pattern->dstring' not
      equal to a null pointer is always true [-Wtautological-pointer-compare]
        if (custom_dash_pattern->dstring != NULL) {
            ~~~~~~~~~~~~~~~~~~~~~^~~~~~~    ~~~~

can cause a real failure.

	Ethan


>                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../term/tkcanvas.trm:694:33: note: change the argument to be the free space in the destination buffer minus the terminating null byte
>                 strncat(tmp_dashpattern, buf, sizeof(tmp_dashpattern) - strlen(tmp_dashpattern));
>                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>                                               sizeof(tmp_dashpattern) - strlen(tmp_dashpattern) - 1
> 
> So I looked into the function TK_dashtype() and noticed two more
> suspicious code (in addition to the line 694 pointed out by clang).
> I attached a patch, but I haven't done any tests (I don't know
> how to use tkcanvas). So please consider my patch just as a suggestion.
> 
> Lines 694 and 685 may be dangerous in the following sense:
> 
> If strlen(src) >=n,
>   strncat(dest, src, n) adds n chars AND a trailing NUL to dest.
>   strncpy(dest, src, n) copies n chars to dest but does not NUL
>   terminate it.
> 
> So the safest way of using these functions is:
>   strncat(dest, src, sizeof(dest) - strlen(dest) - 1);
> and
>   strncpy(dest, src, sizeof(dest) - 1);
>   dest[sizeof(dest)-1] = NUL;
> 
> (line 685 is safe in the present case since dstring is shorter than
> tmp_dashpattern, but ...)
> 
> 
> And what does the line 696
>     tmp_dashpattern[strlen(tmp_dashpattern) - 1] = NUL;
> want to achieve?
> Maybe a simple confusion between strncat and strncpy?
> 
> 
> BTW, are 32 bytes really necessary for buf[]? (line 690).
> sizeof(tmp_dashpattern) = 3*DASHPATTERN_LENGTH = 24 < 32
> (if DASHPATTERN_LENGTH = 8).
>