Bug 206177

Summary:

Out-of-bounds read in wcsncat(3)

Product:

Base System

Reporter:

Alexander Cherepanov <cherepan>

Component:

bin

Assignee:

Brooks Davis <brooks>

Status:

Closed FIXED

Severity:

Affects Some People

CC:

brooks

Priority:

---

Keywords:

patch

Version:

CURRENT

Flags:

brooks: mfc-stable10+
brooks: mfc-stable9+

Hardware:

Any

OS:

Any

Attachments:

Description	Flags
Patch	none
Crashing testcase	none

Description Alexander Cherepanov 2016-01-12 22:40:10 UTC

Created attachment 165467 [details]
Patch

The wcsncat 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.

According to C11, 7.29.4.3.2, wcsncat(s1, s2, n) will append s2 to s1 but will stop as soon as null wide char is met or n wide chars are read. In particular, s2 is not required to be a null-terminated wide string.
(The man page[1] for wcsncat refers to the man page[2] for strncat which erroneously talks about strings only. I filed [3] about it.)

When s2 is an array containing exactly n non-null wide chars, FreeBSD implementation of wcsncat will read one extra wide char. The code[4] for traversing the source array:

50 	        r = s2;
51 	        while (*r && n) {
52 	                *q++ = *r++;
53 	                n--;
54 	        }

"n" in the loop controlling expression makes sure that the loop is terminated after n chars are copied but the dereference in "*r" happens before the "n" check. For example, there would be one dereference when n=0.
Calling the function as wcsncat(s1, NULL, 0) will crash. (Formally speaking, passing NULL as s2 is invalid in C11, a valid crashing testcase 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 strncat.

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

Other BSDs are affected.

[1] https://svnweb.freebsd.org/base/head/lib/libc/string/wmemchr.3?revision=251069&view=markup
[2] https://svnweb.freebsd.org/base/head/lib/libc/string/strcat.3?revision=262890&view=markup
[3] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206176
[4] https://svnweb.freebsd.org/base/head/lib/libc/string/wcsncat.c?revision=188080&view=markup#l50

Comment 1 Alexander Cherepanov 2016-01-12 22:40:59 UTC

Created attachment 165468 [details]
Crashing testcase

Comment 2 Alexander Cherepanov 2016-01-12 22:51:38 UTC

The issue is similar to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206178 .

Comment 3 Brooks Davis freebsd_committer

2016-01-12 23:19:37 UTC

Great catch!  I'll get this committed.

FYI, on CHERI we'll hit this for all buffers not just ones that back into an unmapped page (our pointers have hardware enforced bounds checks).

Comment 4 Alexander Cherepanov 2016-01-13 00:49:37 UTC

Thanks! And thanks for pointing to CHERI.

Comment 5 commit-hook freebsd_committer

2016-01-13 21:49:53 UTC

A commit references this bug:

Author: brooks
Date: Wed Jan 13 21:49:01 UTC 2016
New revision: 293855
URL: https://svnweb.freebsd.org/changeset/base/293855

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:		206177
  Submitted by:	Alexander Cherepanov <cherepan@mccme.ru>
  MFC after:	1 week

Changes:
  head/lib/libc/string/wcsncat.c

Comment 6 commit-hook freebsd_committer

2016-01-20 19:09:27 UTC

A commit references this bug:

Author: brooks
Date: Wed Jan 20 19:08:50 UTC 2016
New revision: 294453
URL: https://svnweb.freebsd.org/changeset/base/294453

Log:
  MFC r293855:

  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:		206177
  Submitted by:	Alexander Cherepanov <cherepan@mccme.ru>

Changes:
_U  stable/10/
  stable/10/lib/libc/string/wcsncat.c

Comment 7 commit-hook freebsd_committer

2016-01-20 19:52:37 UTC

A commit references this bug:

Author: brooks
Date: Wed Jan 20 19:52:01 UTC 2016
New revision: 294456
URL: https://svnweb.freebsd.org/changeset/base/294456

Log:
  MFC r293855:

  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:		206177
  Submitted by:	Alexander Cherepanov <cherepan@mccme.ru>

Changes:
_U  stable/9/lib/libc/
  stable/9/lib/libc/string/wcsncat.c

Comment 8 commit-hook freebsd_committer

2016-01-22 00:08:52 UTC

A commit references this bug:

Author: brooks
Date: Fri Jan 22 00:08:16 UTC 2016
New revision: 294537
URL: https://svnweb.freebsd.org/changeset/base/294537

Log:
  MFC r293855:

  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:		206177
  Submitted by:	Alexander Cherepanov <cherepan@mccme.ru>
  Requested by:	danfe

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