[asterisk-dev] [Code Review]: Patch to make app_system check if a command failed to execute due to permission denied.

[Tilghman Lesher]

> On June 18, 2012, 11:03 a.m., jrose wrote:
> > Hey, I was talking about this issue with Matt Jordan a while ago and he came up with the following:
> > 
> > Since we can't really guarantee under any particular circumstance what a given return code will mean for res here, it'd be more appropriate to leave the code here as is, but set a channel variable based on the res value to allow the author of the dialplan to react to it since they should know what /bin/sh will be (and what the return codes would map to).
> 
> Tilghman Lesher wrote:
>     While that's a reasonable approach in terms of a method by which to get a result, our approach has generally been that for things which return a value, we use a dialplan function, instead.  So, perhaps add a SYSTEM() dialplan function, which returns the result code.
> 
> jrose wrote:
>     Well, that's true, but this is really more of a status thing than an actual return from the System command being ran... and doing it this way isn't unprecedented (app_queue for instance sets QUEUESTATUS which indicates the the status of the queue at exit).  This does remind me though that there is a SHELL dialplan function that works similarly and might also need to be looked at when making these changes.
> 
> Matt Jordan wrote:
>     Hey now, I'm pretty sure I didn't say "channel variable" :-)
>     
>     I think I said we should expose the return code somehow to the user, and that it could be in the dialplan.  Using a function that returns a result code is a perfectly fine implementation of that.
>     
>     From an implementation perspective, I'd lean away from a channel variable.  Many channels would not ever need or care about a generic 'system call return value' variable, and I would hate to have it always around with a channel.  Or worse, have it around when the system call returns, but not have it be present on a channel otherwise - those types of things are rarely documented well, and people would have to know when the variable exists and when it doesn't.
>     
>     What's more, the system call may not even be 'channel specific', that is, it may not change or affect the state of the channel.  The script could print out "Hello world!" - so associating its return value with a channel seems to imply a dependency that does not necessarily exist.
>     
>     Tilghman's suggestion of tying the return code of the script directly with the item that executed the script makes sense: we simply return the return code of the thing that was executed.
> 
> jrose wrote:
>     Ah, alright then.  Sorry for misinterpreting.
>     
>     I'm just going to go ahead and mention that if another function for invoking system commands is added (like func_system), manager will need to be updated to check manager users for SYSTEM write access where app_system and func_shell are currently checked. It'd be a fairly trivial change, but not doing it could lead to a security vulnerability.
> 
> Kevin Fleming wrote:
>     So what we have today is the SYSTEM() dialplan application that attempts to interpret the result code it receives from the executed command (into the SYSTEMSTATUS channel variable, which will either be SUCCESS or FAILURE), and the SHELL() dialplan function, which collects the output generated by the executed command and returns it (but has no ability to return any status if the command execution failed for any reason).
>     
>     Any suggestions on how to improve this situation? SHELL() uses popen() internally, and it can return a result code from the command execution, but we'd need to find a way to make that available to the dialplan.

I addressed a similar situation with func_odbc, as functions created via that mechanism regularly return multiple results, by allowing multiple values to be retrieved from a function by using the ARRAY() assignment function.  The HASH() dialplan function works in a similar way when assigning to it.  So it would be possible for the return value of some dialplan function, whether that is SHELL2(), SYSTEM(), or something else (as long as it is a new function, to ensure backwards compatibility with existing functions) to return multiple values in this form.  Is this a reasonable approach?


- Tilghman


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1956/#review6497
-----------------------------------------------------------


On June 4, 2012, 9:23 a.m., Denis Martinez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1956/
> -----------------------------------------------------------
> 
> (Updated June 4, 2012, 9:23 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> I created a patch that improves the app_system behavior.  The patch should make this application to check if a command failed to execute due to permission denied.
> 
> 
> This addresses bug ASTERISK-19935.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19935
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_system.c 368031 
> 
> Diff: https://reviewboard.asterisk.org/r/1956/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Denis
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120723/84d456f1/attachment-0001.htm>