| Summary: | ASTERISK-24969: Named ACL's do not handle config errors. | ||||
| Reporter: | Corey Farrell (coreyfarrell) | Labels: | |||
| Date Opened: | 2015-04-16 12:06:31 | Date Closed: | 2016-09-23 14:21:01 | ||
| Priority: | Major | Regression? | |||
| Status: | Closed/Complete | Components: | Core/ACL | ||
| Versions: | 11.17.1 13.3.2 | Frequency of Occurrence | |||
| Related Issues: |
| ||||
| Environment: | Attachments: | ( 0) acl.diff ( 1) ASTERISK-24969-acl-critical-startup.patch | |||
| Description: | While investigating ASTERISK-24874 I found that an invalid acl.conf leaves the system vulnerable.
# Invalid acl.conf on startup does not abort startup. ACL's are critical to security so this can be dangerous to allow the system to run in this state. # Even if we do not abort startup, running CLI {{module reload acl}} produces no output, leaving the admin to believe that ACL's were successfully loaded. I'm not convinced this is a security issue, this ticket is locked down for now until others can decide if it should be kept quiet or posted in the open. | ||||
| Comments: | By: Corey Farrell (coreyfarrell) 2015-04-16 13:04:24.846-0500 I've run another test. Start the system with a valid acl.conf, add an error to it, then run {{module reload acl}}. In this case the system does the right thing - it displays errors from the reload, but leaves the existing config in place. If you do a second reloads (without editing the file again), it fails to redisplay the errors. By: Richard Mudgett (rmudgett) 2015-04-16 13:33:48.555-0500 If you didn't touch the acl.conf file for the second reload then it won't try to reload again. By: Corey Farrell (coreyfarrell) 2015-04-16 13:42:33.179-0500 I am suggesting that config_options.c should track the previous load status. If previous status is ACO_PROCESS_ERROR and current status is ACO_PROCESS_UNCHANGED, {{aco_process_config}} should return ACO_PROCESS_ERROR. This way whatever is attempting to do a reload can report that it is still failing. By: Corey Farrell (coreyfarrell) 2015-04-16 14:04:23.929-0500 Another problem with ACO_PROCESS_UNCHANGE is dependencies between configs. If a module is configured to use a non-existent ACL, that module should report a failure on load. As an admin, I would expect to be able to add the missing section to acl.conf, reload it, then reload the module. The fact that the module config is unmodified should not matter. By: Corey Farrell (coreyfarrell) 2015-04-16 14:51:48.106-0500 Patch to abort startup on acl.conf error. I know this would require notes in UPGRADE.txt, just don't know if the change can be accepted. The additional code to allow startup if acl.conf is missing was added to minimize impact on already released branches. Unsure we want that in trunk. Tested against 11: Asterisk starts when acl.conf is valid or missing, stops if acl.conf is invalid. By: Mark Michelson (mmichelson) 2015-04-27 17:46:08.612-0500 Hey Corey, just got caught up on this. Here are my opinions: # I think that classifying this as a "security issue" is a bit of a stretch. It's hard to really even classify this as a "bug" since program behavior is not contrary to what could be expected. If bad configuration is processed, Asterisk prints error messages about it and the faulty configuration is not loaded. Also, this is not exploitable like most security issues. # I do think that the improvements discussed here are good ones, though, since it does lead to a safer environment in the face of poor configuration. I think that the following should be done: ## Error reporting in ACL code should be improved in general. Right now there are some messages that are not very user-friendly (e.g. "Named ACL 'vpn_acl' is already included in the ast_acl container."). Also as you've pointed out, the lack of any messages when reloading unchanged but previously-faulty configuration is not good. I think that this should be fixed in supported versions of Asterisk (11, 13, master) ## I also think that shutting down Asterisk in the case where named ACL configuration is bad is a good idea, but I think it's a bit drastic for current releases. I think this change should be made in master only, with a note in UPGRADE.txt about it. ## It may be worthwhile to perform an audit of modules that have configurable ACLs. In the case where the ACL has a fault, do not allow the object that contains the ACL to load. If the ACL is at module level, then that means the module doesn't load. So the current patch is a good one, except that I'd add some sort of error message that indicates that Asterisk is refusing to start up due to bad named ACL configuration. That takes care of part of the overall fix here. I'll work on the other parts of it starting tomorrow. By: Mark Michelson (mmichelson) 2015-04-28 16:53:13.265-0500 I'm uploading acl.diff. It's a first attempt at making ACL checking much more strict. Any object with a faulty ACL fails to load. The biggest issue I ran across is with res_pjsip_acl.c. In that file, the ACL object will fail to load if the ACL is invalidly formed. However, the ACL object failing to load doesn't prevent the rest of res_pjsip from loading. So for now, that is untouched in the patch. By: Corey Farrell (coreyfarrell) 2015-04-28 23:13:43.454-0500 I haven't had a chance to look at your patch yet, but what if an invalid ACL just blocked all addresses? That is instead of trying to block creation of an object that uses an invalid ACL, just give that object a 'block all' ACL. Not sure if this comment applies to all users of ACL or just res_pjsip_acl. By: Mark Michelson (mmichelson) 2015-04-29 12:21:51.129-0500 For res_pjsip_acl that might be a sensible option since there's not really much else that can be done (at least not without some API breakage). For the other cases, I think that a "block all" ACL would be more surprising than a failure to load the given object. By: Mark Michelson (mmichelson) 2015-04-30 10:44:41.734-0500 I'm going to open this issue up and make it public. I'm also going to put my patch up for review (along with the suggestion of the "block everything" behavior in res_pjsip_acl). I suggest you push your patch up to gerrit as well. Once all patches are merged, this issue can be closed out. By: Rusty Newton (rnewton) 2015-05-17 15:08:34.543-0500 [~coreyfarrell] do you have a patch in progress for this one? I'm just cleaning up some issues and making sure this one still needs to be open. By: Corey Farrell (coreyfarrell) 2015-05-18 21:32:41.614-0500 This needs to be open, I've been a bit busy lately with other issues / work. I still need to post to gerrit for aborting Asterisk startup when acl.conf is invalid. [~mmichelson]: Is any part of your patch going to 11 or 13? | ||||