NetBSD Problem Report #49683

From www@NetBSD.org  Sat Feb 21 21:49:29 2015
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id BDD82A57FE
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 21 Feb 2015 21:49:29 +0000 (UTC)
Message-Id: <20150221214928.83432A65B6@mollari.NetBSD.org>
Date: Sat, 21 Feb 2015 21:49:28 +0000 (UTC)
From: amirpli@gmail.com
Reply-To: amirpli@gmail.com
To: gnats-bugs@NetBSD.org
Subject: Off-by-one comparison in ct_decode_string() leading to out of bound referrence
X-Send-Pr-Version: www-1.0

>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:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Feb 21 21:50:00 +0000 2015
>Closed-Date:    Sat Apr 02 07:19:32 +0000 2016
>Last-Modified:  Sat Apr 02 07:19:32 +0000 2016
>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.

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/49683 CVS commit: src/lib/libedit
Date: Sat, 21 Feb 2015 19:46:58 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Sun Feb 22 00:46:58 UTC 2015

 Modified Files:
 	src/lib/libedit: chartype.c chartype.h

 Log Message:
 PR/49683: Amir Plivatsky: Off-by-one comparison in ct_decode_string() leading
 to out of bounds referrence.
 XXX: pullup-7


 To generate a diff of this commit:
 cvs rdiff -u -r1.10 -r1.11 src/lib/libedit/chartype.c
 cvs rdiff -u -r1.11 -r1.12 src/lib/libedit/chartype.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Amir P <amirpli@gmail.com>
To: gnats-bugs@gnats.netbsd.org
Cc: 
Subject: Re: PR/49683
Date: Sun, 22 Feb 2015 03:56:53 +0200

 --047d7bb04bbefe676b050fa39b1c
 Content-Type: text/plain; charset=UTF-8

 A question:

 The actual change that has been implemented in chartype.c rev. 1.11 is:

 if (conv->csize < ++len)
 if (ct_conv_buff_resize(conv, (size_t)0, len + CT_BUFSIZ) == -1)
 return NULL;

 Please clarify - we need to enlarge conv->wbuff in case its size
 conv->wsize is less than len+1.
 So why does the check above compare len+1 to conv->csize, which is not
 related to the size of conv->wbuff?

 --047d7bb04bbefe676b050fa39b1c
 Content-Type: text/html; charset=UTF-8
 Content-Transfer-Encoding: quoted-printable

 <div dir=3D"rtl"><div class=3D"gmail_default" style=3D"color:rgb(51,0,0)" d=
 ir=3D"ltr">A question:</div><div class=3D"gmail_default" style=3D"color:rgb=
 (51,0,0)" dir=3D"ltr"><br></div><div class=3D"gmail_default" style=3D"color=
 :rgb(51,0,0)" dir=3D"ltr">The actual change that has been implemented in ch=
 artype.c rev. 1.11 is:</div><div class=3D"gmail_default" style=3D"color:rgb=
 (51,0,0)" dir=3D"ltr"><br></div><div class=3D"gmail_default" style=3D"color=
 :rgb(51,0,0)" dir=3D"ltr"><div class=3D"gmail_default" dir=3D"ltr"><span cl=
 ass=3D"" style=3D"white-space:pre">	</span>if (conv-&gt;csize &lt; ++len)</=
 div><div class=3D"gmail_default" dir=3D"ltr"><span class=3D"" style=3D"whit=
 e-space:pre">		</span>if (ct_conv_buff_resize(conv, (size_t)0, len + CT_BUF=
 SIZ) =3D=3D -1)</div><div class=3D"gmail_default" dir=3D"ltr"><span class=
 =3D"" style=3D"white-space:pre">			</span>return NULL;</div><div><br></div>=
 <div>Please clarify - we need to enlarge conv-&gt;wbuff in case its size co=
 nv-&gt;wsize is less than len+1.</div><div>So why does the check above comp=
 are len+1 to=C2=A0conv-&gt;csize, which is not related to the size of=C2=A0=
 conv-&gt;wbuff?</div><div><br></div></div></div>

 --047d7bb04bbefe676b050fa39b1c--

From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, lib-bug-people@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, amirpli@gmail.com
Cc: 
Subject: Re: PR/49683
Date: Sat, 21 Feb 2015 21:16:35 -0500

 On Feb 22,  2:00am, amirpli@gmail.com (Amir P) wrote:
 -- Subject: Re: PR/49683

 |  A question:
 |  
 |  The actual change that has been implemented in chartype.c rev. 1.11 is:
 |  
 |  if (conv->csize < ++len)
 |  if (ct_conv_buff_resize(conv, (size_t)0, len + CT_BUFSIZ) == -1)
 |  return NULL;
 |  
 |  Please clarify - we need to enlarge conv->wbuff in case its size
 |  conv->wsize is less than len+1.
 |  So why does the check above compare len+1 to conv->csize, which is not
 |  related to the size of conv->wbuff?

 It is wrong, I've split the functions.

 christos

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/49683 CVS commit: [netbsd-7] src/lib/libedit
Date: Tue, 14 Apr 2015 05:30:24 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Tue Apr 14 05:30:24 UTC 2015

 Modified Files:
 	src/lib/libedit [netbsd-7]: chartype.c chartype.h

 Log Message:
 Pull up following revision(s) (requested by christos in ticket #679):
 	lib/libedit/chartype.c: revisions 1.11, 1.12
 	lib/libedit/chartype.h: revisions 1.12, 1.13
 PR/49683: Amir Plivatsky: Off-by-one comparison in ct_decode_string() leading
 to out of bounds referrence.
 --
 split the allocation functions, their mixed usage was too confusing.


 To generate a diff of this commit:
 cvs rdiff -u -r1.10 -r1.10.20.1 src/lib/libedit/chartype.c
 cvs rdiff -u -r1.10 -r1.10.18.1 src/lib/libedit/chartype.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Tue, 15 Mar 2016 05:53:00 +0000
State-Changed-Why:
is this fixed?


State-Changed-From-To: feedback->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sat, 02 Apr 2016 07:19:32 +0000
State-Changed-Why:
The submitter accidentally filed a new PR (50984) to say this is fixed.


>Unformatted:

Home	PR Database Search