Bug 207302 - Iconv uses strlen directly on user supplied memory
Summary: Iconv uses strlen directly on user supplied memory
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: David Bright
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-02-18 10:32 UTC by CTurt
Modified: 2018-03-05 16:44 UTC (History)
5 users (show)

See Also:
dab: mfc-stable10+
dab: mfc-stable11+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-02-18 10:32:36 UTC
`iconv_sysctl_add` from `sys/libkern/iconv.c` incorrectly limits the size of user strings, such that several out of bounds reads could have been possible.

static int
iconv_sysctl_add(SYSCTL_HANDLER_ARGS)
{
	struct iconv_converter_class *dcp;
	struct iconv_cspair *csp;
	struct iconv_add_in din;
	struct iconv_add_out dout;
	int error;

	error = SYSCTL_IN(req, &din, sizeof(din));
	if (error)
		return error;
	if (din.ia_version != ICONV_ADD_VER)
		return EINVAL;
	if (din.ia_datalen > ICONV_CSMAXDATALEN)
		return EINVAL;
	if (strlen(din.ia_from) >= ICONV_CSNMAXLEN)
		return EINVAL;
	if (strlen(din.ia_to) >= ICONV_CSNMAXLEN)
		return EINVAL;
	if (strlen(din.ia_converter) >= ICONV_CNVNMAXLEN)
		return EINVAL;
...

Since the `din` struct is directly copied from userland, there is no guarantee that the strings supplied will be NULL terminated. The `strlen` calls could continue reading past the designated buffer sizes.

Declaration of `struct iconv_add_in` is found in `sys/sys/iconv.h`:

struct iconv_add_in {
	int	ia_version;
	char	ia_converter[ICONV_CNVNMAXLEN];
	char	ia_to[ICONV_CSNMAXLEN];
	char	ia_from[ICONV_CSNMAXLEN];
	int	ia_datalen;
	const void *ia_data;
};

Our strings are followed by the `ia_datalen` member, which is checked before the `strlen` calls:

if (din.ia_datalen > ICONV_CSMAXDATALEN)

Since `ICONV_CSMAXDATALEN` has value `0x41000` (and is `unsigned`), this ensures that `din.ia_datalen` contains at least 1 byte of 0, so it is not possible to trigger a read out of bounds of the `struct` however, this code is fragile and could introduce subtle bugs in the future if the `struct` is ever modified.
Comment 1 CTurt 2016-02-18 10:38:34 UTC
Patch: https://github.com/HardenedBSD/hardenedBSD-playground/commit/1bcd4a2c6f3a256b2db03fc9421857a7f7963f34.patch

It may potentially be more pleasing to use:

    if (strnlen(din.ia_from, ICONV_CSNMAXLEN) >= ICONV_CSNMAXLEN)

But I opted for using `sizeof` on the string instead:

    if (strnlen(din.ia_from, sizeof(din.ia_from)) >= ICONV_CSNMAXLEN)
Comment 2 Shawn Webb 2016-03-01 21:00:25 UTC
Any movement on this?
Comment 3 commit-hook freebsd_committer 2018-02-26 18:23:45 UTC
A commit references this bug:

Author: dab
Date: Mon Feb 26 18:23:37 UTC 2018
New revision: 330027
URL: https://svnweb.freebsd.org/changeset/base/330027

Log:
  iconv uses strlen directly on user supplied memory

  `iconv_sysctl_add` from `sys/libkern/iconv.c` incorrectly limits the
  size of user strings, such that several out of bounds reads could have
  been possible.

  static int
  iconv_sysctl_add(SYSCTL_HANDLER_ARGS)
  {
  	struct iconv_converter_class *dcp;
  	struct iconv_cspair *csp;
  	struct iconv_add_in din;
  	struct iconv_add_out dout;
  	int error;

  	error = SYSCTL_IN(req, &din, sizeof(din));
  	if (error)
  		return error;
  	if (din.ia_version != ICONV_ADD_VER)
  		return EINVAL;
  	if (din.ia_datalen > ICONV_CSMAXDATALEN)
  		return EINVAL;
  	if (strlen(din.ia_from) >= ICONV_CSNMAXLEN)
  		return EINVAL;
  	if (strlen(din.ia_to) >= ICONV_CSNMAXLEN)
  		return EINVAL;
  	if (strlen(din.ia_converter) >= ICONV_CNVNMAXLEN)
  		return EINVAL;
  ...

  Since the `din` struct is directly copied from userland, there is no
  guarantee that the strings supplied will be NULL terminated. The
  `strlen` calls could continue reading past the designated buffer
  sizes.

  Declaration of `struct iconv_add_in` is found in `sys/sys/iconv.h`:

  struct iconv_add_in {
  	int	ia_version;
  	char	ia_converter[ICONV_CNVNMAXLEN];
  	char	ia_to[ICONV_CSNMAXLEN];
  	char	ia_from[ICONV_CSNMAXLEN];
  	int	ia_datalen;
  	const void *ia_data;
  };

  Our strings are followed by the `ia_datalen` member, which is checked
  before the `strlen` calls:

  if (din.ia_datalen > ICONV_CSMAXDATALEN)

  Since `ICONV_CSMAXDATALEN` has value `0x41000` (and is `unsigned`),
  this ensures that `din.ia_datalen` contains at least 1 byte of 0, so
  it is not possible to trigger a read out of bounds of the `struct`
  however, this code is fragile and could introduce subtle bugs in the
  future if the `struct` is ever modified.

  PR:		207302
  Submitted by:	CTurt <cturt@hardenedbsd.org>
  Reported by:	CTurt <cturt@hardenedbsd.org>
  Reviewed by:	jhb, vangyzen
  MFC after:	1 week
  Sponsored by:	Dell EMC
  Differential Revision:	https://reviews.freebsd.org/D14521

Changes:
  head/sys/libkern/iconv.c
Comment 4 commit-hook freebsd_committer 2018-03-05 13:58:45 UTC
A commit references this bug:

Author: dab
Date: Mon Mar  5 13:58:04 UTC 2018
New revision: 330505
URL: https://svnweb.freebsd.org/changeset/base/330505

Log:
  MFC r330027

  iconv uses strlen directly on user supplied memory

  `iconv_sysctl_add` from `sys/libkern/iconv.c` incorrectly limits the
  size of user strings, such that several out of bounds reads could have
  been possible.

  static int
  iconv_sysctl_add(SYSCTL_HANDLER_ARGS)
  {
  	struct iconv_converter_class *dcp;
  	struct iconv_cspair *csp;
  	struct iconv_add_in din;
  	struct iconv_add_out dout;
  	int error;

  	error = SYSCTL_IN(req, &din, sizeof(din));
  	if (error)
  		return error;
  	if (din.ia_version != ICONV_ADD_VER)
  		return EINVAL;
  	if (din.ia_datalen > ICONV_CSMAXDATALEN)
  		return EINVAL;
  	if (strlen(din.ia_from) >= ICONV_CSNMAXLEN)
  		return EINVAL;
  	if (strlen(din.ia_to) >= ICONV_CSNMAXLEN)
  		return EINVAL;
  	if (strlen(din.ia_converter) >= ICONV_CNVNMAXLEN)
  		return EINVAL;
  ...

  Since the `din` struct is directly copied from userland, there is no
  guarantee that the strings supplied will be NULL terminated. The
  `strlen` calls could continue reading past the designated buffer
  sizes.

  Declaration of `struct iconv_add_in` is found in `sys/sys/iconv.h`:

  struct iconv_add_in {
  	int	ia_version;
  	char	ia_converter[ICONV_CNVNMAXLEN];
  	char	ia_to[ICONV_CSNMAXLEN];
  	char	ia_from[ICONV_CSNMAXLEN];
  	int	ia_datalen;
  	const void *ia_data;
  };

  Our strings are followed by the `ia_datalen` member, which is checked
  before the `strlen` calls:

  if (din.ia_datalen > ICONV_CSMAXDATALEN)

  Since `ICONV_CSMAXDATALEN` has value `0x41000` (and is `unsigned`),
  this ensures that `din.ia_datalen` contains at least 1 byte of 0, so
  it is not possible to trigger a read out of bounds of the `struct`
  however, this code is fragile and could introduce subtle bugs in the
  future if the `struct` is ever modified.

  PR:		207302
  Submitted by:	CTurt <cturt@hardenedbsd.org>
  Reported by:	CTurt <cturt@hardenedbsd.org>
  Sponsored by:	Dell EMC

Changes:
_U  stable/11/
  stable/11/sys/libkern/iconv.c
Comment 5 commit-hook freebsd_committer 2018-03-05 16:00:29 UTC
A commit references this bug:

Author: dab
Date: Mon Mar  5 16:00:06 UTC 2018
New revision: 330512
URL: https://svnweb.freebsd.org/changeset/base/330512

Log:
  MFC r330027

  iconv uses strlen directly on user supplied memory

  `iconv_sysctl_add` from `sys/libkern/iconv.c` incorrectly limits the
  size of user strings, such that several out of bounds reads could have
  been possible.

  static int
  iconv_sysctl_add(SYSCTL_HANDLER_ARGS)
  {
  	struct iconv_converter_class *dcp;
  	struct iconv_cspair *csp;
  	struct iconv_add_in din;
  	struct iconv_add_out dout;
  	int error;

  	error = SYSCTL_IN(req, &din, sizeof(din));
  	if (error)
  		return error;
  	if (din.ia_version != ICONV_ADD_VER)
  		return EINVAL;
  	if (din.ia_datalen > ICONV_CSMAXDATALEN)
  		return EINVAL;
  	if (strlen(din.ia_from) >= ICONV_CSNMAXLEN)
  		return EINVAL;
  	if (strlen(din.ia_to) >= ICONV_CSNMAXLEN)
  		return EINVAL;
  	if (strlen(din.ia_converter) >= ICONV_CNVNMAXLEN)
  		return EINVAL;
  ...

  Since the `din` struct is directly copied from userland, there is no
  guarantee that the strings supplied will be NULL terminated. The
  `strlen` calls could continue reading past the designated buffer
  sizes.

  Declaration of `struct iconv_add_in` is found in `sys/sys/iconv.h`:

  struct iconv_add_in {
  	int	ia_version;
  	char	ia_converter[ICONV_CNVNMAXLEN];
  	char	ia_to[ICONV_CSNMAXLEN];
  	char	ia_from[ICONV_CSNMAXLEN];
  	int	ia_datalen;
  	const void *ia_data;
  };

  Our strings are followed by the `ia_datalen` member, which is checked
  before the `strlen` calls:

  if (din.ia_datalen > ICONV_CSMAXDATALEN)

  Since `ICONV_CSMAXDATALEN` has value `0x41000` (and is `unsigned`),
  this ensures that `din.ia_datalen` contains at least 1 byte of 0, so
  it is not possible to trigger a read out of bounds of the `struct`
  however, this code is fragile and could introduce subtle bugs in the
  future if the `struct` is ever modified.

  PR:		207302
  Submitted by:	CTurt <cturt@hardenedbsd.org>
  Reported by:	CTurt <cturt@hardenedbsd.org>
  Sponsored by:	Dell EMC

Changes:
_U  stable/10/
  stable/10/sys/libkern/iconv.c