[Libreoffice-commits] online.git: loolwsd/Capabilities.hpp loolwsd/debian loolwsd/LOOLWSD.cpp loolwsd/loolwsd.spec.in loolwsd/Makefile.am loolwsd/PROBLEMS

Tor Lillqvist tml at collabora.com
Mon Feb 29 12:14:19 UTC 2016


 loolwsd/Capabilities.hpp        |   32 +-------------------------------
 loolwsd/LOOLWSD.cpp             |   28 +++++++---------------------
 loolwsd/Makefile.am             |    2 --
 loolwsd/PROBLEMS                |   14 --------------
 loolwsd/debian/loolwsd.postinst |    1 -
 loolwsd/loolwsd.spec.in         |    1 -
 6 files changed, 8 insertions(+), 70 deletions(-)

New commits:
commit 29a3f58f1ac9e0d3bf57247c26a127fa0e87f831
Author: Tor Lillqvist <tml at collabora.com>
Date:   Mon Feb 29 14:06:48 2016 +0200

    The loolwsd program does not need any capabilities
    
    So don't give it any then.
    
    Remove the --uid option and related attempts to handle running loolwsd
    under sudo, to be able to debug it. Now with loolwsd not having
    capabilities, it should work fine to just run it under a debugger
    normally. (For the loolbroker and loolkit processes, attaching to an
    already started process is the way to debug.)

diff --git a/loolwsd/Capabilities.hpp b/loolwsd/Capabilities.hpp
index 6c59c21..8fcd9ba 100644
--- a/loolwsd/Capabilities.hpp
+++ b/loolwsd/Capabilities.hpp
@@ -16,13 +16,6 @@
 
 #include "Util.hpp"
 
-#if ENABLE_DEBUG
-#include <sys/types.h>
-#include <pwd.h>
-
-static int uid = 0;
-#endif
-
 static
 void dropCapability(
 #ifdef __linux
@@ -63,7 +56,7 @@ void dropCapability(
     cap_free(capText);
 
     cap_free(caps);
-#endif
+#else
     // We assume that on non-Linux we don't need to be root to be able to hardlink to files we
     // don't own, so drop root.
     if (geteuid() == 0 && getuid() != 0)
@@ -76,29 +69,6 @@ void dropCapability(
             Log::error("Error: setuid() failed.");
         }
     }
-#if ENABLE_DEBUG
-    if (geteuid() == 0 && getuid() == 0)
-    {
-#ifdef __linux
-        // Argh, awful hack
-        if (capability == CAP_FOWNER)
-            return;
-#endif
-
-        // Running under sudo, probably because being debugged? Let's drop super-user rights.
-        if (uid == 0)
-        {
-            struct passwd *nobody = getpwnam("nobody");
-            if (nobody)
-                uid = nobody->pw_uid;
-            else
-                uid = 65534;
-        }
-        if (setuid(uid) != 0)
-        {
-            Log::error("setuid() failed.");
-        }
-    }
 #endif
 }
 
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 0a7721b..6649f41 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -98,7 +98,6 @@ DEALINGS IN THE SOFTWARE.
 
 #include "Admin.hpp"
 #include "Auth.hpp"
-#include "Capabilities.hpp"
 #include "ChildProcessSession.hpp"
 #include "Common.hpp"
 #include "LOOLProtocol.hpp"
@@ -839,13 +838,6 @@ void LOOLWSD::defineOptions(OptionSet& optionSet)
     optionSet.addOption(Option("test", "", "Interactive testing.")
                         .required(false)
                         .repeatable(false));
-
-#if ENABLE_DEBUG
-    optionSet.addOption(Option("uid", "", "Uid to assume if running under sudo for debugging purposes.")
-                        .required(false)
-                        .repeatable(false)
-                        .argument("uid"));
-#endif
 }
 
 void LOOLWSD::handleOption(const std::string& optionName, const std::string& value)
@@ -878,10 +870,6 @@ void LOOLWSD::handleOption(const std::string& optionName, const std::string& val
         NumPreSpawnedChildren = std::stoi(value);
     else if (optionName == "test")
         LOOLWSD::DoTest = true;
-#if ENABLE_DEBUG
-    else if (optionName == "uid")
-        uid = std::stoull(value);
-#endif
 }
 
 void LOOLWSD::displayHelp()
