[Asterisk-code-review] res_rtp_asterisk: implement ACL mechanism for ICE and STUN addresses. (asterisk[master])

George Joseph asteriskteam at digium.com
Fri Mar 20 08:41:05 CDT 2020


George Joseph has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/13975 )

Change subject: res_rtp_asterisk: implement ACL mechanism for ICE and STUN addresses.
......................................................................

res_rtp_asterisk: implement ACL mechanism for ICE and STUN addresses.

A pure blacklist is not good enough, we need a whitelist mechanism as
well, and the simplest way to do that is to re-use existing ACL
infrastructure.

This makes it simpler to blacklist say an entire block (/24) except a
smaller block (eg, a /29 or even a /32).  Normally you'd need to
recursively split the block, so if you want to blacklist a /24 except
for a /29 you'd end up with a blacklit for a /25, /26, /27 and /28.  I
feel that having an ACL instead of a blacklist only is clearer.

Change-Id: Id57a8df51fcfd3bd85ea67c489c85c6c3ecd7b30
Signed-off-by: Jaco Kroon <jaco at uls.co.za>
---
M configs/samples/rtp.conf.sample
A doc/CHANGES-staging/res_rtp_asterisk_cli.txt
M res/res_rtp_asterisk.c
3 files changed, 127 insertions(+), 103 deletions(-)

Approvals:
  George Joseph: Looks good to me, approved; Approved for Submit



diff --git a/configs/samples/rtp.conf.sample b/configs/samples/rtp.conf.sample
index a94707e..f002449 100644
--- a/configs/samples/rtp.conf.sample
+++ b/configs/samples/rtp.conf.sample
@@ -69,12 +69,14 @@
 ; the wildcard 0.0.0.0 address.  e.g., A PJSIP endpoint binding RTP to a
 ; specific address using the bind_rtp_to_media_address and media_address
 ; options.  Or the PJSIP endpoint specifies an explicit transport that binds
-; to a specific IP address.
+; to a specific IP address.  Blacklisting is done via ACL infrastructure
+; so it's possible to whitelist as well.
 ;
-; e.g. stun_blacklist = 192.168.1.0/255.255.255.0
-;      stun_blacklist = 10.32.77.0/255.255.255.0
+; stun_acl = named_acl
+; stun_deny = 0.0.0.0/0
+; stun_permit = 1.2.3.4/32
 ;
-; stun_blacklist =
+; For historic reasons stun_blacklist is an alias for stun_deny.
 ;
 ; Hostname or address for the TURN server to be used as a relay. The port
 ; number is optional. If omitted the default value of 3478 will be used.
@@ -90,17 +92,19 @@
 ; Password used to authenticate with TURN relay server.
 ; turnpassword=
 ;
-; Subnets to exclude from ICE host, srflx and relay discovery. This is useful
-; to optimize the ICE process where a system has multiple host address ranges
-; and/or physical interfaces and certain of them are not expected to be used
-; for RTP. For example, VPNs and local interconnections may not be suitable or
-; necessary for ICE. Multiple subnets may be listed. If left unconfigured,
-; all discovered host addresses are used.
+; An ACL can be used to determine which discovered addresses to include for
+; ICE, srflx and relay discovery.  This is useful to optimize the ICE process
+; where a system has multiple host address ranges and/or physical interfaces
+; and certain of them are not expected to be used for RTP. For example, VPNs
+; and local interconnections may not be suitable or necessary for ICE. Multiple
+; subnets may be listed. If left unconfigured, all discovered host addresses
+; are used.
 ;
-; e.g. ice_blacklist = 192.168.1.0/255.255.255.0
-;      ice_blacklist = 10.32.77.0/255.255.255.0
+; ice_acl = named_acl
+; ice_deny = 0.0.0.0/0
+; ice_permit = 1.2.3.4/32
 ;
-; ice_blacklist =
+; For historic reasons ice_blacklist is an alias for ice_deny.
 ;
 ; The MTU to use for DTLS packet fragmentation. This option is set to 1200
 ; by default. The minimum MTU is 256.
