From 91dc44887569a01b4e6b5f7c9c0416957aff6924 Mon Sep 17 00:00:00 2001 From: "Rebecca N. Palmer" Date: Sat, 21 Nov 2015 21:37:19 +0000 Subject: [PATCH] Simplify fgValidatePath + minor fix (requires simgear update) Drop fgNormalizePath, use realpath() only As this makes it accept relative paths, always use the returned (absolute) version for the actual file operation to avoid check-to-use races, or where this is not possible (NasalSGPath) explicitly reject relative paths Fix: do_save is a write, not a read --- src/Main/fg_commands.cxx | 20 +++--- src/Main/util.cxx | 126 +++++++++++----------------------- src/Scripting/NasalSGPath.cxx | 13 +++- 3 files changed, 61 insertions(+), 98 deletions(-) diff --git a/src/Main/fg_commands.cxx b/src/Main/fg_commands.cxx index 032db16e6..1eb518562 100644 --- a/src/Main/fg_commands.cxx +++ b/src/Main/fg_commands.cxx @@ -292,13 +292,14 @@ do_load (const SGPropertyNode * arg) if (file.extension() != "sav") file.concat(".sav"); - if (fgValidatePath(file, false).empty()) { + std::string validated_path = fgValidatePath(file, false); + if (validated_path.empty()) { SG_LOG(SG_IO, SG_ALERT, "load: reading '" << file << "' denied " "(unauthorized access)"); return false; } - ifstream input(file.c_str()); + ifstream input(validated_path.c_str()); if (input.good() && fgLoadFlight(input)) { input.close(); SG_LOG(SG_INPUT, SG_INFO, "Restored flight from " << file); @@ -324,7 +325,8 @@ do_save (const SGPropertyNode * arg) if (file.extension() != "sav") file.concat(".sav"); - if (fgValidatePath(file, false).empty()) { + std::string validated_path = fgValidatePath(file, true); + if (validated_path.empty()) { SG_LOG(SG_IO, SG_ALERT, "save: writing '" << file << "' denied " "(unauthorized access)"); return false; @@ -332,7 +334,7 @@ do_save (const SGPropertyNode * arg) bool write_all = arg->getBoolValue("write-all", false); SG_LOG(SG_INPUT, SG_INFO, "Saving flight"); - ofstream output(file.c_str()); + ofstream output(validated_path.c_str()); if (output.good() && fgSaveFlight(output, write_all)) { output.close(); SG_LOG(SG_INPUT, SG_INFO, "Saved flight to " << file); @@ -1175,7 +1177,8 @@ do_load_xml_to_proptree(const SGPropertyNode * arg) } } - if (fgValidatePath(file, false).empty()) { + std::string validated_path = fgValidatePath(file, false); + if (validated_path.empty()) { SG_LOG(SG_IO, SG_ALERT, "loadxml: reading '" << file.str() << "' denied " "(unauthorized directory - authorization no longer follows symlinks; to authorize reading additional directories, add them to --fg-aircraft)"); return false; @@ -1188,7 +1191,7 @@ do_load_xml_to_proptree(const SGPropertyNode * arg) targetnode = const_cast(arg)->getNode("data", true); try { - readProperties(file.c_str(), targetnode, true); + readProperties(validated_path.c_str(), targetnode, true); } catch (const sg_exception &e) { SG_LOG(SG_IO, SG_WARN, "loadxml: " << e.getFormattedMessage()); return false; @@ -1257,7 +1260,8 @@ do_save_xml_from_proptree(const SGPropertyNode * arg) if (file.extension() != "xml") file.concat(".xml"); - if (fgValidatePath(file, true).empty()) { + std::string validated_path = fgValidatePath(file, true); + if (validated_path.empty()) { SG_LOG(SG_IO, SG_ALERT, "savexml: writing to '" << file.str() << "' denied " "(unauthorized directory - authorization no longer follows symlinks)"); return false; @@ -1272,7 +1276,7 @@ do_save_xml_from_proptree(const SGPropertyNode * arg) return false; try { - writeProperties (file.c_str(), sourcenode, true); + writeProperties (validated_path.c_str(), sourcenode, true); } catch (const sg_exception &e) { SG_LOG(SG_IO, SG_WARN, "savexml: " << e.getFormattedMessage()); return false; diff --git a/src/Main/util.cxx b/src/Main/util.cxx index 1974d64b3..286396784 100644 --- a/src/Main/util.cxx +++ b/src/Main/util.cxx @@ -72,39 +72,6 @@ fgGetLowPass (double current, double target, double timeratio) return current; } -// Normalize a path -// Unlike SGPath::realpath, does not require that the file already exists, -// but does require that it be below the starting point -static std::string fgNormalizePath (const std::string& path) -{ - string_list path_parts; - char c; - std::string normed_path = "", this_part = ""; - - for (int pos = 0; ; pos++) { - c = path[pos]; - if (c == '\\') { c = '/'; } - if ((c == '/') || (c == 0)) { - if ((this_part == "/..") || (this_part == "..")) { - if (path_parts.empty()) { return ""; } - path_parts.pop_back(); - } else if ((this_part != "/.") && (this_part != "/")) { - path_parts.push_back(this_part); - } - this_part = ""; - } - if (c == 0) { break; } - this_part = this_part + c; - } - for( string_list::const_iterator it = path_parts.begin(); - it != path_parts.end(); - ++it ) - { - normed_path.append(*it); - } - return normed_path; - } - static string_list read_allowed_paths; static string_list write_allowed_paths; @@ -114,41 +81,51 @@ static string_list write_allowed_paths; // /sim/terrasync/scenery-dir a security hole void fgInitAllowedPaths() { + if(SGPath("ygjmyfvhhnvdoesnotexist").realpath() == "ygjmyfvhhnvdoesnotexist"){ + // Forbid using this version of fgValidatePath() with older + // (not normalizing non-existent files) versions of realpath(), + // as that would be a security hole + flightgear::fatalMessageBox("Nasal initialization error", + "Version mismatch - please update simgear", + ""); + exit(-1); + } read_allowed_paths.clear(); write_allowed_paths.clear(); - std::string fg_root = fgNormalizePath(globals->get_fg_root()); - std::string fg_home = fgNormalizePath(globals->get_fg_home()); - read_allowed_paths.push_back(fg_root + "/*"); - read_allowed_paths.push_back(fg_home + "/*"); + std::string fg_root = SGPath(globals->get_fg_root()).realpath(); + std::string fg_home = SGPath(globals->get_fg_home()).realpath(); +#if defined(_MSC_VER) /*for MS compilers */ || defined(_WIN32) /*needed for non MS windows compilers like MingW*/ + std::string sep = "\\"; +#else + std::string sep = "/"; +#endif + read_allowed_paths.push_back(fg_root + sep + "*"); + read_allowed_paths.push_back(fg_home + sep + "*"); string_list const aircraft_paths = globals->get_aircraft_paths(); for( string_list::const_iterator it = aircraft_paths.begin(); it != aircraft_paths.end(); ++it ) { - read_allowed_paths.push_back(fgNormalizePath(*it) + "/*"); - } - - for( string_list::const_iterator it = read_allowed_paths.begin(); - it != read_allowed_paths.end(); - ++it ) - { // if we get the initialization order wrong, better to have an + // if we get the initialization order wrong, better to have an // obvious error than a can-read-everything security hole... - if (!(it->compare("/*"))){ + if (it->empty() || fg_root.empty() || fg_home.empty()){ flightgear::fatalMessageBox("Nasal initialization error", "Empty string in FG_ROOT, FG_HOME or FG_AIRCRAFT", "or fgInitAllowedPaths() called too early"); exit(-1); } + read_allowed_paths.push_back(SGPath(*it).realpath() + sep + "*"); } - write_allowed_paths.push_back(fg_home + "/*.sav"); - write_allowed_paths.push_back(fg_home + "/*.log"); - write_allowed_paths.push_back(fg_home + "/cache/*"); - write_allowed_paths.push_back(fg_home + "/Export/*"); - write_allowed_paths.push_back(fg_home + "/state/*.xml"); - write_allowed_paths.push_back(fg_home + "/aircraft-data/*.xml"); - write_allowed_paths.push_back(fg_home + "/Wildfire/*.xml"); - write_allowed_paths.push_back(fg_home + "/runtime-jetways/*.xml"); - write_allowed_paths.push_back(fg_home + "/Input/Joysticks/*.xml"); + + write_allowed_paths.push_back(fg_home + sep + "*.sav"); + write_allowed_paths.push_back(fg_home + sep + "*.log"); + write_allowed_paths.push_back(fg_home + sep + "cache" + sep + "*"); + write_allowed_paths.push_back(fg_home + sep + "Export" + sep + "*"); + write_allowed_paths.push_back(fg_home + sep + "state" + sep + "*.xml"); + write_allowed_paths.push_back(fg_home + sep + "aircraft-data" + sep + "*.xml"); + write_allowed_paths.push_back(fg_home + sep + "Wildfire" + sep + "*.xml"); + write_allowed_paths.push_back(fg_home + sep + "runtime-jetways" + sep + "*.xml"); + write_allowed_paths.push_back(fg_home + sep + "Input" + sep + "Joysticks" + sep + "*.xml"); // Check that it works if(!fgValidatePath(globals->get_fg_home() + "/../no.log",true).empty() || @@ -165,9 +142,15 @@ void fgInitAllowedPaths() } } -// Check whether Nasal is allowed to access a path (assumed already normalized) -static std::string fgValidatePath_internal (const std::string& normed_path, bool write) +// Check whether Nasal is allowed to access a path +// Warning: because this always (not just on Windows) converts \ to /, +// and accepts relative paths (check-to-use race if the current directory +// changes), always use the returned path not the original one +std::string fgValidatePath (const std::string& path, bool write) { + // Normalize the path (prevents ../../.. or symlink trickery) + std::string normed_path = SGPath(path).realpath(); + const string_list& allowed_paths(write ? write_allowed_paths : read_allowed_paths); size_t star_pos; @@ -195,37 +178,6 @@ static std::string fgValidatePath_internal (const std::string& normed_path, bool // no match found return ""; } -// Check whether Nasal is allowed to access a path -// Warning: because this always (not just on Windows) converts \ to /, -// if passing a std::string, use the returned path not the original one -// (This warning does not apply to the SGPath variant, as these are -// so converted on creation.) -std::string fgValidatePath (const std::string& path, bool write) -{ - // Normalize the path (prevents ../../.. trickery) - // method 1 allows following symlinks to anywhere - // (https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=780867); - // method 2 doesn't, and is intended to eventually replace it - std::string normed_path1 = fgNormalizePath(path); - SGPath path2 = SGPath(path); - std::string normed_path2; - if (path2.exists()) { - normed_path2 = path2.realpath(); - } else { // realpath can't check non-existent files - normed_path2 = SGPath(path2.dir()).realpath() - + "/" + path2.file(); - } -#if defined(_MSC_VER) /*for MS compilers */ || defined(_WIN32) /*needed for non MS windows compilers like MingW*/ - normed_path2 = SGPath(normed_path2).str(); // convert \ to / -#endif - - // Check - if (fgValidatePath_internal(normed_path1, write).empty() || - fgValidatePath_internal(normed_path2, write).empty()) { - return ""; - } - return normed_path1; -} std::string fgValidatePath(const SGPath& path, bool write) { return fgValidatePath(path.str(),write); } // end of util.cxx diff --git a/src/Scripting/NasalSGPath.cxx b/src/Scripting/NasalSGPath.cxx index 19a74aff2..065aacd22 100644 --- a/src/Scripting/NasalSGPath.cxx +++ b/src/Scripting/NasalSGPath.cxx @@ -38,9 +38,16 @@ typedef nasal::Ghost NasalSGPath; SGPath::Permissions checkIORules(const SGPath& path) { SGPath::Permissions perm; - - perm.read = !fgValidatePath(path.str(), false).empty(); - perm.write = !fgValidatePath(path.str(), true ).empty(); + if (!path.isAbsolute()) { + // SGPath caches permissions, which breaks for relative paths + // if the current directory changes + SG_LOG(SG_NASAL, SG_ALERT, "os.path: file operation on '" << + path.str() << "' access denied (relative paths not accepted; use " + "realpath() to make a path absolute)"); + } + + perm.read = path.isAbsolute() && !fgValidatePath(path.str(), false).empty(); + perm.write = path.isAbsolute() && !fgValidatePath(path.str(), true ).empty(); return perm; } -- 2.39.5