Skip to content

Commit

Permalink
Implement constant time string compare for token
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
edtanous authored and feistjj committed Nov 18, 2019
1 parent d500549 commit 51dae67
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 7 deletions.
22 changes: 22 additions & 0 deletions http/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#include "nlohmann/json.hpp"

#include <openssl/crypto.h>

#include <cstdint>
#include <cstring>
#include <functional>
Expand Down Expand Up @@ -779,5 +781,25 @@ inline std::string dateTimeNow()
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
21 changes: 15 additions & 6 deletions include/sessions.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,19 @@
#include <sdbusplus/bus/match.hpp>

#include "logging.h"
#include "utility.h"

namespace crow
{

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
Expand Down Expand Up @@ -399,19 +405,16 @@ class SessionStore
'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)
{
sessionToken[i] = alphanum[dist(rd)];
}
// 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)];
Expand All @@ -437,6 +440,10 @@ class SessionStore
loginSessionByToken(const std::string_view token)
{
applySessionTimeouts();
if (token.size() != sessionTokenSize)
{
return nullptr;
}
auto sessionIt = authTokens.find(std::string(token));
if (sessionIt == authTokens.end())
{
Expand Down Expand Up @@ -549,7 +556,9 @@ class SessionStore
}

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};
Expand Down
8 changes: 7 additions & 1 deletion include/token_authorization_middleware.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,14 @@ class Middleware
{
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;
}
Expand Down

0 comments on commit 51dae67

Please sign in to comment.