[Home]

Summary:ASTERISK-22905: Prevent Asterisk functions that are 'dangerous' from being executed from external interfaces
Reporter:Matt Jordan (mjordan)Labels:Security
Date Opened:2013-11-25 13:16:02.000-0600Date Closed:2013-12-16 13:54:04.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General Core/ManagerInterface Functions/General Resources/res_agi Resources/res_ari
Versions:12.0.0-beta1 Frequency of
Occurrence
Related
Issues:
causesASTERISK-23084 [patch]rasterisk needlessly prints the AST-2013-007 warning
Environment:Attachments:( 0) live-dangerously-1.8.diff
( 1) live-dangerously-12.diff
( 2) live-dangerously-12-2.diff
Description:The way Asterisk handles functions during variable substitution makes it inherently dangerous for certain functions to be loaded in Asterisk.

For example, having the SHELL function loaded means anyone using AMI/AGI/ARI can use the function as part of a variable set/get action. The fact that such execution can be embedded in fairly complex ways means that there's no concrete way of preventing said function execution unless the system fundamentally prevents it.

This issue proposes the following:
# Modify function registration such that a function can register itself as 'unsafe'. Unsafe functions can be executed from the dialplan (you're in Asterisk, after all), but not from external interfaces during variable substitution/evaluation.
# When AMI/ARI/AGI calls a {{pbx}} function to evaluate a variable, the call should not to the pbx core that the call is coming from an external system. Function evaluation should not occur if the function being evaluated was marked as unsafe.
# To override this behavior, a new {{asterisk.conf}} parameter should be added ({{live_dangerously}}?) that lets the evaluation take place regardless. This preserves the behavior in prior versions (in case people actually want to use SHELL from AMI).

This behavior should be introduced in Asterisk 12 - there's not much sense in introducing this midstream into Asterisk 1.8 or 11, as AMI already contains rudimentary protections for these functions.
Comments:By: David M. Lee (dlee) 2013-12-04 11:13:55.215-0600

Fix for Asterisk 12.
By: David M. Lee (dlee) 2013-12-04 11:29:21.335-0600

I've attached the fix for Asterisk 12. Full description of the patch below.

I do have an open question: should we backport this patch to Asterisk 1.8 and friends? I believe that the same sort of privilege escalation could apply to AMI, depending on how you interpret AMI's permissions.

For example, if an AMI user has {{write = reporting}}, they will be able to execute a {{Getvar}} on {{DB_DELETE()}}, or other side-effecting functions. The intention was to only give the AMI user the "Ability to get information about the system." Given that, this looks like a privilege escalation.

If we do backport it to older versions, I propose that the default value for {{live_dangerously}} be set to yes. This maintains the current behavior (not breaking current apps), but adds a configuration option for anyone on older versions that want the increased security.

Thoughts?

==
This patch allows individual dialplan functions to be marked as 'dangerous', to inhibit their execution from external sources (specifically ARI, but AMI would also be affected).

A 'dangerous' function is one which results in a privilege escalation. For example, if one were to read the channel variable SHELL(rm -rf /) Bad Things™ could happen.

There are functions which one might consider troubling, but might not necessarily be dangerous. Take the FILE() function. While it seems like a bad idea to allow the external system to read any file that Asterisk has read permission for, it is still a read operation and not necessarily a privilege escalation.

Execution from external sources may be enabled by setting 'live_dangerously' to 'yes' in the \[options\] section of asterisk.conf. Although doing so is not recommended.

SHELL(), LOCK(), TRYLOCK(), and UNLOCK() are marked as dangerous.

DB_DELETE() and REALTIME_DESTROY() are marked as dangerous for reads, but now have write functions which can safely be executed. In those cases, the provided value for the write is ignored, and what would have been the return value for the function is discarded.
By: Russell Bryant (russell) 2013-12-04 16:08:35.961-0600

Are there any known ways to get around what's in AMI right now?  I wouldn't bother backporting something unless there's a known way to exploit it.  I suppose if nothing was ever put into place into AGI, that may be a good enough reason to backport it.

Regarding the solution, I think it makes a lot of sense.  +1 to the approach.

Some patch comments ... no bugs, just some commentary and suggestions.

