SECURITY: actually reject messages with improper lengths

transform_reverse checks to see if the incoming message is a multiple
of the block cipher block size.  If the message fails this check, it
sets *errmsg but it fails to return.  Instead, it proceeds to attempt
to decrypt it.  This will involve a buffer read/write overrun.

It transform_reverse fails to check to see if the incoming message is
long enough to contain the IV, MAC and padding.  This can easily be
exploited to cause secnet to segfault.

buffer_unprepend should check that the buffer has enough data in it to
unprepend.  With the other patches, this should be irrelevant, but it
turns various other potential read overrun bugs into crashes.

site_incoming should check that the incoming message is long enough.
Again, without this, this is an buffer read overrun or possibly a
segfault.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

diff --git a/site.c b/site.c
index 624752cec98bcd46f8dbc1ed1c01d313091f59b6..0cd364b8135e963c7b923a684836baec2ccc1294 100644 (file)
--- a/site.c
+++ b/site.c
@@ -1121,6 +1121,9 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf,
                            const struct comm_addr *source)
 {
     struct site *st=sst;
                            const struct comm_addr *source)
 {
     struct site *st=sst;

+
+    if (buf->size < 12) return False;
+

     uint32_t dest=ntohl(*(uint32_t *)buf->start);
 
     if (dest==0) {
     uint32_t dest=ntohl(*(uint32_t *)buf->start);
 
     if (dest==0) {

diff --git a/transform.c b/transform.c
index 8fdf9fd80a350b254bb4e2d7380a31a9babc77f9..f55aa447dfd41c1c64c3dfc6b658010057b77d55 100644 (file)
--- a/transform.c
+++ b/transform.c
@@ -171,6 +171,10 @@ static uint32_t transform_reverse(void *sst, struct buffer_if *buf,
        return 1;
     }
 
        return 1;
     }
 

+    if (buf->size < 4 + 16 + 16) {
+       *errmsg="msg too short";
+       return 1;
+    }

 
     /* CBC */
     memset(iv,0,16);
 
     /* CBC */
     memset(iv,0,16);


@@ -181,6 +185,7 @@ static uint32_t transform_reverse(void *sst, struct buffer_if *buf,
     /* Assert bufsize is multiple of blocksize */
     if (buf->size&0xf) {
        *errmsg="msg not multiple of cipher blocksize";
     /* Assert bufsize is multiple of blocksize */
     if (buf->size&0xf) {
        *errmsg="msg not multiple of cipher blocksize";

+       return 1;

     }
     serpent_encrypt(&ti->cryptkey,iv,iv);
     for (n=buf->start; n<buf->start+buf->size; n+=16)
     }
     serpent_encrypt(&ti->cryptkey,iv,iv);
     for (n=buf->start; n<buf->start+buf->size; n+=16)

diff --git a/util.c b/util.c
index 63fe76f4451a80e2a6c266fffb8c1aabd878d20f..086c23440ffd1fc592495b5235277a9e104f063d 100644 (file)
--- a/util.c
+++ b/util.c
@@ -269,6 +269,7 @@ void *buf_unappend(struct buffer_if *buf, int32_t amount) {
 
 void *buf_unprepend(struct buffer_if *buf, int32_t amount) {
     void *p;
 
 void *buf_unprepend(struct buffer_if *buf, int32_t amount) {
     void *p;

+    if (buf->size < amount) return 0;

     p=buf->start;
     buf->start+=amount;
     buf->size-=amount;
     p=buf->start;
     buf->start+=amount;
     buf->size-=amount;