TG: More SQL injection patches. Added a standardized method of SQL sanitization

[sanitizeSQL()].
Corrected a href list vulnerability that let players spawn Arcane Tomes from
non-e-magged library computers.
Tweaked and disabled forum_activation.dm. Way too many SQL vulnerabilities to
justify keeping it around.
Revision: r3012
Author: 	 only.lurking
This commit is contained in:
Ren Erthilo
2012-04-17 21:19:35 +01:00
parent 3b36e6b161
commit ccc5c217b7
3 changed files with 34 additions and 28 deletions

View File

@@ -35,12 +35,8 @@
return
// Sanitize inputs to avoid SQL injection attacks
accname = dd_replacetext(accname, "'", "''")
accname = dd_replacetext(accname, ";", "")
accname = dd_replacetext(accname, "&", "")
playerkey = dd_replacetext(playerkey, "'", "''")
playerkey = dd_replacetext(playerkey, ";", "")
playerkey = dd_replacetext(playerkey, "&", "")
accname = sanitizeSQL(accname)
playerkey = sanitizeSQL(playerkey)
var/DBQuery/query = dbcon.NewQuery("SELECT user_id FROM [forumsqldb].phpbb_users WHERE username = '[accname]'")
@@ -52,7 +48,7 @@
dbcon.Disconnect()
return
query = dbcon.NewQuery("SELECT pf_byondkey FROM [forumsqldb].phpbb_profile_fields_data WHERE user_id = [uid]")
query = dbcon.NewQuery("SELECT pf_byondkey FROM [forumsqldb].phpbb_profile_fields_data WHERE user_id = '[uid]'")
if(!query.Execute())
src << "Unable to verify whether account is already associated with a BYOND key or not. This error shouldn't occur, please contact an administrator."
dbcon.Disconnect()
@@ -65,7 +61,7 @@
dbcon.Disconnect()
return
query = dbcon.NewQuery("SELECT * FROM [forumsqldb].phpbb_user_group WHERE user_id = [uid] AND group_id = [forum_authenticated_group]")
query = dbcon.NewQuery("SELECT * FROM [forumsqldb].phpbb_user_group WHERE user_id = '[uid]' AND group_id = '[forum_authenticated_group]'")
if(!query.Execute())
src << "Unable to verify whether account is already part of the authenticated group or not. This error should not occur, please contact an administrator."
dbcon.Disconnect()
@@ -75,19 +71,19 @@
dbcon.Disconnect()
return
query = dbcon.NewQuery("INSERT INTO [forumsqldb].phpbb_profile_fields_data (user_id, pf_byondkey) VALUES ([uid], '[playerkey]')") // Remember which key is associated with the account
query = dbcon.NewQuery("INSERT INTO [forumsqldb].phpbb_profile_fields_data (user_id, pf_byondkey) VALUES ('[uid]', '[playerkey]')") // Remember which key is associated with the account
if(!query.Execute())
src << "Unable to associate key with account. Authentication failed."
dbcon.Disconnect()
return
query = dbcon.NewQuery("UPDATE [forumsqldb].phpbb_user_group SET group_id = [forum_authenticated_group] WHERE user_id = [uid] AND group_id = [forum_activated_group]") // Replace 'Registered Users' group with 'Activated Users'
query = dbcon.NewQuery("UPDATE [forumsqldb].phpbb_user_group SET group_id = '[forum_authenticated_group]' WHERE user_id = '[uid]' AND group_id = '[forum_activated_group]'") // Replace 'Registered Users' group with 'Activated Users'
if(!query.Execute())
src << "Unable to move account into authenticated group. This error shouldn't occur, contact an administrator for help. Authentication failed."
dbcon.Disconnect()
return
query = dbcon.NewQuery("UPDATE [forumsqldb].phpbb_users SET group_id = [forum_authenticated_group] WHERE user_id = [uid]") // Change 'default group' the the authenticated group. Not doing so was causing many authenticated accounts to retain their unauthenticated permissions, despite being succesfully authenticated.
query = dbcon.NewQuery("UPDATE [forumsqldb].phpbb_users SET group_id = '[forum_authenticated_group]' WHERE user_id = '[uid]'") // Change 'default group' the the authenticated group. Not doing so was causing many authenticated accounts to retain their unauthenticated permissions, despite being succesfully authenticated.
if(!query.Execute())
src << "Unable to modify default group for account. This error should never occur, contact an administrator for help. Authentication failed."
else
@@ -95,8 +91,12 @@
dbcon.Disconnect()
// This actually opens up a bunch of security holes to the forum DB. Given that it's not used much in the first place,
// I'm going to keep this commented out until we're sure everything's secure. -- TLE
/*
/client/verb/activate_forum_account(var/a as text)
set name = "Activate Forum Account"
set category = "Special Verbs"
set desc = "Associate a tgstation forum account with your BYOND key to enable posting."
associate_key_with_forum(a, src.key)
associate_key_with_forum(a, src.key)
*/