diff --git a/doc/CHANGES-staging/res_rtp_asterisk_cli.txt b/doc/CHANGES-staging/res_rtp_asterisk_cli.txt
new file mode 100644
index 0000000..7b5516d
--- /dev/null
+++ b/doc/CHANGES-staging/res_rtp_asterisk_cli.txt
@@ -0,0 +1,18 @@
+Subject: res_rtp_asterisk
+
+The blacklist mechanism in res_rtp_asterisk for ICE and STUN was converted to
+an ACL mechanism.
+
+As such six now options are now available:
+
+ice_deny
+ice_permit
+ice_acl
+stun_deny
+stun_permit
+stun_acl
+
+These options have their obvious meanings as used elsewhere.
+
+Backwards compatibility was maintained by adding {stun,ice}_blacklist as
+aliases for {stun,ice}_deny.
diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index 366539f..05a993f 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -84,6 +84,7 @@
 #include "asterisk/data_buffer.h"
 #ifdef HAVE_PJPROJECT
 #include "asterisk/res_pjproject.h"
+#include "asterisk/security_events.h"
 #endif
 
 #define MAX_TIMESTAMP_SKEW	640
@@ -212,13 +213,15 @@
 static int turnport = DEFAULT_TURN_PORT;
 static pj_str_t turnusername;
 static pj_str_t turnpassword;
+static struct stasis_subscription *acl_change_sub = NULL;
 
-static struct ast_ha *ice_blacklist = NULL;    /*!< Blacklisted ICE networks */
-static ast_rwlock_t ice_blacklist_lock = AST_RWLOCK_INIT_VALUE;
+/*! ACL for ICE addresses */
+static struct ast_acl_list *ice_acl = NULL;
+static ast_rwlock_t ice_acl_lock = AST_RWLOCK_INIT_VALUE;
 
-/*! Blacklisted networks for STUN requests */
-static struct ast_ha *stun_blacklist = NULL;
-static ast_rwlock_t stun_blacklist_lock = AST_RWLOCK_INIT_VALUE;
+/*! ACL for STUN requests */
+static struct ast_acl_list *stun_acl = NULL;
+static ast_rwlock_t stun_acl_lock = AST_RWLOCK_INIT_VALUE;
 
 /*! \brief Pool factory used by pjlib to allocate memory. */
 static pj_caching_pool cachingpool;
@@ -3424,6 +3427,21 @@
 }
 
 #ifdef HAVE_PJPROJECT
+static void acl_change_stasis_cb(void *data, struct stasis_subscription *sub, struct stasis_message *message);
+
+/*!
+ * \internal
+ * \brief Resets and ACL to empty state.
+ *
+ * \return Nothing
+ */
+static void rtp_unload_acl(ast_rwlock_t *lock, struct ast_acl_list **acl)
+{
+	ast_rwlock_wrlock(lock);
+	*acl = ast_free_acl_list(*acl);
+	ast_rwlock_unlock(lock);
+}
+
 /*!
  * \internal
  * \brief Checks an address against the ICE blacklist
@@ -3435,17 +3453,17 @@
  */
 static int rtp_address_is_ice_blacklisted(const pj_sockaddr_t *address)
 {
-	char buf[PJ_INET6_ADDRSTRLEN];
 	struct ast_sockaddr saddr;
-	int result = 1;
+	int result = 0;
 
-	ast_sockaddr_parse(&saddr, pj_sockaddr_print(address, buf, sizeof(buf), 0), 0);
-
-	ast_rwlock_rdlock(&ice_blacklist_lock);
-	if (!ice_blacklist || (ast_apply_ha(ice_blacklist, &saddr) == AST_SENSE_ALLOW)) {
-		result = 0;
+	if (ast_sockaddr_from_pj_sockaddr(&saddr, address) < 0) {
+		ast_log(LOG_ERROR, "Failed to convert pj_sockddr_t to ast_sockaddr - ICE blacklisting (default)\n");
+		return 1;
 	}
-	ast_rwlock_unlock(&ice_blacklist_lock);
+
+	ast_rwlock_rdlock(&ice_acl_lock);
+	result |= ast_apply_acl_nolog(ice_acl, &saddr) == AST_SENSE_DENY;
+	ast_rwlock_unlock(&ice_acl_lock);
 
 	return result;
 }
@@ -3464,14 +3482,11 @@
  */
 static int stun_address_is_blacklisted(const struct ast_sockaddr *addr)
 {
-	int result = 1;
+	int result = 0;
 
-	ast_rwlock_rdlock(&stun_blacklist_lock);
-	if (!stun_blacklist
-		|| ast_apply_ha(stun_blacklist, addr) == AST_SENSE_ALLOW) {
-		result = 0;
-	}
-	ast_rwlock_unlock(&stun_blacklist_lock);
+	ast_rwlock_rdlock(&stun_acl_lock);
+	result |= ast_apply_acl_nolog(stun_acl, addr) == AST_SENSE_DENY;
+	ast_rwlock_unlock(&stun_acl_lock);
 
 	return result;
 }
@@ -8868,75 +8883,16 @@
 	AST_CLI_DEFINE(handle_cli_rtcp_set_stats, "Enable/Disable RTCP stats"),
 };
 
