Re: catopen/catgets: out of boundary access

[Tobias Stoeckmann]

On Fri, Oct 23, 2015 at 09:19:34PM +0200, Stefan Sperling wrote:
> Now that this is committed, do you intend to audit the runes code as
> well? :-)

Hah, yeah that's the next logical step to do. Except you are faster
than me, then I would probably okay it. ;)

Re: catopen/catgets: out of boundary access

[Stefan Sperling]

On Tue, Oct 06, 2015 at 12:08:42PM +0200, Tobias Stöckmann wrote:
> > On October 6, 2015 at 11:40 AM Stefan Sperling  wrote:
> > What do you think about a similar treatment for locale/rune.c?
> 
> I think you refer to _Read_RuneMagi function,
> which lacks the same input validation.
> 
> Before supplying a patch for that one, I wanted to get some feedback
> for catopen/catgets. If my approach would be wrong or not efficient
> or violating style guide, it would just double the work to maintain
> multiple patches.

Now that this is committed, do you intend to audit the runes code as
well? :-)

Re: catopen/catgets: out of boundary access

[Stefan Sperling]

On Tue, Oct 06, 2015 at 11:57:40PM +0200, Tobias Stoeckmann wrote:
> By the way, this is the second version with miod's feedback. Time to
> send it to tech@ now, too.
> 
> Fixed one issue due to missing braces and less ntohl() calls, which
> makes the code easier to read.

ok with me

