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

See Also:


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
	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?