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@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@abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_: /" safe_characters = "XXX@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@inverse.ca :: +1.514.447.4918 (x135) :: www.inverse.ca Inverse inc. :: Leaders behind SOGo (http://www.sogo.nu) and PacketFence (http://packetfence.org)