Summary:	ASTERISK-24490: Security Vulnerability: CONFBRIDGE function's record_command option allows arbitrary parameters to be passed to MixMonitor, allowing remote execution of commands
Reporter:	Matt Jordan (mjordan)	Labels:	Security
Date Opened:	2014-11-04 14:49:45.000-0600	Date Closed:	2014-11-20 10:58:00.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_confbridge
Versions:	11.13.1 12.6.1 13.0.0	Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) ami-restrict-vars-427335.patch ( 1) confbridge_record_permissions.diff ( 2) inhibit-escalations-427353.patch
Description:	See comments on https://reviewboard.asterisk.org/r/4023/ for more information. To quote Gareth: {quote} The record_file option is not safe because by including two commas, MixMonitor() can be made to execute a command eg: CONFBRIDGE(bridge,record_file) = test.wav,,/usr/bin/touch /tmp/oops.txt -- .wav So instead of registering CONFBRIDGE() as escalating, I could have function_capable_string_allowed_with_auths check for CONFBRIDGE(bridge,record_command) in main/manager.c. As for record_file, I don't know if supporting filenames with commas is desirable. If not I can just have it truncate the filename at the first comma. {quote} My proposal would be to just mark the write functionality of CONFBRIDGE as being unsafe, and let {{live_dangerously}} deal with it.
Comments:	By: Gareth Palmer (gareth) 2014-11-05 19:48:04.569-0600 The patch blocks access to CONFBRIDGE()'s record options as well as MONITOR_EXEC to prevent commands from being executed. FILE() and Record() are now also blocked to prevent users creating files on the filesystem. Use of str(case)str calls have been replaced with str(case)cmp to avoid false-positives when checking the blacklist. By: Corey Farrell (coreyfarrell) 2014-11-06 04:14:47.858-0600 Probably a crazy idea, but what if we extend the concept of live_dangerous to ACO?  That is allow ACO options to be registered as dangerous, and thus prevent them from being written from AMI threads.  This way app_confbridge could register [type=bridge] - record_file as dangerous.. For the record I don't have a problem with just blocking CONFBRIDGE from AMI, but I'm always a fan of more granular permissions.  What I'm afraid of is that we could eventually create a situation where live_dangerously=no causes too many restrictions, forcing admin's to allow dangerous operations. By: Matt Jordan (mjordan) 2014-11-06 08:55:29.618-0600 First thing's first: the patch attached here is not the right way to fix this. We've tried - and failed - way too many times to add more junk to that string compare in manager.c. Even if the patch here "fixes" this issue, it sets a bad precedent - and only solves this problem for AMI (and not, say, ARI). While I like Corey's suggestion, I'm concerned about trying to get too clever here. The permission escalation problems are very difficult to predict completely, and extending this to ACO may break other commands/deployments. The side effects may be difficult to predict - for example, if an AMI command is run that reloads a configuration, and Asterisk is fully booted, this will get performed on the thread executing the action (which is an external system thread). Since that may now run into the ACO system, we may end up breaking remote reloading of modules. It all depends on the implementation, but I think this gets tricky very quickly. The {{live_dangerously}} option was implemented for this exact problem: if you have a function that has side effects which may be dangerous to execute from remote systems, you prevent it from being run by remote systems unless the system administrator wants to allow it. I'm not sure why it wouldn't be appropriate for both functions here. By: Gareth Palmer (gareth) 2014-11-09 19:34:31.881-0600 Here is a new patch, this one is based around the {{live_dangerously}} option. - Register CONFBRIDGE() and EVAL() as escalating. - DB() is now being registered as escalating, because while DBGet, DBPut etc. require EVENT_FLAG_SYSTEM, DB() can be called with just EVENT_FLAG_CALL. - Make it possble to register applications as escalating. - Register System(), TrySystem(), AGI(), EAGI(), DeadAGI(), ExternalIVR(), Monitor(), MixMonitor() and Record() as escalating. Registering Monitor() as escalating prevents command execution via MONITOR_EXEC. By: Kevin Harwell (kharwell) 2014-11-17 18:29:18.277-0600 Hi Gareth, I've been looking into this issue and am trying to understand it better. There are for sure two vulnerabilities with regards to confbridge recording: 1) in the CONFBRIDGE dialplan function 2) in the AMI ConfbridgeStartRecord action Both of those can be easily fixed by changing the way they are registered (I'll upload the patch for that). However, I noticed in your patch you added escalation checks for dialplan applications (among other things). Those those checks feel outside the scope of this particular issue as I do not see them being a security vulnerability (how can a remote user exploit asterisk using a dialplan application?). If there are other vulnerabilities, or you feel that there is a valid reason to add those checks/changes in I'd suggest opening another issue so things can potentially move forward on them from there. Thanks! By: Kevin Harwell (kharwell) 2014-11-17 18:30:50.334-0600 Added patch that escalates the CONFBRIDGE function and makes the ConfbridgeStartRecord AMI action a system call. By: Gareth Palmer (gareth) 2014-11-17 20:23:57.663-0600 Hi Kevin, The manager 'Originate' action can be used to execute a dialplan application. The initial feedback I received was that registering CONFBRIDGE() as escalating was overly-broad and while investigating an alternative option, I came across additional issues. As adding "more junk to that string compare" was rejected (fair enough, both of the application and variable blacklists don't even work properly), so I removed them completely. I will create addtional issues for them as you suggest. The patch you uploaded for CONFBRIDGE() looks like it will resolve the record_file issue.