lib/49683: Off-by-one comparison in ct_decode_string() leading to out of bound referrence

[amirpli%gmail.com@localhost]

>Number:         49683
>Category:       lib
>Synopsis:       Off-by-one comparison in ct_decode_string() leading to out of bound referrence
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Feb 21 21:50:00 +0000 2015
>Originator:     Amir Plivatsky
>Release:        libedit 3.1 20141029
>Organization:
>Environment:
N/A
>Description:
When reading a line which has one more character then the previous one, an out of bound buffer reference happens.

The problem occurs due to an off-by-one comparison in ct_decode_string() (file chartype.c, rev 1.10 as of 20150221):

	if (len > conv->wsize)
		ct_conv_buff_resize(conv, (size_t)0, len + 1);

In ct_conv_buff_resize(), conv->wsize is set to the last argument, i.e to 
len+1. This buffer and conv->wsize are retained over function calls. On the next entry to ct_decode_string() with a line length that is greater by one than in the previous entry to this function, there is a need for a bigger buffer (by 1). However, the check (quoted above) misses to detect that because it compares to the previous conv->wsize which is equal to the previous len+1, and thus now len==conv->wsize and a bigger buffer is not allocated.



>How-To-Repeat:
There is no real need to repeat the problem, since code inspection reveals the problem and the fix.

However, here is how I found it:
Due to an unrelated test, I got an history file with 2 lines, one of length 2481 and the other 2482. The application (which uses libedit) failed on heap-buffer-overflow on start-up when libedit tried to read this history file.
>Fix:
--- old/src/chartype.c	2015-02-21 23:39:29.306548291 +0200
+++ new/src/chartype.c	2015-02-21 23:39:33.218560054 +0200
@@ -123,7 +123,7 @@
 	len = ct_mbstowcs(NULL, s, (size_t)0);
 	if (len == (size_t)-1)
 		return NULL;
-	if (len > conv->wsize)
+	if (len >= conv->wsize)
 		ct_conv_buff_resize(conv, (size_t)0, len + 1);
 	if (!conv->wbuff)
 		return NULL;

I actually implemented this fix and tested that it actually fixes this problem.