View File

@@ -55,19 +55,19 @@ proc/sql_report_death(var/mob/living/carbon/human/H)
return
var/turf/T = H.loc
var/area/placeofdeath = T.loc
var/area/placeofdeath = get_area(T.loc)
var/podname = placeofdeath.name
var/sqlname = dd_replacetext(H.real_name, "'", "''")
var/sqlkey = dd_replacetext(H.key, "'", "''")
var/sqlpod = dd_replacetext(podname, "'", "''")
var/sqlspecial = dd_replacetext(H.mind.special_role, "'", "''")
var/sqljob = dd_replacetext(H.mind.assigned_role, "'", "''")
var/sqlname = sanitizeSQL(H.real_name)
var/sqlkey = sanitizeSQL(H.key)
var/sqlpod = sanitizeSQL(podname)
var/sqlspecial = sanitizeSQL(H.mind.special_role)
var/sqljob = sanitizeSQL(H.mind.assigned_role)
var/laname
var/lakey
if(H.lastattacker)
laname = dd_replacetext(H.lastattacker:real_name, "'", "''")
lakey = dd_replacetext(H.lastattacker:key, "'", "''")
laname = sanitizeSQL(H.lastattacker:real_name)
lakey = sanitizeSQL(H.lastattacker:key)
var/sqltime = time2text(world.realtime, "YYYY-MM-DD hh:mm:ss")
var/coord = "[H.x], [H.y], [H.z]"
//world << "INSERT INTO death (name, byondkey, job, special, pod, tod, laname, lakey, gender, bruteloss, fireloss, brainloss, oxyloss) VALUES ('[sqlname]', '[sqlkey]', '[sqljob]', '[sqlspecial]', '[sqlpod]', '[sqltime]', '[laname]', '[lakey]', '[H.gender]', [H.bruteloss], [H.getFireLoss()], [H.brainloss], [H.getOxyLoss()])"
@@ -92,19 +92,19 @@ proc/sql_report_cyborg_death(var/mob/living/silicon/robot/H)
return
var/turf/T = H.loc
var/area/placeofdeath = T.loc
var/area/placeofdeath = get_area(T.loc)
var/podname = placeofdeath.name
var/sqlname = dd_replacetext(H.real_name, "'", "''")
var/sqlkey = dd_replacetext(H.key, "'", "''")
var/sqlpod = dd_replacetext(podname, "'", "''")
var/sqlspecial = dd_replacetext(H.mind.special_role, "'", "''")
var/sqljob = dd_replacetext(H.mind.assigned_role, "'", "''")
var/sqlname = sanitizeSQL(H.real_name)
var/sqlkey = sanitizeSQL(H.key)
var/sqlpod = sanitizeSQL(podname)
var/sqlspecial = sanitizeSQL(H.mind.special_role)
var/sqljob = sanitizeSQL(H.mind.assigned_role)
var/laname
var/lakey
if(H.lastattacker)
laname = dd_replacetext(H.lastattacker:real_name, "'", "''")
lakey = dd_replacetext(H.lastattacker:key, "'", "''")
laname = sanitizeSQL(H.lastattacker:real_name)
lakey = sanitizeSQL(H.lastattacker:key)
var/sqltime = time2text(world.realtime, "YYYY-MM-DD hh:mm:ss")
var/coord = "[H.x], [H.y], [H.z]"
//world << "INSERT INTO death (name, byondkey, job, special, pod, tod, laname, lakey, gender, bruteloss, fireloss, brainloss, oxyloss) VALUES ('[sqlname]', '[sqlkey]', '[sqljob]', '[sqlspecial]', '[sqlpod]', '[sqltime]', '[laname]', '[lakey]', '[H.gender]', [H.bruteloss], [H.getFireLoss()], [H.brainloss], [H.getOxyLoss()])"

View File

@@ -50,6 +50,12 @@
// - Books shouldn't print straight from the library computer. Make it synch with a machine like the book binder to print instead. This should consume some sort of resource.
// Run all strings to be used in an SQL query through this proc first to properly escape out injection attempts.
/proc/sanitizeSQL(var/t as text)
var/sanitized_text = dd_replacetext(t, "'", "\\'")
sanitized_text = dd_replacetext(sanitized_text, "\"", "\\\"")
return sanitized_text
/obj/structure/bookcase