<div class="gmail_quote">On Fri, Apr 22, 2011 at 2:33 PM, Russell Bryant <span dir="ltr"><<a href="mailto:russell@digium.com">russell@digium.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im"><br>
----- Original Message -----<br>
> Thank you Russell.<br>
><br>
><br>
> Your fix saves other programmer's life, that is fine.<br>
<br>
</div>Well it's just that the bug was deeper than func_shell.<br>
<div class="im"><br>
> But my patch beyonds that, it is efficient ( only call strlen once, no<br>
> strcat ) then the original code, and it also fixs SHELL multiple line<br>
> result breaks AGI parse issue, please check out my diff.<br>
<br>
</div>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(...)).<br>
<br></blockquote><div><br></div><div>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?</div><div>The current code just does:</div>
<div> ast_agi_send(agi->fd, chan, "200 result=1 (%s)\n", ret);</div><div>If this was say, some web scripting language outputting HTML, this could be a XSS attack.</div><div>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.</div>
<div><br></div><div>If nothing else, if new lines are explicitly not supported, res_agi should truncate at the first new line.</div></div>