[cpp-sp] branch main updated: Validate session data format in SPI.
Scott Cantor
cantor.2 at osu.edu
Wed Jun 11 14:59:58 UTC 2025
This is an automated email from the git hooks/post-receive script.
scantor pushed a commit to branch main
in repository cpp-sp.
View the commit online:
http://git.shibboleth.net/view/?p=cpp-sp.git;a=commit;h=44a3e8f377d876989b099425e8571ad7669151ce
The following commit(s) were added to refs/heads/main by this push:
new 44a3e8f3 Validate session data format in SPI.
44a3e8f3 is described below
commit 44a3e8f377d876989b099425e8571ad7669151ce
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Wed Jun 11 10:59:48 2025 -0400
Validate session data format in SPI.
---
shibsp/session/AbstractSessionCache.h | 17 ++++++
shibsp/session/impl/AbstractSessionCache.cpp | 83 +++++++++++++++++++++++++-
shibsp/session/impl/FilesystemSessionCache.cpp | 45 +++++++-------
3 files changed, 119 insertions(+), 26 deletions(-)
diff --git a/shibsp/session/AbstractSessionCache.h b/shibsp/session/AbstractSessionCache.h
index 65af298e..e6bd98db 100644
--- a/shibsp/session/AbstractSessionCache.h
+++ b/shibsp/session/AbstractSessionCache.h
@@ -114,6 +114,23 @@ namespace shibsp {
*/
Category& log() const;
+ /**
+ * Access the shutdown state.
+ */
+ bool isShutdown() const;
+
+ /**
+ * Tests the validity of a session's data structures.
+ *
+ * <p>Note that this is not a policy evaluation of the data in the session but only
+ * of the structure/content to fit the underlying assumptions built into the code.</p>
+ *
+ * @param sessionData the data to examine
+ *
+ * @return true iff the data is valid
+ */
+ static bool isSessionDataValid(DDF& sessionData);
+
private:
static void* cleanup_fn(void*);
void dormant(const std::string& key);
diff --git a/shibsp/session/impl/AbstractSessionCache.cpp b/shibsp/session/impl/AbstractSessionCache.cpp
index 86e94c6c..c30e9268 100644
--- a/shibsp/session/impl/AbstractSessionCache.cpp
+++ b/shibsp/session/impl/AbstractSessionCache.cpp
@@ -101,6 +101,86 @@ SessionCacheSPI::~SessionCacheSPI()
{
}
+bool AbstractSessionCache::isSessionDataValid(DDF& sessionData)
+{
+ // Must be a structure
+ // Must have a non-empty string "app_id" member
+ // Must have a positive longinteger "ts" member
+
+ const char* appId = sessionData["app_id"].string();
+ if (!appId || !*appId) {
+ return false;
+ }
+
+ if (sessionData["ts"].longinteger() < 0) {
+ return false;
+ }
+
+ // Must have a non-empty list "attributes" member with the session's
+ // data. Each attribute must have a non-null name and at least one
+ // non-null string value, and no other value types.
+
+ DDF attrs = sessionData["attributes"];
+ if (!attrs.islist() || attrs.integer() == 0) {
+ return false;
+ }
+
+ DDF attr = attrs.first();
+
+ // If the values aren't in a list, bail.
+ if (!attr.islist()) {
+ return false;
+ }
+
+ // Runs loop for each "value collection", i.e. each attribute.
+ do {
+ // No values?
+ if (attr.integer() == 0) {
+ return false;
+ }
+
+ // Empty/null name?
+ const char* name = attr.name();
+ if (!name || !*name) {
+ return false;
+ }
+
+ DDF val = attr.first();
+
+ // Value not a string?
+ if (!val.isstring()) {
+ return false;
+ }
+
+ // Run loop for each value.
+ do {
+ // Value empty/null?
+ const char* s = val.string();
+ if (!s || !*s) {
+ return false;
+ }
+
+ // Get next value.
+ val = attr.next();
+ } while (val.isstring());
+
+ // A string would continue the loop, so if not null now, bail.
+ if (!val.isnull()) {
+ return false;
+ }
+
+ // Get next attribute.
+ attr = attrs.next();
+ } while (attr.islist());
+
+ // A list would continue the loop, so if not null now, bail.
+ if (!attr.isnull()) {
+ return false;
+ }
+
+ return true;
+}
+
AbstractSessionCache::AbstractSessionCache(const ptree& pt)
: m_log(Category::getInstance(SHIBSP_LOGCAT ".SessionCache")), m_shutdown(false)
{
@@ -545,12 +625,11 @@ bool BasicSession::isValid(SPRequest* request, unsigned int lifetime, unsigned i
if (!timeout || m_lastAccess + timeout > now) {
- // Being locally valid, we want to update the activity timestamp remotely and in the persistent store.
+ // Being locally valid, we want to update the activity timestamp in the persistent store.
// This check also notices a session having been revoked, so implements the concept of the cache being
// "eventually consistent" across agent processes.
if (m_lastAccess - m_lastAccessReported > m_cache.m_storageAccessInterval) {
- // It's been X seconds since we last wrote through to storage...
try {
// Pass a zero to bypass timeout enforcement as we know as well or better than the back-end...
if (!m_cache.cache_touch(request, getID(), 0)) {
diff --git a/shibsp/session/impl/FilesystemSessionCache.cpp b/shibsp/session/impl/FilesystemSessionCache.cpp
index 056591e2..7adbe7e4 100644
--- a/shibsp/session/impl/FilesystemSessionCache.cpp
+++ b/shibsp/session/impl/FilesystemSessionCache.cpp
@@ -32,6 +32,7 @@
#include <fstream>
#ifdef WIN32
+# define _utime utime
# include <sys/utime.h>
#else
# include <utime.h>
@@ -176,7 +177,7 @@ DDF FilesystemSessionCache::cache_read(
if (timeout) {
if (lastAccess == 0) {
- m_spilog.error("timeout specified, but unable to obtain mod time for file (%s)", path.c_str());
+ m_spilog.error("timeout specified, unable to obtain access time for session file (%s)", path.c_str());
return obj;
}
else if (lastAccess + timeout < now) {
@@ -191,10 +192,8 @@ DDF FilesystemSessionCache::cache_read(
is >> obj;
is.close();
- if (obj.isnull()) {
- m_spilog.error("error deserializing session from file (%s)", path.c_str());
- return obj;
- } else if (!obj.isstruct()) {
+
+ if (!isSessionDataValid(obj)) {
obj.destroy();
m_spilog.error("deserialized session from file (%s) was invalid", path.c_str());
return obj;
@@ -202,8 +201,8 @@ DDF FilesystemSessionCache::cache_read(
const char* appId = obj["appId"].string();
if (strcmp(applicationId, appId)) {
- m_spilog.warn("session (%s) issued for application (%s), accessed via application (%s)", key, appId, applicationId);
obj.destroy();
+ m_spilog.warn("session (%s) issued for application (%s), accessed via application (%s)", key, appId, applicationId);
return obj;
}
@@ -211,8 +210,8 @@ DDF FilesystemSessionCache::cache_read(
if (client_addr) {
const char* addr = obj["addr"].string();
if (addr && strcmp(client_addr, addr)) {
- m_spilog.warn("session (%s) invalid, bound to address (%s), accessed from (%s)", key, addr, client_addr);
obj.destroy();
+ m_spilog.info("session (%s) invalid, bound to address (%s), accessed from (%s)", key, addr, client_addr);
cache_remove(request, key);
return obj;
}
@@ -222,7 +221,7 @@ DDF FilesystemSessionCache::cache_read(
time_t start = obj["ts"].longinteger();
if (start + lifetime < now) {
obj.destroy();
- if (m_spilog.isWarnEnabled()) {
+ if (m_spilog.isInfoEnabled()) {
string created(date::format("%FT%TZ", chrono::system_clock::from_time_t(start)));
string expired(date::format("%FT%TZ", chrono::system_clock::from_time_t(start + lifetime)));
m_spilog.info("session (%s) has expired, created (%s), expired (%s)", key, created.c_str(), expired.c_str());
@@ -232,6 +231,10 @@ DDF FilesystemSessionCache::cache_read(
}
}
+ if (utime(path.c_str(), nullptr) != 0) {
+ m_spilog.error("unable to update access time for session (%s), errno=%d", path.c_str(), errno);
+ }
+
return obj;
}
@@ -241,8 +244,13 @@ bool FilesystemSessionCache::cache_touch(SPRequest* request, const char* key, un
if (timeout) {
time_t lastAccess = FileSupport::getModificationTime(path.c_str());
if (lastAccess == 0) {
- m_spilog.error("timeout specified, but unable to obtain mod time for file (%s)", path.c_str());
- cache_remove(request, key);
+ int e = errno;
+ if (e == ENOENT) {
+ m_spilog.info("unable to update access time, session file (%s) did not exist", path.c_str());
+ }
+ else {
+ m_spilog.error("unable to obtain access time for session file (%s), errno=%d", path.c_str(), e);
+ }
return false;
}
else if (lastAccess + timeout < time(nullptr)) {
@@ -255,20 +263,9 @@ bool FilesystemSessionCache::cache_touch(SPRequest* request, const char* key, un
}
}
-#ifdef WIN32
- if (_utime(path.c_str(), nullptr) != 0) {
-#else
if (utime(path.c_str(), nullptr) != 0) {
-#endif
- int e = errno;
- if (e == ENOENT) {
- m_spilog.debug("unable to update access time, session file (%s) did not exist", path.c_str());
- return false;
- }
- else {
- m_spilog.error("unable to update access time for session (%s), errno=%d", path.c_str(), e);
- // Debatable if we fall into returning true, but maybe we don't care?
- }
+ m_spilog.error("unable to update access time for session (%s), errno=%d", path.c_str(), errno);
+ // Debatable if we fall into returning true, but maybe we don't care?
}
return true;
}
@@ -286,6 +283,6 @@ void FilesystemSessionCache::cache_remove(SPRequest* request, const char* key)
}
}
else {
- m_spilog.debug("removed session (%s)", key);
+ m_spilog.debug("removed session file for (%s)", key);
}
}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list