Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [day] [month] [year] [list]
Date: Sun, 27 Aug 2017 19:14:49 +0200
From: Florent Rougon <f.rougon@...e.fr>
To: oss-security@...ts.openwall.com
Subject: CVE-2017-13709: Incorrect access control in FlightGear

Hi,

Please find below the info for CVE-2017-13709. I'm also attaching a
patch combining the security fix applied to FlightGear's 'next'
branch[1] with its parent commit[2], because [1] requires [2] to work
properly.

However, I don't expect the combined patch nor [2] to apply cleanly to
FlightGear 2017.2 or earlier, because commit [3] introduced changes in
the close vicinity of the changes in [2] (two conflicts). If you need to
adapt [2] for such releases, just put the fgInitAllowedPaths() call
after the one to Options::processOptions() in src/Main/fg_init.cxx[4] and
src/Main/main.cxx[5], and you should be good.

I will probably backport the needed changes to a few of the last
releases in the next days: see the FlightGear release branches at [6].

[1] https://sourceforge.net/p/flightgear/flightgear/ci/2a5e3d06b2c0d9f831063afe7e7260bca456d679/
[2] https://sourceforge.net/p/flightgear/flightgear/ci/c7a2aef59979af3e9ff22daabb37bdaadb91cd75/
[3] https://sourceforge.net/p/flightgear/flightgear/ci/b2cc191bc665d13f50360e5508234e653669a372/
[4] https://sourceforge.net/p/flightgear/flightgear/ci/next/tree/src/Main/fg_init.cxx#l1147
[5] https://sourceforge.net/p/flightgear/flightgear/ci/next/tree/src/Main/main.cxx#l543
[6] https://sourceforge.net/p/flightgear/flightgear/ref/next/branches/

Now here is the info for CVE-2017-13709:

[Suggested description]
In FlightGear before version 2017.3.1, Main/logger.cxx in the FGLogger
subsystem allows one to overwrite any file via a resource that affects
the contents of the global Property Tree.

------------------------------------------

[Additional Information]
In FlightGear before version 2017.3.1, the FGLogger subsystem allows
one to overwrite any file the user has write access to (with enough
control over the contents to run arbitrary commands if the target file
is then executed). A resource such as a malicious third-party aircraft
or add-on could exploit this to damage files belonging to the user.

