chiark / gitweb /
integer and buffer overflows: introduce a number of asserts
authorIan Jackson <ijackson@chiark.greenend.org.uk>
Sun, 12 Jun 2011 21:23:15 +0000 (22:23 +0100)
committerIan Jackson <ijackson@chiark.greenend.org.uk>
Sun, 26 Jun 2011 11:07:17 +0000 (12:07 +0100)
In various places we add and increment integers, hoping that they
don't overflow.  We also prepend and append things to our internal
buffer, which is of fixed size, without checking that they will fit.

This means that malicious configuration (for example, long site names)
might be able to take over the secnet program.

So, add a whole lot of checking.  Many of these places don't have a
sensible way to return an error; in those cases we assert.  Some of
the checks are off-by-one in the sense that they say "assert(x<...)"
when "<=" would be OK too.  This is done to avoid having to think too
hard about fenceposts, as it's a simple way to avoid introducing bugs.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Signed-off-by: Richard Kettlewell <richard@greenend.org.uk>
conffile.c
conffile.fl
dh.c
ipaddr.c
log.c
netlink.c
secnet.c
secnet.h
site.c
util.c

index db7c58e..565e37f 100644 (file)
@@ -3,6 +3,8 @@
 /* #define DUMP_PARSE_TREE */
 
 #include "secnet.h"
+#include <assert.h>
+#include <limits.h>
 #include <stdio.h>
 #include <string.h>
 #include "conffile.h"
@@ -195,6 +197,7 @@ static void ptree_dump(struct p_node *n, uint32_t d)
        default:       printf("**unknown primitive type**\n"); break;
        }
     } else {
+       assert(d<INT_MAX);
        printf("%s: (%s line %d)\n",ntype(n->type),n->loc.file,n->loc.line);
        ptree_indent(d);
        printf("  |-"); ptree_dump(n->l, d+1);
@@ -562,7 +565,7 @@ uint32_t list_length(list_t *a)
 {
     uint32_t l=0;
     list_t *i;
-    for (i=a; i; i=i->next) l++;
+    for (i=a; i; i=i->next) { assert(l < INT_MAX); l++; }
     return l;
 }
 
@@ -685,6 +688,9 @@ string_t dict_read_string(dict_t *dict, cstring_t key, bool_t required,
     if (i->type!=t_string) {
        cfgfatal(loc,desc,"\"%s\" must be a string\n",key);
     }
+    if (strlen(i->data.string) > INT_MAX/10) {
+       cfgfatal(loc,desc,"\"%s\" is unreasonably long\n",key);
+    }
     r=i->data.string;
     return r;
 }
@@ -700,6 +706,9 @@ uint32_t dict_read_number(dict_t *dict, cstring_t key, bool_t required,
     if (i->type!=t_number) {
        cfgfatal(loc,desc,"\"%s\" must be a number\n",key);
     }
+    if (i->data.number >= 0x80000000) {
+        cfgfatal(loc,desc,"\"%s\" must fit into a 32-bit signed integer\n",key);
+    }
     r=i->data.number;
     return r;
 }
index 1747be2..b375f3b 100644 (file)
@@ -6,6 +6,8 @@
 %option never-interactive
 
 %{
+#include <assert.h>
+#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -100,6 +102,7 @@ include                     BEGIN(incl);
        Message(M_FATAL,"config file %s line %d: %s\n",config_file,
                config_lineno,"``include'' requires a filename");
        BEGIN(INITIAL);
+       assert(config_lineno < INT_MAX);
        ++config_lineno;
        ++yynerrs;
 }
diff --git a/dh.c b/dh.c
index 91d08ce..fff9b99 100644 (file)
--- a/dh.c
+++ b/dh.c
@@ -1,5 +1,6 @@
 #include <stdio.h>
 #include <gmp.h>
+#include <limits.h>
 
 #include "secnet.h"
 #include "util.h"
@@ -113,7 +114,16 @@ static list_t *dh_apply(closure_t *self, struct cloc loc, dict_t *context,
            cfgfatal(loc,"diffie-hellman","modulus must be a prime\n");
        }
     }
-    st->ops.len=mpz_sizeinbase(&st->p,2)/8;
+
+    size_t sz=mpz_sizeinbase(&st->p,2)/8;
+    if (sz>INT_MAX) {
+       cfgfatal(loc,"diffie-hellman","modulus far too large\n");
+    }
+    if (mpz_cmp(&st->g,&st->p) >= 0) {
+       cfgfatal(loc,"diffie-hellman","generator must be less than modulus\n");
+    }
+
+    st->ops.len=sz;
 
     return new_closure(&st->cl);
 }
index d8661da..433db62 100644 (file)
--- a/ipaddr.c
+++ b/ipaddr.c
@@ -2,6 +2,8 @@
    inspired by the 'ipaddr.py' library from Cendio Systems AB. */
 
 #include "secnet.h"
+#include <limits.h>
+#include <assert.h>
 #include <stdio.h>
 #include <string.h>
 #include "ipaddr.h"