> Index: catopen.c
> ===
> RCS file: /cvs/src/lib/libc/nls/catopen.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 catopen.c
> --- catopen.c 5 Sep 2015 11:25:30 -   1.17
> +++ catopen.c 14 Sep 2015 18:27:00 -
> @@ -30,20 +30,24 @@
>  
>  #define _NLS_PRIVATE
>  
> -#include 
> -#include 
> -#include 
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
> +#include 
>  #include 
> +#include 
> +#include 
> +#include 
> +
> +#define MAXIMUM(a, b)(((a) > (b)) ? (a) : (b))
>  
>  #define NLS_DEFAULT_PATH 
> "/usr/share/nls/%L/%N.cat:/usr/share/nls/%l.%c/%N.cat:/usr/share/nls/%l/%N.cat"
>  #define NLS_DEFAULT_LANG "C"
>  
> -static nl_catd load_msgcat(const char *);
> +static nl_catd   load_msgcat(const char *);
> +static int   verify_msgcat(nl_catd);
>  
>  nl_catd
>  catopen(const char *name, int oflag)
> @@ -165,6 +169,8 @@ load_msgcat(const char *path)
>   void *data;
>   int fd;
>  
> + catd = NULL;
> +
>   if ((fd = open(path, O_RDONLY|O_CLOEXEC)) == -1)
>   return (nl_catd) -1;
>  
> @@ -173,24 +179,106 @@ load_msgcat(const char *path)
>   return (nl_catd) -1;
>   }
>  
> - data = mmap(0, (size_t) st.st_size, PROT_READ, MAP_SHARED, fd, 
> (off_t)0);
> - close (fd);
> -
> - if (data == MAP_FAILED) {
> + if (st.st_size > INT_MAX || st.st_size < sizeof (struct _nls_cat_hdr)) {
> + errno = EINVAL;
> + close (fd);
>   return (nl_catd) -1;
>   }
>  
> - if (ntohl(((struct _nls_cat_hdr *) data)->__magic) != _NLS_MAGIC) {
> - munmap(data, (size_t) st.st_size);
> - return (nl_catd) -1;
> - }
> + data = mmap(0, (size_t)st.st_size, PROT_READ, MAP_SHARED, fd, (off_t)0);
> + close (fd);
>  
> - if ((catd = malloc(sizeof (*catd))) == 0) {
> - munmap(data, (size_t) st.st_size);
> + if (data == MAP_FAILED)
>   return (nl_catd) -1;
> - }
> +
> + if (ntohl(((struct _nls_cat_hdr *) data)->__magic) != _NLS_MAGIC)
> + goto invalid;
> +
> + if ((catd = malloc(sizeof (*catd))) == 0)
> + goto invalid;
>  
>   catd->__data = data;
>   catd->__size = st.st_size;
> +
> + if (verify_msgcat(catd))
> + goto invalid;
> +
>   return catd;
> +
> +invalid:
> + free(catd);
> + munmap(data, (size_t) st.st_size);
> + errno = EINVAL;
> + return (nl_catd) -1;
>  }
> +
> +static int
> +verify_msgcat(nl_catd catd)
> +{
> + struct _nls_cat_hdr *cat;
> + struct _nls_set_hdr *set;
> + struct _nls_msg_hdr *msg;
> + size_t remain;
> + int hdr_offset, i, index, j, msgs, nmsgs, nsets, off, txt_offset;
> +
> + remain = catd->__size;
> + cat = (struct _nls_cat_hdr *) catd->__data;
> +
> + hdr_offset = ntohl(cat->__msg_hdr_offset);
> + nsets = ntohl(cat->__nsets);
> + txt_offset = ntohl(cat->__msg_txt_offset);
> +
> + /* catalog must contain at least one set and no negative offsets */
> + if (nsets < 1 || hdr_offset < 0 || txt_offset < 0)
> + return (1);
> +
> + remain -= sizeof (*cat);
> +
> + /* check if offsets or set size overflow */
> + if (remain <= hdr_offset || remain <= ntohl(cat->__msg_txt_offset) ||
> + remain / sizeof (*set) < nsets)
> + return (1);
> +
> + set = (struct _nls_set_hdr *) ((char *) catd->__data + sizeof (*cat));
> +
> + /* make sure that msg has space for at least one index */
> + if (remain - hdr_offset < sizeof(*msg))
> + return (1);
> +
> + msg = (struct _nls_msg_hdr *) ((char *) catd->__data + sizeof (*cat)
> + + hdr_offset);
> +
> + /* validate and retrieve largest string offset from sets */
> + off = 0;
> + for (i = 0; i < nsets; i++) {
> + index = ntohl(set[i].__index);
> + nmsgs = ntohl(set[i].__nmsgs);
> + /* set must contain at least one message */
> + if (index < 0 || nmsgs < 1)
> + return (1);
> +
> + if (INT_MAX - nmsgs < index)
> + return (1);
> + msgs = index + nmsgs;
> +
> + /* avoid msg index overflow */
> + if ((remain - hdr_offset) / sizeof(*msg) < msgs)
> + return (1);
> +
> + /* retrieve largest string offset */
> + for (j = index; j < nmsgs; j++) {
> + if (ntohl(msg[j].__offset) < 0)
> + return (1);
> + off = MAXIMUM(off, ntohl(msg[j].__offset));
> + }
> + }
> +
> +   

Re: catopen/catgets: out of boundary access

[Tobias Stoeckmann]

By the way, this is the second version with miod's feedback. Time to
send it to tech@ now, too.

Fixed one issue due to missing braces and less ntohl() calls, which
makes the code easier to read.

Index: catopen.c
===
RCS file: /cvs/src/lib/libc/nls/catopen.c,v
retrieving revision 1.17
diff -u -p -r1.17 catopen.c
--- catopen.c   5 Sep 2015 11:25:30 -   1.17
+++ catopen.c   14 Sep 2015 18:27:00 -
@@ -30,20 +30,24 @@
 
 #define _NLS_PRIVATE
 
-#include 
-#include 
-#include 
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
+#include 
 #include 
+#include 
+#include 
+#include 
+
+#define MAXIMUM(a, b)  (((a) > (b)) ? (a) : (b))
 
 #define NLS_DEFAULT_PATH 
"/usr/share/nls/%L/%N.cat:/usr/share/nls/%l.%c/%N.cat:/usr/share/nls/%l/%N.cat"
 #define NLS_DEFAULT_LANG "C"
 
-static nl_catd load_msgcat(const char *);
+static nl_catd load_msgcat(const char *);
+static int verify_msgcat(nl_catd);
 
 nl_catd
 catopen(const char *name, int oflag)
@@ -165,6 +169,8 @@ load_msgcat(const char *path)
void *data;
int fd;
 
+   catd = NULL;
+
if ((fd = open(path, O_RDONLY|O_CLOEXEC)) == -1)
return (nl_catd) -1;
 
@@ -173,24 +179,106 @@ load_msgcat(const char *path)
return (nl_catd) -1;
}
 
-   data = mmap(0, (size_t) st.st_size, PROT_READ, MAP_SHARED, fd, 
(off_t)0);
-   close (fd);
-
-   if (data == MAP_FAILED) {
+   if (st.st_size > INT_MAX || st.st_size < sizeof (struct _nls_cat_hdr)) {
+   errno = EINVAL;
+   close (fd);
return (nl_catd) -1;
}
 
-   if (ntohl(((struct _nls_cat_hdr *) data)->__magic) != _NLS_MAGIC) {
-   munmap(data, (size_t) st.st_size);
-   return (nl_catd) -1;
-   }
+   data = mmap(0, (size_t)st.st_size, PROT_READ, MAP_SHARED, fd, (off_t)0);
+   close (fd);
 
-   if ((catd = malloc(sizeof (*catd))) == 0) {
-   munmap(data, (size_t) st.st_size);
+   if (data == MAP_FAILED)
return (nl_catd) -1;
-   }
+
+   if (ntohl(((struct _nls_cat_hdr *) data)->__magic) != _NLS_MAGIC)
+   goto invalid;
+
+   if ((catd = malloc(sizeof (*catd))) == 0)
+   goto invalid;
 
catd->__data = data;
catd->__size = st.st_size;
+
+   if (verify_msgcat(catd))
+   goto invalid;
+
return catd;
+
+invalid:
+   free(catd);
+   munmap(data, (size_t) st.st_size);
+   errno = EINVAL;
+   return (nl_catd) -1;
 }