1) Making DB_DELETE() writable seems really awkward ... I don't really have a suggestion, though.

2)

{code}
@@ -140,6 +140,14 @@
/*! Write function, if write is supported */
ast_acf_write_fn_t write; /*!< Write function, if write is supported */
struct ast_module *mod;         /*!< Module this custom function belongs to */
+ int dangerous_read:1;           /*!< The read function is to be considered
+ * 'dangerous', and should not be run directly
+ * from external interfaces (AMI, ARI, etc.)
+ * \since 12 */
+ int dangerous_write:1;          /*!< The write function is to be considerd
+ * 'dangerous', and should not be run directly
+ * from external interfaces (AMI, ARI, etc.)
+ * \since 12 */
AST_RWLIST_ENTRY(ast_custom_function) acflist;
};
{code}

I would mark this 'unsigned int'.  A signed bit doesn't make a whole lot of sense.  :-)

This change also breaks ABI, which isn't great for a backport.  It would be nice to version this to allow for additions like this without breaking ABI, but that's another issue...

3)

{code}
/*!
+ * \brief Set to true (non-zero) to globally allow all dangerous dialplan
+ * functions to run.
+ */
+static int live_dangerously;
{code}

This could also go in ast_options (include/asterisk/options.h).  I don't feel strongly about it.

4)

{code}
+ ast_log(LOG_WARNING, "Enabling dangerous function execution from external sources!\n");
{code}

It might be a good idea to add a wiki link to this log message so people have a place to go look for more detail on what this is about.  It sounds scary and isn't immediately obvious what it's talking about, IMO.

5)

{code}
+static int dangerous_functions_allowed(struct ast_custom_function *acfptr)
+{
+ int *thread_disable_dangerous;
+
+ if (!acfptr) {
+ return 0;
+ }
+
+ thread_disable_dangerous = ast_threadstorage_get(
+ &thread_disable_dangerous_tl, sizeof(*thread_disable_dangerous));
+
+ if (thread_disable_dangerous == NULL) {
+ ast_log(LOG_ERROR, "Error checking thread's ability to run dangerous functions\n");
+ return 0;
+ }
+
+ if (*thread_disable_dangerous) {
+ if (live_dangerously) {
+ /* Global setting overrides the thread's preference */
+ ast_debug(2, "Executing %s from a dangerous context\n",
+ acfptr->name);
{code}

This appears to be the only place where acfptr is used in this function.  However, this function isn't the one doing the execution.  How about don't pass it in at all, and log this closer to the execution, where the log message makes more sense?

6)

{code}
+;live_dangerously = no ; Enable the execution of 'dangerous' dialplan
+ ; functions from external sources (ARI, AMI,
+ ; etc.) These functions (such as SHELL) are
+ ; considered dangerous because they can allow
+ ; privilege escalation.
+ ; Default no.
{code}

Something like 'allow_external_dangerous_function_usage' is more accurate, if you're not too offended by the length.  'live_dangerously' is funny, but not very descriptive as to what it's doing.

7) Finally, if it's decided not to backport this, I think you could make an argument for just committing this like usual and not doing an advisory, since then it would be an issue that was never in released code, right?
By: Michael L. Young (elguero) 2013-12-04 19:08:24.398-0600

I agree with Russell's comments above.

One patch observation:

{noformat}
Index: funcs/func_realtime.c
===================================================================
--- funcs/func_realtime.c (https://origsvn.digium.com/svn/asterisk/branches/12) (revision 403347)
+++ funcs/func_realtime.c (working copy)
@@ -439,28 +439,31 @@
return -1;
}

- resultslen = 0;
- n = 0;
- for (var = head; var; n++, var = var->next)
- resultslen += strlen(var->name) + strlen(var->value);
- /* add space for delimiters and final '\0' */
- resultslen += n * (strlen(args.delim1) + strlen(args.delim2)) + 1;
+ if (len > 0) {
+ resultslen = 0;
+ n = 0;
+ for (var = head; var; n++, var = var->next)
+ resultslen += strlen(var->name) + strlen(var->value);
+ /* add space for delimiters and final '\0' */
+ resultslen += n * (strlen(args.delim1) + strlen(args.delim2)) + 1;
{noformat}

That for should probably have brackets added to it.

In regards to point 6 in the prior comment, while I do like the humor, I agree about it being more descriptive. Throwing out some ideas to add to Russell's suggestion: 'external_run_all_functions', 'run_all_functions_from_external_src', 'externally_use_all_functions', 'use_all_functions_externally'... Not that I like anyone of them in particular.  I kind of prefer Russel's suggestion... just brainstorming to help add to the discussion.

I have the same concern as Russell in regards to AGI.  If AGI doesn't have any protection in place, then I would say it needs to be backported.

{quote}
If we do backport it to older versions, I propose that the default value for live_dangerously be set to yes. This maintains the current behavior (not breaking current apps), but adds a configuration option for anyone on older versions that want the increased security.
{quote}

I am on the fence about setting the default to "yes"... I know we do not want to break apps mid-stream.  There doesn't appear to be a clear win, win situation here.  We enable it and then we hear the outcry from those who do not pay attention to security advisories or CHANGES that we are allowing Asterisk to remain insecure by default.  If we default to "no", then we hear the outcry from those who blindly upgrade to the latest release because we broke their apps.

Personally, security trumps inconvenience.  It will force those who upgrade blindly to find out about the security risk and then to consciously put their system at risk if they choose to do so.  It shifts the responsibility to the end user and they have no one else to blame but themselves if they are ever exploited.
By: Tilghman Lesher (tilghman) 2013-12-04 21:02:02.497-0600

Even FILE() is inherently "dangerous", in that 1) you can write to files with it, and 2) you can read any file that the Asterisk server has access to.  For example, sip.conf is readable, along will all of the passwords within.  That could definitely lead to privilege escalation.

