From fc81258d138d5e7d3f82d84c52314e7344c4a158 Mon Sep 17 00:00:00 2001 From: Florent Rougon Date: Mon, 31 Oct 2016 21:48:32 +0100 Subject: [PATCH] Add FGGlobals::get/set_download_dir(), unify TerraSync and download dirs setup Similar to the existing FGGlobals::get/set_terrasync_dir(), add FGGlobals::get_download_dir() and FGGlobals::set_download_dir() methods, and of course the corresponding FGGlobals::download_dir public member variable. FGGlobals::set_download_dir() stores the realpath() of the given directory, including into the /sim/paths/download-dir property, which is marked as read-only just as /sim/terrasync/scenery-dir already is. Handle the setup of the TerraSync and download dirs all in the same place (Options::processOptions()), since most of the work is already done there. This allows one to get rid of fgOptTerrasyncDir() and fgOptDownloadDir(), and to make it easier to see that globals->set_terrasync_dir() (resp. globals->set_download_dir()) is called on the correct SGPath, regardless of whether --terrasync-dir (resp. --download-dir) was passed. Always create the TerraSync and download dirs when they don't already exist, regardless of whether --terrasync-dir or --download-dir has been given on the command line. Add comments explaining how to avoid security pitfalls with download and TerraSync dirs (cf. discussion around ). Adjust indentation where it was too broken, hampering readbility. --- src/Main/globals.cxx | 25 +++++++++++++ src/Main/globals.hxx | 17 ++++++++- src/Main/options.cxx | 88 ++++++++++++++++++++++---------------------- 3 files changed, 84 insertions(+), 46 deletions(-) diff --git a/src/Main/globals.cxx b/src/Main/globals.cxx index ecbf5ffab..eea170783 100644 --- a/src/Main/globals.cxx +++ b/src/Main/globals.cxx @@ -416,6 +416,31 @@ void FGGlobals::clear_fg_scenery() } +// The 'path' argument to this method must come from trustworthy code, because +// the method grants read permissions to Nasal code for many files beneath +// 'path'. In particular, don't call this method with a 'path' value taken +// from the property tree or any other Nasal-writable place. +void FGGlobals::set_download_dir(const SGPath& path) +{ + SGPath abspath(path.realpath()); + download_dir = abspath; + + append_read_allowed_paths(abspath / "Aircraft"); + append_read_allowed_paths(abspath / "AI"); + append_read_allowed_paths(abspath / "Liveries"); + // If in use, abspath / TerraSync will be added to 'extra_read_allowed_paths' + // by FGGlobals::append_fg_scenery(), as any scenery path. + + SGPropertyNode *n = fgGetNode("/sim/paths/download-dir", true); + n->setAttribute(SGPropertyNode::WRITE, true); + n->setStringValue(abspath.utf8Str()); + n->setAttribute(SGPropertyNode::WRITE, false); +} + +// The 'path' argument to this method must come from trustworthy code, because +// the method grants read permissions to Nasal code for all files beneath +// 'path'. In particular, don't call this method with a 'path' value taken +// from the property tree or any other Nasal-writable place. void FGGlobals::set_terrasync_dir(const SGPath &path) { if (terrasync_dir.realpath() != SGPath(fgGetString("/sim/terrasync/scenery-dir")).realpath()) { diff --git a/src/Main/globals.hxx b/src/Main/globals.hxx index 104d288d9..310f0987b 100644 --- a/src/Main/globals.hxx +++ b/src/Main/globals.hxx @@ -104,7 +104,9 @@ private: // Users home directory for data SGPath fg_home; - //Terrasync directory + // Download directory + SGPath download_dir; + // Terrasync directory SGPath terrasync_dir; // Roots of FlightGear scenery tree @@ -221,7 +223,20 @@ public: const SGPath &get_fg_home () const { return fg_home; } void set_fg_home (const SGPath &home); + const SGPath &get_download_dir () const { return download_dir; } + // The 'path' argument to set_download_dir() must come from trustworthy + // code, because the method grants read permissions to Nasal code for many + // files beneath 'path'. In particular, don't call this method with a + // 'path' value taken from the property tree or any other Nasal-writable + // place. + void set_download_dir (const SGPath &path); + const SGPath &get_terrasync_dir () const { return terrasync_dir; } + // The 'path' argument to set_terrasync_dir() must come from trustworthy + // code, because the method grants read permissions to Nasal code for all + // files beneath 'path'. In particular, don't call this method with a + // 'path' value taken from the property tree or any other Nasal-writable + // place. void set_terrasync_dir (const SGPath &path); const PathList &get_fg_scenery () const { return fg_scenery; } diff --git a/src/Main/options.cxx b/src/Main/options.cxx index 54a55371f..0208045ae 100644 --- a/src/Main/options.cxx +++ b/src/Main/options.cxx @@ -856,26 +856,6 @@ fgOptFgScenery( const char *arg ) return FG_OPTIONS_OK; } -static int -fgOptTerrasyncDir( const char *arg ) -{ - SGPath p = SGPath::fromLocal8Bit(arg); - globals->set_terrasync_dir(p); - return FG_OPTIONS_OK; -} - -static int -fgOptDownloadDir( const char *arg ) -{ - SGPath p = SGPath::fromLocal8Bit(arg); - globals->append_read_allowed_paths(p / "Aircraft"); - globals->append_read_allowed_paths(p / "AI"); - globals->append_read_allowed_paths(p / "Liveries"); - // p / TerraSync is allowed later if in use - fgSetString("/sim/paths/download-dir", p.utf8Str()); - return FG_OPTIONS_OK; -} - static int fgOptAllowNasalRead( const char *arg ) { @@ -1627,8 +1607,8 @@ struct OptionDesc { {"materials-file", true, OPTION_STRING, "/sim/rendering/materials-file", false, "", 0 }, {"disable-terrasync", false, OPTION_BOOL, "/sim/terrasync/enabled", false, "", 0 }, {"enable-terrasync", false, OPTION_BOOL, "/sim/terrasync/enabled", true, "", 0 }, - {"terrasync-dir", true, OPTION_FUNC, "", false, "", fgOptTerrasyncDir }, - {"download-dir", true, OPTION_FUNC, "", false, "", fgOptDownloadDir }, + {"terrasync-dir", true, OPTION_IGNORE, "", false, "", 0 }, + {"download-dir", true, OPTION_IGNORE, "", false, "", 0 }, {"allow-nasal-read", true, OPTION_FUNC | OPTION_MULTI, "", false, "", fgOptAllowNasalRead }, {"geometry", true, OPTION_FUNC, "", false, "", fgOptGeometry }, {"bpp", true, OPTION_FUNC, "", false, "", fgOptBpp }, @@ -2354,36 +2334,54 @@ OptionResult Options::processOptions() globals->append_fg_scenery(SGPath::pathsFromEnv("FG_SCENERY")); } -// download dir fix-up - SGPath downloadDir = SGPath::fromLocal8Bit(valueForOption("download-dir").c_str()); + // Download dir fix-up + SGPath downloadDir = SGPath::fromLocal8Bit( + valueForOption("download-dir").c_str()); if (downloadDir.isNull()) { downloadDir = defaultDownloadDir(); - SG_LOG(SG_GENERAL, SG_INFO, "Using default download dir: " << downloadDir); + SG_LOG(SG_GENERAL, SG_INFO, + "Using default download dir: " << downloadDir); } else { - simgear::Dir d(downloadDir); - if (!d.exists()) { - SG_LOG(SG_GENERAL, SG_INFO, "Creating requested download dir: " << downloadDir); - d.create(0755); - } + SG_LOG(SG_GENERAL, SG_INFO, + "Using explicit download dir: " << downloadDir); } -// terrasync directory fixup - SGPath terrasyncDir = globals->get_terrasync_dir(); - if (terrasyncDir.isNull()) { - SGPath p(downloadDir); - p.append("TerraSync"); - terrasyncDir = p; + simgear::Dir d(downloadDir); + if (!d.exists()) { + SG_LOG(SG_GENERAL, SG_INFO, + "Creating download dir: " << downloadDir); + d.create(0755); + } - simgear::Dir d(terrasyncDir); - if (!d.exists()) { - d.create(0755); - } + // This is safe because the value of 'downloadDir' is trustworthy. In + // particular, it can't be influenced by Nasal code, not even indirectly + // via a Nasal-writable place such as the property tree. + globals->set_download_dir(downloadDir); - SG_LOG(SG_GENERAL, SG_INFO, "Using default TerraSync: " << terrasyncDir); - globals->set_terrasync_dir(p); - } else { - SG_LOG(SG_GENERAL, SG_INFO, "Using explicit TerraSync dir: " << terrasyncDir); - } + // TerraSync directory fixup + SGPath terrasyncDir = SGPath::fromLocal8Bit( + valueForOption("terrasync-dir").c_str()); + if (terrasyncDir.isNull()) { + terrasyncDir = downloadDir / "TerraSync"; + // No “default” qualifier here, because 'downloadDir' may be non-default + SG_LOG(SG_GENERAL, SG_INFO, + "Using TerraSync dir: " << terrasyncDir); + } else { + SG_LOG(SG_GENERAL, SG_INFO, + "Using explicit TerraSync dir: " << terrasyncDir); + } + + d = simgear::Dir(terrasyncDir); + if (!d.exists()) { + SG_LOG(SG_GENERAL, SG_INFO, + "Creating TerraSync dir: " << terrasyncDir); + d.create(0755); + } + + // This is safe because the value of 'terrasyncDir' is trustworthy. In + // particular, it can't be influenced by Nasal code, not even indirectly + // via a Nasal-writable place such as the property tree. + globals->set_terrasync_dir(terrasyncDir); // check if we setup a scenery path so far bool addFGDataScenery = globals->get_fg_scenery().empty();