[Openvpn-devel] range-checking bug in route.c

[Gert Doering]

Hi,

when implementing the missing helpers for saving/restoring ipv6 route
option lists, I noticed that the existing code has safeguards against
buffer overruns, but they do not work in all circumstances.

Imagine:

 - source list: capacity = 20, n = 5 (only 5 items in there)
 - destination list: capacity = 10

the check was "src->n > dst->capacity", which is OK, but then the full
source list of "20 items" is copied over to dst - overrunning the memory.

Change to compare src->capacity to dst->capacity.  (We could change
the logic to copy only src->n items instead, and restore dst->capacity
later on, but that's just another way to fix things :) ).

Applies to master, on top of the previous patch.

gert
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             g...@greenie.muc.de
fax: +49-89-35655025                        g...@net.informatik.tu-muenchen.de

From 95597af44714ccd350e849a0fc14e89db7e1529e Mon Sep 17 00:00:00 2001
From: Gert Doering <g...@greenie.muc.de>
List-Post: openvpn-devel@lists.sourceforge.net
Date: Fri, 30 Dec 2011 21:42:13 +0100
Subject: [PATCH] Fix list-overrun checks in copy_route_[ipv6_]option_list()

The old code checks how many items are in use(!) in the source
list, but then copies the full list over the destination memory
arena.  Check the source list *capacity*.

Signed-off-by: Gert Doering <g...@greenie.muc.de>
---
 route.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/route.c b/route.c
index e95292c..ce17f1f 100644
--- a/route.c
+++ b/route.c
@@ -121,8 +121,8 @@ void
 copy_route_option_list (struct route_option_list *dest, const struct 
route_option_list *src)
 {
   const size_t src_size = array_mult_safe (sizeof(struct route_option), 
src->capacity, sizeof(struct route_option_list));
-  if (src->n > dest->capacity)
-    msg (M_FATAL, PACKAGE_NAME " ROUTE: (copy) number of route options in src 
(%d) is greater than route list capacity in dest (%d)", src->n, dest->capacity);
+  if (src->capacity > dest->capacity)
+    msg (M_FATAL, PACKAGE_NAME " ROUTE: (copy) number of route options in src 
(%d) is greater than route list capacity in dest (%d)", src->capacity, 
dest->capacity);
   memcpy (dest, src, src_size);
 }
 
@@ -131,8 +131,8 @@ copy_route_ipv6_option_list (struct route_ipv6_option_list 
*dest,
                             const struct route_ipv6_option_list *src)
 {
   const size_t src_size = array_mult_safe (sizeof(struct route_ipv6_option), 
src->capacity, sizeof(struct route_ipv6_option_list));
-  if (src->n > dest->capacity)
-    msg (M_FATAL, PACKAGE_NAME " ROUTE: (copy) number of route options in src 
(%d) is greater than route list capacity in dest (%d)", src->n, dest->capacity);
+  if (src->capacity > dest->capacity)
+    msg (M_FATAL, PACKAGE_NAME " ROUTE: (copy) number of route options in src 
(%d) is greater than route list capacity in dest (%d)", src->capacity, 
dest->capacity);
   memcpy (dest, src, src_size);
 }
 
-- 
1.7.3.4

pgp415xnmVnaZ.pgp
Description: PGP signature