[Libreoffice-commits] online.git: loolwsd/DocumentBroker.cpp loolwsd/LOOLWSD.cpp loolwsd/LOOLWSD.hpp loolwsd/loolwsd.xml loolwsd/Storage.hpp

[Ashod Nakashian]

 loolwsd/DocumentBroker.cpp |    5 ++++-
 loolwsd/LOOLWSD.cpp        |   11 +++++++++++
 loolwsd/LOOLWSD.hpp        |    1 +
 loolwsd/Storage.hpp        |   12 ++++++++++++
 loolwsd/loolwsd.xml        |    1 +
 5 files changed, 29 insertions(+), 1 deletion(-)

New commits:
commit 9485b4fe636758732efbd45f0abde974be472a1d
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Thu Apr 7 00:32:28 2016 -0400

    loolwsd: disable loading of local docs by default
    
    Loading documents from the local filesystem
    opens the door to security issues.
    
    By default filesystem storage is disabled,
    even if enabled in the config file. The
    only way to enable it is to set the
    allowlocalstorage command-line argument.
    
    Change-Id: Ib8f57377260817436d101a16757aab38276cbdcd
    Reviewed-on: https://gerrit.libreoffice.org/23881
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>

diff --git a/loolwsd/DocumentBroker.cpp b/loolwsd/DocumentBroker.cpp
index 393ba9a..90f60bc 100644
--- a/loolwsd/DocumentBroker.cpp
+++ b/loolwsd/DocumentBroker.cpp
@@ -82,7 +82,10 @@ void DocumentBroker::validate(const Poco::URI& uri)
 {
     Log::info("Validating: " + uri.toString());
     auto storage = createStorage("", "", uri);
-    storage->getFileInfo(uri);
+    if (storage == nullptr || !storage->getFileInfo(uri).isValid())
+    {
+        throw std::runtime_error("Invalid URI or access denied.");
+    }
 }
 
 bool DocumentBroker::load(const std::string& jailId)
diff --git a/loolwsd/LOOLWSD.cpp b/loolwsd/LOOLWSD.cpp
index 46860d0..47c3d62 100644
--- a/loolwsd/LOOLWSD.cpp
+++ b/loolwsd/LOOLWSD.cpp
@@ -964,6 +964,7 @@ std::string LOOLWSD::ChildRoot;
 std::string LOOLWSD::LoSubPath = "lo";
 std::string LOOLWSD::FileServerRoot;
 std::string LOOLWSD::AdminCreds;
+bool LOOLWSD::AllowLocalStorage = false;
 
 unsigned int LOOLWSD::NumPreSpawnedChildren = 0;
 bool LOOLWSD::DoTest = false;
@@ -1033,6 +1034,10 @@ void LOOLWSD::initialize(Application& self)
         NumPreSpawnedChildren = config().getUInt("num_prespawn_children", 10);
     }
 
+    // This overrides whatever is in the config file,
+    // which forces admins to set this flag on the command-line.
+    config().setBool("storage.filesystem[@allow]", AllowLocalStorage);
+
     ServerApplication::initialize(self);
 }
 
@@ -1138,6 +1143,10 @@ void LOOLWSD::defineOptions(OptionSet& optionSet)
                         .repeatable(false)
                         .argument("directory"));
 
+    optionSet.addOption(Option("allowlocalstorage", "", "When true will allow highly insecure loading of files from local storage.")
+                        .required(false)
+                        .repeatable(false));
+
     optionSet.addOption(Option("test", "", "Interactive testing.")
                         .required(false)
                         .repeatable(false));
@@ -1175,6 +1184,8 @@ void LOOLWSD::handleOption(const std::string& optionName, const std::string& val
         NumPreSpawnedChildren = std::stoi(value);
     else if (optionName == "admincreds")
         AdminCreds = value;
+    else if (optionName == "allowlocalstorage")
+        AllowLocalStorage = true;
     else if (optionName == "test")
         LOOLWSD::DoTest = true;
 }
diff --git a/loolwsd/LOOLWSD.hpp b/loolwsd/LOOLWSD.hpp
index ea87f50..6cd8c80 100644
--- a/loolwsd/LOOLWSD.hpp
+++ b/loolwsd/LOOLWSD.hpp
@@ -46,6 +46,7 @@ public:
     static std::string LoSubPath;
     static std::string FileServerRoot;
     static std::string AdminCreds;
+    static bool AllowLocalStorage;
 
     static
     std::string GenSessionId()
diff --git a/loolwsd/Storage.hpp b/loolwsd/Storage.hpp
index 9e46f3d..24023a2 100644
--- a/loolwsd/Storage.hpp
+++ b/loolwsd/Storage.hpp
@@ -13,6 +13,7 @@
 
 #include <string>
 
+#include <Poco/Util/Application.h>
 #include <Poco/URI.h>
 
 #include "Auth.hpp"
@@ -26,6 +27,11 @@ public:
     class FileInfo
     {
     public:
+        bool isValid() const
+        {
+            return !Filename.empty() && Size > 0;
+        }
+
         std::string Filename;
         Poco::Timestamp ModifiedTime;
         size_t Size;
@@ -139,6 +145,12 @@ std::unique_ptr<StorageBase> createStorage(const std::string& jailRoot, const st
 {
     if (uri.isRelative() || uri.getScheme() == "file")
     {
+        if (!Poco::Util::Application::instance().config().getBool("storage.filesystem[@allow]", false))
+        {
+            Log::error("Local Storage is disabled by default. Specify allowlocalstorage on the command-line to enable.");
+            return nullptr;
+        }
+
         Log::info("Public URI [" + uri.toString() + "] is a file.");
         return std::unique_ptr<StorageBase>(new LocalStorage(jailRoot, jailPath, uri.getPath()));
     }
diff --git a/loolwsd/loolwsd.xml b/loolwsd/loolwsd.xml
index 7d356b1..dfa909c 100644
--- a/loolwsd/loolwsd.xml
+++ b/loolwsd/loolwsd.xml
@@ -20,6 +20,7 @@
     </ssl>
 
     <storage desc="Backend storage">
+        <filesystem allow="false" />
         <wopi desc="Allow/deny wopi storage. Mutually exclusive with webdav." allow="true">
             <host desc="Hostname to allow">localhost</host>
             <max_file_size desc="Maximum document size in bytes to load. 0 for unlimited." type="uint">0</max_file_size>