Make cookie auth check all headers
Currently, the Cookie auth only checks the first cookie header in a
request. This works fine for most things, because a lot of
implementations (browsers) seem to either put the Cookie headers in
alphabetical order, or put them in the order in which they were stored
which in the case of bmcweb, is also alphabetical.
Well, http2 blows this up, because cookies could potentially be in any
order, given the hpack compression techniques, so there's no promise
that Cookie[0] is the Session cookie.
This commit reworks the authentication code to call beasts "equal_range"
getter, which returns the range of all headers that matched. This
allows us to attempt to parse the cookies in whatever order they might
have been received.
The auth routine only tries to log in the first cookie matching
SESSION=, and do not try to handle duplicates, as this might allow
attackers to negate the anti brute force measures by testing multiple
passwords at once
Tested:
With http2 enabled, the UI can now log in more consistently, and in
addition, the HTML redfish pages function more consistently when using
cookie auth.
Redfish service validator passes.
Change-Id: I3a61a5a654f62096ff19cfbfaf0a10f30a1a3605
Signed-off-by: Ed Tanous <ed@tanous.net>
diff --git a/include/authentication.hpp b/include/authentication.hpp
index fbc2267..ad9759b 100644
--- a/include/authentication.hpp
+++ b/include/authentication.hpp
@@ -126,60 +126,65 @@
performCookieAuth(boost::beast::http::verb method [[maybe_unused]],
const boost::beast::http::header<true>& reqHeader)
{
- BMCWEB_LOG_DEBUG("[AuthMiddleware] Cookie authentication");
+ using headers = boost::beast::http::header<true>;
+ std::pair<headers::const_iterator, headers::const_iterator> cookies =
+ reqHeader.equal_range(boost::beast::http::field::cookie);
- std::string_view cookieValue = reqHeader["Cookie"];
- if (cookieValue.empty())
+ for (auto it = cookies.first; it != cookies.second; it++)
{
- return nullptr;
- }
+ std::string_view cookieValue = it->value();
+ BMCWEB_LOG_DEBUG("Checking cookie {}", cookieValue);
+ auto startIndex = cookieValue.find("SESSION=");
+ if (startIndex == std::string::npos)
+ {
+ BMCWEB_LOG_DEBUG(
+ "Cookie was present, but didn't look like a session {}",
+ cookieValue);
+ continue;
+ }
+ startIndex += sizeof("SESSION=") - 1;
+ auto endIndex = cookieValue.find(';', startIndex);
+ if (endIndex == std::string::npos)
+ {
+ endIndex = cookieValue.size();
+ }
+ std::string_view authKey = cookieValue.substr(startIndex,
+ endIndex - startIndex);
- auto startIndex = cookieValue.find("SESSION=");
- if (startIndex == std::string::npos)
- {
- return nullptr;
- }
- startIndex += sizeof("SESSION=") - 1;
- auto endIndex = cookieValue.find(';', startIndex);
- if (endIndex == std::string::npos)
- {
- endIndex = cookieValue.size();
- }
- std::string_view authKey = cookieValue.substr(startIndex,
- endIndex - startIndex);
-
- std::shared_ptr<persistent_data::UserSession> sessionOut =
- persistent_data::SessionStore::getInstance().loginSessionByToken(
- authKey);
- if (sessionOut == nullptr)
- {
- return nullptr;
- }
- sessionOut->cookieAuth = true;
+ std::shared_ptr<persistent_data::UserSession> sessionOut =
+ persistent_data::SessionStore::getInstance().loginSessionByToken(
+ authKey);
+ if (sessionOut == nullptr)
+ {
+ return nullptr;
+ }
+ sessionOut->cookieAuth = true;
#ifndef BMCWEB_INSECURE_DISABLE_CSRF_PREVENTION
- // RFC7231 defines methods that need csrf protection
- if (method != boost::beast::http::verb::get)
- {
- std::string_view csrf = reqHeader["X-XSRF-TOKEN"];
- // Make sure both tokens are filled
- if (csrf.empty() || sessionOut->csrfToken.empty())
+ // RFC7231 defines methods that need csrf protection
+ if (method != boost::beast::http::verb::get)
{
- return nullptr;
- }
+ std::string_view csrf = reqHeader["X-XSRF-TOKEN"];
+ // Make sure both tokens are filled
+ if (csrf.empty() || sessionOut->csrfToken.empty())
+ {
+ return nullptr;
+ }
- if (csrf.size() != persistent_data::sessionTokenSize)
- {
- return nullptr;
+ if (csrf.size() != persistent_data::sessionTokenSize)
+ {
+ return nullptr;
+ }
+ // Reject if csrf token not available
+ if (!crow::utility::constantTimeStringCompare(
+ csrf, sessionOut->csrfToken))
+ {
+ return nullptr;
+ }
}
- // Reject if csrf token not available
- if (!crow::utility::constantTimeStringCompare(csrf,
- sessionOut->csrfToken))
- {
- return nullptr;
- }
- }
#endif
- return sessionOut;
+ return sessionOut;
+ }
+ return nullptr;
}
#endif