safe_characters issue
Fabrice Durand
fdurand at inverse.ca
Wed Jun 17 21:27:26 CEST 2020
The attached file didn't appear. ( commit id :
c87363675831f74c53028f947521a55319160b4c
7cc5f6154daaca64948cd1053bffa2a11b35ba85
50f18e02597c1d1d5ef7975826159a09fd81f7bf
46d1879513303ea9d311ea0f06c10211c9e4e4a2)
```
diff -ruN
freeradius-server-3.0.21.orig/raddb/mods-config/sql/main/mysql/queries.conf
freeradius-server-3.0.21/raddb/mods-config/sql/main/mysql/queries.conf
---
freeradius-server-3.0.21.orig/raddb/mods-config/sql/main/mysql/queries.conf
2020-03-24 10:55:09.000000000 -0400
+++
freeradius-server-3.0.21/raddb/mods-config/sql/main/mysql/queries.conf
2020-06-17 14:47:54.547176147 -0400
@@ -4,22 +4,10 @@
#
# $Id: 51560a71ed819a95bc0f5ccc352efe69e374f7c5 $
-# Use the driver specific SQL escape method.
-#
-# If you enable this configuration item, the "safe_characters"
-# configuration is ignored. FreeRADIUS then uses the MySQL escape
-# functions to escape input strings. The only downside to making this
-# change is that the MySQL escaping method is not the same the one
-# used by FreeRADIUS. So characters which are NOT in the
-# "safe_characters" list will now be stored differently in the database.
-#
-#auto_escape = yes
-
# Safe characters list for sql queries. Everything else is replaced
# with their mime-encoded equivalents.
# The default list should be ok
-# Using 'auto_escape' is preferred
-safe_characters =
"@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_: /"
+#safe_characters =
"@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_: /"
#######################################################################
# Connection config
diff -ruN
freeradius-server-3.0.21.orig/raddb/mods-config/sql/main/postgresql/queries.conf
freeradius-server-3.0.21/raddb/mods-config/sql/main/postgresql/queries.conf
---
freeradius-server-3.0.21.orig/raddb/mods-config/sql/main/postgresql/queries.conf
2020-03-24 10:55:09.000000000 -0400
+++
freeradius-server-3.0.21/raddb/mods-config/sql/main/postgresql/queries.conf
2020-06-17 14:47:54.547176147 -0400
@@ -4,21 +4,9 @@
#
# $Id: da82467aea53046e2dd55e1a6986a73b7429b856 $
-# Use the driver specific SQL escape method.
-#
-# If you enable this configuration item, the "safe_characters"
-# configuration is ignored. FreeRADIUS then uses the PostgreSQL escape
-# functions to escape input strings. The only downside to making this
-# change is that the PostgreSQL escaping method is not the same the one
-# used by FreeRADIUS. So characters which are NOT in the
-# "safe_characters" list will now be stored differently in the database.
-#
-#auto_escape = yes
-
# Safe characters list for sql queries. Everything else is replaced
# with their mime-encoded equivalents.
# The default list should be ok
-# Using 'auto_escape' is preferred
# safe_characters =
"@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_: /"
#######################################################################
diff -ruN
freeradius-server-3.0.21.orig/src/modules/rlm_sql/drivers/rlm_sql_mysql/rlm_sql_mysql.c
freeradius-server-3.0.21/src/modules/rlm_sql/drivers/rlm_sql_mysql/rlm_sql_mysql.c
---
freeradius-server-3.0.21.orig/src/modules/rlm_sql/drivers/rlm_sql_mysql/rlm_sql_mysql.c
2020-03-24 10:55:09.000000000 -0400
+++
freeradius-server-3.0.21/src/modules/rlm_sql/drivers/rlm_sql_mysql/rlm_sql_mysql.c
2020-06-17 14:47:54.547176147 -0400
@@ -811,21 +811,6 @@
return mysql_affected_rows(conn->sock);
}
-static size_t sql_escape_func(UNUSED REQUEST *request, char *out,
size_t outlen, char const *in, void *arg)
-{
- size_t inlen;
- rlm_sql_handle_t *handle = talloc_get_type_abort(arg,
rlm_sql_handle_t);
- rlm_sql_mysql_conn_t *conn = handle->conn;
-
- /* Check for potential buffer overflow */
- inlen = strlen(in);
- if ((inlen * 2 + 1) > outlen) return 0;
- /* Prevent integer overflow */
- if ((inlen * 2 + 1) <= inlen) return 0;
-
- return mysql_real_escape_string(conn->sock, out, in, inlen);
-}
-
/* Exported to rlm_sql */
extern rlm_sql_module_t rlm_sql_mysql;
@@ -844,6 +829,5 @@
.sql_free_result = sql_free_result,
.sql_error = sql_error,
.sql_finish_query = sql_finish_query,
- .sql_finish_select_query = sql_finish_query,
- .sql_escape_func = sql_escape_func
+ .sql_finish_select_query = sql_finish_query
};
diff -ruN
freeradius-server-3.0.21.orig/src/modules/rlm_sql/drivers/rlm_sql_postgresql/rlm_sql_postgresql.c
freeradius-server-3.0.21/src/modules/rlm_sql/drivers/rlm_sql_postgresql/rlm_sql_postgresql.c
---
freeradius-server-3.0.21.orig/src/modules/rlm_sql/drivers/rlm_sql_postgresql/rlm_sql_postgresql.c
2020-03-24 10:55:09.000000000 -0400
+++
freeradius-server-3.0.21/src/modules/rlm_sql/drivers/rlm_sql_postgresql/rlm_sql_postgresql.c
2020-06-17 14:47:54.547176147 -0400
@@ -553,28 +553,6 @@
return conn->affected_rows;
}
-static size_t sql_escape_func(UNUSED REQUEST *request, char *out,
size_t outlen, char const *in, void *arg)
-{
- size_t inlen, ret;
- rlm_sql_handle_t *handle = talloc_get_type_abort(arg,
rlm_sql_handle_t);
- rlm_sql_postgres_conn_t *conn = handle->conn;
- int err;
-
- /* Check for potential buffer overflow */
- inlen = strlen(in);
- if ((inlen * 2 + 1) > outlen) return 0;
- /* Prevent integer overflow */
- if ((inlen * 2 + 1) <= inlen) return 0;
-
- ret = PQescapeStringConn(conn->db, out, in, inlen, &err);
- if (err) {
- REDEBUG("Error escaping string \"%s\": %s", in,
PQerrorMessage(conn->db));
- return 0;
- }
-
- return ret;
-}
-
/* Exported to rlm_sql */
extern rlm_sql_module_t rlm_sql_postgresql;
rlm_sql_module_t rlm_sql_postgresql = {
@@ -589,6 +567,5 @@
.sql_error = sql_error,
.sql_finish_query = sql_free_result,
.sql_finish_select_query = sql_free_result,
- .sql_affected_rows = sql_affected_rows,
- .sql_escape_func = sql_escape_func
+ .sql_affected_rows = sql_affected_rows
};
diff -ruN freeradius-server-3.0.21.orig/src/modules/rlm_sql/rlm_sql.c
freeradius-server-3.0.21/src/modules/rlm_sql/rlm_sql.c
--- freeradius-server-3.0.21.orig/src/modules/rlm_sql/rlm_sql.c
2020-03-24 10:55:09.000000000 -0400
+++ freeradius-server-3.0.21/src/modules/rlm_sql/rlm_sql.c 2020-06-17
14:47:54.547176147 -0400
@@ -110,7 +110,6 @@
#endif
{ "safe-characters", FR_CONF_OFFSET(PW_TYPE_STRING |
PW_TYPE_DEPRECATED, rlm_sql_config_t, allowed_chars), NULL },
{ "safe_characters", FR_CONF_OFFSET(PW_TYPE_STRING,
rlm_sql_config_t, allowed_chars),
"@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_: /" },
- { "auto_escape", FR_CONF_OFFSET(PW_TYPE_BOOLEAN, rlm_sql_config_t,
driver_specific_escape), "no" },
/*
* This only works for a few drivers.
@@ -123,8 +122,6 @@
CONF_PARSER_TERMINATOR
};
-static size_t sql_escape_for_xlat_func(REQUEST *request, char *out,
size_t outlen, char const *in, void *arg);
-
/*
* Fall-Through checking function from rlm_files.c
*/
@@ -378,8 +375,7 @@
static size_t sql_escape_func(UNUSED REQUEST *request, char *out,
size_t outlen,
char const *in, void *arg)
{
- rlm_sql_handle_t *handle = talloc_get_type_abort(arg,
rlm_sql_handle_t);
- rlm_sql_t *inst = handle->inst;
+ rlm_sql_t *inst = arg;
size_t len = 0;
while (in[0]) {
@@ -482,28 +478,6 @@
return len;
}
-/** Passed as the escape function to map_proc and sql xlat methods
- *
- * The variant reserves a connection for the escape functions to use,
and releases it after
- * escaping is complete.
- */
-static size_t sql_escape_for_xlat_func(REQUEST *request, char *out,
size_t outlen, char const *in, void *arg)
-{
- size_t ret;
- rlm_sql_t *inst = talloc_get_type_abort(arg, rlm_sql_t);
- rlm_sql_handle_t *handle;
-
- handle = fr_connection_get(inst->pool);
- if (!handle) {
- out[0] = '\0';
- return 0;
- }
- ret = inst->sql_escape_func(request, out, outlen, in, handle);
- fr_connection_release(inst->pool, handle);
-
- return ret;
-}
-
/*
* Set the SQL user name.
*
@@ -573,7 +547,7 @@
if (!inst->config->groupmemb_query) return 0;
- if (radius_axlat(&expanded, request, inst->config->groupmemb_query,
sql_escape_for_xlat_func, inst) < 0) return -1;
+ if (radius_axlat(&expanded, request, inst->config->groupmemb_query,
sql_escape_func, inst) < 0) return -1;
ret = rlm_sql_select_query(inst, request, handle, expanded);
talloc_free(expanded);
@@ -752,7 +726,7 @@
* Expand the group query
*/
if (radius_axlat(&expanded, request,
inst->config->authorize_group_check_query,
- inst->sql_escape_func, *handle) < 0) {
+ sql_escape_func, inst) < 0) {
REDEBUG("Error generating query");
rcode = RLM_MODULE_FAIL;
goto finish;
@@ -802,7 +776,7 @@
* Now get the reply pairs since the paircompare matched
*/
if (radius_axlat(&expanded, request,
inst->config->authorize_group_reply_query,
- inst->sql_escape_func, *handle) < 0) {
+ sql_escape_func, inst) < 0) {
REDEBUG("Error generating query");
rcode = RLM_MODULE_FAIL;
goto finish;
@@ -937,7 +911,7 @@
/*
* Register the SQL xlat function
*/
- xlat_register(inst->name, sql_xlat, sql_escape_for_xlat_func, inst);
+ xlat_register(inst->name, sql_xlat, sql_escape_func, inst);
return 0;
}
@@ -1071,14 +1045,6 @@
inst->sql_select_query = rlm_sql_select_query;
inst->sql_fetch_row = rlm_sql_fetch_row;
- /*
- * Either use the module specific escape function
- * or our default one.
- */
- inst->sql_escape_func = inst->module->sql_escape_func &&
inst->config->driver_specific_escape ?
- inst->module->sql_escape_func :
- sql_escape_func;
-
if (inst->module->mod_instantiate) {
CONF_SECTION *cs;
char const *name;
@@ -1187,7 +1153,7 @@
VALUE_PAIR *vp;
if (radius_axlat(&expanded, request,
inst->config->authorize_check_query,
- inst->sql_escape_func, handle) < 0) {
+ sql_escape_func, inst) < 0) {
REDEBUG("Error generating query");
rcode = RLM_MODULE_FAIL;
goto error;
@@ -1239,7 +1205,7 @@
* Now get the reply pairs since the paircompare matched
*/
if (radius_axlat(&expanded, request,
inst->config->authorize_reply_query,
- inst->sql_escape_func, handle) < 0) {
+ sql_escape_func, inst) < 0) {
REDEBUG("Error generating query");
rcode = RLM_MODULE_FAIL;
goto error;
@@ -1459,7 +1425,7 @@
goto finish;
}
- if (radius_axlat(&expanded, request, value,
inst->sql_escape_func, handle) < 0) {
+ if (radius_axlat(&expanded, request, value, sql_escape_func,
inst) < 0) {
rcode = RLM_MODULE_FAIL;
goto finish;
@@ -1611,15 +1577,15 @@
return RLM_MODULE_FAIL;
}
- /* initialize the sql socket */
- handle = fr_connection_get(inst->pool);
- if (!handle) {
- talloc_free(expanded);
+ if (radius_axlat(&expanded, request,
inst->config->simul_count_query, sql_escape_func, inst) < 0) {
sql_unset_user(inst, request);
return RLM_MODULE_FAIL;
}
- if (radius_axlat(&expanded, request,
inst->config->simul_count_query, inst->sql_escape_func, handle) < 0) {
+ /* initialize the sql socket */
+ handle = fr_connection_get(inst->pool);
+ if (!handle) {
+ talloc_free(expanded);
sql_unset_user(inst, request);
return RLM_MODULE_FAIL;
}
@@ -1661,7 +1627,7 @@
goto finish;
}
- if (radius_axlat(&expanded, request,
inst->config->simul_verify_query, inst->sql_escape_func, handle) < 0) {
+ if (radius_axlat(&expanded, request,
inst->config->simul_verify_query, sql_escape_func, inst) < 0) {
rcode = RLM_MODULE_FAIL;
goto finish;
diff -ruN freeradius-server-3.0.21.orig/src/modules/rlm_sql/rlm_sql.h
freeradius-server-3.0.21/src/modules/rlm_sql/rlm_sql.h
--- freeradius-server-3.0.21.orig/src/modules/rlm_sql/rlm_sql.h
2020-03-24 10:55:09.000000000 -0400
+++ freeradius-server-3.0.21/src/modules/rlm_sql/rlm_sql.h 2020-06-17
14:47:54.547176147 -0400
@@ -126,7 +126,6 @@
//!< stale sessions.
char const *allowed_chars; //!< Chars which done
need escaping..
- bool driver_specific_escape; //!< Use the driver
specific SQL escape method
uint32_t query_timeout; //!< How long to allow
queries to run for.
char const *connect_query; //!< Query executed
after establishing
@@ -212,8 +211,6 @@
sql_rcode_t (*sql_finish_query)(rlm_sql_handle_t *handle,
rlm_sql_config_t *config);
sql_rcode_t (*sql_finish_select_query)(rlm_sql_handle_t *handle,
rlm_sql_config_t *config);
-
- xlat_escape_t sql_escape_func;
} rlm_sql_module_t;
struct sql_inst {
```
Le 20-06-17 à 15 h 23, Fabrice Durand a écrit :
> Hello Alan,
>
> sorry to bother you.
>
> I did one last test and built new binary and revert few commits in
> 3.0.21 (attached to this email) in the rlm_sql modules and now
> freeradius take the safe_characters values from sql_degraded{...}
> section and not the sql {...} section anymore. (like it worked in
> freeradius 3.0.13)
>
> My skills in C are not good enough to find the issue but it looks that
> the "sql_escape_func" use the main sql section.
>
> Regards
>
> Fabrice
>
>
> Le 20-06-17 à 12 h 10, Alan DeKok a écrit :
>> On Jun 17, 2020, at 11:11 AM, Fabrice Durand <fdurand at inverse.ca> wrote:
>>> I am not sure it's the case, the only place i defined
>>> safe_characters is in mods-enabled/sql
>> OK.
>>
>>> I attached 2 debug outputs and the sql files used , the one with
>>> safe_characters defined in the sql {...} section (who works) and the
>>> other one with safe_characters defined in sql sql_degraded{...}
>>> (that doesn't works)
>>>
>>> For me it looks that even if you define safe_characters in another
>>> section than the sql {...} one the code doesn't use it and use the
>>> one from the sql {...} section instead.
>> Except that the code *always* looks at definition of
>> safe_characters in the current configuration.
>>
>> There's nothing in the rlm_sql source which says "search for the
>> base SQL module and use that".
>>
>>> I did exactly the same tests on the FreeRADIUS version 3.0.13 (i am
>>> using another path for the configuration files than /etc/radiusd, so
>>> the files didn't changed) and it takes the safe_characters defined
>>> in the sql_degraded section.
>>>
>>> Btw setting the safe_characters in sql{...} fixed my issue, but it
>>> looks to be a regression.
>> I just took the current v3.0.x head, and created a "sql sql2"
>> module, which uses MySQL. The main "sql" module is using sqlite. I
>> edited the safe_characters definition in mods-config, and I see:
>>
>> $ radiusd -X | grep safe
>> safe_characters =
>> "@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_:
>> /äéöüàâæçèéêëîïôœùûüaÿÄÉÖÜßÀÂÆÇÈÉÊËÎÏÔŒÙÛÜŸ"
>> safe_characters =
>> "YYY at abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_:
>> /"
>> safe_characters =
>> "XXX at abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_:
>> /"
>>
>> The first one is from rlm_expr. The second is from the sqlite
>> definition that I edited. The third one is from the MySQL definition
>> that I edited.
>>
>> And from the debug output you posted:
>>
>> # Loading module "sql_degraded" from file
>> /usr/local/pf/raddb/mods-enabled/sql
>> sql sql_degraded {
>> driver = "rlm_sql_mysql"
>> server = "127.0.0.1"
>> port = 3306
>> login = "pf"
>> password = <<< secret >>>
>> radius_db = "pf"
>> read_groups = yes
>> read_profiles = yes
>> read_clients = no
>> delete_stale_sessions = yes
>> sql_user_name = "%{User-Name}"
>> default_user_profile = ""
>> client_query = "SELECT id,nasname,shortname,type,secret FROM nas"
>> group_membership_query = ""
>> safe_characters =
>> "@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_:
>> /(),'"
>>
>>
>> So that seems to work.
>>
>> Alan DeKok.
>>
>>
>> -
>> List info/subscribe/unsubscribe? See
>> http://www.freeradius.org/list/users.html
>
--
Fabrice Durand
fdurand at inverse.ca :: +1.514.447.4918 (x135) :: www.inverse.ca
Inverse inc. :: Leaders behind SOGo (http://www.sogo.nu) and PacketFence (http://packetfence.org)
More information about the Freeradius-Users
mailing list