Bug 206178 - Out-of-bounds read in wcslcat(3)
Summary: Out-of-bounds read in wcslcat(3)
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Brooks Davis
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-01-12 22:43 UTC by Alexander Cherepanov
Modified: 2016-01-22 00:13 UTC (History)
1 user (show)

See Also:
brooks: mfc-stable10+
brooks: mfc-stable9+


Attachments
Crashing testcase (538 bytes, text/x-csrc)
2016-01-12 22:43 UTC, Alexander Cherepanov
no flags Details
Patch (323 bytes, patch)
2016-01-12 22:46 UTC, Alexander Cherepanov
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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