As to the option name, I'd suggest risk_model=low for enabling everything, and risk_model=high for disabling those functions from external sources.  In fact, we may find additional things to disable under the risk_model=high nomenclature, so not referring to specific things (like dialplan functions) with the option name would be preferential.  It's also short and directly identifies the purpose of the setting.  I'd even go one further and suggest to administrators that if their Asterisk server is running in a secured network environment, then setting risk_model=low is fine, but if their server is on an unsecured network, on the border of a network, or even sitting outside with a public IP, that administrators set their risk_model to high.  This also permits the possibility of future values (such as 'medium', 'medium-low', etc.) that may provide some mix of security settings.  I really dislike the addition of yes/no values, because they tend to get extended in weird ways (think joinempty=yes|no|strict|loose in queues.conf for a good example of why starting out with yes/no is a bad idea for settings that do multiple things).

In terms of backporting the patches, my inclination is yes, but we should keep the default risk model low, so we avoid breakage as much as possible.  Placing the information within README-SERIOUSLY.bestpractices.txt should be sufficient for making users aware of the potential for problems.  That file is, after all, where we place other notifications of possible security issues that can only be sufficiently addressed by the administrator of the system.
By: David M. Lee (dlee) 2013-12-05 13:21:48.747-0600

bq. Are there any known ways to get around what's in AMI right now? I wouldn't bother backporting something unless there's a known way to exploit it. I suppose if nothing was ever put into place into AGI, that may be a good enough reason to backport it.

AMI blacklists SHELL() and EVAL(), but not other privilege escalating functions.

I haven't looked into AGI yet. Especially things like SHELL() and FILE() would be problems there.

bq. 1) Making DB_DELETE() writable seems really awkward ... I don't really have a suggestion, though.

Agreed that it's awkward, and agreed that I can't think of a better way to do it.

bq. This change also breaks ABI, which isn't great for a backport. It would be nice to version this to allow for additions like this without breaking ABI, but that's another issue...

Any thoughts on the best approach for not breaking the ABI?

bq. This appears to be the only place where acfptr is used in this function. However, this function isn't the one doing the execution. How about don't pass it in at all, and log this closer to the execution, where the log message makes more sense?

I thought about that. It ended up complicating the caller logic too much getting it to log the message 1) on dangerous functions 2) on a thread that normally wouldn't execute them 3) but will execute because live_dangerously was set to yes.

bq. Something like 'allow_external_dangerous_function_usage' is more accurate, if you're not too offended by the length. 'live_dangerously' is funny, but not very descriptive as to what it's doing.

You people have no sense of humor.