The security fix
(https://sourceforge.net/p/flightgear/flightgear/ci/2a5e3d06b2c0d9f831063afe7e7260bca456d679/)
requires its parent commit
(https://sourceforge.net/p/flightgear/flightgear/ci/c7a2aef59979af3e9ff22daabb37bdaadb91cd75/)
to work correctly.

We are not aware of any malicious resource exploiting the problem.

The fix will be in FlightGear 2017.3.1 (expected in a few days).

------------------------------------------

[Vulnerability Type]
Incorrect Access Control

------------------------------------------

[Vendor of Product]
FlightGear (http://flightgear.org/)

------------------------------------------

[Affected Product Code Base]
FlightGear - Affected: releases earlier than 2017.3.1 (at least since
version 2.0.0).

------------------------------------------

[Affected Component]
source file: src/Main/logger.cxx in the FlightGear repository
(https://sourceforge.net/p/flightgear/flightgear/ci/next/tree/src/Main/logger.cxx)
executable: fgfs

------------------------------------------

[Attack Type]
Local

------------------------------------------

[Impact Code execution]
true

------------------------------------------

[Impact Denial of Service]
true

------------------------------------------

[CVE Impact Other]
Allows one to overwrite any file the user has write access to, and to
control a significant part of the written contents. This leads to code
execution using .bashrc and such.

------------------------------------------

[Attack Vectors]
Trick users into installing a resource that enables logging to a
chosen file, via properties /logging/log/... For instance, a malicious
third-party aircraft or add-on could do that (add-ons loaded via 'fgfs
--addon=...').

------------------------------------------

[Reference]
https://sourceforge.net/p/flightgear/flightgear/ci/2a5e3d06b2c0d9f831063afe7e7260bca456d679/
https://sourceforge.net/p/flightgear/flightgear/ci/c7a2aef59979af3e9ff22daabb37bdaadb91cd75/

------------------------------------------

[Has vendor confirmed or acknowledged the vulnerability?]
true

------------------------------------------

[Discoverer]
wkitty42

-- 
Florent

commit a7c0c2b72ac2f2f5ce64624512c3758ec3422a02
Author: Florent Rougon <f.rougon@...e.fr>
Date:   Sat Aug 26 17:49:30 2017 +0200

    Call fgInitAllowedPaths() earlier + fix for CVE-2017-13709
    
    This is, for convenience, the combination of two commits:
    
      https://sourceforge.net/p/flightgear/flightgear/ci/c7a2aef59979af3e9ff22daabb37bdaadb91cd75/
    
    and
    
      https://sourceforge.net/p/flightgear/flightgear/ci/2a5e3d06b2c0d9f831063afe7e7260bca456d679/
    
    Indeed, the second commit wouldn't work correctly without the first one:
    fgInitAllowedPaths() must be called before FGLogger::init() gets to run
    in order for fgValidatePath() to be usable there for the security check.

diff --git a/src/Main/fg_init.cxx b/src/Main/fg_init.cxx
index 8205d3f0e..cccb61c62 100644
--- a/src/Main/fg_init.cxx
+++ b/src/Main/fg_init.cxx
@@ -1145,6 +1145,11 @@ void fgStartNewReset()
     
     flightgear::Options::sharedInstance()->processOptions();
 
+    // Rebuild the lists of allowed paths for cases where a path comes from an
+    // untrusted source, such as the global property tree (this uses $FG_HOME
+    // and other paths set by Options::processOptions()).
+    fgInitAllowedPaths();
+
     const auto& resMgr = simgear::EmbeddedResourceManager::instance();
     // The language was (re)set in processOptions()
     const string locale = globals->get_locale()->getPreferredLanguage();
diff --git a/src/Main/logger.cxx b/src/Main/logger.cxx
index 6c18162c3..00b6833ca 100644
--- a/src/Main/logger.cxx
+++ b/src/Main/logger.cxx
@@ -9,12 +9,17 @@
 
 #include "logger.hxx"
 
-#include <fstream>
+#include <ios>
 #include <string>
+#include <cstdlib>
 
 #include <simgear/debug/logstream.hxx>
+#include <simgear/io/iostreams/sgstream.hxx>
+#include <simgear/misc/sg_path.hxx>
 
 #include "fg_props.hxx"
+#include "globals.hxx"
+#include "util.hxx"
 
 using std::string;
 using std::endl;
@@ -59,6 +64,25 @@ FGLogger::init ()
         child->setStringValue("filename", filename.c_str());
     }
 
+    // Security: the path comes from the global Property Tree; it *must* be
+    //           validated before we overwrite the file.
+    const SGPath authorizedPath = fgValidatePath(SGPath::fromUtf8(filename),
+                                                 /* write */ true);
+
+    if (authorizedPath.isNull()) {
+      const string propertyPath = child->getChild("filename")
+                                       ->getPath(/* simplify */ true);
+      const string msg =
+        "The FGLogger logging system, via the '" + propertyPath + "' property, "
+        "was asked to write to '" + filename + "', however this path is not "
+        "authorized for writing anymore for security reasons. " +
+        "Please choose another location, for instance in the $FG_HOME/Export "
+        "folder (" + (globals->get_fg_home() / "Export").utf8Str() + ").";
+
+      SG_LOG(SG_GENERAL, SG_ALERT, msg);
+      exit(EXIT_FAILURE);
+    }
+
     string delimiter = child->getStringValue("delimiter");
     if (delimiter.empty()) {
         delimiter = ",";
@@ -68,7 +92,8 @@ FGLogger::init ()
     log.interval_ms = child->getLongValue("interval-ms");
     log.last_time_ms = globals->get_sim_time_sec() * 1000;
     log.delimiter = delimiter.c_str()[0];
-    log.output = new std::ofstream(filename.c_str());
+    // Security: use the return value of fgValidatePath()
+    log.output = new sg_ofstream(authorizedPath, std::ios_base::out);
     if (!log.output) {
       SG_LOG(SG_GENERAL, SG_ALERT, "Cannot write log to " << filename);
       continue;
diff --git a/src/Main/logger.hxx b/src/Main/logger.hxx
index 3d2146a83..de6209756 100644
--- a/src/Main/logger.hxx
+++ b/src/Main/logger.hxx
@@ -6,10 +6,10 @@
 #ifndef __LOGGER_HXX
 #define __LOGGER_HXX 1
 
-#include <iosfwd>
 #include <vector>
 
 #include <simgear/compiler.h>
+#include <simgear/io/iostreams/sgstream.hxx>
 #include <simgear/structure/subsystem_mgr.hxx>
 #include <simgear/props/props.hxx>
 
@@ -39,7 +39,7 @@ private:
     Log ();
     virtual ~Log ();
     std::vector<SGPropertyNode_ptr> nodes;
-    std::ostream * output;
+    sg_ofstream * output;
     long interval_ms;
     double last_time_ms;
     char delimiter;
diff --git a/src/Main/main.cxx b/src/Main/main.cxx
index c72db1c3a..3264d8d8f 100644
--- a/src/Main/main.cxx
+++ b/src/Main/main.cxx
@@ -541,6 +541,11 @@ int fgMainInit( int argc, char **argv )
         return EXIT_SUCCESS;
     }
 
+    // Set the lists of allowed paths for cases where a path comes from an
+    // untrusted source, such as the global property tree (this uses $FG_HOME
+    // and other paths set by Options::processOptions()).
+    fgInitAllowedPaths();
+
     const auto& resMgr = simgear::EmbeddedResourceManager::createInstance();
     initFlightGearEmbeddedResources();
     // The language was set in processOptions()
diff --git a/src/Scripting/NasalSys.cxx b/src/Scripting/NasalSys.cxx
index 89a1e0f96..9e37a2e93 100644
--- a/src/Scripting/NasalSys.cxx
+++ b/src/Scripting/NasalSys.cxx
@@ -911,10 +911,6 @@ void FGNasalSys::init()
       .member("simulatedTime", &TimerObj::isSimTime, &f_timerObj_setSimTime)
       .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);


[ CONTENT OF TYPE application/pgp-signature SKIPPED ]

Powered by blists - more mailing lists

Your e-mail address:

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Powered by Openwall GNU/*/Linux - Powered by OpenVZ