Package: release.debian.org Severity: normal User: release.debian....@packages.debian.org Usertags: unblock
Dear release team, please unblock the package flightgear-3.0.0-5 as recently uploaded to unstable. It fixes a security issue by disallowing nasal scripts to access or modify files, see #780712. I kept the packaging changes as minimal as possible. A debdiff and the patch are attached for review. unblock flightgear/3.0.0-5 Regards Markus Wanner
diff -Nru flightgear-3.0.0/debian/changelog flightgear-3.0.0/debian/changelog --- flightgear-3.0.0/debian/changelog 2014-11-07 17:27:50.000000000 +0100 +++ flightgear-3.0.0/debian/changelog 2015-03-18 11:19:39.000000000 +0100 @@ -1,3 +1,10 @@ +flightgear (3.0.0-5) unstable; urgency=high + + * Add patch 6a30e70.patch to better restrict file access from + nasal scripts. Closes: #780712. + + -- Markus Wanner <mar...@bluegap.ch> Wed, 18 Mar 2015 08:45:21 +0100 + flightgear (3.0.0-4) unstable; urgency=medium * Add patch 750939.patch. Closes: #750939. diff -Nru flightgear-3.0.0/debian/patches/6a30e7.patch flightgear-3.0.0/debian/patches/6a30e7.patch # patch attached directly for better readability diff -Nru flightgear-3.0.0/debian/patches/series flightgear-3.0.0/debian/patches/series --- flightgear-3.0.0/debian/patches/series 2014-10-27 11:33:44.000000000 +0100 +++ flightgear-3.0.0/debian/patches/series 2015-03-18 08:48:58.000000000 +0100 @@ -2,3 +2,4 @@ nasal-fix.patch fix-mobile-tacan.patch 750939.patch +6a30e7.patch
Description: Restrict file access for Nasal scripts. Stop using property listener for fgValidatePath . This was insecure: while removelistener() won't remove it, there are other ways to remove a listener from Nasal Author: Rebecca N. Palmer <rebecca_pal...@zoho.com> Last-Update: 13-03-2015 Origin: http://sourceforge.net/p/flightgear/flightgear/ci/6a30e7086ea2f1a060dd77dab6e7e8a15b43e82d --- a/src/Main/util.cxx +++ b/src/Main/util.cxx @@ -33,6 +33,7 @@ #include <simgear/math/SGLimits.hxx> #include <simgear/math/SGMisc.hxx> +#include <GUI/MessageBox.hxx> #include "fg_io.hxx" #include "fg_props.hxx" #include "globals.hxx" @@ -71,32 +72,142 @@ return current; } -// Write out path to validation node and read it back in. A Nasal -// listener is supposed to replace the path with a validated version -// or an empty string otherwise. -const char *fgValidatePath (const char *str, bool write) +static string_list read_allowed_paths; +static string_list write_allowed_paths; + +// Allowed paths here are absolute, and may contain _one_ *, +// which matches any string +// FG_SCENERY is deliberately not allowed, as it would make +// /sim/terrasync/scenery-dir a security hole +void fgInitAllowedPaths() { - SGPropertyNode_ptr r, w; - r = fgGetNode("/sim/paths/validate/read", true); - r->setAttribute(SGPropertyNode::READ, true); - r->setAttribute(SGPropertyNode::WRITE, true); - - w = fgGetNode("/sim/paths/validate/write", true); - w->setAttribute(SGPropertyNode::READ, true); - w->setAttribute(SGPropertyNode::WRITE, true); - - SGPropertyNode *prop = write ? w : r; - prop->setStringValue(str); - const char *result = prop->getStringValue(); - return result[0] ? result : 0; + read_allowed_paths.clear(); + write_allowed_paths.clear(); + read_allowed_paths.push_back(globals->get_fg_root() + "/*"); + read_allowed_paths.push_back(globals->get_fg_home() + "/*"); + 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(*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 + // obvious error than a can-read-everything security hole... + if (!(it->compare("/*"))){ + flightgear::fatalMessageBox("Nasal initialization error", + "Empty string in FG_ROOT, FG_HOME or FG_AIRCRAFT", + "or fgInitAllowedPaths() called too early"); + exit(-1); + } + } + write_allowed_paths.push_back("/tmp/*.xml"); + write_allowed_paths.push_back(globals->get_fg_home() + "/*.sav"); + write_allowed_paths.push_back(globals->get_fg_home() + "/*.log"); + write_allowed_paths.push_back(globals->get_fg_home() + "/cache/*"); + write_allowed_paths.push_back(globals->get_fg_home() + "/Export/*"); + write_allowed_paths.push_back(globals->get_fg_home() + "/state/*.xml"); + write_allowed_paths.push_back(globals->get_fg_home() + "/aircraft-data/*.xml"); + write_allowed_paths.push_back(globals->get_fg_home() + "/Wildfire/*.xml"); + write_allowed_paths.push_back(globals->get_fg_home() + "/runtime-jetways/*.xml"); + write_allowed_paths.push_back(globals->get_fg_home() + "/Input/Joysticks/*.xml"); + + if(!fgValidatePath(globals->get_fg_home() + "/../no.log",true).empty() || + !fgValidatePath(globals->get_fg_home() + "/no.lot",true).empty() || + fgValidatePath((globals->get_fg_home() + "/nolog").c_str(),true) || + !fgValidatePath(globals->get_fg_home() + "no.log",true).empty() || + !fgValidatePath("..\\" + globals->get_fg_home() + "/no.log",false).empty() || + fgValidatePath("/tmp/no.xml",false) || + fgValidatePath(globals->get_fg_home() + "/./ff/../Export\\yes..gg",true).empty() || + !fgValidatePath((globals->get_fg_home() + "/aircraft-data/yes..xml").c_str(),true) || + fgValidatePath(globals->get_fg_root() + "/./\\yes.bmp",false).empty()) { + flightgear::fatalMessageBox("Nasal initialization error", + "fgInitAllowedPaths() does not work", + ""); + exit(-1); + } } -//------------------------------------------------------------------------------ -std::string fgValidatePath(const std::string& path, bool write) +// 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) { - const char* validate_path = fgValidatePath(path.c_str(), write); - return std::string(validate_path ? validate_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; + } + +// Check whether Nasal is allowed to access a path +std::string fgValidatePath (const std::string& path, bool write) +{ + const string_list& allowed_paths(write ? write_allowed_paths : read_allowed_paths); + int star_pos; + + // Normalize the path (prevents ../../.. trickery) + std::string normed_path = fgNormalizePath(path); + + // Check against each allowed pattern + for( string_list::const_iterator it = allowed_paths.begin(); + it != allowed_paths.end(); + ++it ) + { + star_pos = it->find('*'); + if (star_pos == std::string::npos) { + if (!(it->compare(normed_path))) { + return normed_path; + } + } else { + if ((it->size()-1 <= normed_path.size()) /* long enough to be a potential match */ + && !(it->substr(0,star_pos) + .compare(normed_path.substr(0,star_pos))) /* before-star parts match */ + && !(it->substr(star_pos+1,it->size()-star_pos-1) + .compare(normed_path.substr(star_pos+1+normed_path.size()-it->size(), + it->size()-star_pos-1))) /* after-star parts match */) { + return normed_path; + } + } + } + // no match found + return ""; +} +// s.c_str() becomes invalid when s is destroyed, so need a static s +std::string validate_path_temp; +const char* fgValidatePath(const char* path, bool write) +{ + validate_path_temp = fgValidatePath(std::string(path), write); + if(validate_path_temp.empty()){ + return 0; + } + return validate_path_temp.c_str(); +} // end of util.cxx --- a/src/Main/util.hxx +++ b/src/Main/util.hxx @@ -36,7 +36,7 @@ double fgGetLowPass (double current, double target, double timeratio); /** - * Validation listener interface for io.nas, used by fgcommands. + * File access control, used by Nasal and fgcommands. * @param path Path to be validated * @param write True for write operations and false for read operations. * @return The validated path on success or 0 if access denied. @@ -44,4 +44,9 @@ const char *fgValidatePath (const char *path, bool write); std::string fgValidatePath(const std::string& path, bool write); +/** + * Set allowed paths for fgValidatePath + */ +void fgInitAllowedPaths(); + #endif // __UTIL_HXX --- a/src/Scripting/NasalSys.cxx +++ b/src/Scripting/NasalSys.cxx @@ -800,6 +800,9 @@ .member("singleShot", &TimerObj::isSingleShot, &TimerObj::setSingleShot) .member("isRunning", &TimerObj::isRunning); + // Set allowed paths for Nasal I/O + fgInitAllowedPaths(); + // Now load the various source files in the Nasal directory simgear::Dir nasalDir(SGPath(globals->get_fg_root(), "Nasal")); loadScriptDirectory(nasalDir);
signature.asc
Description: OpenPGP digital signature