-#ifdef HAVE_PJPROJECT
-/*!
- * \internal
- * \brief Clear the configured blacklist.
- * \since 13.16.0
- *
- * \param lock R/W lock protecting the blacklist
- * \param blacklist List to clear
- *
- * \return Nothing
- */
-static void blacklist_clear(ast_rwlock_t *lock, struct ast_ha **blacklist)
-{
-	ast_rwlock_wrlock(lock);
-	ast_free_ha(*blacklist);
-	*blacklist = NULL;
-	ast_rwlock_unlock(lock);
-}
-
-/*!
- * \internal
- * \brief Load the blacklist configuration.
- * \since 13.16.0
- *
- * \param cfg Raw config file options.
- * \param option_name Blacklist option name
- * \param lock R/W lock protecting the blacklist
- * \param blacklist List to load
- *
- * \return Nothing
- */
-static void blacklist_config_load(struct ast_config *cfg, const char *option_name,
-	ast_rwlock_t *lock, struct ast_ha **blacklist)
-{
-	struct ast_variable *var;
-
-	ast_rwlock_wrlock(lock);
-	for (var = ast_variable_browse(cfg, "general"); var; var = var->next) {
-		if (!strcasecmp(var->name, option_name)) {
-			struct ast_ha *na;
-			int ha_error = 0;
-
-			na = ast_append_ha("d", var->value, *blacklist, &ha_error);
-			if (!na) {
-				ast_log(LOG_WARNING, "Invalid %s value: %s\n",
-					option_name, var->value);
-			} else {
-				*blacklist = na;
-			}
-			if (ha_error) {
-				ast_log(LOG_ERROR,
-					"Bad %s configuration value line %d: %s\n",
-					option_name, var->lineno, var->value);
-			}
-		}
-	}
-	ast_rwlock_unlock(lock);
-}
-#endif
-
-static int rtp_reload(int reload)
+static int rtp_reload(int reload, int by_external_config)
 {
 	struct ast_config *cfg;
 	const char *s;
-	struct ast_flags config_flags = { reload ? CONFIG_FLAG_FILEUNCHANGED : 0 };
+	struct ast_flags config_flags = { (reload && !by_external_config) ? CONFIG_FLAG_FILEUNCHANGED : 0 };
 
 #ifdef HAVE_PJPROJECT
 	struct ast_variable *var;
 	struct ast_ice_host_candidate *candidate;
+	int acl_subscription_flag = 0;
 #endif
 
 	cfg = ast_config_load2("rtp.conf", "rtp", config_flags);
@@ -8972,8 +8928,6 @@
 	turnusername = pj_str(NULL);
 	turnpassword = pj_str(NULL);
 	host_candidate_overrides_clear();
-	blacklist_clear(&ice_blacklist_lock, &ice_blacklist);
-	blacklist_clear(&stun_blacklist_lock, &stun_blacklist);
 #endif
 
 #if defined(HAVE_OPENSSL) && (OPENSSL_VERSION_NUMBER >= 0x10001000L) && !defined(OPENSSL_NO_SRTP)
@@ -9107,11 +9061,45 @@
 	}
 	AST_RWLIST_UNLOCK(&host_candidates);
 
