From patchwork Mon Jun 8 22:02:29 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gert Doering X-Patchwork-Id: 1150 Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director7.mail.ord1d.rsapps.net ([172.30.191.6]) by backend30.mail.ord1d.rsapps.net with LMTP id iBgSKmNC316UcgAAIUCqbw for ; Tue, 09 Jun 2020 04:03:47 -0400 Received: from proxy10.mail.ord1d.rsapps.net ([172.30.191.6]) by director7.mail.ord1d.rsapps.net with LMTP id uF4FKmNC314tUwAAovjBpQ ; Tue, 09 Jun 2020 04:03:47 -0400 Received: from smtp23.gate.ord1d ([172.30.191.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by proxy10.mail.ord1d.rsapps.net with LMTP id 2CbIKWNC314cZAAAfSg8FQ ; Tue, 09 Jun 2020 04:03:47 -0400 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO X-Virus-Scanned: OK X-Orig-To: openvpnslackdevel@openvpn.net X-Originating-Ip: [216.105.38.7] Authentication-Results: smtp23.gate.ord1d.rsapps.net; iprev=pass policy.iprev="216.105.38.7"; spf=pass smtp.mailfrom="openvpn-devel-bounces@lists.sourceforge.net" smtp.helo="lists.sourceforge.net"; dkim=fail (signature verification failed) header.d=sourceforge.net; dkim=fail (signature verification failed) header.d=sf.net; dmarc=none (p=nil; dis=none) header.from=greenie.muc.de X-Suspicious-Flag: YES X-Classification-ID: be6f050a-aa27-11ea-bed4-525400bfb165-1-1 Received: from [216.105.38.7] ([216.105.38.7:37528] helo=lists.sourceforge.net) by smtp23.gate.ord1d.rsapps.net (envelope-from ) (ecelerity 4.2.38.62370 r(:)) with ESMTPS (cipher=DHE-RSA-AES256-GCM-SHA384) id 70/DE-07380-2624FDE5; Tue, 09 Jun 2020 04:03:47 -0400 Received: from [127.0.0.1] (helo=sfs-ml-4.v29.lw.sourceforge.com) by sfs-ml-4.v29.lw.sourceforge.com with esmtp (Exim 4.90_1) (envelope-from ) id 1jiZDm-0001FX-2l; Tue, 09 Jun 2020 08:02:58 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-4.v29.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jiZDa-0001Ep-Hq for openvpn-devel@lists.sourceforge.net; Tue, 09 Jun 2020 08:02:46 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Transfer-Encoding:MIME-Version:References: In-Reply-To:Message-Id:Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=T9aQxd2EN62WSYSyhR8r/IZh+Zf0t5y+TNtWAb5oXZs=; b=M9epoFudTjLFjaJreymRfC+awj MljKSFiE55idWInkIPKs+3lSapzST8T9w3ohwn94uy+twPGtDJyvmVKxHt7xo5o2BSBUr6yRo6jyX p68AddcaoYlfzCrlF+ABfGYqI7I62g1VmGAft/BqTK3zh4v7YzjEsbH680SSr8tYficQ=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To:Message-Id: Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=T9aQxd2EN62WSYSyhR8r/IZh+Zf0t5y+TNtWAb5oXZs=; b=CuL5L4ksZFGXupAIytrIuiO+yE E9rU3qZC5BcwItqhuP71Q/IL6hnncpo1LudVjUXSaJn5WOZNKAaYhKV4g3FXlvtHw3hysmvm6PlnF f9TmiNzbic/Av0sX0X6zyqKGhvipP6Bs0mT9hflT8I/rd0KllAT0A8qshh/bM60W+QxA=; Received: from chekov.greenie.muc.de ([193.149.48.178]) by sfi-mx-1.v28.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92.2) id 1jiZDY-00CVyS-7w for openvpn-devel@lists.sourceforge.net; Tue, 09 Jun 2020 08:02:46 +0000 Received: from chekov.greenie.muc.de (localhost [127.0.0.1]) by chekov.greenie.muc.de (8.15.2/8.15.2) with ESMTPS id 05982UgN002607 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO) for ; Tue, 9 Jun 2020 10:02:30 +0200 (CEST) (envelope-from gert@chekov.greenie.muc.de) Received: (from gert@localhost) by chekov.greenie.muc.de (8.15.2/8.15.2/Submit) id 05982Tjh002606 for openvpn-devel@lists.sourceforge.net; Tue, 9 Jun 2020 10:02:29 +0200 (CEST) (envelope-from gert) From: Gert Doering To: openvpn-devel@lists.sourceforge.net Date: Tue, 9 Jun 2020 10:02:29 +0200 Message-Id: <20200609080229.2564-1-gert@greenie.muc.de> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20200530000600.1680-1-a@unstable.cc> References: <20200530000600.1680-1-a@unstable.cc> MIME-Version: 1.0 X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. 0.0 SPF_NONE SPF: sender does not publish an SPF Record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record X-Headers-End: 1jiZDY-00CVyS-7w Subject: [Openvpn-devel] [PATCH] Simplify pool size handling, fix possible array overrun on pool reading. X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox 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 Acked-by: Antonio Quartulli --- 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; 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; };