common/Authorization.cpp | 33 +++------------------------------
common/Util.cpp | 34 +++++++++++++++++++++++++++++++++-
common/Util.hpp | 7 +++++++
test/WhiteBoxTests.cpp | 12 ++++++++++++
4 files changed, 55 insertions(+), 31 deletions(-)
New commits:
commit dbc562d9abc997b196fd6d4e5e71f42d442488d0
Author: Ashod Nakashian <ashod.nakash...@collabora.co.uk>
AuthorDate: Sat Jun 20 11:32:09 2020 -0400
Commit: Ashod Nakashian <a...@collabora.com>
CommitDate: Tue Aug 11 20:04:20 2020 +0200
wsd: parse headers with Poco::MessageHeader
Our header parses was overly simplistic and
didn't support a number of corner cases that
rfc2616 specifies (folding, for example). The
new approach is to simply normalize the headers by
removing invalid line-breaks and then let the
MessageHeader parser take care of parsing the
headers individually, which we then set on the request.
The new utility setHttpHeaders should be used
whenever we need to set a header in an request
to make sure it are sanitized and valid.
Change-Id: Ifa16fa9364f42183316749276c5d0a4c556cb740
Reviewed-on: https://gerrit.libreoffice.org/c/online/+/96371
Tested-by: Jenkins
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoff...@gmail.com>
Reviewed-by: Ashod Nakashian <a...@collabora.com>
diff --git a/common/Authorization.cpp b/common/Authorization.cpp
index 93c5704fc..5613aa91a 100644
--- a/common/Authorization.cpp
+++ b/common/Authorization.cpp
@@ -45,42 +45,15 @@ void
Authorization::authorizeRequest(Poco::Net::HTTPRequest& request) const
switch (_type)
{
case Type::Token:
- request.set("Authorization", "Bearer " + _data);
+ Util::setHttpHeaders(request, "Authorization: Bearer " + _data);
+ assert(request.has("Authorization") && "HTTPRequest missing
Authorization header");
break;
case Type::Header:
- {
// there might be more headers in here; like
// Authorization: Basic ....
// X-Something-Custom: Huh
- // Split based on \n's or \r's and trim, to avoid nonsense in the
- // headers
- StringVector tokens(Util::tokenizeAnyOf(_data, "\n\r"));
- for (auto it = tokens.begin(); it != tokens.end(); ++it)
- {
- std::string token = tokens.getParam(*it);
-
- size_t separator = token.find_first_of(':');
- if (separator != std::string::npos)
- {
- size_t headerStart = token.find_first_not_of(' ', 0);
- size_t headerEnd = token.find_last_not_of(' ', separator -
1);
-
- size_t valueStart = token.find_first_not_of(' ', separator
+ 1);
- size_t valueEnd = token.find_last_not_of(' ');
-
- // set the header
- if (headerStart != std::string::npos && headerEnd !=
std::string::npos &&
- valueStart != std::string::npos && valueEnd !=
std::string::npos)
- {
- size_t headerLength = headerEnd - headerStart + 1;
- size_t valueLength = valueEnd - valueStart + 1;
-
- request.set(token.substr(headerStart, headerLength),
token.substr(valueStart, valueLength));
- }
- }
- }
+ Util::setHttpHeaders(request, _data);
break;
- }
default:
// assert(false);
throw BadRequestException("Invalid HTTP request type");
diff --git a/common/Util.cpp b/common/Util.cpp
index e0ce00250..f1cd61b69 100644
--- a/common/Util.cpp
+++ b/common/Util.cpp
@@ -59,7 +59,7 @@
#include <Poco/Util/Application.h>
#include "Common.hpp"
-#include "Log.hpp"
+#include <common/Log.hpp>
#include "Protocol.hpp"
#include "Util.hpp"
@@ -953,6 +953,38 @@ namespace Util
return std::ctime(&t);
}
+ void setHttpHeaders(Poco::Net::HTTPRequest& request, const std::string&
headers)
+ {
+ // Look for either \r or \n and replace them with a single \r\n
+ // as prescribed by rfc2616 as valid header delimeter, removing
+ // any invalid line breaks, to avoid nonsense in the headers.
+ const StringVector tokens = Util::tokenizeAnyOf(headers, "\n\r");
+ const std::string header = tokens.cat("\r\n", 0);
+ try
+ {
+ // Now parse to preserve folded headers and other
+ // corner cases that is conformant to the rfc,
+ // detecting any errors and/or invalid entries.
+ // NB: request.read() expects full message and will fail.
+ Poco::Net::MessageHeader msgHeader;
+ std::istringstream iss(header);
+ msgHeader.read(iss);
+ for (const auto& entry : msgHeader)
+ {
+ // Set each header entry.
+ request.set(Util::trimmed(entry.first),
Util::trimmed(entry.second));
+ }
+ }
+ catch (const Poco::Exception& ex)
+ {
+ LOG_ERR("Invalid HTTP header [" << header << "]: " <<
ex.displayText());
+ }
+ catch (const std::exception& ex)
+ {
+ LOG_ERR("Invalid HTTP header [" << header << "]: " << ex.what());
+ }
+ }
+
bool isFuzzing()
{
#if LIBFUZZER
diff --git a/common/Util.hpp b/common/Util.hpp
index 9dbfebe8b..bb81cfb75 100644
--- a/common/Util.hpp
+++ b/common/Util.hpp
@@ -35,6 +35,7 @@
#include <Poco/File.h>
#include <Poco/Path.h>
#include <Poco/RegularExpression.h>
+#include <Poco/Net/HTTPRequest.h>
#define LOK_USE_UNSTABLE_API
#include <LibreOfficeKit/LibreOfficeKitEnums.h>
@@ -1082,6 +1083,12 @@ int main(int argc, char**argv)
/// conversion from steady_clock for debugging / tracing
std::string getSteadyClockAsString(const
std::chrono::steady_clock::time_point &time);
+ /// Set the request header by splitting multiple entries by \r or \n.
+ /// Needed to sanitize user-provided http headers, after decoding.
+ /// This is much more tolerant to line breaks than the rfc allows.
+ /// Note: probably should move to a more appropriate home.
+ void setHttpHeaders(Poco::Net::HTTPRequest& request, const std::string&
headers);
+
/// Automatically execute code at end of current scope.
/// Used for exception-safe code.
class ScopeGuard
diff --git a/test/WhiteBoxTests.cpp b/test/WhiteBoxTests.cpp
index 13a4ac572..1982f328a 100644
--- a/test/WhiteBoxTests.cpp
+++ b/test/WhiteBoxTests.cpp
@@ -487,6 +487,18 @@ void WhiteBoxTests::testTokenizerTokenizeAnyOf()
LOK_ASSERT_EQUAL(static_cast<size_t>(2), tokens.size());
LOK_ASSERT_EQUAL(std::string("A"), tokens[0]);
LOK_ASSERT_EQUAL(std::string("Z"), tokens[1]);
+
+ tokens =
Util::tokenizeAnyOf(std::string("A\rB\nC\n\rD\r\nE\r\rF\n\nG\r\r\n\nH"),
+ delimiters);
+ LOK_ASSERT_EQUAL(static_cast<std::size_t>(8), tokens.size());
+ LOK_ASSERT_EQUAL(std::string("A"), tokens[0]);
+ LOK_ASSERT_EQUAL(std::string("B"), tokens[1]);
+ LOK_ASSERT_EQUAL(std::string("C"), tokens[2]);
+ LOK_ASSERT_EQUAL(std::string("D"), tokens[3]);
+ LOK_ASSERT_EQUAL(std::string("E"), tokens[4]);
+ LOK_ASSERT_EQUAL(std::string("F"), tokens[5]);
+ LOK_ASSERT_EQUAL(std::string("G"), tokens[6]);
+ LOK_ASSERT_EQUAL(std::string("H"), tokens[7]);
}
void WhiteBoxTests::testReplace()
_______________________________________________
Libreoffice-commits mailing list
libreoffice-comm...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
