Add fixes for CVE-2008-1614

diff --git a/debian/changelog b/debian/changelog
index 7e1a720..c6265db 100644 (file)
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+suphp (0.6.2-2+etch0) stable-security; urgency=high
+
+  * The following security issue is addressed with this update:
+    - CVE-2008-1614: allows local users to gain privileges via a race
+    condition in symlinks handling
+
+ -- Emmanuel Lacour <elacour@home-dn.net>  Fri, 11 Apr 2008 11:31:28 +0200
+
 suphp (0.6.2-2) unstable; urgency=low
 
   * remove apache 1.x package (closes: #429079)

diff --git a/src/API.hpp b/src/API.hpp
index 47f71e3..c079888 100644 (file)
--- a/src/API.hpp
+++ b/src/API.hpp
@@ -157,6 +157,12 @@ namespace suPHP {
        virtual GroupInfo File_getGroup(const File& file) const
            throw (SystemException) =0;
        
+        /**
+         * Checks whether a file is a symlink
+         */
+        virtual bool File_isSymlink(const File& file) const 
+            throw (SystemException) =0;
+        
        /**
         * Runs another program (replaces current process)
         */

diff --git a/src/API_Linux.cpp b/src/API_Linux.cpp
index 97c1e90..a504603 100644 (file)
--- a/src/API_Linux.cpp
+++ b/src/API_Linux.cpp
@@ -225,10 +225,10 @@ std::string suPHP::API_Linux::GroupInfo_getGroupname(const GroupInfo& ginfo)
 
 bool suPHP::API_Linux::File_exists(const File& file) const {
     struct stat dummy;
-    if (::stat(file.getPath().c_str(), &dummy) == 0)
-       return true;
+    if (::lstat(file.getPath().c_str(), &dummy) == 0)
+        return true;
     else
-       return false;
+        return false;
 }
 
 std::string suPHP::API_Linux::File_getRealPath(const File& file) const
@@ -304,7 +304,7 @@ std::string suPHP::API_Linux::File_getRealPath(const File& file) const
 bool suPHP::API_Linux::File_hasPermissionBit(const File& file, FileMode perm) 
     const throw (SystemException) {
     struct stat temp;
-    if (stat(file.getPath().c_str(), &temp) == -1) {
+    if (lstat(file.getPath().c_str(), &temp) == -1) {
        throw SystemException(std::string("Could not stat \"")
                              + file.getPath() + "\": "
                              + ::strerror(errno), __FILE__, __LINE__);
@@ -362,7 +362,7 @@ bool suPHP::API_Linux::File_hasPermissionBit(const File& file, FileMode perm)
 UserInfo suPHP::API_Linux::File_getUser(const File& file) const
     throw (SystemException) {
     struct stat temp;
-    if (stat(file.getPath().c_str(), &temp) == -1) {
+    if (lstat(file.getPath().c_str(), &temp) == -1) {
        throw SystemException(std::string("Could not stat \"")
                              + file.getPath() + "\": "
                              + ::strerror(errno), __FILE__, __LINE__);
@@ -373,7 +373,7 @@ UserInfo suPHP::API_Linux::File_getUser(const File& file) const
 GroupInfo suPHP::API_Linux::File_getGroup(const File& file) const
     throw (SystemException) {
     struct stat temp;
-    if (stat(file.getPath().c_str(), &temp) == -1) {
+    if (lstat(file.getPath().c_str(), &temp) == -1) {
        throw SystemException(std::string("Could not stat \"")
                              + file.getPath() + "\": "
                              + ::strerror(errno), __FILE__, __LINE__);
@@ -382,6 +382,11 @@ GroupInfo suPHP::API_Linux::File_getGroup(const File& file) const
 }
 
 
+bool suPHP::API_Linux::File_isSymlink(const File& file) const throw (SystemException) {
+    return this->isSymlink(file.getPath());
+}
+
+
 void suPHP::API_Linux::execute(std::string program, const CommandLine& cline,
                               const Environment& env) const
     throw (SystemException) {

diff --git a/src/API_Linux.hpp b/src/API_Linux.hpp
index a547d40..83b07a4 100644 (file)
--- a/src/API_Linux.hpp
+++ b/src/API_Linux.hpp
@@ -169,6 +169,11 @@ namespace suPHP {
        virtual GroupInfo File_getGroup(const File& file) const
            throw (SystemException);
 
+        /**
+         * Checks whether a file is a symlink
+         */
+        virtual bool File_isSymlink(const File& file) const throw (SystemException);
+        
        /**
         * Runs another program (replaces current process)
         */

diff --git a/src/Application.cpp b/src/Application.cpp
index bb7ce50..72e6731 100644 (file)
--- a/src/Application.cpp
+++ b/src/Application.cpp
@@ -177,6 +177,7 @@ void suPHP::Application::checkScriptFile(
     throw (SystemException, SoftException) {
     Logger& logger = API_Helper::getSystemAPI().getSystemLogger();
     File scriptFile = File(scriptFilename);
+    File realScriptFile = File(scriptFile.getRealPath());
 
     // Check wheter file exists
     if (!scriptFile.exists()) {
@@ -184,11 +185,13 @@ void suPHP::Application::checkScriptFile(
        logger.logWarning(error);
        throw SoftException(error, __FILE__, __LINE__);
     }
-    
-    // Get full path to script file
-
-    File realScriptFile = File(scriptFile.getRealPath());
-    File directory = realScriptFile.getParentDirectory();
+    if (!realScriptFile.exists()) {
+        std::string error = "File " + realScriptFile.getPath() 
+            + " referenced by symlink " +scriptFile.getPath() 
+            + " does not exist";
+        logger.logWarning(error);
+        throw SoftException(error, __FILE__, __LINE__);
+    }
     
     // Check wheter script is in docroot
     if (realScriptFile.getPath().find(config.getDocroot()) != 0) {
@@ -213,8 +216,19 @@ void suPHP::Application::checkScriptFile(
        logger.logWarning(error);
        throw SoftException(error, __FILE__, __LINE__);
     }
+    if (config.getCheckVHostDocroot()
+        && scriptFile.getPath().find(environment.getVar("DOCUMENT_ROOT")) 
+        != 0) {
+        
+        std::string error = "File \"" + scriptFile.getPath()
+            + "\" is not in document root of Vhost \""
+            + environment.getVar("DOCUMENT_ROOT") + "\"";
+        logger.logWarning(error);
+        throw SoftException(error, __FILE__, __LINE__);
+    }
 
-    // Check script and directory permissions
+    // Check script permissions
+    // Directories will be checked later
     if (!realScriptFile.hasUserReadBit()) {
        std::string error = "File \"" + realScriptFile.getPath()
            + "\" not readable";
@@ -231,14 +245,6 @@ void suPHP::Application::checkScriptFile(
        throw SoftException(error, __FILE__, __LINE__);
     }
     
-    if (!config.getAllowDirectoryGroupWriteable() 
-       && directory.hasGroupWriteBit()) {
-       std::string error = "Directory \"" + directory.getPath()
-           + "\" is writeable by group";
-       logger.logWarning(error);
-       throw SoftException(error, __FILE__, __LINE__);
-    }
-
     if (!config.getAllowFileOthersWriteable()
        && realScriptFile.hasOthersWriteBit()) {
        std::string error = "File \"" + realScriptFile.getPath()
@@ -247,14 +253,6 @@ void suPHP::Application::checkScriptFile(
        throw SoftException(error, __FILE__, __LINE__);
     }
     
-    if (!config.getAllowDirectoryOthersWriteable()
-       && directory.hasOthersWriteBit()) {
-       std::string error = "Directory \"" + directory.getPath()
-           + "\" is writeable by others";
-       logger.logWarning(error);
-       throw SoftException(error, __FILE__, __LINE__);
-    }
-
     // Check UID/GID of symlink is matching target
     if (scriptFile.getUser() != realScriptFile.getUser()
        || scriptFile.getGroup() != realScriptFile.getGroup()) {
@@ -274,7 +272,8 @@ void suPHP::Application::changeProcessPermissions(
     UserInfo targetUser;
     GroupInfo targetGroup;
 
-    File scriptFile = File(File(scriptFilename).getRealPath());
+    File scriptFile = File(scriptFilename);
+    File realScriptFile = File(scriptFile.getRealPath());
     API& api = API_Helper::getSystemAPI();
     Logger& logger = api.getSystemLogger();
 
@@ -360,7 +359,11 @@ void suPHP::Application::changeProcessPermissions(
        throw SoftException(error, __FILE__, __LINE__);
     }
 #endif // OPT_USERGROUP_PARANOID    
-
+    
+    // Check directory ownership and permissions
+    checkParentDirectories(realScriptFile, targetUser, config);
+    checkParentDirectories(scriptFile, targetUser, config);
+    
     // Common code used for all modes
 
     // Set new group first, because we still need super-user privileges
@@ -480,6 +483,43 @@ void suPHP::Application::executeScript(const std::string& scriptFilename,
 }
 
 
+void suPHP::Application::checkParentDirectories(const File& file,
+                                               const UserInfo& owner,
+                                               const Configuration& config) const throw (SoftException) {
+    File directory = file;
+    Logger& logger = API_Helper::getSystemAPI().getSystemLogger();
+    do {
+        directory = directory.getParentDirectory();
+        
+        UserInfo directoryOwner = directory.getUser();
+        if (directoryOwner != owner && !directoryOwner.isSuperUser()) {
+            std::string error = "Directory " + directory.getPath()
+                + " is not owned by " + owner.getUsername();
+            logger.logWarning(error);
+            throw SoftException(error, __FILE__, __LINE__);
+        }
+        
+        if (!directory.isSymlink()
+            && !config.getAllowDirectoryGroupWriteable() 
+            && directory.hasGroupWriteBit()) {
+            std::string error = "Directory \"" + directory.getPath()
+                + "\" is writeable by group";
+            logger.logWarning(error);
+            throw SoftException(error, __FILE__, __LINE__);
+        }
+        
+        if (!directory.isSymlink()
+            && !config.getAllowDirectoryOthersWriteable()
+            && directory.hasOthersWriteBit()) {
+            std::string error = "Directory \"" + directory.getPath()
+                + "\" is writeable by others";
+            logger.logWarning(error);
+            throw SoftException(error, __FILE__, __LINE__);
+        }
+    } while (directory.getPath() != "/");
+}
+
+
 int main(int argc, char **argv) {
     try {
        API& api = API_Helper::getSystemAPI();

diff --git a/src/Application.hpp b/src/Application.hpp
index be7ed82..0aed9c8 100644 (file)
--- a/src/Application.hpp
+++ b/src/Application.hpp
@@ -107,6 +107,14 @@ namespace suPHP {
                           const Configuration& config) const
            throw (SoftException);
 
+         
+         /**
+          * Checks ownership and permissions for parent directories
+          */
+         void checkParentDirectories(const File& file,
+                                     const UserInfo& owner,
+                                     const Configuration& config) const
+             throw (SoftException);
 
     public:
        /**

diff --git a/src/File.cpp b/src/File.cpp
index 6c6d303..b0b20c2 100644 (file)
--- a/src/File.cpp
+++ b/src/File.cpp
@@ -57,6 +57,9 @@ std::string suPHP::File::getRealPath() const throw (SystemException) {
 File suPHP::File::getParentDirectory() const {
     std::string path = this->getPath();
     path = path.substr(0, path.rfind('/'));
+    if (path.length() == 0) {
+        path = "/";
+    }
     return File(path);
 }
 
@@ -104,3 +107,7 @@ UserInfo suPHP::File::getUser() const throw (SystemException) {
 GroupInfo suPHP::File::getGroup() const throw (SystemException) {
     return API_Helper::getSystemAPI().File_getGroup(*this);
 }
+
+bool suPHP::File::isSymlink() const throw (SystemException) {
+    return API_Helper::getSystemAPI().File_isSymlink(*this);
+}

diff --git a/src/File.hpp b/src/File.hpp
index 3268fb7..cf839e7 100644 (file)
--- a/src/File.hpp
+++ b/src/File.hpp
@@ -143,7 +143,11 @@ namespace suPHP {
         * Returns owning group of file
         */
        GroupInfo getGroup() const throw (SystemException);
-       
+
+        /**
+         * Checks whether this file is a symlink
+         */
+        bool isSymlink() const throw (SystemException);
     };
 };