SQL escaping

[Arran Cudbard-Bell]

On 21 Sep 2012, at 01:33, Phil Mayers <p.mayers at IMPERIAL.AC.UK> wrote:

> On 09/20/2012 05:45 PM, Arran Cudbard-Bell wrote:
> 
>> So you can ignore those safe in the knowledge they'll be gone by the time we release 3.0.
> 
> If we're happy to throw away the single-char xlat, then this should be a workng patch:
> 
> https://github.com/philmayers/freeradius-server/commit/5d979e9f81fb493464aed50d155c752108fc2815
> 
> It does the escaping once, in radius_xlat, and doesn't pass the escape func down the stack. It fixes up all uses of the various changed prototypes, and makes use of the new context argument to make safe-characters a per-instance rather than global in the various SQL modules.

Ok.

> It compiles and fires up and does basic xlat here for me.

Cool

> If we want to retain the single-char xlat I can add these in on top.

Retain the ones that haven't been removed yet. They should be removed in commits adding the equivalent functionality.

> The rlm_sqlcounter code needs extensive testing and review; I made some rather more extensive changes there for what seemed like simplicity, specifically I removed it's SQL escaping entirely, and rely on:
> 
> %{sql:select '%{var}'}

That makes sense.

> 
> ...the sql_xlat function in the parent SQL module correctly escaping eveything to the right of the ":".
> 
> If the basic approach looks right, I can rework cosmetic or naming as needed and once that is in "master", do a 2nd patchset which adds an "escape" function to the SQL driver layer, and gives the option of per-connection escaping for postgresl/mysql.

I'm presuming that properly escaped strings will be converted back to there unescaped form when they get inserted into the database? Applications that relied on FreeRADIUS to do the escaping of special characters would then be vulnerable to various other kinds of injection attacks.

I've copied the safe character code into another xlat function (could you pull and update that as part of your patch set too). In your second patch set please wrap user modifiable attributes e.g. User-Name/User-Password where they're used in INSERT/UPDATE queries with %{encode:}, this should minimise the risk of administrators inadvertently exposing other applications to raw user provided data.

-Arran