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

diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index 253160a92961e6140f9a811ce81b03f796f285d3..bb9c52bb82e78093223780da30696c3206919354 100644 (file)
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -13746,6 +13746,7 @@ static int set_config(const char *config_file, int reload, int forced)
                } 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 602d38d014a1ca972545e58a71177fb923778809..37935be4b63f41b849c199d495e3a12a96629239 100644 (file)
--- a/channels/chan_mgcp.c
+++ b/channels/chan_mgcp.c
@@ -4104,7 +4104,19 @@ static struct mgcp_gateway *build_gateway(char *cat, struct ast_variable *v)
                        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 aa616cde117a182cfc0c028d2463ac19d87e43f4..648957671924b1ead8759579445de565f42808d5 100644 (file)
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -30609,7 +30609,9 @@ static struct sip_peer *build_peer(const char *name, struct ast_variable *v, str
                                        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 @@ static struct sip_peer *build_peer(const char *name, struct ast_variable *v, str
                                        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 @@ static int reload_config(enum channelreloadreason reason)
                        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 5f30a13b56bb11c1e48a150d242da0272604a5ee..1d7d65af4c14b4a758e23e0243074aefd4a884f6 100644 (file)
--- a/channels/chan_skinny.c
+++ b/channels/chan_skinny.c
@@ -8044,7 +8044,14 @@ static void config_parse_variables(int type, void *item, struct ast_variable *vp
                        }
                } 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 4379aa5c60bbcf6ddddeaf3bcc5f01cdbe414b66..51f811c02544e50b0ead8c903d17a612480078e6 100644 (file)
--- a/channels/chan_unistim.c
+++ b/channels/chan_unistim.c
@@ -6395,6 +6395,89 @@ static struct unistim_line *find_line_by_number(struct unistim_device *d, const
        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 @@ static struct unistim_device *build_device(const char *cat, const struct ast_var
                } 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 @@ static int reload_config(void)
        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 92b675443bdc5278574294877361f87d9cbe54e3..d133b2a0719dfd0c500d2a87e62e6f3b4670e1d6 100644 (file)
--- a/main/acl.c
+++ b/main/acl.c
@@ -479,7 +479,7 @@ void ast_append_acl(const char *sense, const char *stuff, struct ast_acl_list **
                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 2ff9df930f3e269ee0cb87acf3d9ea57b26ba27f..846f6e6043bc7c94d6f66f8476ae41081338e36f 100644 (file)
--- a/main/manager.c
+++ b/main/manager.c
@@ -8977,7 +8977,13 @@ static int __init_manager(int reload, int by_external_config)
                        } 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 a04f2b422c6309483bd27fab69e8f088fd11d3fc..151ebededdb7bf96db4bf7e143527d2d783e822f 100644 (file)
--- a/res/res_pjsip_acl.c
+++ b/res/res_pjsip_acl.c
@@ -238,8 +238,21 @@ static int acl_handler(const struct aco_option *opt, struct ast_variable *var, v
 
        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;