Check for overflow in storage APIs.

diff --git a/Shibboleth.sln b/Shibboleth.sln
index b6e1e00..83a50e5 100644 (file)
--- a/Shibboleth.sln
+++ b/Shibboleth.sln
@@ -252,10 +252,8 @@ Global
                {B2423DCE-048D-4BAA-9AB9-F5D1FCDD3D25}.Release|x64.ActiveCfg = Release|x64
                {B2423DCE-048D-4BAA-9AB9-F5D1FCDD3D25}.Release|x64.Build.0 = Release|x64
                {666A63A7-983F-4C19-8411-207F24305198}.Debug|Win32.ActiveCfg = Debug|Win32
-               {666A63A7-983F-4C19-8411-207F24305198}.Debug|Win32.Build.0 = Debug|Win32
                {666A63A7-983F-4C19-8411-207F24305198}.Debug|x64.ActiveCfg = Debug|x64
                {666A63A7-983F-4C19-8411-207F24305198}.Release|Win32.ActiveCfg = Release|Win32
-               {666A63A7-983F-4C19-8411-207F24305198}.Release|Win32.Build.0 = Release|Win32
                {666A63A7-983F-4C19-8411-207F24305198}.Release|x64.ActiveCfg = Release|x64
                {A2140D6E-C2C6-4329-84E3-2F530CEBE445}.Debug|Win32.ActiveCfg = Debug|Win32
                {A2140D6E-C2C6-4329-84E3-2F530CEBE445}.Debug|Win32.Build.0 = Debug|Win32