+
+static int
+verify_msgcat(nl_catd catd)
+{
+   struct _nls_cat_hdr *cat;
+   struct _nls_set_hdr *set;
+   struct _nls_msg_hdr *msg;
+   size_t remain;
+   int hdr_offset, i, index, j, msgs, nmsgs, nsets, off, txt_offset;
+
+   remain = catd->__size;
+   cat = (struct _nls_cat_hdr *) catd->__data;
+
+   hdr_offset = ntohl(cat->__msg_hdr_offset);
+   nsets = ntohl(cat->__nsets);
+   txt_offset = ntohl(cat->__msg_txt_offset);
+
+   /* catalog must contain at least one set and no negative offsets */
+   if (nsets < 1 || hdr_offset < 0 || txt_offset < 0)
+   return (1);
+
+   remain -= sizeof (*cat);
+
+   /* check if offsets or set size overflow */
+   if (remain <= hdr_offset || remain <= ntohl(cat->__msg_txt_offset) ||
+   remain / sizeof (*set) < nsets)
+   return (1);
+
+   set = (struct _nls_set_hdr *) ((char *) catd->__data + sizeof (*cat));
+
+   /* make sure that msg has space for at least one index */
+   if (remain - hdr_offset < sizeof(*msg))
+   return (1);
+
+   msg = (struct _nls_msg_hdr *) ((char *) catd->__data + sizeof (*cat)
+   + hdr_offset);
+
+   /* validate and retrieve largest string offset from sets */
+   off = 0;
+   for (i = 0; i < nsets; i++) {
+   index = ntohl(set[i].__index);
+   nmsgs = ntohl(set[i].__nmsgs);
+   /* set must contain at least one message */
+   if (index < 0 || nmsgs < 1)
+   return (1);
+
+   if (INT_MAX - nmsgs < index)
+   return (1);
+   msgs = index + nmsgs;
+
+   /* avoid msg index overflow */
+   if ((remain - hdr_offset) / sizeof(*msg) < msgs)
+   return (1);
+
+   /* retrieve largest string offset */
+   for (j = index; j < nmsgs; j++) {
+   if (ntohl(msg[j].__offset) < 0)
+   return (1);
+   off = MAXIMUM(off, ntohl(msg[j].__offset));
+   }
+   }
+
+   /* check if largest string offset is nul-terminated */
+   if (remain - txt_offset < off ||
+   memchr((char *) catd->__data + sizeof(*cat) + txt_offset + off,
+   '\0', remain - txt_offset - off) == NULL)
+

Re: catopen/catgets: out of boundary access

[Stefan Sperling]

On Thu, Sep 03, 2015 at 11:01:59PM +0200, Tobias Stoeckmann wrote:
> Hi,
> 
> our catopen implementation does not check the parsed message catalog,
> making it vulnerable to all sorts of out of boundary accesses.

This is interesting stuff, but I haven't found time to read through it yet.

What do you think about a similar treatment for locale/rune.c?

Re: catopen/catgets: out of boundary access

[Tobias Stöckmann]

> On October 6, 2015 at 11:40 AM Stefan Sperling  wrote:
> What do you think about a similar treatment for locale/rune.c?

I think you refer to _Read_RuneMagi function,
which lacks the same input validation.

Before supplying a patch for that one, I wanted to get some feedback
for catopen/catgets. If my approach would be wrong or not efficient
or violating style guide, it would just double the work to maintain
multiple patches.

catopen/catgets: out of boundary access

[Tobias Stoeckmann]

Hi,

our catopen implementation does not check the parsed message catalog,
making it vulnerable to all sorts of out of boundary accesses.

Take this minimalistic proof of concept file:

$ printf '\xff\x88\xff\x89\x01\x00\x00\x00' > poc.cat

If you are too lazy to write code to open it yourself, take this one:

---poc.c---
#include 
#include 
#include 

int
main(int argc, char *argv[])
{
nl_catd cat;

if (argc != 2) {
fprintf(stderr, "usage: poc file.cat\n");
return (1);
}

if ((cat = catopen(argv[1], 0)) == (nl_catd) -1)
err(2, "catopen");
printf("%s\n", catgets(cat, 1, 1, "default text"));
catclose(cat);
return (0);
}
---poc.c---

$ ./poc $PWD/poc.cat # yes, it takes an absolute path
Segmentation fault (core dumped)
$ _

I've added all sorts of checks, making sure that whatever offset and
index is inside the catalog is actually valid. Unfortunately it looks
rather messy, because I even have to check if there are negative
values in it -- it's all int32_t.

There are also cases in which catopen() returns -1 but does not set
errno properly. I took the glibc approach and set errno to EINVAL
whenever we encounter an invalid value.

Also, make sure that we directly ignore files which are too small or
too large.

Successfully passes the libc.cat files we have in base, so I'm rather
confident that there are no false positives.

Any advices to make this look nicer though? Or how to handle INT_MAX
and int32_t types? They are basically the same, can I trust that it's
true on all our archs?


Tobias

Index: lib/libc/nls/catopen.c
===
RCS file: /cvs/src/lib/libc/nls/catopen.c,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 catopen.c
--- catopen.c   16 Jan 2015 16:48:51 -  1.16
+++ catopen.c   3 Sep 2015 20:48:07 -
@@ -30,20 +30,24 @@
 
 #define _NLS_PRIVATE
 
-#include 
-#include 
-#include 
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
+#include 
 #include 
+#include 
+#include 
+#include 
+
+#define MAXIMUM(a, b)  (((a) > (b)) ? (a) : (b))
 
 #define NLS_DEFAULT_PATH 
"/usr/share/nls/%L/%N.cat:/usr/share/nls/%l.%c/%N.cat:/usr/share/nls/%l/%N.cat"
 #define NLS_DEFAULT_LANG "C"
 
-static nl_catd load_msgcat(const char *);
+static nl_catd load_msgcat(const char *);
+static int verify_msgcat(nl_catd);
 
 /* ARGSUSED */
 nl_catd
@@ -173,24 +177,106 @@ load_msgcat(const char *path)
return (nl_catd) -1;
}
 
