All, I'd like to add a patch that uses the correct escaping function - for example, PQescapeStringConn in libpq - when expanding SQL strings. For various reason we find the "safe-characters" encoding a bit limiting for us. I did look at adding a per-module selectable escaping function with the current "safe" option as the default and a "\xNN" for non-ascii characters, but this again has some issues. It seems like it might be a good idea to use the underlying SQL driver escape function if one is present. Unfortunately this is a bit more complex than it first looks. In particular, the RADIUS_ESCAPE_FUNC prototype only receives the in, out and length params - no reference to the request or any other context it is called from. Also, it's not beyond the realm of possibility that someone might want to interpolate strings into a query other than arguments e.g. update control { Tmp-String-0 := "table1" } update control { Tmp-Integer-0 := "%{sql:select * from %{control:Tmp-String-0} ...}" } Does anyone have any insight into how to go about this? In particular, I note that the libpq API requires a reference to the connection object you're about to send the query down, because per-connection attributes (like client encoding) might affect the escaping. This could be doubly troublesome if you are talking to >1 backend with distinct SQL settings (a bad idea I know). Suggestions welcome - is this more trouble than it's worth? Cheers, Phil
update control { Tmp-String-0 := "table1" } update control { Tmp-Integer-0 := "%{sql:select * from %{control:Tmp-String-0} ...}" }
Does anyone have any insight into how to go about this?
I guess you could have a special xlat function that does double expansion... That's what you really want here, it's not something specific to the sql module. update control { Tmp-String-2 := "%{eval:%%{sql:SELECT * FROM %{control:Tmp-String-0} WHERE username='%%{User-Name}'}}" } That'd let you do what you wanted right? There might already be one buried in the depths of the server somewhere.
In particular, I note that the libpq API requires a reference to the connection object you're about to send the query down, because per-connection attributes (like client encoding) might affect the escaping. This could be doubly troublesome if you are talking to >1 backend with distinct SQL settings (a bad idea I know).
Yeah +1 for the escape function accepting a context pointer. -Arran
Phil Mayers wrote:
I'd like to add a patch that uses the correct escaping function - for example, PQescapeStringConn in libpq - when expanding SQL strings. For various reason we find the "safe-characters" encoding a bit limiting for us.
Yup.
It seems like it might be a good idea to use the underlying SQL driver escape function if one is present.
That would be preferable.
Unfortunately this is a bit more complex than it first looks. In particular, the RADIUS_ESCAPE_FUNC prototype only receives the in, out and length params - no reference to the request or any other context it is called from.
That should be fixed.
Does anyone have any insight into how to go about this? In particular, I note that the libpq API requires a reference to the connection object you're about to send the query down, because per-connection attributes (like client encoding) might affect the escaping. This could be doubly troublesome if you are talking to >1 backend with distinct SQL settings (a bad idea I know).
Suggestions welcome - is this more trouble than it's worth?
It's worth it. My $0.02 is to do a series of patches. add REQUEST and context (void*) to the RADIUS_ESCAPE_FUNC. Add it to the prototypes, to all modules (as UNUSED), and have xlat.c store the context, and pass it and REQUEST to the calling function then, add the proper pass of the context in LDAP, SQL, etc. individually. Have it pass the right context, and then use it in the escaping function. Of course, this should all be in the 3.0 branch.
On 20/09/12 07:21, Alan DeKok wrote:
add REQUEST and context (void*) to the RADIUS_ESCAPE_FUNC.
Add it to the prototypes, to all modules (as UNUSED), and have xlat.c store the context, and pass it and REQUEST to the calling function
then, add the proper pass of the context in LDAP, SQL, etc. individually. Have it pass the right context, and then use it in the escaping function.
Actually I've started to have a doubt about this having spent some time looking at it. The xlat stuff is a bit more complex than I first appreciated. There are quite a few places where the escape func is just ignored when passed into *_xlat handlers, and even in quite a few places in radius_xlat itself (most of the single-string expansions that aren't passed off to valuepair2str). In addition, literally the only places the escape function are used in the source are rlm_ldap, rlm_sql and rlm_rest, so it's a both a limited-use code path, but very important. I don't know that I really want to touch it now! [I did spot a bug in rlm_ldap though - one-liner pull request submitted] I'll give it some more thought in a week or so, when I've finished my current assignment.
On 20 Sep 2012, at 17:30, Phil Mayers <p.mayers@imperial.ac.uk> wrote:
On 20/09/12 07:21, Alan DeKok wrote:
add REQUEST and context (void*) to the RADIUS_ESCAPE_FUNC.
Add it to the prototypes, to all modules (as UNUSED), and have xlat.c store the context, and pass it and REQUEST to the calling function
then, add the proper pass of the context in LDAP, SQL, etc. individually. Have it pass the right context, and then use it in the escaping function.
Actually I've started to have a doubt about this having spent some time looking at it.
The xlat stuff is a bit more complex than I first appreciated. There are quite a few places where the escape func is just ignored when passed into *_xlat handlers
Hmm ok, yes we should probably pass it into the radius_xlat call in those functions. I'm not sure why it's not currently... I was just following what was there already.
, and even in quite a few places in radius_xlat itself (most of the single-string expansions that aren't passed off to valuepair2str).
Oh i've started to kill those off. I've removed all the duplicate ones and i've found a GPLd version of strftime, so i'm just going to add a %{time} expansion. Then there's Request ID %I, which we can just declare a callback for, and %Z, which i'm not convinced there's any real usecase for now we have the detail module. So you can ignore those safe in the knowledge they'll be gone by the time we release 3.0.
In addition, literally the only places the escape function are used in the source are rlm_ldap, rlm_sql and rlm_rest, so it's a both a limited-use code path, but very important.
Fewer places to fix it ;)!
I don't know that I really want to touch it now!
[I did spot a bug in rlm_ldap though - one-liner pull request submitted]
Yeah i'll pull it as soon as GitHub stops being sucky. -Arran
On 09/20/2012 05:45 PM, Arran Cudbard-Bell wrote:
On 20 Sep 2012, at 17:30, Phil Mayers <p.mayers@imperial.ac.uk> wrote:
On 20/09/12 07:21, Alan DeKok wrote:
add REQUEST and context (void*) to the RADIUS_ESCAPE_FUNC.
Add it to the prototypes, to all modules (as UNUSED), and have xlat.c store the context, and pass it and REQUEST to the calling function
then, add the proper pass of the context in LDAP, SQL, etc. individually. Have it pass the right context, and then use it in the escaping function.
Actually I've started to have a doubt about this having spent some time looking at it.
The xlat stuff is a bit more complex than I first appreciated. There are quite a few places where the escape func is just ignored when passed into *_xlat handlers
Hmm ok, yes we should probably pass it into the radius_xlat call in those functions. I'm not sure why it's not currently... I was just following what was there already.
Actually, further thought suggests passing the escape func around is probably not necessary *at all*. It could be removed from the signature of valuepair2str and decode_attribute along with regsitered *_xlat funcs. The only place it needs to be is in radius_xlat, right after decode_attribute: decode_attrbute(tmp_buf, ...) func(tmp_buf, ..., ctx); i.e. do the escaping after all the xlat/expansion has been done. All calls to radius_xlat from *within* that code path can pass NULL; the final result is all that needs to be escaped. Unless I'm missing something, this should be the "right" thing to do. It has the virtue of avoiding passing func and ctx around when it's only used in a few places. It also eliminates the need for the dummy xlat_copy. Thoughts?
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/5d979e9f81fb493464aed... 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. It compiles and fires up and does basic xlat here for me. If we want to retain the single-char xlat I can add these in on top. 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}'} ...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.
On 09/21/2012 01:33 AM, Phil Mayers 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:
On minor note, some modules (e.g. rlm_ldap) aren't converted since I wasn't compiling them; I'll do the rest and batch is as appropriate changesets for pulling if people want to move forward.
On 21 Sep 2012, at 01:33, Phil Mayers <p.mayers@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/5d979e9f81fb493464aed...
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
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
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'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".
On 21 Sep 2012, at 15:05, Phil Mayers <p.mayers@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
On 21/09/12 15:52, Arran Cudbard-Bell wrote:
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.
Ok, have a look at: https://github.com/philmayers/freeradius-server/tree/escape-context I've worked hard to break the changes up into a series of small commits that should be easy to review. It doesn't touch the 1-char xlats, and tries to be as minimal as possible. The last few commits actually make use of the new argument to radius_xlat, specifically the SQL modules "safe-characters" is now per-instance, and not a static global variable. Which is good. So, there are no changes to SQL escaping method - just the addition and basic use of escape function context/request arguments. If you think this is ok and "pull" it, I'll work up patches next week to actually add driver-based SQL escaping as an option. I agree we should leave the default as-is.
Phil Mayers wrote:
The last few commits actually make use of the new argument to radius_xlat, specifically the SQL modules "safe-characters" is now per-instance, and not a static global variable. Which is good.
That's good.
So, there are no changes to SQL escaping method - just the addition and basic use of escape function context/request arguments.
If you think this is ok and "pull" it, I'll work up patches next week to actually add driver-based SQL escaping as an option. I agree we should leave the default as-is.
I'll try to review the code. The only thing for me is that the escaping has to be done recursively. e.g. SQL asking for an expansion of "foo %{Bar}" means that the "foo" portion should be copied as-is. But the %{Bar} portion is untrusted, and MUST be escaped. From looking at the code, it looks pretty good. My only $0.02 is: https://github.com/philmayers/freeradius-server/commit/08de5a57f202aebabdbd0... It does a malloc. I'd just declare (or re-use) a buffer on the stack. malloc() is slow and painful for temporary storage. Alan DeKok.
On 25/09/12 20:16, Alan DeKok wrote:
The only thing for me is that the escaping has to be done recursively. e.g. SQL asking for an expansion of "foo %{Bar}" means that the "foo" portion should be copied as-is. But the %{Bar} portion is untrusted, and MUST be escaped.
It should do that - I tested it locally with a pretty big variety of xlats on the SQL and LDAP modules, including nested xlats. However - TBH I find the code in decode_attribute() and rad_copy_variable() a bit hairy. It wasn't obvious to me for example that this: %{some %{var}:-other %{text}} ...isn't legal syntax. I can think of circumstances that might be useful. So, that being the case, if someone could compile and test the xlat stuff to destruction to check I haven't missed anything, that would be a good thing (tm). If the general approach is ok, I'll apply this patch to our testing radius server, and might work up a branch with SQL-native escaping to really give it a torture test.
From looking at the code, it looks pretty good. My only $0.02 is:
https://github.com/philmayers/freeradius-server/commit/08de5a57f202aebabdbd0...
It does a malloc. I'd just declare (or re-use) a buffer on the stack. malloc() is slow and painful for temporary storage.
The main reason I used a malloc there was that I didn't want to truncate any output, since all the code upward of the stack uses pointer/length arguments. And variable-length arrays: char buf[freespace]; ...are a GCC-ism. But I take your point, and I guess a static buffer is OK - we're reading from a "char buf[8192]" at that point anyway. Do you want me to re-do the patch stream or just modify the commit if/when it gets merged?
Phil Mayers wrote:
It should do that - I tested it locally with a pretty big variety of xlats on the SQL and LDAP modules, including nested xlats.
Good, thanks.
However - TBH I find the code in decode_attribute() and rad_copy_variable() a bit hairy.
The code in decode_attribute() is revolting. The rad_copy_variable() should be OK, as it's role is much simpler.
It wasn't obvious to me for example that this:
%{some %{var}:-other %{text}}
...isn't legal syntax. I can think of circumstances that might be useful.
Yeah.
If the general approach is ok, I'll apply this patch to our testing radius server, and might work up a branch with SQL-native escaping to really give it a torture test.
OK, thanks.
The main reason I used a malloc there was that I didn't want to truncate any output, since all the code upward of the stack uses pointer/length arguments. And variable-length arrays:
char buf[freespace];
...are a GCC-ism. But I take your point, and I guess a static buffer is OK - we're reading from a "char buf[8192]" at that point anyway.
Yeah. If there's an unused buffer at that point, just re-use it...
Do you want me to re-do the patch stream or just modify the commit if/when it gets merged?
I don't like pain. Send a pull request for this work, and another one for the malloc fix. Alan DeKok.
On 26/09/12 11:50, Alan DeKok wrote:
Do you want me to re-do the patch stream or just modify the commit if/when it gets merged?
I don't like pain. Send a pull request for this work, and another one for the malloc fix.
Ok, I've submitted pull request 101; I rebased the original branch to master then cherry-picked the commits across *except* the one with the malloc, which changed to use a static buffer. So it should all be good. Once this is in, I'll add the SQL escaping stuff.
Phil Mayers wrote:
On 26/09/12 11:50, Alan DeKok wrote: Ok, I've submitted pull request 101; I rebased the original branch to master then cherry-picked the commits across *except* the one with the malloc, which changed to use a static buffer. So it should all be good.
Once this is in, I'll add the SQL escaping stuff.
Done, thanks. Alan DeKok.
participants (3)
-
Alan DeKok -
Arran Cudbard-Bell -
Phil Mayers