diff --git a/odbc-store/odbc-store.cpp b/odbc-store/odbc-store.cpp
index d84400c..6f7aede 100644 (file)
--- a/odbc-store/odbc-store.cpp
+++ b/odbc-store/odbc-store.cpp
@@ -100,6 +100,9 @@ namespace {
     static const XMLCh isolationLevel[] =   UNICODE_LITERAL_14(i,s,o,l,a,t,i,o,n,L,e,v,e,l);
     static const XMLCh ConnectionString[] = UNICODE_LITERAL_16(C,o,n,n,e,c,t,i,o,n,S,t,r,i,n,g);
     static const XMLCh RetryOnError[] =     UNICODE_LITERAL_12(R,e,t,r,y,O,n,E,r,r,o,r);
+    static const XMLCh contextSize[] =      UNICODE_LITERAL_11(c,o,n,t,e,x,t,S,i,z,e);
+    static const XMLCh keySize[] =          UNICODE_LITERAL_7(k,e,y,S,i,z,e);
+    static const XMLCh stringSize[] =       UNICODE_LITERAL_10(s,t,r,i,n,g,S,i,z,e);
 
     // RAII for ODBC handles
     struct ODBCConn {
@@ -124,6 +127,10 @@ namespace {
         ODBCStorageService(const DOMElement* e);
         virtual ~ODBCStorageService();
 
+        const Capabilities& getCapabilities() const {
+            return m_caps;
+        }
+
         bool createString(const char* context, const char* key, const char* value, time_t expiration) {
             return createRow(STRING_TABLE, context, key, value, expiration);
         }
@@ -185,6 +192,7 @@ namespace {
         void cleanup();
 
         Category& m_log;
+        Capabilities m_caps;
         int m_cleanupInterval;
         CondWait* shutdown_wait;
         Thread* cleanup_thread;
@@ -262,7 +270,8 @@ namespace {
 };
 
 ODBCStorageService::ODBCStorageService(const DOMElement* e) : m_log(Category::getInstance("XMLTooling.StorageService")),
-   m_cleanupInterval(900), shutdown_wait(nullptr), cleanup_thread(nullptr), shutdown(false), m_henv(SQL_NULL_HANDLE), m_isolation(SQL_TXN_SERIALIZABLE)
+    m_caps(XMLHelper::getAttrInt(e, 255, contextSize), XMLHelper::getAttrInt(e, 255, keySize), XMLHelper::getAttrInt(e, 255, stringSize)),
+    m_cleanupInterval(900), shutdown_wait(nullptr), cleanup_thread(nullptr), shutdown(false), m_henv(SQL_NULL_HANDLE), m_isolation(SQL_TXN_SERIALIZABLE)
 {
 #ifdef _DEBUG
     xmltooling::NDC ndc("ODBCStorageService");

diff --git a/shibsp/handler/impl/AbstractHandler.cpp b/shibsp/handler/impl/AbstractHandler.cpp
index 5e95b39..3a522e7 100644 (file)
--- a/shibsp/handler/impl/AbstractHandler.cpp
+++ b/shibsp/handler/impl/AbstractHandler.cpp
@@ -294,8 +294,14 @@ void Handler::preserveRelayState(const Application& application, HTTPResponse& r
                         string rsKey;
                         SAMLConfig::getConfig().generateRandomBytes(rsKey,32);
                         rsKey = SAMLArtifact::toHex(rsKey);
-                        if (!storage->createString("RelayState", rsKey.c_str(), relayState.c_str(), time(nullptr) + 600))
-                            throw IOException("Collision generating in-memory relay state key.");
+                        if (relayState.length() <= storage->getCapabilities().getStringSize()) {
+                            if (!storage->createString("RelayState", rsKey.c_str(), relayState.c_str(), time(nullptr) + 600))
+                                throw IOException("Collision generating in-memory relay state key.");
+                        }
+                        else {
+                            if (!storage->createText("RelayState", rsKey.c_str(), relayState.c_str(), time(nullptr) + 600))
+                                throw IOException("Collision generating in-memory relay state key.");
+                        }
                         relayState = string(mech.second-3) + ':' + rsKey;
                     }
                     else {
@@ -351,8 +357,15 @@ void Handler::recoverRelayState(
                             absolutize(request, relayState);
                             return;
                         }
-                        else
+                        else if (storage->readText("RelayState",ssid.c_str(),&relayState) > 0) {
+                            if (clear)
+                                storage->deleteText("RelayState",ssid.c_str());
+                            absolutize(request, relayState);
+                            return;
+                        }
+                        else {
                             relayState.erase();
+                        }
                     }
                     else {
                         string msg("Storage-backed RelayState with invalid StorageService ID (");
@@ -640,7 +653,7 @@ void AbstractHandler::preservePostData(
                 rsKey = SAMLArtifact::toHex(rsKey);
                 ostringstream out;
                 out << postData;
-                if (!storage->createString("PostData", rsKey.c_str(), out.str().c_str(), time(nullptr) + 600))
+                if (!storage->createText("PostData", rsKey.c_str(), out.str().c_str(), time(nullptr) + 600))
                     throw IOException("Attempted to insert duplicate storage key.");
                 postkey = string(mech.second-3) + ':' + rsKey;
             }
@@ -702,8 +715,8 @@ DDF AbstractHandler::recoverPostData(
 #ifndef SHIBSP_LITE
                     StorageService* storage = conf.getServiceProvider()->getStorageService(ssid.c_str());
                     if (storage) {
-                        if (storage->readString("PostData", key, &ssid) > 0) {
-                            storage->deleteString("PostData", key);
+                        if (storage->readText("PostData", key, &ssid) > 0) {
+                            storage->deleteText("PostData", key);
                             istringstream inret(ssid);
                             DDF ret;
                             inret >> ret;

diff --git a/shibsp/impl/StorageServiceSessionCache.cpp b/shibsp/impl/StorageServiceSessionCache.cpp
index cb46aad..9447573 100644 (file)
--- a/shibsp/impl/StorageServiceSessionCache.cpp
+++ b/shibsp/impl/StorageServiceSessionCache.cpp
@@ -628,11 +628,14 @@ void StoredSession::addAssertion(Assertion* assertion)
 
     if (!m_cache->m_storage)
         throw ConfigurationException("Session modification requires a StorageService.");
-
-    if (!assertion)
+    else if (!assertion)
         throw FatalProfileException("Unknown object type passed to session for storage.");
 
     auto_ptr_char id(assertion->getID());
+    if (!id.get() || !*id.get())
+        throw IOException("Assertion did not carry an ID.");
+    else if (strlen(id.get()) > m_cache->m_storage->getCapabilities().getKeySize())
+        throw IOException("Assertion ID ($1) exceeds allowable storage key size.", params(1, id.get()));
 
     m_cache->m_log.debug("adding assertion (%s) to session (%s)", id.get(), getID());
 
@@ -858,8 +861,9 @@ void SSCache::test()
 void SSCache::insert(const char* key, time_t expires, const char* name, const char* index)
 {
     string dup;
-    if (strlen(name) > 255) {
-        dup = string(name).substr(0,255);
+    unsigned int storageLimit = m_storage_lite->getCapabilities().getKeySize();
+    if (strlen(name) > storageLimit) {
+        dup = string(name).substr(0, storageLimit);
         name = dup.c_str();
     }
 
@@ -935,13 +939,14 @@ void SSCache::insert(
     auto_ptr_char entity_id(issuer ? issuer->getEntityID() : nullptr);
     auto_ptr_char name(nameid ? nameid->getName() : nullptr);
 
-    if (nameid) {
+    if (name.get() && *name.get()) {
         // Check for a pending logout.
-        char namebuf[256];
-        strncpy(namebuf, name.get(), 255);
-        namebuf[255] = 0;
+        unsigned int storageLimit = m_storage_lite->getCapabilities().getKeySize();
+        string namebuf = name.get();
+        if (namebuf.length() > storageLimit)
+            namebuf = namebuf.substr(0, storageLimit);
         string pending;
-        int ver = m_storage_lite->readText("Logout", namebuf, &pending);
+        int ver = m_storage_lite->readText("Logout", namebuf.c_str(), &pending);
         if (ver > 0) {
             DDF pendobj;
             DDFJanitor jpend(pendobj);
@@ -1053,8 +1058,10 @@ void SSCache::insert(
                 ostringstream tokenstr;
                 tokenstr << *(*t);
                 auto_ptr_char tokenid((*t)->getID());
-                if (!m_storage->createText(key.get(), tokenid.get(), tokenstr.str().c_str(), now + cacheTimeout))
-                    throw IOException("duplicate assertion ID ($1)", params(1, tokenid.get()));
+                if (!tokenid.get() || !*tokenid.get() || strlen(tokenid.get()) > m_storage->getCapabilities().getKeySize())
+                    throw IOException("Assertion ID is missing or exceeds key size of storage service.");
+                else if (!m_storage->createText(key.get(), tokenid.get(), tokenstr.str().c_str(), now + cacheTimeout))
+                    throw IOException("Duplicate assertion ID ($1)", params(1, tokenid.get()));
             }
         }
         catch (exception& ex) {
@@ -1177,8 +1184,9 @@ vector<string>::size_type SSCache::logout(
 
     m_log.info("request to logout sessions from (%s) for (%s)", entityID.get() ? entityID.get() : "unknown", name.get());
 
-    if (strlen(name.get()) > 255)
-        const_cast<char*>(name.get())[255] = 0;
+    unsigned int storageLimit = m_storage_lite->getCapabilities().getKeySize();
+    if (strlen(name.get()) > storageLimit)
+        const_cast<char*>(name.get())[storageLimit] = 0;
 
     DDF obj;
     DDFJanitor jobj(obj);

diff --git a/shibsp/impl/XMLServiceProvider.cpp b/shibsp/impl/XMLServiceProvider.cpp
index ac41b16..b4a4628 100644 (file)
--- a/shibsp/impl/XMLServiceProvider.cpp
+++ b/shibsp/impl/XMLServiceProvider.cpp
@@ -2147,6 +2147,10 @@ void XMLConfig::receive(DDF& in, ostream& out)
                 if (in["clear"].integer())
                     storage->deleteString("RelayState",key);
             }
+            else if (storage->readText("RelayState",key,&relayState)>0) {
+                if (in["clear"].integer())
+                    storage->deleteText("RelayState",key);
+            }
         }
         else {
             Category::getInstance(SHIBSP_LOGCAT".ServiceProvider").error(
@@ -2168,9 +2172,12 @@ void XMLConfig::receive(DDF& in, ostream& out)
         string rsKey;
         StorageService* storage = getStorageService(id);
         if (storage) {
-            SAMLConfig::getConfig().generateRandomBytes(rsKey,20);
+            SAMLConfig::getConfig().generateRandomBytes(rsKey,32);
             rsKey = SAMLArtifact::toHex(rsKey);
-            storage->createString("RelayState", rsKey.c_str(), value, time(nullptr) + 600);
+            if (strlen(value) <= storage->getCapabilities().getStringSize())
+                storage->createString("RelayState", rsKey.c_str(), value, time(nullptr) + 600);
+            else
+                storage->createText("RelayState", rsKey.c_str(), value, time(nullptr) + 600);
         }
         else {
             Category::getInstance(SHIBSP_LOGCAT".ServiceProvider").error(
@@ -2192,8 +2199,8 @@ void XMLConfig::receive(DDF& in, ostream& out)
         string postData;
         StorageService* storage = getStorageService(id);
         if (storage) {
-            if (storage->readString("PostData",key,&postData) > 0) {
-                storage->deleteString("PostData",key);
+            if (storage->readText("PostData",key,&postData) > 0) {
+                storage->deleteText("PostData",key);
             }
         }
         else {
@@ -2220,11 +2227,11 @@ void XMLConfig::receive(DDF& in, ostream& out)
         string rsKey;
         StorageService* storage = getStorageService(id);
         if (storage) {
-            SAMLConfig::getConfig().generateRandomBytes(rsKey,20);
+            SAMLConfig::getConfig().generateRandomBytes(rsKey,32);
             rsKey = SAMLArtifact::toHex(rsKey);
             ostringstream params;
             params << in["parameters"];
-            storage->createString("PostData", rsKey.c_str(), params.str().c_str(), time(nullptr) + 600);
+            storage->createText("PostData", rsKey.c_str(), params.str().c_str(), time(nullptr) + 600);
         }
         else {
             Category::getInstance(SHIBSP_LOGCAT".ServiceProvider").error(