From 2a758f9f80a0074585f6a4c603cde9be03502d4e Mon Sep 17 00:00:00 2001 From: Emmanuel Lacour Date: Fri, 11 Apr 2008 09:38:33 +0000 Subject: [PATCH] Add fixes for CVE-2008-1614 --- debian/changelog | 8 +++++ src/API.hpp | 6 ++++ src/API_Linux.cpp | 17 +++++++---- src/API_Linux.hpp | 5 +++ src/Application.cpp | 88 ++++++++++++++++++++++++++++++++++++++--------------- src/Application.hpp | 8 +++++ src/File.cpp | 7 +++++ src/File.hpp | 6 +++- 8 files changed, 114 insertions(+), 31 deletions(-) diff --git a/debian/changelog b/debian/changelog index 7e1a720..c6265db 100644 --- 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 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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); }; }; -- 2.11.0