From 1ef73dbc370297cb0df779c73e796788404fd829 Mon Sep 17 00:00:00 2001 From: AndrewL97 Date: Sun, 7 Nov 2021 18:30:49 +1000 Subject: [PATCH] Map load unit testing and directory whitelisting - Mojave Sun is a really cool downstream, like and subscribe for longer PR names. oranges was here and made this github PR name much longer, thereby proving once and for all that upstreams do add value to the downstream not just endless amounts of refactoring because we can't write any api's in a modular fashion the first time around so have to update them later to be modular. Anyway do you think a taco counts as a sandwich? (#62620) About The Pull Request This is a fix for map not load anymore because of security changes Why It's Good For The Game Maps are good as they encourage gameplay and differentiate ss13 from a classic MUD game Changelog cl add: unit test for map load add: directory param to map load + whitelist for data and _maps add: advertising for mojave sun in tg commit logs /cl --- code/__DEFINES/maps.dm | 6 +++ code/_compile_options.dm | 1 + .../configuration/configuration.dm | 2 +- code/controllers/subsystem/mapping.dm | 2 +- code/datums/map_config.dm | 19 +++++++-- code/modules/unit_tests/_unit_tests.dm | 1 + code/modules/unit_tests/load_map_security.dm | 42 +++++++++++++++++++ 7 files changed, 68 insertions(+), 5 deletions(-) create mode 100644 code/modules/unit_tests/load_map_security.dm diff --git a/code/__DEFINES/maps.dm b/code/__DEFINES/maps.dm index 9a5222bef7f..aba2b715b8d 100644 --- a/code/__DEFINES/maps.dm +++ b/code/__DEFINES/maps.dm @@ -26,9 +26,15 @@ require only minor tweaks. #define SPACERUIN_MAP_EDGE_PAD 15 + /// Path for the next_map.json file, if someone, for some messed up reason, wants to change it. #define PATH_TO_NEXT_MAP_JSON "data/next_map.json" +/// List of directories we can load map .json files from +#define MAP_DIRECTORY_MAPS "_maps" +#define MAP_DIRECTORY_DATA "data" +#define MAP_DIRECTORY_WHITELIST list(MAP_DIRECTORY_MAPS,MAP_DIRECTORY_DATA) + /// Special map path value for custom adminloaded stations. #define CUSTOM_MAP_PATH "custom" diff --git a/code/_compile_options.dm b/code/_compile_options.dm index 6d654f5a094..83038d30137 100644 --- a/code/_compile_options.dm +++ b/code/_compile_options.dm @@ -49,6 +49,7 @@ #ifdef LOWMEMORYMODE #define FORCE_MAP "runtimestation" +#define FORCE_MAP_DIRECTORY "_maps" #endif //Update this whenever you need to take advantage of more recent byond features diff --git a/code/controllers/configuration/configuration.dm b/code/controllers/configuration/configuration.dm index 2c35f1bbc65..d1b5c728da1 100644 --- a/code/controllers/configuration/configuration.dm +++ b/code/controllers/configuration/configuration.dm @@ -348,7 +348,7 @@ Example config: switch (command) if ("map") - currentmap = load_map_config(data) + currentmap = load_map_config(data, MAP_DIRECTORY_MAPS) 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 7ac377c15ea..220a9f5509a 100644 --- a/code/controllers/subsystem/mapping.dm +++ b/code/controllers/subsystem/mapping.dm @@ -51,7 +51,7 @@ SUBSYSTEM_DEF(mapping) /datum/controller/subsystem/mapping/New() ..() #ifdef FORCE_MAP - config = load_map_config(FORCE_MAP) + config = load_map_config(FORCE_MAP, FORCE_MAP_DIRECTORY) #else config = load_map_config(error_if_missing = FALSE) #endif diff --git a/code/datums/map_config.dm b/code/datums/map_config.dm index 33fdec46219..7d28390bd5b 100644 --- a/code/datums/map_config.dm +++ b/code/datums/map_config.dm @@ -45,16 +45,29 @@ * Proc handling the loading of map configs. Will return the default map config using [/proc/load_default_map_config] if the loading of said file fails for any reason whatsoever, so we always have a working map for the server to run. * Arguments: * * filename - Name of the config file for the map we want to load. The .json file extension is added during the proc, so do not specify filenames with the extension. + * * directory - Name of the directory containing our .json - Must be in MAP_DIRECTORY_WHITELIST. We default this to MAP_DIRECTORY_MAPS as it will likely be the most common usecase. If no filename is set, we ignore this. * * error_if_missing - Bool that says whether failing to load the config for the map will be logged in log_world or not as it's passed to LoadConfig(). * * Returns the config for the map to load. */ -/proc/load_map_config(filename = null, error_if_missing = TRUE) +/proc/load_map_config(filename = null, directory = null, error_if_missing = TRUE) var/datum/map_config/config = load_default_map_config() + if(filename) // If none is specified, then go to look for next_map.json, for map rotation purposes. - filename = "_maps/[filename].json" + + //Default to MAP_DIRECTORY_MAPS if no directory is passed + if(directory) + if(!(directory in MAP_DIRECTORY_WHITELIST)) + log_world("map directory not in whitelist: [directory] for map [filename]") + return config + else + directory = MAP_DIRECTORY_MAPS + + filename = "[directory]/[filename].json" else filename = PATH_TO_NEXT_MAP_JSON + + if (!config.LoadConfig(filename, error_if_missing)) qdel(config) return load_default_map_config() @@ -174,4 +187,4 @@ . += "_maps/[map_path]/[file]" /datum/map_config/proc/MakeNextMap() - return config_filename == "data/next_map.json" || fcopy(config_filename, "data/next_map.json") + return config_filename == PATH_TO_NEXT_MAP_JSON || fcopy(config_filename, PATH_TO_NEXT_MAP_JSON) diff --git a/code/modules/unit_tests/_unit_tests.dm b/code/modules/unit_tests/_unit_tests.dm index c4cb3d07687..b40fb5cfeae 100644 --- a/code/modules/unit_tests/_unit_tests.dm +++ b/code/modules/unit_tests/_unit_tests.dm @@ -70,6 +70,7 @@ #include "hydroponics_harvest.dm" #include "hydroponics_self_mutations.dm" #include "keybinding_init.dm" +#include "load_map_security.dm" #include "machine_disassembly.dm" #include "medical_wounds.dm" #include "merge_type.dm" diff --git a/code/modules/unit_tests/load_map_security.dm b/code/modules/unit_tests/load_map_security.dm new file mode 100644 index 00000000000..a06f184adea --- /dev/null +++ b/code/modules/unit_tests/load_map_security.dm @@ -0,0 +1,42 @@ +// replace runtimestation with another valid _maps .json if not present +#define VALID_TEST_MAP "runtimestation" + +/// Tests to ensure we can load a map from a whitelisted directory (_maps), but not a non-whitelisted directory (i.e "fartyShitPants") +/datum/unit_test/load_map_security + +/datum/unit_test/load_map_security/Run() + + // Copy our valid map into a bad directory + // We can technically load from /unitTestTempDir by passing it in our map name + // But it should fail when passed as a directory + fcopy("_maps/[VALID_TEST_MAP].json", "data/load_map_security_temp/[VALID_TEST_MAP].json") + + //Attempt to load our configs + + // test load from _maps - this should pass + var/datum/map_config/maps_config = load_map_config(VALID_TEST_MAP, MAP_DIRECTORY_MAPS) + + // test load from data - this should pass + // this also confirms that our fcopy worked for our bad_config test + var/datum/map_config/data_config = load_map_config("load_map_security_temp/[VALID_TEST_MAP]", MAP_DIRECTORY_DATA) + + // data/load_map_security_temp/ is not in our whitelist, this should fail + var/datum/map_config/bad_config = load_map_config(VALID_TEST_MAP,"data/load_map_security_temp") + + + // Check we can load from _maps + TEST_ASSERT(!maps_config.defaulted, "Failed to load: _maps/[VALID_TEST_MAP]") + + // Check we can load from data and ensure that our fcopy setup worked + TEST_ASSERT(!data_config.defaulted, "Failed to load: data/load_map_security_temp/[VALID_TEST_MAP]") + + // Check we can't load from "bad directory" + TEST_ASSERT(bad_config.defaulted, "Loaded from non-whitelisted directory: data/load_map_security_temp/[VALID_TEST_MAP]") + + +/datum/unit_test/load_map_security/Destroy() + // Clean up our temp directory + fdel("data/load_map_security_temp/") + return ..() + +#undef VALID_TEST_MAP