integer and buffer overflows: introduce a number of asserts

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>

diff --git a/conffile.c b/conffile.c
index db7c58e31b447beb07d89dc1716ae2fd0ad38203..565e37ff9b8ed608ea867106aaebc17156d3b8f9 100644 (file)
--- a/conffile.c
+++ b/conffile.c
@@ -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;
 }

diff --git a/conffile.fl b/conffile.fl
index 1747be26fb321407edeb44ed124c644c13d80ee7..b375f3b5851c17a45c673ee06396d430b462fae5 100644 (file)
--- a/conffile.fl
+++ b/conffile.fl
@@ -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 91d08ced2dc805a839dcc3bf9b471c6387a2f0bc..fff9b998c54062eaa473968640f40acc6b6f8a5d 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);
 }

diff --git a/ipaddr.c b/ipaddr.c
index d8661dafcd4535560d3b83988096ad293ac4a763..433db62ef406b7912a52a09f5d641080d9b3821d 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 837ed55acf8c9530a2e4d9f4e87feec3b433449e..55f1ee125cbfba848002f1c536e8ad89e3bb3247 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'))) {

diff --git a/netlink.c b/netlink.c
index 76ac91c31679f2d89e39fadd77fbe7764e818a28..f6d4e72920ab7b9a28422ae200353e4a231dcb24 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;

diff --git a/secnet.c b/secnet.c
index 16fa198161d414e3071842eb98f283ac7d8c2dbc..f6931b5e36fa0fcf291710842664b71165597890 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;

diff --git a/secnet.h b/secnet.h
index 2b913d9854802b9cd15135e45ebc0e039bc10d2d..3f32302bc272a699dc742b91f2232f327c1b7853 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 e542b7d8173b44ac35ad1aaade05e57d97dbdef2..8ef8f5a6e44cea77d7aeef4c0360d7888b5742bb 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 997979ac93b4e99b498602077f72519755eb68c6..86a9cd82d55eddf5134b0135f996ac90af922159 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);
 }