From 8a064ee545dd3cc2cfaf23590189b18c805f686a Mon Sep 17 00:00:00 2001 From: MrStonedOne Date: Wed, 29 Oct 2014 04:12:07 -0700 Subject: [PATCH 1/5] Removes extra sql sanitize procs in favor of sanitizeSQL() --- code/modules/admin/DB ban/functions.dm | 6 +++--- code/modules/admin/banappearance.dm | 2 +- code/modules/client/client procs.dm | 8 ++++---- code/modules/research/message_server.dm | 20 +++++++------------- 4 files changed, 15 insertions(+), 21 deletions(-) diff --git a/code/modules/admin/DB ban/functions.dm b/code/modules/admin/DB ban/functions.dm index c7d9b68037d..329fac62e98 100644 --- a/code/modules/admin/DB ban/functions.dm +++ b/code/modules/admin/DB ban/functions.dm @@ -113,7 +113,7 @@ datum/admins/proc/DB_ban_record(var/bantype, var/mob/banned_mob, var/duration = else adminwho += ", [C]" - reason = sql_sanitize_text(reason) + reason = sanitizeSQL(reason) if(maxadminbancheck) var/DBQuery/adm_query = dbcon.NewQuery("SELECT count(id) AS num FROM [format_table_name("ban")] WHERE (a_ckey = '[a_ckey]') AND (bantype = 'ADMIN_PERMABAN' OR (bantype = 'ADMIN_TEMPBAN' AND expiration_time > Now())) AND isnull(unbanned)") @@ -235,14 +235,14 @@ datum/admins/proc/DB_ban_edit(var/banid = null, var/param = null) usr << "Invalid ban id. Contact the database admin" return - reason = sql_sanitize_text(reason) + reason = sanitizeSQL(reason) var/value switch(param) if("reason") if(!value) value = input("Insert the new reason for [pckey]'s ban", "New Reason", "[reason]", null) as null|text - value = sql_sanitize_text(value) + value = sanitizeSQL(value) if(!value) usr << "Cancelled" return diff --git a/code/modules/admin/banappearance.dm b/code/modules/admin/banappearance.dm index 991fa9ad362..be59efff4f7 100644 --- a/code/modules/admin/banappearance.dm +++ b/code/modules/admin/banappearance.dm @@ -99,7 +99,7 @@ proc/DB_ban_isappearancebanned(var/playerckey) if(!dbcon.IsConnected()) return - var/sqlplayerckey = sql_sanitize_text(ckey(playerckey)) + var/sqlplayerckey = sanitizeSQL(ckey(playerckey)) var/DBQuery/query = dbcon.NewQuery("SELECT id FROM [format_table_name("ban")] WHERE CKEY = '[sqlplayerckey]' AND ((bantype = 'APPEARANCE_PERMABAN') OR (bantype = 'APPEARANCE_TEMPBAN' AND expiration_time > Now())) AND unbanned != 1") query.Execute() diff --git a/code/modules/client/client procs.dm b/code/modules/client/client procs.dm index 9cec6893165..e9876b2d1df 100644 --- a/code/modules/client/client procs.dm +++ b/code/modules/client/client procs.dm @@ -163,7 +163,7 @@ var/next_external_rsc = 0 if(!dbcon.IsConnected()) return - var/sql_ckey = sql_sanitize_text(src.ckey) + var/sql_ckey = sanitizeSQL(src.ckey) var/DBQuery/query = dbcon.NewQuery("SELECT id, datediff(Now(),firstseen) as age FROM [format_table_name("player")] WHERE ckey = '[sql_ckey]'") query.Execute() @@ -198,9 +198,9 @@ var/next_external_rsc = 0 if(src.holder && src.holder.rank) admin_rank = src.holder.rank.name - var/sql_ip = sql_sanitize_text(src.address) - var/sql_computerid = sql_sanitize_text(src.computer_id) - var/sql_admin_rank = sql_sanitize_text(admin_rank) + var/sql_ip = sanitizeSQL(src.address) + var/sql_computerid = sanitizeSQL(src.computer_id) + var/sql_admin_rank = sanitizeSQL(admin_rank) if(sql_id) diff --git a/code/modules/research/message_server.dm b/code/modules/research/message_server.dm index c5dd5d2ae50..be9331a9c7f 100644 --- a/code/modules/research/message_server.dm +++ b/code/modules/research/message_server.dm @@ -290,17 +290,11 @@ var/obj/machinery/blackbox_recorder/blackbox var/DBQuery/query_insert = dbcon.NewQuery(sql) query_insert.Execute() -// Sanitize inputs to avoid SQL injection attacks -proc/sql_sanitize_text(var/text) - text = replacetext(text, "'", "''") - text = replacetext(text, ";", "") - text = replacetext(text, "&", "") - return text proc/feedback_set(var/variable,var/value) if(!blackbox) return - variable = sql_sanitize_text(variable) + variable = sanitizeSQL(variable) var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) @@ -311,7 +305,7 @@ proc/feedback_set(var/variable,var/value) proc/feedback_inc(var/variable,var/value) if(!blackbox) return - variable = sql_sanitize_text(variable) + variable = sanitizeSQL(variable) var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) @@ -322,7 +316,7 @@ proc/feedback_inc(var/variable,var/value) proc/feedback_dec(var/variable,var/value) if(!blackbox) return - variable = sql_sanitize_text(variable) + variable = sanitizeSQL(variable) var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) @@ -333,8 +327,8 @@ proc/feedback_dec(var/variable,var/value) proc/feedback_set_details(var/variable,var/details) if(!blackbox) return - variable = sql_sanitize_text(variable) - details = sql_sanitize_text(details) + variable = sanitizeSQL(variable) + details = sanitizeSQL(details) var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) @@ -345,8 +339,8 @@ proc/feedback_set_details(var/variable,var/details) proc/feedback_add_details(var/variable,var/details) if(!blackbox) return - variable = sql_sanitize_text(variable) - details = sql_sanitize_text(details) + variable = sanitizeSQL(variable) + details = sanitizeSQL(details) var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) From 138567b7cfad8b88b56f3ae550185ec75879664a Mon Sep 17 00:00:00 2001 From: MrStonedOne Date: Wed, 29 Oct 2014 04:15:46 -0700 Subject: [PATCH 2/5] Removes some sql related lag caused by too many clients connecting at once after a restart (and other minor fixes to client sql logic) Clients will now spread out their sql database connection logging and related ckey by ip/cid searching at world start so it doesn't all happen at once. Fixes a minor bug with related ckey searching where only one would be returned. Fixes a minor bug where related ckey searching would return the client's own ckey. --- code/modules/client/client procs.dm | 56 ++++++++++++++--------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/code/modules/client/client procs.dm b/code/modules/client/client procs.dm index e9876b2d1df..d1cd74964fd 100644 --- a/code/modules/client/client procs.dm +++ b/code/modules/client/client procs.dm @@ -133,7 +133,13 @@ var/next_external_rsc = 0 if((global.comms_key == "default_pwd" || length(global.comms_key) <= 6) && global.comms_allowed) //It's the default value or less than 6 characters long, but it somehow didn't disable comms. src << "The server's API key is either too short or is the default value! Consider changing it immediately!" - log_client_to_db() + set_client_age_from_db() + + if (!ticker || ticker.current_state == GAME_STATE_PREGAME) + spawn (rand(10,150)) + sync_client_with_db() + else + sync_client_with_db() send_resources() @@ -153,10 +159,8 @@ var/next_external_rsc = 0 return ..() - -/client/proc/log_client_to_db() - - if ( IsGuestKey(src.key) ) +/client/proc/set_client_age_from_db() + if (IsGuestKey(src.key)) return establish_db_connection() @@ -167,35 +171,37 @@ var/next_external_rsc = 0 var/DBQuery/query = dbcon.NewQuery("SELECT id, datediff(Now(),firstseen) as age FROM [format_table_name("player")] WHERE ckey = '[sql_ckey]'") query.Execute() - var/sql_id = 0 - while(query.NextRow()) - sql_id = query.item[1] + + while (query.NextRow()) player_age = text2num(query.item[2]) break - var/DBQuery/query_ip = dbcon.NewQuery("SELECT ckey FROM [format_table_name("player")] WHERE ip = '[address]'") + +/client/proc/sync_client_with_db() + if (IsGuestKey(src.key)) + return + + establish_db_connection() + if (!dbcon.IsConnected()) + return + + var/sql_ckey = sanitizeSQL(src.ckey) + + var/DBQuery/query_ip = dbcon.NewQuery("SELECT ckey FROM [format_table_name("player")] WHERE ip = '[address]' AND ckey != '[sql_ckey]'") query_ip.Execute() related_accounts_ip = "" while(query_ip.NextRow()) related_accounts_ip += "[query_ip.item[1]], " - break - var/DBQuery/query_cid = dbcon.NewQuery("SELECT ckey FROM [format_table_name("player")] WHERE computerid = '[computer_id]'") + var/DBQuery/query_cid = dbcon.NewQuery("SELECT ckey FROM [format_table_name("player")] WHERE computerid = '[computer_id]' AND ckey != '[sql_ckey]'") query_cid.Execute() related_accounts_cid = "" - while(query_cid.NextRow()) + while (query_cid.NextRow()) related_accounts_cid += "[query_cid.item[1]], " - break - //Just the standard check to see if it's actually a number - if(sql_id) - if(istext(sql_id)) - sql_id = text2num(sql_id) - if(!isnum(sql_id)) - return var/admin_rank = "Player" - if(src.holder && src.holder.rank) + if (src.holder && src.holder.rank) admin_rank = src.holder.rank.name var/sql_ip = sanitizeSQL(src.address) @@ -203,14 +209,8 @@ var/next_external_rsc = 0 var/sql_admin_rank = sanitizeSQL(admin_rank) - if(sql_id) - //Player already identified previously, we need to just update the 'lastseen', 'ip' and 'computer_id' variables - var/DBQuery/query_update = dbcon.NewQuery("UPDATE [format_table_name("player")] SET lastseen = Now(), ip = '[sql_ip]', computerid = '[sql_computerid]', lastadminrank = '[sql_admin_rank]' WHERE id = [sql_id]") - query_update.Execute() - else - //New player!! Need to insert all the stuff - var/DBQuery/query_insert = dbcon.NewQuery("INSERT INTO [format_table_name("player")] (id, ckey, firstseen, lastseen, ip, computerid, lastadminrank) VALUES (null, '[sql_ckey]', Now(), Now(), '[sql_ip]', '[sql_computerid]', '[sql_admin_rank]')") - query_insert.Execute() + var/DBQuery/query_insert = dbcon.NewQuery("INSERT INTO [format_table_name("player")] (id, ckey, firstseen, lastseen, ip, computerid, lastadminrank) VALUES (null, '[sql_ckey]', Now(), Now(), '[sql_ip]', '[sql_computerid]', '[sql_admin_rank]') ON DUPLICATE KEY UPDATE lastseen = VALUES(lastseen), ip = VALUES(ip), computerid = VALUES(computerid)") + query_insert.Execute() //Logging player access var/serverip = "[world.internet_address]:[world.port]" From 40d884dbfadf33b060220b524925ed16403e47d1 Mon Sep 17 00:00:00 2001 From: MrStonedOne Date: Wed, 29 Oct 2014 04:22:35 -0700 Subject: [PATCH 3/5] Feedback reporting now uses 1 insert query with mutiple rows Rather then making a new query for each feedback item. Each query can take between 25ms and 100ms, and this is a static per-query overhead, so removing the 60 or so needless querys will speed things up. --- code/modules/research/message_server.dm | 76 ++++++++++++++----------- 1 file changed, 43 insertions(+), 33 deletions(-) diff --git a/code/modules/research/message_server.dm b/code/modules/research/message_server.dm index be9331a9c7f..240e1d144ce 100644 --- a/code/modules/research/message_server.dm +++ b/code/modules/research/message_server.dm @@ -123,27 +123,27 @@ var/global/list/obj/machinery/message_server/message_servers = list() value = param_value /datum/feedback_variable/proc/inc(var/num = 1) - if(isnum(value)) + if (isnum(value)) value += num else value = text2num(value) - if(isnum(value)) + if (isnum(value)) value += num else value = num /datum/feedback_variable/proc/dec(var/num = 1) - if(isnum(value)) + if (isnum(value)) value -= num else value = text2num(value) - if(isnum(value)) + if (isnum(value)) value -= num else value = -num /datum/feedback_variable/proc/set_value(var/num) - if(isnum(num)) + if (isnum(num)) value = num /datum/feedback_variable/proc/get_value() @@ -153,12 +153,12 @@ var/global/list/obj/machinery/message_server/message_servers = list() return variable /datum/feedback_variable/proc/set_details(var/text) - if(istext(text)) + if (istext(text)) details = text /datum/feedback_variable/proc/add_details(var/text) - if(istext(text)) - if(!details) + if (istext(text)) + if (!details) details = text else details += " [text]" @@ -198,14 +198,14 @@ var/obj/machinery/blackbox_recorder/blackbox //Only one can exsist in the world! /obj/machinery/blackbox_recorder/New() - if(blackbox) - if(istype(blackbox,/obj/machinery/blackbox_recorder)) + if (blackbox) + if (istype(blackbox,/obj/machinery/blackbox_recorder)) qdel(src) blackbox = src /obj/machinery/blackbox_recorder/Destroy() var/turf/T = locate(1,1,2) - if(T) + if (T) blackbox = null var/obj/machinery/blackbox_recorder/BR = new/obj/machinery/blackbox_recorder(T) BR.msg_common = msg_common @@ -226,8 +226,8 @@ var/obj/machinery/blackbox_recorder/blackbox ..() /obj/machinery/blackbox_recorder/proc/find_feedback_datum(var/variable) - for(var/datum/feedback_variable/FV in feedback) - if(FV.get_variable() == variable) + for (var/datum/feedback_variable/FV in feedback) + if (FV.get_variable() == variable) return FV var/datum/feedback_variable/FV = new(variable) feedback += FV @@ -241,10 +241,10 @@ var/obj/machinery/blackbox_recorder/blackbox var/pda_msg_amt = 0 var/rc_msg_amt = 0 - for(var/obj/machinery/message_server/MS in world) - if(MS.pda_msgs.len > pda_msg_amt) + for (var/obj/machinery/message_server/MS in world) + if (MS.pda_msgs.len > pda_msg_amt) pda_msg_amt = MS.pda_msgs.len - if(MS.rc_msgs.len > rc_msg_amt) + if (MS.rc_msgs.len > rc_msg_amt) rc_msg_amt = MS.rc_msgs.len feedback_set_details("radio_usage","") @@ -269,63 +269,73 @@ var/obj/machinery/blackbox_recorder/blackbox //This proc is only to be called at round end. /obj/machinery/blackbox_recorder/proc/save_all_data_to_sql() - if(!feedback) return + if (!feedback) return round_end_data_gathering() //round_end time logging and some other data processing establish_db_connection() - if(!dbcon.IsConnected()) return + if (!dbcon.IsConnected()) return var/round_id var/DBQuery/query = dbcon.NewQuery("SELECT MAX(round_id) AS round_id FROM [format_table_name("feedback")]") query.Execute() - while(query.NextRow()) + while (query.NextRow()) round_id = query.item[1] - if(!isnum(round_id)) + if (!isnum(round_id)) round_id = text2num(round_id) round_id++ - for(var/datum/feedback_variable/FV in feedback) - var/sql = "INSERT INTO [format_table_name("feedback")] VALUES (null, Now(), [round_id], \"[FV.get_variable()]\", [FV.get_value()], \"[FV.get_details()]\")" - var/DBQuery/query_insert = dbcon.NewQuery(sql) - query_insert.Execute() + var/sqlrowlist = "" + + + for (var/datum/feedback_variable/FV in feedback) + if (sqlrowlist != "") + sqlrowlist += ", " //a comma (,) at the start of the first row to insert will trigger a SQL error + + sqlrowlist += "(null, Now(), [round_id], \"[FV.get_variable()]\", [FV.get_value()], \"[FV.get_details()]\")" + + if (sqlrowlist == "") + return + + var/DBQuery/query_insert = dbcon.NewQuery("INSERT DELAYED IGNORE INTO [format_table_name("feedback")] VALUES " + sqlrowlist) + query_insert.Execute() proc/feedback_set(var/variable,var/value) - if(!blackbox) return + if (!blackbox) return variable = sanitizeSQL(variable) var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) - if(!FV) return + if (!FV) return FV.set_value(value) proc/feedback_inc(var/variable,var/value) - if(!blackbox) return + if (!blackbox) return variable = sanitizeSQL(variable) var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) - if(!FV) return + if (!FV) return FV.inc(value) proc/feedback_dec(var/variable,var/value) - if(!blackbox) return + if (!blackbox) return variable = sanitizeSQL(variable) var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) - if(!FV) return + if (!FV) return FV.dec(value) proc/feedback_set_details(var/variable,var/details) - if(!blackbox) return + if (!blackbox) return variable = sanitizeSQL(variable) details = sanitizeSQL(details) @@ -337,13 +347,13 @@ proc/feedback_set_details(var/variable,var/details) FV.set_details(details) proc/feedback_add_details(var/variable,var/details) - if(!blackbox) return + if (!blackbox) return variable = sanitizeSQL(variable) details = sanitizeSQL(details) var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) - if(!FV) return + if (!FV) return FV.add_details(details) \ No newline at end of file From 6f0666138aab45bdf5cd6d926830ee460df51105 Mon Sep 17 00:00:00 2001 From: MrStonedOne Date: Wed, 29 Oct 2014 18:02:21 -0700 Subject: [PATCH 4/5] Ensures spawned proc is never called if the client goes away --- code/modules/client/client procs.dm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/code/modules/client/client procs.dm b/code/modules/client/client procs.dm index d1cd74964fd..6d8a2e6d1d8 100644 --- a/code/modules/client/client procs.dm +++ b/code/modules/client/client procs.dm @@ -137,7 +137,8 @@ var/next_external_rsc = 0 if (!ticker || ticker.current_state == GAME_STATE_PREGAME) spawn (rand(10,150)) - sync_client_with_db() + if (src) + sync_client_with_db() else sync_client_with_db() From afbeb03d9fab9bdab27f8a6f8a183062543724cc Mon Sep 17 00:00:00 2001 From: MrStonedOne Date: Wed, 29 Oct 2014 18:19:47 -0700 Subject: [PATCH 5/5] Fixes admin only vv related sql injection exploit in feedback reporting. Noticed this while looking over my PR, by storing (and trusting) the escaped version, an admin could edit this in vv and do unspeakable things to the database --- code/modules/research/message_server.dm | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/code/modules/research/message_server.dm b/code/modules/research/message_server.dm index 240e1d144ce..9bcc805b2c6 100644 --- a/code/modules/research/message_server.dm +++ b/code/modules/research/message_server.dm @@ -147,6 +147,8 @@ var/global/list/obj/machinery/message_server/message_servers = list() value = num /datum/feedback_variable/proc/get_value() + if (!isnum(value)) + return 0 return value /datum/feedback_variable/proc/get_variable() @@ -292,7 +294,7 @@ var/obj/machinery/blackbox_recorder/blackbox if (sqlrowlist != "") sqlrowlist += ", " //a comma (,) at the start of the first row to insert will trigger a SQL error - sqlrowlist += "(null, Now(), [round_id], \"[FV.get_variable()]\", [FV.get_value()], \"[FV.get_details()]\")" + sqlrowlist += "(null, Now(), [round_id], \"[sanitizeSQL(FV.get_variable())]\", [FV.get_value()], \"[sanitizeSQL(FV.get_details())]\")" if (sqlrowlist == "") return @@ -304,8 +306,6 @@ var/obj/machinery/blackbox_recorder/blackbox proc/feedback_set(var/variable,var/value) if (!blackbox) return - variable = sanitizeSQL(variable) - var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) if (!FV) return @@ -315,8 +315,6 @@ proc/feedback_set(var/variable,var/value) proc/feedback_inc(var/variable,var/value) if (!blackbox) return - variable = sanitizeSQL(variable) - var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) if (!FV) return @@ -326,8 +324,6 @@ proc/feedback_inc(var/variable,var/value) proc/feedback_dec(var/variable,var/value) if (!blackbox) return - variable = sanitizeSQL(variable) - var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) if (!FV) return @@ -337,9 +333,6 @@ proc/feedback_dec(var/variable,var/value) proc/feedback_set_details(var/variable,var/details) if (!blackbox) return - variable = sanitizeSQL(variable) - details = sanitizeSQL(details) - var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) if(!FV) return @@ -349,9 +342,6 @@ proc/feedback_set_details(var/variable,var/details) proc/feedback_add_details(var/variable,var/details) if (!blackbox) return - variable = sanitizeSQL(variable) - details = sanitizeSQL(details) - var/datum/feedback_variable/FV = blackbox.find_feedback_datum(variable) if (!FV) return