+   if (st.st_size > INT_MAX || st.st_size < sizeof (struct _nls_cat_hdr)) {
+   errno = EINVAL;
+   close (fd);
+   return (nl_catd) -1;
+   }
+
data = mmap(0, (size_t) st.st_size, PROT_READ, MAP_SHARED, fd, 
(off_t)0);
close (fd);
 
-   if (data == MAP_FAILED) {
+   if (data == MAP_FAILED)
return (nl_catd) -1;
-   }
 
-   if (ntohl(((struct _nls_cat_hdr *) data)->__magic) != _NLS_MAGIC) {
-   munmap(data, (size_t) st.st_size);
-   return (nl_catd) -1;
-   }
+   if (ntohl(((struct _nls_cat_hdr *) data)->__magic) != _NLS_MAGIC)
+   goto invalid;
 
-   if ((catd = malloc(sizeof (*catd))) == 0) {
-   munmap(data, (size_t) st.st_size);
-   return (nl_catd) -1;
-   }
+   if ((catd = malloc(sizeof (*catd))) == 0)
+   goto invalid;
 
catd->__data = data;
catd->__size = st.st_size;
+
+   if (verify_msgcat(catd)) {
+   free(catd);
+   goto invalid;
+   }
+
return catd;
+
+invalid:
+   munmap(data, (size_t) st.st_size);
+   errno = EINVAL;
+   return (nl_catd) -1;
 }
+
+static int
+verify_msgcat(nl_catd catd)
+{
+   struct _nls_cat_hdr *cat;
+   struct _nls_set_hdr *set;
+   struct _nls_msg_hdr *msg;
+   size_t remain;
+   int i, j, msgs, off;
+
+   remain = catd->__size;
+   cat = (struct _nls_cat_hdr *) catd->__data;
+
+   /* catalog must contain at least one set and no negative offsets */
+   if (ntohl(cat->__nsets) < 1 ||
+   ntohl(cat->__msg_hdr_offset) < 0 ||
+   ntohl(cat->__msg_txt_offset) < 0)
+   return (1);
+
+   remain -= sizeof (*cat);
+
+   /* check if offsets or set size overflow */
+   if (remain <= ntohl(cat->__msg_hdr_offset) ||
+   remain <= ntohl(cat->__msg_txt_offset) ||
+   remain / sizeof (*set) < ntohl(cat->__nsets))
+   return (1);
+
+   set = (struct _nls_set_hdr *) ((char *) catd->__data + sizeof (*cat));
+
+   /* make sure that msg has space for at least one index */
+   if (remain - ntohl(cat->__msg_hdr_offset) < sizeof(*msg))
+   return (1);
+
+   msg = (struct _nls_msg_hdr *) ((char *) catd->__data + sizeof (*cat)
+   + ntohl(cat->__msg_hdr_offset));
+
+   /* validate and retrieve