[sr-dev] pseudo variable cache + app_lua = potential memory leak

[Daniel-Constantin Mierla]

Hello,

indeed, the pv names cache doesn't invalidate items at runtime, relying
on the old model of static name string that evaluates its variables to
get the dynamic name value.

I would be fine with solution to overcome this:

 1) have some threshold of items/memory after which to start removing
items. Ideally, this should be done only with the variables that have
dynamic names (e,g for $sht(...), but not for $ru) -- iirc, there are
some flags or type field in declaration of the pv that can be extended
to cover this case.

 2) have a core parameter (might be used only by the app_lua and the
other embedded languages modules) to control whether to cache or not pv
name specs for vars with dynamic name

One thig to double check is whether we need or not a free function for
pv name spec, might be that some particular vars allocate memory some
just point to other values...

Somehow it is pretty much obvious there would be security implications
when taking values from incoming traffic without sanity/safety checks.

Cheers,
Daniel


On 12/07/16 14:19, Timo Teras wrote:
> Hi,
>
> I was recently debugging a memory leak in Kamailio running lua code.
> And it turned out to be rather nasty interaction with the PV cache and
> the Lua code. Technically, this is bug in Kamailio. However, we ended
> up fixing the lua code because the same bug also introduces security
> issues.
>
> The problem is that PV cache never releases cached items. Normally this
> is not a problem since in .cfg files you can only access PVs with fixed
> name. In cases where dynamic part is needed, like $sht() access based
> on variable, the expansion is done in the PV expression evaluation and
> the actual PV name is still static.
>
> However, app_lua exposes sr.pv.* where the PV name is a string. The lua
> application is free to construct it as it wishes. The case I had was
> using lua code to expand a variable, and then do sr.pv.get/sets on the
> expanded "$sht(table=>"..key..")" type expression.
>
> This resulted PV core leaking cached entry for each unique key. So I
> wonder if should limit the PV cache size, and e.g. make the bucket
> chain size limited (start releasing oldest cache entries when the
> bucket becomes full)? Or if this is not possible, e.g. due to memory
> allocation semantics, at least put big fat warning to the app_lua's
> sr.pv.* documentation on this.
>
> I ended up just fixing the application to set it first to "$var(tmp)"
> and then use that in the PV query. The security implication is
> noticeable if 'key' comes from untrusted source (e.g. SIP message) as
> then an attacker can inject string for PV expansion...
>
> Thanks,
> Timo
>
> _______________________________________________
> sr-dev mailing list
> sr-dev at lists.sip-router.org
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

-- 
Daniel-Constantin Mierla
http://www.asipto.com - http://www.kamailio.org
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda