[asterisk-dev] Final call for changes to Asterisk 1.4 and 1.6.2

[Tim Ringenbach]

On Fri, Apr 22, 2011 at 2:33 PM, Russell Bryant <russell at digium.com> wrote:

>
> ----- Original Message -----
> > Thank you Russell.
> >
> >
> > Your fix saves other programmer's life, that is fine.
>
> Well it's just that the bug was deeper than func_shell.
>
> > But my patch beyonds that, it is efficient ( only call strlen once, no
> > strcat ) then the original code, and it also fixs SHELL multiple line
> > result breaks AGI parse issue, please check out my diff.
>
> I do agree that some of those changes are good for performance.  However, I
> don't want to make the change that is replacing \n with \r in the output.  I
> don't think SHELL() should be touching the output at all.  If the command
> you are running is causing you some trouble with AGI parsing, then wrap it
> in something like BASE64_ENCODE(SHELL(...)).
>
>
While I agree that a fix like that doesn't belong in SHELL(), shouldn't
res_agi do something to avoid outputting things that break it's protocol?
The current code just does:
 ast_agi_send(agi->fd, chan, "200 result=1 (%s)\n", ret);
If this was say, some web scripting language outputting HTML, this could be
a XSS attack.
This would be a bit harder to exploit, but lets say the variable contains
user controlled data that happens to be malicious, and then uses a new line
and makes the next line look like the result of the next AGI command that
the AGI issues, that maybe is supposed to return trusted data.

If nothing else, if new lines are explicitly not supported, res_agi should
truncate at the first new line.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110422/a5e3fa24/attachment.htm>