possible buffer overflow in tkcanvas?

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

possible buffer overflow in tkcanvas?

Jun T.
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));
                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../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).


------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
gnuplot-beta mailing list
[hidden email]
Membership management via: https://lists.sourceforge.net/lists/listinfo/gnuplot-beta

tkcanvas-str.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: possible buffer overflow in tkcanvas?

Jun T.

2016/02/13 02:47, Jun T. <[hidden email]> wrote:
> And what does the line 696
>    tmp_dashpattern[strlen(tmp_dashpattern) - 1] = NUL;
> want to achieve?

Sorry, this line must be retained.
It's used for eliminating the trailing space ' '
(assuming that buf[] is not truncated by strncat()).




------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
gnuplot-beta mailing list
[hidden email]
Membership management via: https://lists.sourceforge.net/lists/listinfo/gnuplot-beta

tkcanvas-str.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: possible buffer overflow in tkcanvas?

sfeam
In reply to this post by Jun T.

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).

>


------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
gnuplot-beta mailing list
[hidden email]
Membership management via: https://lists.sourceforge.net/lists/listinfo/gnuplot-beta