SQL escaping

[Arran Cudbard-Bell]

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

> On 21/09/12 13:14, Arran Cudbard-Bell wrote:
> 
>> I'm presuming that properly escaped strings will be converted back to
>> there unescaped form when they get inserted into the database?
> 
> Yes. If the NAS sends:
> 
> User-Name = "foo'bar"
> Some-Attr = "some\ntext"

> 
> ...then then SQL escape function will generate a string
> (depending on what the DB driver does for escaping):
> 
> insert into x (username,msg) values (E'foo''bar', E'some<newline>
> text')
> 
> ...and the value in the SQL column will be:
> 
> foo'bar
> some<newline>text

This probably won't cause that many issues, but should probably be noted somewhere.

>> Applications that relied on FreeRADIUS to do the escaping of special
>> characters would then be vulnerable to various other kinds of
>> injection attacks.
> 
> The idea would be that by using the DB escape code, you eliminate *all* vulnerability to injection attacks. Xlat will always generate correctly quoted strings.
> 
> But I think you're talking about other apps that then *read* the data FR has inserted, in which case see below.

I was.

> 
>> 
>> 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.
> 
> My intention was to make the escaping mode a setting on the SQL instance that defaults to safe-characters, but can be set to "native".


I've not seen any other application that pre-escapes data that it inserts to the database beyond handling the specific requirements of the protocol/markup language. It's not generally expected behaviour, so I don't think it should be the default behaviour.

However, the SQL module has been around for nearly a decade now, so it'd be good to provide backwards compatibility with the default config, for attribute that commonly contain user input, hence suggesting the default config still escape User-Name/User-Password.

-Arran