-	/* Read ICE blacklist configuration lines */
-	blacklist_config_load(cfg, "ice_blacklist", &ice_blacklist_lock, &ice_blacklist);
+	ast_rwlock_wrlock(&ice_acl_lock);
+	ast_rwlock_wrlock(&stun_acl_lock);
 
-	/* Read STUN blacklist configuration lines */
-	blacklist_config_load(cfg, "stun_blacklist", &stun_blacklist_lock, &stun_blacklist);
+	ice_acl = ast_free_acl_list(ice_acl);
+	stun_acl = ast_free_acl_list(stun_acl);
+
+	for (var = ast_variable_browse(cfg, "general"); var; var = var->next) {
+		const char* sense = NULL;
+		struct ast_acl_list **acl = NULL;
+		if (strncasecmp(var->name, "ice_", 4) == 0) {
+			sense = var->name + 4;
+			acl = &ice_acl;
+		} else if (strncasecmp(var->name, "stun_", 5) == 0) {
+			sense = var->name + 5;
+			acl = &stun_acl;
+		} else {
+			continue;
+		}
+
+		if (strcasecmp(sense, "blacklist") == 0) {
+			sense = "deny";
+		}
+
+		if (strcasecmp(sense, "acl") && strcasecmp(sense, "permit") && strcasecmp(sense, "deny")) {
+			continue;
+		}
+
+		ast_append_acl(sense, var->value, acl, NULL, &acl_subscription_flag);
+	}
+	ast_rwlock_unlock(&ice_acl_lock);
+	ast_rwlock_unlock(&stun_acl_lock);
+
+	if (acl_subscription_flag && !acl_change_sub) {
+		acl_change_sub = stasis_subscribe(ast_security_topic(), acl_change_stasis_cb, NULL);
+		stasis_subscription_accept_message_type(acl_change_sub, ast_named_acl_change_type());
+		stasis_subscription_set_filter(acl_change_sub, STASIS_SUBSCRIPTION_FILTER_SELECTIVE);
+	} else if (!acl_subscription_flag && acl_change_sub) {
+		acl_change_sub = stasis_unsubscribe_and_join(acl_change_sub);
+	}
 #endif
 #if defined(HAVE_OPENSSL) && (OPENSSL_VERSION_NUMBER >= 0x10001000L) && !defined(OPENSSL_NO_SRTP)
 	if ((s = ast_variable_retrieve(cfg, "general", "dtls_mtu"))) {
@@ -9136,7 +9124,7 @@
 
 static int reload_module(void)
 {
-	rtp_reload(1);
+	rtp_reload(1, 0);
 	return 0;
 }
 
@@ -9154,6 +9142,16 @@
 	ast_pjproject_caching_pool_destroy(&cachingpool);
 	pj_shutdown();
 }
+
+static void acl_change_stasis_cb(void *data, struct stasis_subscription *sub, struct stasis_message *message)
+{
+	if (stasis_message_type(message) != ast_named_acl_change_type()) {
+		return;
+	}
+
+	/* There is no simple way to just reload the ACLs, so just execute a forced reload. */
+	rtp_reload(1, 1);
+}
 #endif
 
 static int load_module(void)
@@ -9234,7 +9232,7 @@
 		return AST_MODULE_LOAD_DECLINE;
 	}
 
-	rtp_reload(0);
+	rtp_reload(0, 0);
 
 	return AST_MODULE_LOAD_SUCCESS;
 }
@@ -9254,6 +9252,10 @@
 	host_candidate_overrides_clear();
 	pj_thread_register_check();
 	rtp_terminate_pjproject();
+
+	acl_change_sub = stasis_unsubscribe_and_join(acl_change_sub);
+	rtp_unload_acl(&ice_acl_lock, &ice_acl);
+	rtp_unload_acl(&stun_acl_lock, &stun_acl);
 #endif
 
 	return 0;

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13975
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Id57a8df51fcfd3bd85ea67c489c85c6c3ecd7b30
Gerrit-Change-Number: 13975
Gerrit-PatchSet: 3
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200320/474e44b0/attachment-0001.html>


More information about the asterisk-code-review mailing list