iconv uses strlen directly on user supplied memory

[dab]

Summary

This change comes from PR 207302, submitted by CTurt <[e-mail]> of Hardened BSD. The writeup from the PR is included below:

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.