[asterisk-commits] Restrict functionality when ACLs are misconfigured. (asterisk[master])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Tue May 5 10:13:23 CDT 2015
Joshua Colp has submitted this change and it was merged.
Change subject: Restrict functionality when ACLs are misconfigured.
......................................................................
Restrict functionality when ACLs are misconfigured.
This patch has two main purposes:
1) Improve warning messages when ACLs are configured improperly.
2) Prevent misconfigured ACLs from allowing potentially unwanted
traffic.
To acomplish point (2) in most cases, whatever configuration object that
the ACL belonged to was not allowed to load.
The one exception is res_pjsip_acl. In that case, ACLs are their own
configuration object. Furthermore, the module loading code has no
indication that a ACL configuration had a failure. So the tactic taken
here is to create an ACL that just blocks everything.
ASTERISK-24969
Reported by Corey Farrell
Change-Id: I2ebcb6959cefad03cea4d81401be946203fcacae
---
M channels/chan_iax2.c
M channels/chan_mgcp.c
M channels/chan_sip.c
M channels/chan_skinny.c
M channels/chan_unistim.c
M main/acl.c
M main/manager.c
M res/res_pjsip_acl.c
8 files changed, 146 insertions(+), 88 deletions(-)
Approvals:
Joshua Colp: Looks good to me, approved; Verified
Corey Farrell: Looks good to me, but someone else must approve
diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index 253160a..bb9c52b 100644
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -13746,6 +13746,7 @@
} else if (!strcasecmp(v->name, "calltokenoptional")) {
if (add_calltoken_ignore(v->value)) {
ast_log(LOG_WARNING, "Invalid calltokenoptional address range - '%s' line %d\n", v->value, v->lineno);
+ return -1;
}
} else if (!strcasecmp(v->name, "calltokenexpiration")) {
int temp = -1;
diff --git a/channels/chan_mgcp.c b/channels/chan_mgcp.c
index 602d38d..37935be 100644
--- a/channels/chan_mgcp.c
+++ b/channels/chan_mgcp.c
@@ -4104,7 +4104,19 @@
ast_sockaddr_to_sin(&tmp, &gw->defaddr);
} else if (!strcasecmp(v->name, "permit") ||
!strcasecmp(v->name, "deny")) {
- gw->ha = ast_append_ha(v->name, v->value, gw->ha, NULL);
+ int acl_error = 0;
+ gw->ha = ast_append_ha(v->name, v->value, gw->ha, &acl_error);
+ if (acl_error) {
+ ast_log(LOG_ERROR, "Invalid ACL '%s' specified for MGCP gateway '%s' on line %d. Not creating.\n",
+ v->value, cat, v->lineno);
+ if (!gw_reload) {
+ ast_mutex_destroy(&gw->msgs_lock);
+ ast_free(gw);
+ } else {
+ gw->delme = 1;
+ }
+ return NULL;
+ }
} else if (!strcasecmp(v->name, "port")) {
gw->addr.sin_port = htons(atoi(v->value));
} else if (!strcasecmp(v->name, "context")) {
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index aa616cd..6489576 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -30609,7 +30609,9 @@
ast_append_acl(v->name, v->value, &peer->acl, &ha_error, &acl_change_subscription_needed);
}
if (ha_error) {
- ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s\n", v->lineno, v->value);
+ ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s. Deleting peer\n", v->lineno, v->value);
+ sip_unref_peer(peer, "Removing peer due to bad ACL configuration");
+ return NULL;
}
} else if (!strcasecmp(v->name, "contactpermit") || !strcasecmp(v->name, "contactdeny") || !strcasecmp(v->name, "contactacl")) {
int ha_error = 0;
@@ -30617,13 +30619,17 @@
ast_append_acl(v->name + 7, v->value, &peer->contactacl, &ha_error, &acl_change_subscription_needed);
}
if (ha_error) {
- ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s\n", v->lineno, v->value);
+ ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s. Deleting peer\n", v->lineno, v->value);
+ sip_unref_peer(peer, "Removing peer due to bad contact ACL configuration");
+ return NULL;
}
} else if (!strcasecmp(v->name, "directmediapermit") || !strcasecmp(v->name, "directmediadeny") || !strcasecmp(v->name, "directmediaacl")) {
int ha_error = 0;
ast_append_acl(v->name + 11, v->value, &peer->directmediaacl, &ha_error, &acl_change_subscription_needed);
if (ha_error) {
- ast_log(LOG_ERROR, "Bad directmedia ACL entry in configuration line %d : %s\n", v->lineno, v->value);
+ ast_log(LOG_ERROR, "Bad directmedia ACL entry in configuration line %d : %s. Deleting peer\n", v->lineno, v->value);
+ sip_unref_peer(peer, "Removing peer due to bad direct media ACL configuration");
+ return NULL;
}
} else if (!strcasecmp(v->name, "port")) {
peer->portinuri = 1;
@@ -31567,7 +31573,8 @@
int ha_error = 0;
ast_append_acl(v->name + 7, v->value, &sip_cfg.contact_acl, &ha_error, &acl_change_subscription_needed);
if (ha_error) {
- ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s\n", v->lineno, v->value);
+ ast_log(LOG_ERROR, "Bad ACL entry in configuration line %d : %s. Failing to load chan_sip.so\n", v->lineno, v->value);
+ return -1;
}
} else if (!strcasecmp(v->name, "rtautoclear")) {
int i = atoi(v->value);
diff --git a/channels/chan_skinny.c b/channels/chan_skinny.c
index 5f30a13..1d7d65a 100644
--- a/channels/chan_skinny.c
+++ b/channels/chan_skinny.c
@@ -8044,7 +8044,14 @@
}
} else if (!strcasecmp(v->name, "permit") || !strcasecmp(v->name, "deny")) {
if (type & (TYPE_DEVICE)) {
- CDEV->ha = ast_append_ha(v->name, v->value, CDEV->ha, NULL);
+ int acl_error = 0;
+
+ CDEV->ha = ast_append_ha(v->name, v->value, CDEV->ha, &acl_error);
+ if (acl_error) {
+ ast_log(LOG_ERROR, "Invalid ACL '%s' on line '%d'. Deleting device\n",
+ v->value, v->lineno);
+ CDEV->prune = 1;
+ }
continue;
}
} else if (!strcasecmp(v->name, "allow")) {
diff --git a/channels/chan_unistim.c b/channels/chan_unistim.c
index 4379aa5..51f811c 100644
--- a/channels/chan_unistim.c
+++ b/channels/chan_unistim.c
@@ -6395,6 +6395,89 @@
return ret;
}
+static void delete_device(struct unistim_device *d)
+{
+ struct unistim_line *l;
+ struct unistim_subchannel *sub;
+
+ if (unistimdebug) {
+ ast_verb(0, "Removing device '%s'\n", d->name);
+ }
+ AST_LIST_LOCK(&d->subs);
+ AST_LIST_TRAVERSE_SAFE_BEGIN(&d->subs, sub, list){
+ if (sub->subtype == SUB_REAL) {
+ if (!sub) {
+ ast_log(LOG_ERROR, "Device '%s' without a subchannel !, aborting\n",
+ d->name);
+ ast_config_destroy(cfg);
+ return 0;
+ }
+ if (sub->owner) {
+ ast_log(LOG_WARNING,
+ "Device '%s' was not deleted : a call is in progress. Try again later.\n",
+ d->name);
+ d = d->next;
+ continue;
+ }
+ }
+ if (sub->subtype == SUB_THREEWAY) {
+ ast_log(LOG_WARNING,
+ "Device '%s' with threeway call subchannels allocated, aborting.\n",
+ d->name);
+ break;
+ }
+ AST_LIST_REMOVE_CURRENT(list);
+ ast_mutex_destroy(&sub->lock);
+ ast_free(sub);
+ }
+ AST_LIST_TRAVERSE_SAFE_END
+ AST_LIST_UNLOCK(&d->subs);
+
+
+ AST_LIST_LOCK(&d->lines);
+ AST_LIST_TRAVERSE_SAFE_BEGIN(&d->lines, l, list){
+ AST_LIST_REMOVE_CURRENT(list);
+ ast_mutex_destroy(&l->lock);
+ unistim_line_destroy(l);
+ }
+ AST_LIST_TRAVERSE_SAFE_END
+ AST_LIST_UNLOCK(&d->lines);
+
+ if (d->session) {
+ if (sessions == d->session) {
+ sessions = d->session->next;
+ } else {
+ s = sessions;
+ while (s) {
+ if (s->next == d->session) {
+ s->next = d->session->next;
+ break;
+ }
+ s = s->next;
+ }
+ }
+ ast_mutex_destroy(&d->session->lock);
+ ast_free(d->session);
+ }
+ if (devices == d) {
+ devices = d->next;
+ } else {
+ struct unistim_device *d2 = devices;
+ while (d2) {
+ if (d2->next == d) {
+ d2->next = d->next;
+ break;
+ }
+ d2 = d2->next;
+ }
+ }
+ if (d->tz) {
+ d->tz = ast_tone_zone_unref(d->tz);
+ }
+ ast_mutex_destroy(&d->lock);
+ ast_free(d);
+}
+
static struct unistim_device *build_device(const char *cat, const struct ast_variable *v)
{
struct unistim_device *d;
@@ -6486,7 +6569,14 @@
} else if (!strcasecmp(v->name, "tn")) {
ast_copy_string(d->extension_number, v->value, sizeof(d->extension_number));
} else if (!strcasecmp(v->name, "permit") || !strcasecmp(v->name, "deny")) {
- d->ha = ast_append_ha(v->name, v->value, d->ha, NULL);
+ int acl_error = 0;
+ d->ha = ast_append_ha(v->name, v->value, d->ha, &acl_error);
+ if (acl_error) {
+ ast_log(LOG_ERROR, "Invalid ACL '%s' specified for device '%s' on line %d. Deleting device\n",
+ v->value, cat, v->lineno);
+ delete_device(d);
+ return NULL;
+ }
} else if (!strcasecmp(v->name, "context")) {
ast_copy_string(d->context, v->value, sizeof(d->context));
} else if (!strcasecmp(v->name, "maintext0")) {
@@ -6853,85 +6943,7 @@
d = devices;
while (d) {
if (d->to_delete) {
- struct unistim_line *l;
- struct unistim_subchannel *sub;
-
- if (unistimdebug) {
- ast_verb(0, "Removing device '%s'\n", d->name);
- }
- AST_LIST_LOCK(&d->subs);
- AST_LIST_TRAVERSE_SAFE_BEGIN(&d->subs, sub, list){
- if (sub->subtype == SUB_REAL) {
- if (!sub) {
- ast_log(LOG_ERROR, "Device '%s' without a subchannel !, aborting\n",
- d->name);
- ast_config_destroy(cfg);
- return 0;
- }
- if (sub->owner) {
- ast_log(LOG_WARNING,
- "Device '%s' was not deleted : a call is in progress. Try again later.\n",
- d->name);
- d = d->next;
- continue;
- }
- }
- if (sub->subtype == SUB_THREEWAY) {
- ast_log(LOG_WARNING,
- "Device '%s' with threeway call subchannels allocated, aborting.\n",
- d->name);
- break;
- }
- AST_LIST_REMOVE_CURRENT(list);
- ast_mutex_destroy(&sub->lock);
- ast_free(sub);
- }
- AST_LIST_TRAVERSE_SAFE_END
- AST_LIST_UNLOCK(&d->subs);
-
-
- AST_LIST_LOCK(&d->lines);
- AST_LIST_TRAVERSE_SAFE_BEGIN(&d->lines, l, list){
- AST_LIST_REMOVE_CURRENT(list);
- ast_mutex_destroy(&l->lock);
- unistim_line_destroy(l);
- }
- AST_LIST_TRAVERSE_SAFE_END
- AST_LIST_UNLOCK(&d->lines);
-
- if (d->session) {
- if (sessions == d->session) {
- sessions = d->session->next;
- } else {
- s = sessions;
- while (s) {
- if (s->next == d->session) {
- s->next = d->session->next;
- break;
- }
- s = s->next;
- }
- }
- ast_mutex_destroy(&d->session->lock);
- ast_free(d->session);
- }
- if (devices == d) {
- devices = d->next;
- } else {
- struct unistim_device *d2 = devices;
- while (d2) {
- if (d2->next == d) {
- d2->next = d->next;
- break;
- }
- d2 = d2->next;
- }
- }
- if (d->tz) {
- d->tz = ast_tone_zone_unref(d->tz);
- }
- ast_mutex_destroy(&d->lock);
- ast_free(d);
+ delete_device(d);
d = devices;
continue;
}
diff --git a/main/acl.c b/main/acl.c
index 92b6754..d133b2a 100644
--- a/main/acl.c
+++ b/main/acl.c
@@ -479,7 +479,7 @@
AST_LIST_TRAVERSE(working_list, current, list) {
if (!strcasecmp(current->name, tmp)) { /* ACL= */
/* Inclusion of the same ACL multiple times isn't a catastrophic error, but it will raise the error flag and skip the entry. */
- ast_log(LOG_ERROR, "Named ACL '%s' is already included in the ast_acl container.", tmp);
+ ast_log(LOG_ERROR, "Named ACL '%s' occurs multiple times in ACL definition. Please update your ACL configuration.", tmp);
if (error) {
*error = 1;
}
diff --git a/main/manager.c b/main/manager.c
index 2ff9df9..846f6e6 100644
--- a/main/manager.c
+++ b/main/manager.c
@@ -8977,7 +8977,13 @@
} else if (!strcasecmp(var->name, "deny") ||
!strcasecmp(var->name, "permit") ||
!strcasecmp(var->name, "acl")) {
- ast_append_acl(var->name, var->value, &user->acl, NULL, &acl_subscription_flag);
+ int acl_error = 0;
+
+ ast_append_acl(var->name, var->value, &user->acl, &acl_error, &acl_subscription_flag);
+ if (acl_error) {
+ ast_log(LOG_ERROR, "Invalid ACL '%s' for manager user '%s' on line %d. Deleting user\n");
+ user->keep = 0;
+ }
} else if (!strcasecmp(var->name, "read") ) {
user->readperm = get_perm(var->value);
} else if (!strcasecmp(var->name, "write") ) {
diff --git a/res/res_pjsip_acl.c b/res/res_pjsip_acl.c
index a04f2b4..151ebed 100644
--- a/res/res_pjsip_acl.c
+++ b/res/res_pjsip_acl.c
@@ -238,8 +238,21 @@
if (!strncmp(var->name, "contact_", 8)) {
ast_append_acl(var->name + 8, var->value, &sip_acl->contact_acl, &error, &ignore);
+ if (error) {
+ ast_log(LOG_ERROR, "Bad contact ACL '%s' at line '%d' of pjsip.conf\n",
+ var->value, var->lineno);
+ }
} else {
ast_append_acl(var->name, var->value, &sip_acl->acl, &error, &ignore);
+ if (error) {
+ ast_log(LOG_ERROR, "Bad ACL '%s' at line '%d' of pjsip.conf\n",
+ var->value, var->lineno);
+ }
+ }
+
+ if (error) {
+ ast_log(LOG_ERROR, "There is an error in ACL configuration. Blocking ALL SIP traffic.\n");
+ ast_append_acl("deny", "0.0.0.0/0.0.0.0", &sip_acl->acl, NULL, &ignore);
}
return error;
--
To view, visit https://gerrit.asterisk.org/311
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I2ebcb6959cefad03cea4d81401be946203fcacae
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
More information about the asterisk-commits
mailing list