@@ -922,7 +910,13 @@ Process::PID LOOLWSD::createBroker()
 int LOOLWSD::main(const std::vector<std::string>& /*args*/)
 {
     Log::initialize("wsd");
-
+#ifdef __linux
+    if (geteuid() == 0)
+    {
+        Log::error("Don't run this as root");
+        return Application::EXIT_USAGE;
+    }
+#endif
     //Environment::set("LOK_PREINIT", "1");
     //Environment::set("LOK_FORK", "1");
     //Environment::set("LD_BIND_NOW", "1");
@@ -992,14 +986,6 @@ int LOOLWSD::main(const std::vector<std::string>& /*args*/)
         return Application::EXIT_SOFTWARE;
     }
 
-#ifdef __linux
-    dropCapability(CAP_SYS_CHROOT);
-    dropCapability(CAP_MKNOD);
-    dropCapability(CAP_FOWNER);
-#else
-    dropCapability();
-#endif
-
     // Configure the Server.
     // Note: TCPServer internally uses the default
     // ThreadPool to dispatch connections.
diff --git a/loolwsd/Makefile.am b/loolwsd/Makefile.am
index a8674de..802876a 100644
--- a/loolwsd/Makefile.am
+++ b/loolwsd/Makefile.am
@@ -45,11 +45,9 @@ clean-cache:
 all-local: loolwsd loolbroker
 	if test "$$BUILDING_FROM_RPMBUILD" != yes; then \
 	    if test `uname -s` = Linux; then \
-		sudo @SETCAP@ cap_fowner,cap_mknod,cap_sys_chroot=ep loolwsd; \
 		sudo @SETCAP@ cap_fowner,cap_mknod,cap_chown,cap_sys_chroot=ep loolbroker; \
 		sudo @SETCAP@ cap_fowner,cap_mknod,cap_chown,cap_sys_chroot=ep loolkit; \
 	    else \
-		sudo chown root loolwsd && sudo chmod u+s loolwsd; \
 		sudo chown root loolbroker && sudo chmod u+s loolbroker; \
 		sudo chown root loolbroker && sudo chmod u+s loolkit; \
 	    fi; \
diff --git a/loolwsd/PROBLEMS b/loolwsd/PROBLEMS
index 17bdab6..38fc312 100644
--- a/loolwsd/PROBLEMS
+++ b/loolwsd/PROBLEMS
@@ -44,17 +44,3 @@
 - Occasionally Control-C (SIGINT) doesn't shut fown loolwsd. One has
   to kill it with SIGKILL. Which of course leaves all the chroot jails
   around.
-
-- I don't think the loolwsd program actually any longer does anything
-  that would need any capability. Still it is given cap_fowner,
-  cap_mknod, and cap_sys_chroot in Makefile.am (and the Debian and RPM
-  packaging), and it calls dropCapability() to drop those (without
-  having used them for anything). It would obviously be better to not
-  give it those capabilities in the first place. Unfortunately this is
-  a bit complicated because the dropCapability() function also does a
-  bit of a dance around the possibility that somebody is running
-  loolwsd under sudo. Does anybody actually do that? It seems like a
-  very bad idea. (Yes, I know I added that code myself, as a debugging
-  aid, but I dare not drop it now in case it is somebody's "normal"
-  method of working when debugging this, or even when not debugging.)
-  Also, the Capabilities.hpp defines a *static* variable uid...
diff --git a/loolwsd/debian/loolwsd.postinst b/loolwsd/debian/loolwsd.postinst
index ae4bb9e..909332a 100755
--- a/loolwsd/debian/loolwsd.postinst
+++ b/loolwsd/debian/loolwsd.postinst
@@ -4,7 +4,6 @@ set -e
 
 case "$1" in
     configure)
-	setcap cap_fowner,cap_mknod,cap_sys_chroot=ep /usr/bin/loolwsd || true
 	setcap cap_fowner,cap_mknod,cap_chown,cap_sys_chroot=ep /usr/bin/loolkit || true
 	setcap cap_fowner,cap_mknod,cap_chown,cap_sys_chroot=ep /usr/bin/loolbroker || true
 
diff --git a/loolwsd/loolwsd.spec.in b/loolwsd/loolwsd.spec.in
index c2dce99..54ddf17 100644
--- a/loolwsd/loolwsd.spec.in
+++ b/loolwsd/loolwsd.spec.in
@@ -69,7 +69,6 @@ echo "0 0 */1 * * root find /var/cache/loolwsd -name \"*.png\" -a -atime +10 -ex
 %service_add_pre loolwsd.service
 
 %post
-setcap cap_fowner,cap_mknod,cap_sys_chroot=ep /usr/bin/loolwsd
 setcap cap_fowner,cap_mknod,cap_chown,cap_sys_chroot=ep /usr/bin/loolbroker
 setcap cap_fowner,cap_mknod,cap_chown,cap_sys_chroot=ep /usr/bin/loolkit
 


More information about the Libreoffice-commits mailing list