Bug 206178

Summary: Out-of-bounds read in wcslcat(3)
Product: Base System Reporter: Alexander Cherepanov <cherepan>
Component: binAssignee: Brooks Davis <brooks>
Status: Closed FIXED    
Severity: Affects Some People CC: brooks
Priority: --- Keywords: patch
Version: CURRENTFlags: brooks: mfc-stable10+
brooks: mfc-stable9+
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Crashing testcase
none
Patch none

Description Alexander Cherepanov 2016-01-12 22:43:51 UTC 
Created attachment 165469 [details]
Crashing testcase

The wcslcat function could read several bytes behind the end of its input buffer. This could lead to a crash if the buffer happens to immediately precede an unmapped page (or when dst=NULL and n=0).

The strlcat function[1] and, hence[2], wcslcat function are documented to work with the destination buffer not containing NUL. In this case FreeBSD implementation of wcslcat will read one extra wide char. The code[3] for traversing the destination array:

56 	        /* Find the end of dst and adjust bytes left but don't go past end */
57 	        while (*d != '\0' && n-- != 0)
58 	                d++;

"n-- != 0" in the loop controlling expression makes sure that the loop is terminated after n chars are examined but the dereference in "*d != '\0'" happens before the "n" check. For example, there would be one dereference when n=0.
In particular, wcslcat(NULL, L"", 0) will crash. A crashing testcase with non-null dst is attached.

To fix it, it's enough to swap the checks in the while loop (patch attached). Or all the code could be changed to match strlcat.

The issue has security consequences but the function is rarely used so severity seems very low.

Other BSDs are affected except for OpenBSD which fixed it in [4].

The issue is similar to [5].

[1] https://svnweb.freebsd.org/base/head/lib/libc/string/strlcpy.3?revision=257720&view=markup#l86
[2] https://svnweb.freebsd.org/base/head/lib/libc/string/wmemchr.3?revision=251069&view=markup
[3] https://svnweb.freebsd.org/base/head/lib/libc/string/wcslcat.c?revision=188080&view=markup#l56
[4] http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libc/string/wcslcat.c?rev=1.4&content-type=text/x-cvsweb-markup
[5] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206177
Comment 1 Alexander Cherepanov 2016-01-12 22:46:48 UTC 
Created attachment 165470 [details]
Patch
Comment 2 Brooks Davis freebsd_committer freebsd_triage 2016-01-12 23:20:40 UTC 
Great catch!  I'll get this committed.
Comment 3 commit-hook freebsd_committer freebsd_triage 2016-01-13 21:50:55 UTC 
A commit references this bug:

Author: brooks
Date: Wed Jan 13 21:50:09 UTC 2016
New revision: 293856
URL: https://svnweb.freebsd.org/changeset/base/293856

Log:
  Avoid reading pass the end of the source buffer when it is not NUL
  terminated.

  If this buffer is adjacent to an unmapped page or a version of C with
  bounds checked is used this may result in a crash.

  PR:		206178
  Submitted by:	Alexander Cherepanov <cherepan@mccme.ru>
  MFC after:	1 week

Changes:
  head/lib/libc/string/wcslcat.c
Comment 4 commit-hook freebsd_committer freebsd_triage 2016-01-20 19:26:32 UTC 
A commit references this bug:

Author: brooks
Date: Wed Jan 20 19:26:05 UTC 2016
New revision: 294455
URL: https://svnweb.freebsd.org/changeset/base/294455

Log:
  MFC r293856:

  Avoid reading pass the end of the source buffer when it is not NUL
  terminated.

  If this buffer is adjacent to an unmapped page or a version of C with
  bounds checked is used this may result in a crash.

  PR:		206178
  Submitted by:	Alexander Cherepanov <cherepan@mccme.ru>

Changes:
_U  stable/10/
  stable/10/lib/libc/string/wcslcat.c
Comment 5 commit-hook freebsd_committer freebsd_triage 2016-01-20 19:57:39 UTC 
A commit references this bug:

Author: brooks
Date: Wed Jan 20 19:56:44 UTC 2016
New revision: 294457
URL: https://svnweb.freebsd.org/changeset/base/294457

Log:
  MFC r293856:

  Avoid reading pass the end of the source buffer when it is not NUL
  terminated.

  If this buffer is adjacent to an unmapped page or a version of C with
  bounds checked is used this may result in a crash.

  PR:		206178
  Submitted by:	Alexander Cherepanov <cherepan@mccme.ru>

Changes:
_U  stable/9/lib/libc/
  stable/9/lib/libc/string/wcslcat.c
Comment 6 commit-hook freebsd_committer freebsd_triage 2016-01-22 00:13:54 UTC 
A commit references this bug:

Author: brooks
Date: Fri Jan 22 00:13:18 UTC 2016
New revision: 294538
URL: https://svnweb.freebsd.org/changeset/base/294538

Log:
  MFC r293856:

  Avoid reading pass the end of the source buffer when it is not NUL
  terminated.

  If this buffer is adjacent to an unmapped page or a version of C with
  bounds checked is used this may result in a crash.

  PR:		206178
  Submitted by:	Alexander Cherepanov <cherepan@mccme.ru>
  Requested by:	danfe

Changes:
_U  stable/8/lib/libc/
  stable/8/lib/libc/string/wcslcat.c