Implement constant time string compare for token
The sessions implementation previously used operator== for session
comparisons. While unlikely to be attackable in the current
implementation, due to the time smearing in a number of cases, modern
security practices recommend using constant time comparison.
Tested By:
Logged into the webui, and observed no change to login flows. Logged
into redfish using Token Auth, and observed no changes. Closed a
previous session, then reopened with the new session information to
verify user sessions are restored properly and still work.
Change-Id: Ie759e4da67ba004fd8c327f177951ac756ea6799
Signed-off-by: Ed Tanous <ed.tanous@intel.com>
Signed-off-by: James Feist <james.feist@linux.intel.com>
diff --git a/http/utility.h b/http/utility.h
index 3ea5806..ee88e5a 100644
--- a/http/utility.h
+++ b/http/utility.h
@@ -2,6 +2,8 @@
#include "nlohmann/json.hpp"
+#include <openssl/crypto.h>
+
#include <cstdint>
#include <cstring>
#include <functional>
@@ -779,5 +781,25 @@
return getDateTime(time);
}
+inline bool constantTimeStringCompare(const std::string_view a,
+ const std::string_view b)
+{
+ // Important note, this function is ONLY constant time if the two input
+ // sizes are the same
+ if (a.size() != b.size())
+ {
+ return false;
+ }
+ return CRYPTO_memcmp(a.data(), b.data(), a.size()) == 0;
+}
+
+struct ConstantTimeCompare
+{
+ bool operator()(const std::string_view a, const std::string_view b) const
+ {
+ return constantTimeStringCompare(a, b);
+ }
+};
+
} // namespace utility
} // namespace crow
diff --git a/include/sessions.hpp b/include/sessions.hpp
index f58c676..6e74f25 100644
--- a/include/sessions.hpp
+++ b/include/sessions.hpp
@@ -11,6 +11,7 @@
#include <sdbusplus/bus/match.hpp>
#include "logging.h"
+#include "utility.h"
namespace crow
{
@@ -18,6 +19,11 @@
namespace persistent_data
{
+// entropy: 20 characters, 62 possibilities. log2(62^20) = 119 bits of
+// entropy. OWASP recommends at least 64
+// https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#session-id-entropy
+constexpr std::size_t sessionTokenSize = 20;
+
enum class PersistenceType
{
TIMEOUT, // User session times out after a predetermined amount of time
@@ -399,11 +405,8 @@
'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p',
'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'};
- // entropy: 30 characters, 62 possibilities. log2(62^30) = 178 bits of
- // entropy. OWASP recommends at least 60
- // https://www.owasp.org/index.php/Session_Management_Cheat_Sheet#Session_ID_Entropy
std::string sessionToken;
- sessionToken.resize(20, '0');
+ sessionToken.resize(sessionTokenSize, '0');
std::uniform_int_distribution<size_t> dist(0, alphanum.size() - 1);
for (size_t i = 0; i < sessionToken.size(); ++i)
{
@@ -411,7 +414,7 @@
}
// Only need csrf tokens for cookie based auth, token doesn't matter
std::string csrfToken;
- csrfToken.resize(20, '0');
+ csrfToken.resize(sessionTokenSize, '0');
for (size_t i = 0; i < csrfToken.size(); ++i)
{
csrfToken[i] = alphanum[dist(rd)];
@@ -437,6 +440,10 @@
loginSessionByToken(const std::string_view token)
{
applySessionTimeouts();
+ if (token.size() != sessionTokenSize)
+ {
+ return nullptr;
+ }
auto sessionIt = authTokens.find(std::string(token));
if (sessionIt == authTokens.end())
{
@@ -549,7 +556,9 @@
}
std::chrono::time_point<std::chrono::steady_clock> lastTimeoutUpdate;
- boost::container::flat_map<std::string, std::shared_ptr<UserSession>>
+ std::unordered_map<std::string, std::shared_ptr<UserSession>,
+ std::hash<std::string>,
+ crow::utility::ConstantTimeCompare>
authTokens;
std::random_device rd;
bool needWrite{false};
diff --git a/include/token_authorization_middleware.hpp b/include/token_authorization_middleware.hpp
index 7e4e3bb..e9eb65f 100644
--- a/include/token_authorization_middleware.hpp
+++ b/include/token_authorization_middleware.hpp
@@ -223,8 +223,14 @@
{
return nullptr;
}
+
+ if (csrf.size() != crow::persistent_data::sessionTokenSize)
+ {
+ return nullptr;
+ }
// Reject if csrf token not available
- if (csrf != session->csrfToken)
+ if (!crow::utility::constantTimeStringCompare(csrf,
+ session->csrfToken))
{
return nullptr;
}