bq. 7) Finally, if it's decided not to backport this, I think you could make an argument for just committing this like usual and not doing an advisory, since then it would be an issue that was never in released code, right?

Agreed.

bq. That for should probably have brackets added to it.

Done.

bq. Even FILE() is inherently "dangerous", in that 1) you can write to files with it, and 2) you can read any file that the Asterisk server has access to. For example, sip.conf is readable, along will all of the passwords within. That could definitely lead to privilege escalation.

Great point. Dangerousifying.
By: David M. Lee (dlee) 2013-12-05 13:38:19.658-0600

bq. I suppose if nothing was ever put into place into AGI, that may be a good enough reason to backport it.

Actually, AGI's are interesting because they actually execute on the PBX thread. The patch will need some work to avoid executing dangerous functions from AGI.
By: David M. Lee (dlee) 2013-12-05 13:40:42.693-0600

Updated patch for Asterisk 12.
By: David M. Lee (dlee) 2013-12-05 13:42:02.619-0600

Updated patch
By: Russell Bryant (russell) 2013-12-05 13:56:18.103-0600

Actually, is limiting AGI at all even a concern?  Isn't AGI typically viewed and used as a dialplan control alternative?  All of this seems fair game for AGI.

The lines are definitely blurred when talking about the FastAGI and AsyncAGI variants, since those are technically external.  I still think that they should be viewed as given full Asterisk privileges as if they were the dialplan.  There is nothing that even pretends to give you access controls to put on AGI clients like you do for normal AMI clients.

So, I think I would limit this to AMI (and the new stuff).
By: Russell Bryant (russell) 2013-12-05 14:00:04.198-0600

{quote}
Any thoughts on the best approach for not breaking the ABI?
{quote}

I think the only way to do it would be to not add those fields to the struct at all, and maintain the registry separately using a new API.

{code}
ast_this_is_dangerous(acfptr, AST_DANGEROUS_READ); /* or something like that ... */
{code}

and then some new internal registry of stuff that was marked as dangerous with separate data structures.

The impact is binary modules already built and expected to remain compatible with release branch updates.  Any that register functions would break after the current change.
By: Tilghman Lesher (tilghman) 2013-12-05 14:13:19.018-0600

BTW, why are LOCK/UNLOCK/TRYLOCK considered "dangerous" functions?  I suspect I know why, though the reasoning has a pitfall, but you may have thought of something different that I haven't considered.
By: David M. Lee (dlee) 2013-12-06 10:00:56.417-0600

bq. BTW, why are LOCK/UNLOCK/TRYLOCK considered "dangerous" functions? I suspect I know why, though the reasoning has a pitfall, but you may have thought of something different that I haven't considered.

'Dangerous' is a bit of a misnomer in that case. Those are functions that modify the system via a read, so they are a privilege escalation.
By: Tilghman Lesher (tilghman) 2013-12-06 10:25:49.420-0600

They do, but because they require a channel to run (and any locks obtained disappear when the channel is hung up), the effects are temporary.  If I'm reading the code correctly, anybody who used mutual exclusion in the dialplan with these functions (and SIP channel) would have to turn off this security mode for their dialplan to continue working.  In effect, we'd ensure that none of those people could _ever_ turn this security feature on, which I don't think is a good policy.
By: David M. Lee (dlee) 2013-12-06 10:37:49.694-0600

bq. Actually, is limiting AGI at all even a concern? Isn't AGI typically viewed and used as a dialplan control alternative? All of this seems fair game for AGI.

Agreed. AGI has no privileges associated with it, so there's no opportunity for a privilege escalation.
By: David M. Lee (dlee) 2013-12-06 10:39:11.794-0600

bq. [...] anybody who used mutual exclusion in the dialplan with these functions (and SIP channel) would have to turn off this security mode for their dialplan to continue working.

No. Dialplan threads are marked to allow dangerous functions to run, so the \{UN,TRY,}LOCK() functions should still work fine; just as they always have. So will SHELL(), FILE(), etc.
By: Tilghman Lesher (tilghman) 2013-12-06 11:07:07.736-0600

Okay, if that's what your intent is, that's fine.

