Package: release.debian.org Severity: normal User: release.debian.org@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 <markus@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_palmer@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);
Attachment:
signature.asc
Description: OpenPGP digital signature