[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#780719: marked as done (unblock: flightgear/3.0.0-5)

Your message dated Fri, 20 Mar 2015 17:41:02 +0000
with message-id <49b201782e3157fa3abc9933747af284@mail.adsl.funky-badger.org>
and subject line Re: Bug#780719: unblock: flightgear/3.0.0-5
has caused the Debian Bug report #780719,
regarding unblock: flightgear/3.0.0-5
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@bugs.debian.org
immediately.)


-- 
780719: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=780719
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems
--- Begin Message --- 
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


--- End Message ---
--- Begin Message --- 
On 2015-03-20 17:01, Markus Wanner wrote:

Adam,

On 03/20/2015 05:19 PM, Adam D. Barratt wrote:

The latter's potentially a fairly important point. One of the reasons
that insecure tempfile handling is an issue is that if you write to or
truncate a file in /tmp and that file is a symlink to another file the
result can be that the destination file is modified.


I appreciate your feedback. Given Rebecca's answers, these are valid
concerns that should be addressed.


Thank you both for engaging in the discussion.


However, they seem unrelated to this unblock request.
Well, they're related to the extent that they suggest potential room to tighten up the security fix. 


So I'm not sure
how you want me to proceed. I'd still prefer to get this first
mitigation patch through. It got applied upstream, so it already has
some testing mileage. I see no point in delaying it on the grounds that
it fixes only one and not all issues. It certainly didn't introduce any
of the issues you're pointing out.
Indeed, I agree that the new version is certainly an improvement over the version currently in Jessie and as such I've unblocked it.
When I first looked at the diff, it wasn't entirely clear to me whether the /tmp entry was a test entry that had been left in by accident or had some purpose that I was missing, as it seemed incongruous. I'm still not convinced that it's a good idea, but it's no worse than the previous functionality. 

Regards,

Adam

--- End Message ---
Reply to: