From 3de38b11b8e8e6196d8aa4b0a314db02db6af288 Mon Sep 17 00:00:00 2001 From: TheFakeElon <59686430+TheFakeElon@users.noreply.github.com> Date: Fri, 5 Nov 2021 11:31:13 +1100 Subject: [PATCH] [s] Security vulnerability patch (#62568) About The Pull Request In my personal, subjective opinion; trialmins should not, in fact, be able to read and delete server/box configuration files on a whim. cl server: Patches multiple(?) arbitrary file related vulnerabilities /cl --- code/__DEFINES/text.dm | 3 +++ code/__HELPERS/_string_lists.dm | 23 +++++++++++-------- code/__HELPERS/files.dm | 1 - code/_globalvars/regexes.dm | 6 ++--- .../configuration/configuration.dm | 4 ++-- code/controllers/subsystem/mapping.dm | 8 +++---- code/datums/map_config.dm | 5 ++-- code/game/machinery/camera/camera.dm | 9 ++++---- code/modules/awaymissions/zlevel.dm | 1 + code/modules/tgs/v3210/api.dm | 2 ++ code/modules/tgui/tgui_window.dm | 3 +++ 11 files changed, 38 insertions(+), 27 deletions(-) diff --git a/code/__DEFINES/text.dm b/code/__DEFINES/text.dm index 48487531d0f..b3b4c2a0880 100644 --- a/code/__DEFINES/text.dm +++ b/code/__DEFINES/text.dm @@ -17,3 +17,6 @@ ///Adds a html style to a text string. Hacky, but that's how inputted text appear on paper sheets after going through the UI. #define PAPER_MARK_TEXT(text, color, font) "[text]\n \n" + +/// Folder directory for strings +#define STRING_DIRECTORY "strings" diff --git a/code/__HELPERS/_string_lists.dm b/code/__HELPERS/_string_lists.dm index 5dab48761ec..17cf04ce8fd 100644 --- a/code/__HELPERS/_string_lists.dm +++ b/code/__HELPERS/_string_lists.dm @@ -7,8 +7,9 @@ GLOBAL_LIST(string_cache) GLOBAL_VAR(string_filename_current_key) -/proc/strings_replacement(filename, key, directory = "strings") - load_strings_file(filename, directory) +/proc/strings_replacement(filename, key) + filename = SANITIZE_FILENAME(filename) + load_strings_file(filename) if((filename in GLOB.string_cache) && (key in GLOB.string_cache[filename])) var/response = pick(GLOB.string_cache[filename][key]) @@ -16,19 +17,21 @@ GLOBAL_VAR(string_filename_current_key) response = r.Replace(response, /proc/strings_subkey_lookup) return response else - CRASH("strings list not found: [directory]/[filename], index=[key]") + CRASH("strings list not found: [STRING_DIRECTORY]/[filename], index=[key]") -/proc/strings(filename as text, key as text, directory = "strings") - load_strings_file(filename, directory) +/proc/strings(filename as text, key as text) + filename = SANITIZE_FILENAME(filename) + load_strings_file(filename) if((filename in GLOB.string_cache) && (key in GLOB.string_cache[filename])) return GLOB.string_cache[filename][key] else - CRASH("strings list not found: [directory]/[filename], index=[key]") + CRASH("strings list not found: [STRING_DIRECTORY]/[filename], index=[key]") /proc/strings_subkey_lookup(match, group1) return pick_list(GLOB.string_filename_current_key, group1) -/proc/load_strings_file(filename, directory = "strings") +/proc/load_strings_file(filename) + filename = SANITIZE_FILENAME(filename) // in case we're called directly GLOB.string_filename_current_key = filename if(filename in GLOB.string_cache) return //no work to do @@ -36,7 +39,7 @@ GLOBAL_VAR(string_filename_current_key) if(!GLOB.string_cache) GLOB.string_cache = new - if(fexists("[directory]/[filename]")) - GLOB.string_cache[filename] = json_load("[directory]/[filename]") + if(fexists("[STRING_DIRECTORY]/[filename]")) + GLOB.string_cache[filename] = json_load("[STRING_DIRECTORY]/[filename]") else - CRASH("file not found: [directory]/[filename]") + CRASH("file not found: [STRING_DIRECTORY]/[filename]") diff --git a/code/__HELPERS/files.dm b/code/__HELPERS/files.dm index 3d39a8b52a6..8cbe58a7fa9 100644 --- a/code/__HELPERS/files.dm +++ b/code/__HELPERS/files.dm @@ -1,4 +1,3 @@ - /** * For FTP requests. (i.e. downloading runtime logs.) * diff --git a/code/_globalvars/regexes.dm b/code/_globalvars/regexes.dm index cfe7586044d..6f5205686af 100644 --- a/code/_globalvars/regexes.dm +++ b/code/_globalvars/regexes.dm @@ -10,9 +10,9 @@ GLOBAL_DATUM_INIT(is_color, /regex, regex("^#\[0-9a-fA-F]{6}$")) //finds text strings recognized as links on discord. Mainly used to stop embedding. GLOBAL_DATUM_INIT(has_discord_embeddable_links, /regex, regex("(https?://\[^\\s|<\]{2,})")) -//All < and > characters +//All < and > characters GLOBAL_DATUM_INIT(angular_brackets, /regex, regex(@"[<>]", "g")) -//All characters forbidden by filenames: ", \, \n, \t, /, ?, %, *, :, |, <, > -GLOBAL_DATUM_INIT(filename_forbidden_chars, /regex, regex(@{""|[\\\n\t/?%*:|<>]"}, "g")) +//All characters forbidden by filenames: ", \, \n, \t, /, ?, %, *, :, |, <, >, .. +GLOBAL_DATUM_INIT(filename_forbidden_chars, /regex, regex(@{""|[\\\n\t/?%*:|<>]|\.\."}, "g")) // had to use the OR operator for quotes instead of putting them in the character class because it breaks the syntax highlighting otherwise. diff --git a/code/controllers/configuration/configuration.dm b/code/controllers/configuration/configuration.dm index e21a030de7f..2c35f1bbc65 100644 --- a/code/controllers/configuration/configuration.dm +++ b/code/controllers/configuration/configuration.dm @@ -32,7 +32,7 @@ /// A regex that matches words blocked IC, but not in PDAs var/static/regex/ic_outside_pda_filter_regex - + /// A regex that matches words soft blocked IC var/static/regex/soft_ic_filter_regex @@ -348,7 +348,7 @@ Example config: switch (command) if ("map") - currentmap = load_map_config("_maps/[data].json") + currentmap = load_map_config(data) if(currentmap.defaulted) log_config("Failed to load map config for [data]!") currentmap = null diff --git a/code/controllers/subsystem/mapping.dm b/code/controllers/subsystem/mapping.dm index 18e30c10886..2d4e24a86b1 100644 --- a/code/controllers/subsystem/mapping.dm +++ b/code/controllers/subsystem/mapping.dm @@ -404,9 +404,9 @@ GLOBAL_LIST_EMPTY(the_station_areas) /datum/controller/subsystem/mapping/proc/preloadRuinTemplates() // Still supporting bans by filename - var/list/banned = generateMapList("[global.config.directory]/lavaruinblacklist.txt") - banned += generateMapList("[global.config.directory]/spaceruinblacklist.txt") - banned += generateMapList("[global.config.directory]/iceruinblacklist.txt") + var/list/banned = generateMapList("lavaruinblacklist.txt") + banned += generateMapList("spaceruinblacklist.txt") + banned += generateMapList("iceruinblacklist.txt") for(var/item in sort_list(subtypesof(/datum/map_template/ruin), /proc/cmp_ruincost_priority)) var/datum/map_template/ruin/ruin_type = item @@ -431,7 +431,7 @@ GLOBAL_LIST_EMPTY(the_station_areas) space_ruins_templates[R.name] = R /datum/controller/subsystem/mapping/proc/preloadShuttleTemplates() - var/list/unbuyable = generateMapList("[global.config.directory]/unbuyableshuttles.txt") + var/list/unbuyable = generateMapList("unbuyableshuttles.txt") for(var/item in subtypesof(/datum/map_template/shuttle)) var/datum/map_template/shuttle/shuttle_type = item diff --git a/code/datums/map_config.dm b/code/datums/map_config.dm index 6fab47eed12..7fbac43aaf7 100644 --- a/code/datums/map_config.dm +++ b/code/datums/map_config.dm @@ -34,14 +34,15 @@ /// Dictionary of job sub-typepath to template changes dictionary var/job_changes = list() -/proc/load_map_config(filename = "data/next_map.json", default_to_box, delete_after, error_if_missing = TRUE) +/proc/load_map_config(filename = "next_map.json", default_to_box, delete_after, error_if_missing = TRUE) + filename = "_maps/[filename].json" var/datum/map_config/config = new if (default_to_box) return config if (!config.LoadConfig(filename, error_if_missing)) qdel(config) config = new /datum/map_config // Fall back to Box - if (delete_after) + else if (delete_after) fdel(filename) return config diff --git a/code/game/machinery/camera/camera.dm b/code/game/machinery/camera/camera.dm index 9cb9e8306ce..b6b8d0b7368 100644 --- a/code/game/machinery/camera/camera.dm +++ b/code/game/machinery/camera/camera.dm @@ -328,10 +328,9 @@ itemname = pressed_pda.name info = pressed_pda.notehtml - var/sanitized_name = sanitize(itemname) - + itemname = sanitize(itemname) to_chat(paper_user, span_notice("You hold \the [itemname] up to the camera...")) - paper_user.log_talk(sanitized_name, LOG_GAME, log_globally=TRUE, tag="Pressed to camera") + paper_user.log_talk(itemname, LOG_GAME, log_globally=TRUE, tag="Pressed to camera") paper_user.changeNext_move(CLICK_CD_MELEE) for(var/mob/potential_viewer in GLOB.player_list) @@ -343,11 +342,11 @@ to_chat(AI, "[span_name(paper_user)] holds \a [itemname] up to one of your cameras ...") else to_chat(AI, "[paper_user] holds \a [itemname] up to one of your cameras ...") - AI.log_talk(sanitized_name, LOG_VICTIM, tag="Pressed to camera from [key_name(paper_user)]", log_globally=FALSE) + AI.log_talk(itemname, LOG_VICTIM, tag="Pressed to camera from [key_name(paper_user)]", log_globally=FALSE) AI.last_paper_seen = "[itemname][info]" else if (potential_viewer.client.eye == src) to_chat(potential_viewer, "[span_name("[paper_user]")] holds \a [itemname] up to one of the cameras ...") - potential_viewer.log_talk(sanitized_name, LOG_VICTIM, tag="Pressed to camera from [key_name(paper_user)]", log_globally=FALSE) + potential_viewer.log_talk(itemname, LOG_VICTIM, tag="Pressed to camera from [key_name(paper_user)]", log_globally=FALSE) potential_viewer << browse(text("[][]", itemname, info), text("window=[]", itemname)) return diff --git a/code/modules/awaymissions/zlevel.dm b/code/modules/awaymissions/zlevel.dm index 9d433396ad4..3c91fafdfc9 100644 --- a/code/modules/awaymissions/zlevel.dm +++ b/code/modules/awaymissions/zlevel.dm @@ -43,6 +43,7 @@ GLOBAL_LIST_INIT(potentialConfigRandomZlevels, generateConfigMapList(directory = /proc/generateMapList(filename) . = list() + filename = "[global.config.directory]/[SANITIZE_FILENAME(filename)]" var/list/Lines = world.file2list(filename) if(!Lines.len) diff --git a/code/modules/tgs/v3210/api.dm b/code/modules/tgs/v3210/api.dm index 63823251001..257a7886365 100644 --- a/code/modules/tgs/v3210/api.dm +++ b/code/modules/tgs/v3210/api.dm @@ -54,6 +54,8 @@ return "" /datum/tgs_api/v3210/proc/file2list(filename) + if(IsAdminAdvancedProcCall()) + CRASH("Attempted to read file via admin call") return splittext(trim_left(trim_right(file2text(filename))), "\n") /datum/tgs_api/v3210/OnWorldNew(minimum_required_security_level) diff --git a/code/modules/tgui/tgui_window.dm b/code/modules/tgui/tgui_window.dm index af6b5bdc74c..185586e40e2 100644 --- a/code/modules/tgui/tgui_window.dm +++ b/code/modules/tgui/tgui_window.dm @@ -321,3 +321,6 @@ // Resend the assets for(var/asset in sent_assets) send_asset(asset) + +/datum/tgui_window/vv_edit_var(var_name, var_value) + return var_name != NAMEOF(src, id) && ..()