@@ -31,6 +33,7 @@ static void subnet_list_set_len(struct subnet_list *a, uint32_t l)
     uint32_t na;
 
     if (l>a->alloc) {
+       assert(a->alloc < (int)(INT_MAX/sizeof(*nd))-EXTEND_ALLOC_BY);
        na=a->alloc+EXTEND_ALLOC_BY;
        nd=realloc(a->list,sizeof(*nd)*na);
        if (!nd) {
@@ -45,6 +48,7 @@ static void subnet_list_set_len(struct subnet_list *a, uint32_t l)
 void subnet_list_append(struct subnet_list *a, uint32_t prefix, uint32_t len)
 {
     struct subnet *sn;
+    assert(a->entries < INT_MAX);
     subnet_list_set_len(a,a->entries+1);
     sn=&a->list[a->entries-1];
     sn->prefix=prefix;
@@ -114,6 +118,7 @@ static void ipset_set_len(struct ipset *a, uint32_t l)
     uint32_t na;
 
     if (l>a->a) {
+       assert(a->a < INT_MAX-EXTEND_ALLOC_BY);
        na=a->a+EXTEND_ALLOC_BY;
        nd=realloc(a->d,sizeof(*nd)*na);
        if (!nd) {
diff --git a/log.c b/log.c
index 837ed55..55f1ee1 100644 (file)
--- a/log.c
+++ b/log.c
@@ -25,6 +25,7 @@ static void vMessage(uint32_t class, const char *message, va_list args)
     if (secnet_is_daemon) {
        /* Messages go to the system log interface */
        bp=strlen(buff);
+       assert(bp < MESSAGE_BUFLEN);
        vsnprintf(buff+bp,MESSAGE_BUFLEN-bp,message,args);
        /* Each line is sent separately */
        while ((nlp=strchr(buff,'\n'))) {
index 76ac91c..f6d4e72 100644 (file)
--- a/netlink.c
+++ b/netlink.c
@@ -98,6 +98,8 @@ their use.
 */
 
 #include <string.h>
+#include <assert.h>
+#include <limits.h>
 #include "secnet.h"
 #include "util.h"
 #include "ipaddr.h"
@@ -770,8 +772,10 @@ static void netlink_phase_hook(void *sst, uint32_t new_phase)
                           "netlink_phase_hook");
     /* Fill the table */
     i=0;
-    for (c=st->clients; c; c=c->next)
+    for (c=st->clients; c; c=c->next) {
+       assert(i<INT_MAX);
        st->routes[i++]=c;
+    }
     /* Sort the table in descending order of priority */
     qsort(st->routes,st->n_clients,sizeof(*st->routes),
          netlink_compare_client_priority);
@@ -911,6 +915,7 @@ static closure_t *netlink_inst_create(struct netlink *st,
     c->kup=False;
     c->next=st->clients;
     st->clients=c;
+    assert(st->n_clients < INT_MAX);
     st->n_clients++;
 
     return &c->cl;
index 16fa198..f6931b5 100644 (file)
--- a/secnet.c
+++ b/secnet.c
@@ -1,5 +1,7 @@
 #include "secnet.h"
 #include <stdio.h>
+#include <assert.h>
+#include <limits.h>
 #include <string.h>
 #include <getopt.h>
 #include <errno.h>
@@ -230,6 +232,7 @@ void register_for_poll(void *st, beforepoll_fn *before,
     i->max_nfds=max_nfds;
     i->nfds=0;
     i->desc=desc;
+    assert(total_nfds < INT_MAX - max_nfds);
     total_nfds+=max_nfds;
     i->next=reg;
     reg=i;
index 2b913d9..3f32302 100644 (file)
--- a/secnet.h
+++ b/secnet.h
@@ -126,6 +126,7 @@ extern string_t dict_read_string(dict_t *dict, cstring_t key, bool_t required,
 extern uint32_t dict_read_number(dict_t *dict, cstring_t key, bool_t required,
                                 cstring_t desc, struct cloc loc,
                                 uint32_t def);
+  /* return value can safely be assigned to int32_t */
 extern bool_t dict_read_bool(dict_t *dict, cstring_t key, bool_t required,
                             cstring_t desc, struct cloc loc, bool_t def);
 struct flagstr {
diff --git a/site.c b/site.c
index e542b7d..8ef8f5a 100644 (file)
--- a/site.c
+++ b/site.c
@@ -1266,6 +1266,8 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context,
     sprintf(st->tunname,"%s<->%s",st->localname,st->remotename);
 
     /* The information we expect to see in incoming messages of type 1 */
+    /* fixme: lots of unchecked overflows here, but the results are only
+       corrupted packets rather than undefined behaviour */
     st->setupsiglen=strlen(st->remotename)+strlen(st->localname)+8;
     st->setupsig=safe_malloc(st->setupsiglen,"site_apply");
     put_uint32(st->setupsig+0,LABEL_MSG1);
diff --git a/util.c b/util.c
index 997979a..86a9cd8 100644 (file)
--- a/util.c
+++ b/util.c
@@ -243,12 +243,14 @@ void buffer_init(struct buffer_if *buffer, uint32_t max_start_pad)
 
 void *buf_append(struct buffer_if *buf, uint32_t amount) {
     void *p;
+    assert(buf->size <= buf->len - amount);
     p=buf->start + buf->size;
     buf->size+=amount;
     return p;
 }
 
 void *buf_prepend(struct buffer_if *buf, uint32_t amount) {
+    assert(amount <= buf->start - buf->base);
     buf->size+=amount;
     return buf->start-=amount;
 }
@@ -273,6 +275,7 @@ void buf_append_string(struct buffer_if *buf, cstring_t s)
     uint16_t len;
 
     len=strlen(s);
+    /* fixme: if string is longer than 65535, result is a corrupted packet */
     buf_append_uint16(buf,len);
     memcpy(buf_append(buf,len),s,len);
 }