+/*!
+ * \brief Enable the execution of dangerous functions for the current thread.
+ */
+void ast_thread_disable_dangerous_functions(void);
+

I think your nomenclature throughout this patch is entirely backwards.  I read "disable dangerous functions" and I think "Okay, this disables the use of those functions", not "this RE-ENABLES the use of these functions."  If you reverse the language on these variables and function names, I think the patch would be a lot more clear.
By: David M. Lee (dlee) 2013-12-10 08:58:35.963-0600

Backported patch for 1.8
By: David M. Lee (dlee) 2013-12-10 09:01:57.677-0600

I've backported the patch to 1.8. For 11 and earlier, {{live_dangerously}} will default to {{yes}}; in 12 we will default it to {{no}}.

* Added a comment in {{README-SERIOUSLY.bestpractices.txt}} about disabling the setting.
* Improved consistency of the verbage and docs in {{pbx.c}} and {{pbx.h}}. The code's more clear now.
* ABI is backward compatible.
* Updated the XML docs of all affected functions.
By: Tilghman Lesher (tilghman) 2013-12-10 09:14:28.582-0600

Ask yourself a question:  when would thread_inhibits_escalations() ever return false?
By: David M. Lee (dlee) 2013-12-10 09:45:47.899-0600

bq. Ask yourself a question: when would thread_inhibits_escalations() ever return false?

In the PBX threads...

Actually, any thread not started by an {{ast_tcptls_server}}.
By: Tzafrir Cohen (tzafrir) 2013-12-10 12:28:30.281-0600

If a manager user has 'command' write permissions, wouldn't it make sense to assume that the system administrator meant to give that user unlimited access to the PBX? Unlimited access to the PBX from AMI can also be a feature.
By: David M. Lee (dlee) 2013-12-10 12:50:42.191-0600

bq. If a manager user has 'command' write permissions, wouldn't it make sense to assume that the system administrator meant to give that user unlimited access to the PBX? Unlimited access to the PBX from AMI can also be a feature.

The manager 'command' write permission means "Permission to run CLI commands", as it's documented. It shouldn't give them more than that.

"Unlimited access" might be a feature, but we'd have to define what "unlimited" means, define permissions around it, implement it, etc. That's beyond the scope of this patch.
By: Tzafrir Cohen (tzafrir) 2013-12-10 13:01:41.328-0600

OK. I admit I made that up. In fact, I believe that 73.5% of the Asterisk sysadmins don't know what those permissions mean and just put in the permissions stanza:

read = system,call,log,verbose,agent,user,config,dtmf,reporting,cdr,dialplan
write = system,call,agent,user,config,command,reporting,originate,message

or something similar that they copied from somewhere on the internet and just want the user to get full access.
By: Tzafrir Cohen (tzafrir) 2013-12-10 13:11:06.646-0600

Also note that an AMI user with config write permission and originate write permission has practically dialplan execution permission: UpdateConfig + Originate. So I would say that if a user has config write permission, I see no point in trying to try to prevent that user from running dialplan commands directly.
By: Walter Doekes (wdoekes) 2013-12-13 08:56:12.387-0600

Slightly off topic, but related nevertheless.

In ASTERISK-22982 I ran into the issue that there are scenarios where dialplan functions are called with a dummy channel (the CEL cel_custom backend).

In that particular case, the {{pbx_substitute_variables_helper}} is called to perform certain text substitutions (CSV_QUOTE), but it *shouldn't* be allowed to call functions that operate on channel internals (which may very well not be initialized) other than variables.

I could picture a flag added to functions that say: this-function-reads-variables-only. If there is a function that does more, the {{pbx_substitute_variables_helper}} could refuse to do the substitution and avoid a crash.

Just throwing it out there..
By: David M. Lee (dlee) 2013-12-13 09:16:33.177-0600

IMO, functions should be written so that they're a lot less crash-y when invoked with a dummy channel.
By: abelbeck (abelbeck) 2014-01-06 13:53:31.576-0600

For the WiKi URL:  See https://wiki.asterisk.org/wiki/x/1gKfAQ for more details.

The last line, "For Asterisk 11 and earlier"... the logic is reversed.
By: Michael L. Young (elguero) 2014-01-06 15:34:29.226-0600

@abelbeck Thanks, it has been corrected.