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

[jrose]

> 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.

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.


- jrose


-----------------------------------------------------------
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/20120618/ff305656/attachment-0001.htm>