| Message ID | 20200609080229.2564-1-gert@greenie.muc.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Simplify pool size handling, fix possible array overrun on pool reading. | expand |
Hi, On 09/06/2020 10:02, Gert Doering wrote: > Remove separate ipv4.size and ipv6.size in the pool structure, return > to a single pool_size, which is also the allocated array size. > > All calls to ifconfig_pool_size() change to "pool->size" now. > > pool->size is set to the size of the active pool, or if both IPv4 and IPv6 > are in use, to the smaller size (same underlying logic as in 452113155e7, > but really put it into the size field). > > This fixes a SIGSEGV crash if an ifconfig-pool-persist file is loaded > that has IPv6 and no IPv4 (= ipv6 handle is used) and that has more > entries than the IPv4 pool size (comparison was done with ipv6.size, > not with actual pool size), introduced by commit 6a8cd033b18. > > While at it, fix pool size calculation for IPv6 pools >= /112 > (too many -1), introduced by commit 452113155e7. Thanks for taking care of these issues :-) > > Signed-off-by: Gert Doering <gert@greenie.muc.de> > --- > src/openvpn/pool.c | 96 ++++++++++++++++++++-------------------------- > src/openvpn/pool.h | 3 +- > 2 files changed, 42 insertions(+), 57 deletions(-) > > diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c > index 29667623..f60cf291 100644 > --- a/src/openvpn/pool.c > +++ b/src/openvpn/pool.c > @@ -59,29 +59,6 @@ ifconfig_pool_entry_free(struct ifconfig_pool_entry *ipe, bool hard) > } > } > > -static const int > -ifconfig_pool_size(const struct ifconfig_pool *pool) > -{ > - int min = INT_MAX; > - > - if (!pool || (!pool->ipv4.enabled && !pool->ipv6.enabled)) > - { > - return 0; > - } > - > - if (pool->ipv4.enabled) > - { > - min = pool->ipv4.size; > - } > - > - if (pool->ipv6.enabled && pool->ipv6.size < min) > - { > - min = pool->ipv6.size; > - } > - > - return min; > -} > - > static int > ifconfig_pool_find(struct ifconfig_pool *pool, const char *common_name) > { > @@ -89,9 +66,8 @@ ifconfig_pool_find(struct ifconfig_pool *pool, const char *common_name) > time_t earliest_release = 0; > int previous_usage = -1; > int new_usage = -1; > - int pool_size = ifconfig_pool_size(pool); > > - for (i = 0; i < pool_size; ++i) > + for (i = 0; i < pool->size; ++i) > { > struct ifconfig_pool_entry *ipe = &pool->list[i]; > if (!ipe->in_use) > @@ -179,12 +155,13 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start, > { > struct gc_arena gc = gc_new(); > struct ifconfig_pool *pool = NULL; > - int pool_size = -1; > + int pool_ipv4_size = -1, pool_ipv6_size = -1; > > ASSERT(start <= end && end - start < IFCONFIG_POOL_MAX); > ALLOC_OBJ_CLEAR(pool, struct ifconfig_pool); > > pool->duplicate_cn = duplicate_cn; > + pool->size = 0; This is not needed. The object is already cleared out after allocation by the ALLOC_OBJ_CLEAR() macro. > > pool->ipv4.enabled = ipv4_pool; > > @@ -195,26 +172,28 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start, > { > case IFCONFIG_POOL_30NET: > pool->ipv4.base = start & ~3; > - pool->ipv4.size = (((end | 3) + 1) - pool->ipv4.base) >> 2; > + pool_ipv4_size = (((end | 3) + 1) - pool->ipv4.base) >> 2; > break; > > case IFCONFIG_POOL_INDIV: > pool->ipv4.base = start; > - pool->ipv4.size = end - start + 1; > + pool_ipv4_size = end - start + 1; > break; > > default: > ASSERT(0); > } > > - if (pool->ipv4.size < 2) > + if (pool_ipv4_size < 2) > { > msg(M_FATAL, "IPv4 pool size is too small (%d), must be at least 2", > - pool->ipv4.size); > + pool_ipv4_size); > } > > msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv4: base=%s size=%d", > - print_in_addr_t(pool->ipv4.base, 0, &gc), pool->ipv4.size); > + print_in_addr_t(pool->ipv4.base, 0, &gc), pool_ipv4_size); > + > + pool->size = pool_ipv4_size; > } > > /* IPv6 pools are always "INDIV" type */ > @@ -239,49 +218,57 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start, > * the address. > * > * Example: if we have netbits=31, the first bit has to be zero'd, > - * the following operation first computes off=0x3fffff and then uses > - * mask to extract the wanted bits from base > + * the following operation first computes mask=0x3fffff and then > + * uses mask to extract the wanted bits from base > */ > - uint32_t mask = (1 << (128 - ipv6_netbits - 1)) - 1; > + uint32_t mask = (1 << (128 - ipv6_netbits) ) - 1; please remove the space between the 2 parenthesis. The computation now looks correct :-) > base &= mask; > } > > pool->ipv6.base = ipv6_base; > - pool->ipv6.size = ipv6_netbits > 112 > + pool_ipv6_size = ipv6_netbits >= 112 > ? (1 << (128 - ipv6_netbits)) - base > : IFCONFIG_POOL_MAX; > > - if (pool->ipv6.size < 2) > + if (pool_ipv6_size < 2) > { > msg(M_FATAL, "IPv6 pool size is too small (%d), must be at least 2", > - pool->ipv6.size); > + pool_ipv6_size); > } > > msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv6: base=%s size=%d netbits=%d", > - print_in6_addr(pool->ipv6.base, 0, &gc), pool->ipv6.size, > + print_in6_addr(pool->ipv6.base, 0, &gc), pool_ipv6_size, > ipv6_netbits); > + > + /* if there is no v4 pool, or the v6 pool is smaller, use > + * v6 pool size as "unified pool size" > + */ > + if ( pool->size <= 0 || pool_ipv6_size < pool->size ) please remove the spaces after the opening and before the closing parenthesis. > + { > + pool->size = pool_ipv6_size; > + } > } > > if (pool->ipv4.enabled && pool->ipv6.enabled) > { > - if (pool->ipv4.size < pool->ipv6.size) > + if (pool_ipv4_size < pool_ipv6_size) > { > msg(M_INFO, "NOTE: IPv4 pool size is %d, IPv6 pool size is %d. " > "IPv4 pool size limits the number of clients that can be " > - "served from the pool", pool->ipv4.size, pool->ipv6.size); > + "served from the pool", pool_ipv4_size, pool_ipv6_size); > } > - else if (pool->ipv4.size > pool->ipv6.size) > + else if (pool_ipv4_size > pool_ipv6_size) > { > msg(M_WARN, "WARNING: IPv4 pool size is %d, IPv6 pool size is %d. " > "IPv6 pool size limits the number of clients that can be " > "served from the pool. This is likely a MISTAKE - please check " > - "your configuration", pool->ipv4.size, pool->ipv6.size); > + "your configuration", pool_ipv4_size, pool_ipv6_size); > } > } > > - pool_size = ifconfig_pool_size(pool); > + ASSERT(pool->size>0); please add spaces around the '>' operator. Asserting it sounds good - this happens during initialization and it's ok to abort in that case. However, we don't expect this to happen unless this function is misused by new code. > > - ALLOC_ARRAY_CLEAR(pool->list, struct ifconfig_pool_entry, pool_size); > + ALLOC_ARRAY_CLEAR(pool->list, struct ifconfig_pool_entry, pool->size); > > gc_free(&gc); > return pool; > @@ -292,9 +279,9 @@ ifconfig_pool_free(struct ifconfig_pool *pool) > { > if (pool) > { > - int i, pool_size = ifconfig_pool_size(pool); > + int i; > > - for (i = 0; i < pool_size; ++i) > + for (i = 0; i < pool->size; ++i) > { > ifconfig_pool_entry_free(&pool->list[i], true); > } > @@ -358,9 +345,8 @@ bool > ifconfig_pool_release(struct ifconfig_pool *pool, ifconfig_pool_handle hand, const bool hard) > { > bool ret = false; > - int pool_size = ifconfig_pool_size(pool); > > - if (pool && hand >= 0 && hand < pool_size) > + if (pool && hand >= 0 && hand < pool->size) > { > ifconfig_pool_entry_free(&pool->list[hand], hard); > ret = true; > @@ -396,7 +382,7 @@ ifconfig_pool_ip_base_to_handle(const struct ifconfig_pool *pool, const in_addr_ > ASSERT(0); > } > > - if (ret < 0 || ret >= pool->ipv4.size) > + if (ret < 0 || ret >= pool->size) > { > ret = -1; > } > @@ -436,7 +422,7 @@ ifconfig_pool_ipv6_base_to_handle(const struct ifconfig_pool *pool, > | in_addr->s6_addr[15]; > > ret = addr - base; > - if (ret < 0 || ret >= pool->ipv6.size) > + if (ret < 0 || ret >= pool->size) > { > ret = -1; > } > @@ -449,7 +435,7 @@ ifconfig_pool_handle_to_ip_base(const struct ifconfig_pool *pool, ifconfig_pool_ > { > in_addr_t ret = 0; > > - if (pool->ipv4.enabled && hand >= 0 && hand < pool->ipv4.size) > + if (pool->ipv4.enabled && hand >= 0 && hand < pool->size) > { > switch (pool->ipv4.type) > { > @@ -479,7 +465,7 @@ ifconfig_pool_handle_to_ipv6_base(const struct ifconfig_pool *pool, ifconfig_poo > struct in6_addr ret = IN6ADDR_ANY_INIT; > > /* IPv6 pools are always INDIV (--linear) */ > - if (pool->ipv6.enabled && hand >= 0 && hand < pool->ipv6.size) > + if (pool->ipv6.enabled && hand >= 0 && hand < pool->size) > { > ret = add_in6_addr( pool->ipv6.base, hand ); > } > @@ -504,9 +490,9 @@ ifconfig_pool_list(const struct ifconfig_pool *pool, struct status_output *out) > if (pool && out) > { > struct gc_arena gc = gc_new(); > - int i, pool_size = ifconfig_pool_size(pool); > + int i; > > - for (i = 0; i < pool_size; ++i) > + for (i = 0; i < pool->size; ++i) > { > const struct ifconfig_pool_entry *e = &pool->list[i]; > struct in6_addr ip6; > @@ -720,7 +706,7 @@ ifconfig_pool_read(struct ifconfig_pool_persist *persist, struct ifconfig_pool * > */ > if (h >= 0) > { > - msg(M_INFO, "succeeded -> ifconfig_pool_set()"); > + msg(M_INFO, "succeeded -> ifconfig_pool_set(hand=%d)",h); > ifconfig_pool_set(pool, cn_buf, h, persist->fixed); > } > } > diff --git a/src/openvpn/pool.h b/src/openvpn/pool.h > index 6af04645..b06424c9 100644 > --- a/src/openvpn/pool.h > +++ b/src/openvpn/pool.h > @@ -55,13 +55,12 @@ struct ifconfig_pool > bool enabled; > enum pool_type type; > in_addr_t base; > - int size; > } ipv4; > struct { > bool enabled; > struct in6_addr base; > - unsigned int size; > } ipv6; > + int size; > struct ifconfig_pool_entry *list; > }; > > The rest looks good, thanks! I am ACKing this patch under the assumption that those little changes are applied before merging: Acked-by: Antonio Quartulli <a@unstable.cc>
Patch has been applied to the master branch.
I have amended most whitespace as requested, and removed the
"pool-> size = 0" initialization (not needed, indeed).
I have left the brackets on the pool size calculation as they were -
uncrustify says "that is also ok", and I find visibly grouping the
different parts of the formula helps understanding what it does.
commit e7c0cd996f35965172c5def9531b6ab9ca10c389
Author: Gert Doering
Date: Tue Jun 9 10:02:29 2020 +0200
Simplify pool size handling, fix possible array overrun on pool reading.
Signed-off-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Antonio Quartulli <a@unstable.cc>
Message-Id: <20200609080229.2564-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20006.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c index 29667623..f60cf291 100644 --- a/src/openvpn/pool.c +++ b/src/openvpn/pool.c @@ -59,29 +59,6 @@ ifconfig_pool_entry_free(struct ifconfig_pool_entry *ipe, bool hard) } } -static const int -ifconfig_pool_size(const struct ifconfig_pool *pool) -{ - int min = INT_MAX; - - if (!pool || (!pool->ipv4.enabled && !pool->ipv6.enabled)) - { - return 0; - } - - if (pool->ipv4.enabled) - { - min = pool->ipv4.size; - } - - if (pool->ipv6.enabled && pool->ipv6.size < min) - { - min = pool->ipv6.size; - } - - return min; -} - static int ifconfig_pool_find(struct ifconfig_pool *pool, const char *common_name) { @@ -89,9 +66,8 @@ ifconfig_pool_find(struct ifconfig_pool *pool, const char *common_name) time_t earliest_release = 0; int previous_usage = -1; int new_usage = -1; - int pool_size = ifconfig_pool_size(pool); - for (i = 0; i < pool_size; ++i) + for (i = 0; i < pool->size; ++i) { struct ifconfig_pool_entry *ipe = &pool->list[i]; if (!ipe->in_use) @@ -179,12 +155,13 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start, { struct gc_arena gc = gc_new(); struct ifconfig_pool *pool = NULL; - int pool_size = -1; + int pool_ipv4_size = -1, pool_ipv6_size = -1; ASSERT(start <= end && end - start < IFCONFIG_POOL_MAX); ALLOC_OBJ_CLEAR(pool, struct ifconfig_pool); pool->duplicate_cn = duplicate_cn; + pool->size = 0; pool->ipv4.enabled = ipv4_pool; @@ -195,26 +172,28 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start, { case IFCONFIG_POOL_30NET: pool->ipv4.base = start & ~3; - pool->ipv4.size = (((end | 3) + 1) - pool->ipv4.base) >> 2; + pool_ipv4_size = (((end | 3) + 1) - pool->ipv4.base) >> 2; break; case IFCONFIG_POOL_INDIV: pool->ipv4.base = start; - pool->ipv4.size = end - start + 1; + pool_ipv4_size = end - start + 1; break; default: ASSERT(0); } - if (pool->ipv4.size < 2) + if (pool_ipv4_size < 2) { msg(M_FATAL, "IPv4 pool size is too small (%d), must be at least 2", - pool->ipv4.size); + pool_ipv4_size); } msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv4: base=%s size=%d", - print_in_addr_t(pool->ipv4.base, 0, &gc), pool->ipv4.size); + print_in_addr_t(pool->ipv4.base, 0, &gc), pool_ipv4_size); + + pool->size = pool_ipv4_size; } /* IPv6 pools are always "INDIV" type */ @@ -239,49 +218,57 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start, * the address. * * Example: if we have netbits=31, the first bit has to be zero'd, - * the following operation first computes off=0x3fffff and then uses - * mask to extract the wanted bits from base + * the following operation first computes mask=0x3fffff and then + * uses mask to extract the wanted bits from base */ - uint32_t mask = (1 << (128 - ipv6_netbits - 1)) - 1; + uint32_t mask = (1 << (128 - ipv6_netbits) ) - 1; base &= mask; } pool->ipv6.base = ipv6_base; - pool->ipv6.size = ipv6_netbits > 112 + pool_ipv6_size = ipv6_netbits >= 112 ? (1 << (128 - ipv6_netbits)) - base : IFCONFIG_POOL_MAX; - if (pool->ipv6.size < 2) + if (pool_ipv6_size < 2) { msg(M_FATAL, "IPv6 pool size is too small (%d), must be at least 2", - pool->ipv6.size); + pool_ipv6_size); } msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv6: base=%s size=%d netbits=%d", - print_in6_addr(pool->ipv6.base, 0, &gc), pool->ipv6.size, + print_in6_addr(pool->ipv6.base, 0, &gc), pool_ipv6_size, ipv6_netbits); + + /* if there is no v4 pool, or the v6 pool is smaller, use + * v6 pool size as "unified pool size" + */ + if ( pool->size <= 0 || pool_ipv6_size < pool->size ) + { + pool->size = pool_ipv6_size; + } } if (pool->ipv4.enabled && pool->ipv6.enabled) { - if (pool->ipv4.size < pool->ipv6.size) + if (pool_ipv4_size < pool_ipv6_size) { msg(M_INFO, "NOTE: IPv4 pool size is %d, IPv6 pool size is %d. " "IPv4 pool size limits the number of clients that can be " - "served from the pool", pool->ipv4.size, pool->ipv6.size); + "served from the pool", pool_ipv4_size, pool_ipv6_size); } - else if (pool->ipv4.size > pool->ipv6.size) + else if (pool_ipv4_size > pool_ipv6_size) { msg(M_WARN, "WARNING: IPv4 pool size is %d, IPv6 pool size is %d. " "IPv6 pool size limits the number of clients that can be " "served from the pool. This is likely a MISTAKE - please check " - "your configuration", pool->ipv4.size, pool->ipv6.size); + "your configuration", pool_ipv4_size, pool_ipv6_size); } } - pool_size = ifconfig_pool_size(pool); + ASSERT(pool->size>0); - ALLOC_ARRAY_CLEAR(pool->list, struct ifconfig_pool_entry, pool_size); + ALLOC_ARRAY_CLEAR(pool->list, struct ifconfig_pool_entry, pool->size); gc_free(&gc); return pool; @@ -292,9 +279,9 @@ ifconfig_pool_free(struct ifconfig_pool *pool) { if (pool) { - int i, pool_size = ifconfig_pool_size(pool); + int i; - for (i = 0; i < pool_size; ++i) + for (i = 0; i < pool->size; ++i) { ifconfig_pool_entry_free(&pool->list[i], true); } @@ -358,9 +345,8 @@ bool ifconfig_pool_release(struct ifconfig_pool *pool, ifconfig_pool_handle hand, const bool hard) { bool ret = false; - int pool_size = ifconfig_pool_size(pool); - if (pool && hand >= 0 && hand < pool_size) + if (pool && hand >= 0 && hand < pool->size) { ifconfig_pool_entry_free(&pool->list[hand], hard); ret = true; @@ -396,7 +382,7 @@ ifconfig_pool_ip_base_to_handle(const struct ifconfig_pool *pool, const in_addr_ ASSERT(0); } - if (ret < 0 || ret >= pool->ipv4.size) + if (ret < 0 || ret >= pool->size) { ret = -1; } @@ -436,7 +422,7 @@ ifconfig_pool_ipv6_base_to_handle(const struct ifconfig_pool *pool, | in_addr->s6_addr[15]; ret = addr - base; - if (ret < 0 || ret >= pool->ipv6.size) + if (ret < 0 || ret >= pool->size) { ret = -1; } @@ -449,7 +435,7 @@ ifconfig_pool_handle_to_ip_base(const struct ifconfig_pool *pool, ifconfig_pool_ { in_addr_t ret = 0; - if (pool->ipv4.enabled && hand >= 0 && hand < pool->ipv4.size) + if (pool->ipv4.enabled && hand >= 0 && hand < pool->size) { switch (pool->ipv4.type) { @@ -479,7 +465,7 @@ ifconfig_pool_handle_to_ipv6_base(const struct ifconfig_pool *pool, ifconfig_poo struct in6_addr ret = IN6ADDR_ANY_INIT; /* IPv6 pools are always INDIV (--linear) */ - if (pool->ipv6.enabled && hand >= 0 && hand < pool->ipv6.size) + if (pool->ipv6.enabled && hand >= 0 && hand < pool->size) { ret = add_in6_addr( pool->ipv6.base, hand ); } @@ -504,9 +490,9 @@ ifconfig_pool_list(const struct ifconfig_pool *pool, struct status_output *out) if (pool && out) { struct gc_arena gc = gc_new(); - int i, pool_size = ifconfig_pool_size(pool); + int i; - for (i = 0; i < pool_size; ++i) + for (i = 0; i < pool->size; ++i) { const struct ifconfig_pool_entry *e = &pool->list[i]; struct in6_addr ip6; @@ -720,7 +706,7 @@ ifconfig_pool_read(struct ifconfig_pool_persist *persist, struct ifconfig_pool * */ if (h >= 0) { - msg(M_INFO, "succeeded -> ifconfig_pool_set()"); + msg(M_INFO, "succeeded -> ifconfig_pool_set(hand=%d)",h); ifconfig_pool_set(pool, cn_buf, h, persist->fixed); } } diff --git a/src/openvpn/pool.h b/src/openvpn/pool.h index 6af04645..b06424c9 100644 --- a/src/openvpn/pool.h +++ b/src/openvpn/pool.h @@ -55,13 +55,12 @@ struct ifconfig_pool bool enabled; enum pool_type type; in_addr_t base; - int size; } ipv4; struct { bool enabled; struct in6_addr base; - unsigned int size; } ipv6; + int size; struct ifconfig_pool_entry *list; };
Remove separate ipv4.size and ipv6.size in the pool structure, return to a single pool_size, which is also the allocated array size. All calls to ifconfig_pool_size() change to "pool->size" now. pool->size is set to the size of the active pool, or if both IPv4 and IPv6 are in use, to the smaller size (same underlying logic as in 452113155e7, but really put it into the size field). This fixes a SIGSEGV crash if an ifconfig-pool-persist file is loaded that has IPv6 and no IPv4 (= ipv6 handle is used) and that has more entries than the IPv4 pool size (comparison was done with ipv6.size, not with actual pool size), introduced by commit 6a8cd033b18. While at it, fix pool size calculation for IPv6 pools >= /112 (too many -1), introduced by commit 452113155e7. Signed-off-by: Gert Doering <gert@greenie.muc.de> --- src/openvpn/pool.c | 96 ++++++++++++++++++++-------------------------- src/openvpn/pool.h | 3 +- 2 files changed, 42 insertions(+), 57 deletions(-)