[Libreoffice-commits] online.git: test/integration-http-server.cpp wsd/LOOLWSD.cpp

[Libreoffice Gerrit user]

 test/integration-http-server.cpp |   30 ++++++++++++++++++++++++++++++
 wsd/LOOLWSD.cpp                  |   22 ++++++++++++++--------
 2 files changed, 44 insertions(+), 8 deletions(-)

New commits:
commit b4b0e9c6d4d649e010e7742efae670ed541f78a2
Author:     Tamás Zolnai <tamas.zolnai at collabora.com>
AuthorDate: Thu Oct 25 10:48:10 2018 +0200
Commit:     Tamás Zolnai <tamas.zolnai at collabora.com>
CommitDate: Thu Oct 25 10:48:52 2018 +0200

    Handle X-Forwarded-For with more secure
    
    Check all pariticipating IPs to be allowed to use convert-to
    functionality. In a simple use case it means the reverse-proxy's
    and the actual client's IP.
    
    Change-Id: I4ef9cb14a1c3003cba6c66f6e99d5b54b2c3b2b8

diff --git a/test/integration-http-server.cpp b/test/integration-http-server.cpp
index ec25653c0..500a9ffdf 100644
--- a/test/integration-http-server.cpp
+++ b/test/integration-http-server.cpp
@@ -424,6 +424,36 @@ void HTTPServerTest::testConvertToWithForwardedClientIP()
             actualString = actualString.substr(3);
         CPPUNIT_ASSERT_EQUAL(expectedStream.str(), actualString); // <- we got the converted file
     }
+
+    // Test a forwarded header with three IPs, one is not allowed -> request is denied.
+    {
+        const std::string srcPath = FileUtil::getTempFilePath(TDOC, "hello.odt", "testConvertToWithForwardedClientIP_");
+        std::unique_ptr<Poco::Net::HTTPClientSession> session(helpers::createSession(_uri));
+        session->setTimeout(Poco::Timespan(2, 0)); // 2 seconds.
+
+        Poco::Net::HTTPRequest request(Poco::Net::HTTPRequest::HTTP_POST, "/lool/convert-to");
+        CPPUNIT_ASSERT(!request.has("X-Forwarded-For"));
+        request.add("X-Forwarded-For", _uri.getHost() + ", "
+                                       + getNotAllowedTestServerURI().getHost() + ", "
+                                       + _uri.getHost());
+        Poco::Net::HTMLForm form;
+        form.setEncoding(Poco::Net::HTMLForm::ENCODING_MULTIPART);
+        form.set("format", "txt");
+        form.addPart("data", new Poco::Net::FilePartSource(srcPath));
+        form.prepareSubmit(request);
+        form.write(session->sendRequest(request));
+
+        Poco::Net::HTTPResponse response;
+        std::stringstream actualStream;
+        std::istream& responseStream = session->receiveResponse(response);
+        Poco::StreamCopier::copyStream(responseStream, actualStream);
+
+        // Remove the temp files.
+        FileUtil::removeFile(srcPath);
+
+        std::string actualString = actualStream.str();
+        CPPUNIT_ASSERT(actualString.empty()); // <- we did not get the converted file
+    }
 }
 
 CPPUNIT_TEST_SUITE_REGISTRATION(HTTPServerTest);
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index d3495e5f3..30bd1b965 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -1942,26 +1942,32 @@ public:
     }
     bool allowConvertTo(const std::string &address, const Poco::Net::HTTPRequest& request)
     {
-        std::string clientAddress = address;
-        std::string clientHost = request.getHost();
+        std::string addressToCheck = address;
+        std::string hostToCheck = request.getHost();
+        bool allow = allowPostFrom(addressToCheck) || StorageBase::allowedWopiHost(hostToCheck);
+
+        // Handle forwarded header and make sure all participating IPs are allowed
         if(request.has("X-Forwarded-For"))
         {
             std::string fowardedData = request.get("X-Forwarded-For");
-            size_t sepPos = fowardedData.find_first_of(',');
-            if(sepPos != std::string::npos)
+            std::vector<std::string> tokens = LOOLProtocol::tokenize(fowardedData, ',');
+            for(std::string& token : tokens)
             {
-                clientAddress = fowardedData.substr(0, sepPos);
+                addressToCheck = Util::trim(token);
                 try
                 {
-                    clientHost = Poco::Net::DNS::resolve(clientAddress).name();
+                    hostToCheck = Poco::Net::DNS::resolve(addressToCheck).name();
+                    allow &= allowPostFrom(addressToCheck) || StorageBase::allowedWopiHost(hostToCheck);
                 }
                 catch (const Poco::Exception& exc)
                 {
-                    LOG_WRN("Poco::Net::DNS::resolve(\"" << clientAddress << "\") failed: " << exc.displayText());
+                    LOG_WRN("Poco::Net::DNS::resolve(\"" << addressToCheck << "\") failed: " << exc.displayText());
+                    // We can't find out the hostname, check the IP only
+                    allow &= allowPostFrom(addressToCheck);
                 }
             }
         }
-        return allowPostFrom(clientAddress) || StorageBase::allowedWopiHost(clientHost);
+        return allow;
     }
 
 private: