[Bug 207302] Iconv uses strlen directly on user supplied memory

[bugzilla-noreply at freebsd.org]

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207302

            Bug ID: 207302
           Summary: Iconv uses strlen directly on user supplied memory
           Product: Base System
           Version: 11.0-CURRENT
          Hardware: Any
                OS: Any
            Status: New
          Severity: Affects Some People
          Priority: ---
         Component: kern
          Assignee: freebsd-bugs at FreeBSD.org
          Reporter: cturt at hardenedbsd.org

`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.

-- 
You are receiving this mail because:
You are the assignee for the bug.