https://issues.shibboleth.net/jira/browse/SSPCPP-352
authorScott Cantor <cantor.2@osu.edu>
Fri, 3 Feb 2012 04:03:45 +0000 (04:03 +0000)
committerScott Cantor <cantor.2@osu.edu>
Fri, 3 Feb 2012 04:03:45 +0000 (04:03 +0000)
15 files changed:
adfs/adfs.cpp
schemas/shibboleth-2.0-native-sp-config.xsd
shibsp/Application.cpp
shibsp/Application.h
shibsp/handler/impl/AbstractHandler.cpp
shibsp/handler/impl/AssertionConsumerService.cpp
shibsp/handler/impl/LocalLogoutInitiator.cpp
shibsp/handler/impl/SAML2Logout.cpp
shibsp/handler/impl/SAML2LogoutInitiator.cpp
shibsp/handler/impl/SAML2NameIDMgmt.cpp
shibsp/handler/impl/SAML2SessionInitiator.cpp
shibsp/handler/impl/Shib1SessionInitiator.cpp
shibsp/handler/impl/WAYFSessionInitiator.cpp
shibsp/impl/XMLServiceProvider.cpp
shibsp/internal.h

index 7a6172f..2e9e0c7 100644 (file)
@@ -361,6 +361,7 @@ pair<bool,long> ADFSSessionInitiator::run(SPRequest& request, string& entityID,
         // Since we're passing the ACS by value, we need to compute the return URL,
         // so we'll need the target resource for real.
         recoverRelayState(app, request, request, target, false);
+        app.limitRedirect(request, target.c_str());
 
         acClass = getString("authnContextClassRef", request);
     }
@@ -961,6 +962,10 @@ pair<bool,long> ADFSLogoutInitiator::doRequest(
                 );
         }
 
+        const char* returnloc = httpRequest.getParameter("return");
+        if (returnloc)
+            application.limitRedirect(httpRequest, returnloc);
+
         // Log the request.
         scoped_ptr<LogoutEvent> logout_event(newLogoutEvent(application, &httpRequest, session));
         if (logout_event) {
@@ -968,12 +973,19 @@ pair<bool,long> ADFSLogoutInitiator::doRequest(
             application.getServiceProvider().getTransactionLog()->write(*logout_event);
         }
 
-        const URLEncoder* urlenc = XMLToolingConfig::getConfig().getURLEncoder();
-        const char* returnloc = httpRequest.getParameter("return");
         auto_ptr_char dest(ep->getLocation());
         string req=string(dest.get()) + (strchr(dest.get(),'?') ? '&' : '?') + "wa=wsignout1.0";
-        if (returnloc)
-            req += "&wreply=" + urlenc->encode(returnloc);
+        if (returnloc) {
+            req += "&wreply=";
+            if (*returnloc == '/') {
+                string s(returnloc);
+                httpRequest.absolutize(s);
+                req += XMLToolingConfig::getConfig().getURLEncoder()->encode(s.c_str());
+            }
+            else {
+                req += XMLToolingConfig::getConfig().getURLEncoder()->encode(returnloc);
+            }
+        }
         ret.second = httpResponse.sendRedirect(req.c_str());
         ret.first = true;
 
@@ -1045,7 +1057,16 @@ pair<bool,long> ADFSLogout::run(SPRequest& request, bool isHandler) const
         }
     }
 
-    if (param)
-        return make_pair(true, request.sendRedirect(param));
+    if (param) {
+        if (*param == '/') {
+            string p(param);
+            request.absolutize(p);
+            return make_pair(true, request.sendRedirect(p.c_str()));
+        }
+        else {
+            app.limitRedirect(request, param);
+            return make_pair(true, request.sendRedirect(param));
+        }
+    }
     return sendLogoutPage(app, request, request, "global");
 }
index e835c76..6f731d6 100644 (file)
     </restriction>
   </simpleType>
 
-  <simpleType name="relayStateLimitType">
+  <simpleType name="redirectLimitType">
     <restriction base="string">
       <enumeration value="none"/>
       <enumeration value="exact"/>
       <enumeration value="host"/>
       <enumeration value="whitelist"/>
+      <enumeration value="exact+whitelist"/>
+      <enumeration value="host+whitelist"/>
     </restriction>
   </simpleType>
 
     <attribute name="postTemplate" type="conf:string"/>
     <attribute name="postExpire" type="boolean"/>
     <attribute name="relayState" type="conf:string"/>
-    <attribute name="relayStateLimit" type="conf:relayStateLimitType"/>
+    <attribute name="relayStateLimit" type="conf:redirectLimitType"/>
     <attribute name="relayStateWhitelist" type="conf:listOfURIs"/>
+    <attribute name="redirectLimit" type="conf:redirectLimitType"/>
+    <attribute name="redirectWhitelist" type="conf:listOfURIs"/>
     <anyAttribute namespace="##other" processContents="lax"/>
   </complexType>
 
index 43139f3..2fc42f3 100644 (file)
@@ -160,3 +160,7 @@ const Handler* Application::getAssertionConsumerServiceByProtocol(const XMLCh* p
     const vector<const Handler*>& handlers = getAssertionConsumerServicesByBinding(b.get());
     return handlers.empty() ? nullptr : handlers.front();
 }
+
+void Application::limitRedirect(const GenericRequest& request, const char* url) const
+{
+}
index f1d70f3..dde11f2 100644 (file)
@@ -37,6 +37,7 @@
 
 namespace xmltooling {
     class XMLTOOL_API CredentialResolver;
+    class XMLTOOL_API GenericRequest;
     class XMLTOOL_API RWLock;
     class XMLTOOL_API SOAPTransport;
     class XMLTOOL_API StorageService;
@@ -320,6 +321,16 @@ namespace shibsp {
          * @param handlers  array to populate
          */
         virtual void getHandlers(std::vector<const Handler*>& handlers) const=0;
+
+        /**
+         * Checks a proposed redirect URL against application-specific settings for legal redirects,
+         * such as same-host restrictions or whitelisted domains, and raises a SecurityPolicyException
+         * in the event of a violation.
+         *
+         * @param request   the request leading to the redirect
+         * @param url       an absolute URL to validate
+         */
+        virtual void limitRedirect(const xmltooling::GenericRequest& request, const char* url) const;
     };
 
 #if defined (_MSC_VER)
index fe112b5..9136242 100644 (file)
@@ -75,7 +75,6 @@ using namespace boost;
 using namespace std;
 
 namespace shibsp {
-
     SHIBSP_DLLLOCAL PluginManager< Handler,string,pair<const DOMElement*,const char*> >::Factory SAML1ConsumerFactory;
     SHIBSP_DLLLOCAL PluginManager< Handler,string,pair<const DOMElement*,const char*> >::Factory SAML2ConsumerFactory;
     SHIBSP_DLLLOCAL PluginManager< Handler,string,pair<const DOMElement*,const char*> >::Factory SAML2ArtifactResolutionFactory;
@@ -103,53 +102,6 @@ namespace shibsp {
             buf += (DIGITS[0x0F & b2]);
         }
     }
-
-    void SHIBSP_DLLLOCAL limitRelayState(
-        Category& log, const Application& application, const HTTPRequest& httpRequest, const char* relayState
-        ) {
-        const PropertySet* sessionProps = application.getPropertySet("Sessions");
-        if (sessionProps) {
-            pair<bool,const char*> relayStateLimit = sessionProps->getString("relayStateLimit");
-            if (relayStateLimit.first && strcmp(relayStateLimit.second, "none")) {
-                vector<string> whitelist;
-                if (!strcmp(relayStateLimit.second, "exact")) {
-                    // Scheme and hostname have to match.
-                    if (httpRequest.isDefaultPort()) {
-                        whitelist.push_back(string(httpRequest.getScheme()) + httpRequest.getHostname() + '/');
-                    }
-                    whitelist.push_back(
-                        string(httpRequest.getScheme()) + "://" + httpRequest.getHostname() + ':' + lexical_cast<string>(httpRequest.getPort()) + '/'
-                        );
-                }
-                else if (!strcmp(relayStateLimit.second, "host")) {
-                    // Allow any scheme or port.
-                    whitelist.push_back(string("https://") + httpRequest.getHostname() + '/');
-                    whitelist.push_back(string("http://") + httpRequest.getHostname() + '/');
-                    whitelist.push_back(string("https://") + httpRequest.getHostname() + ':');
-                    whitelist.push_back(string("http://") + httpRequest.getHostname() + ':');
-                }
-                else if (!strcmp(relayStateLimit.second, "whitelist")) {
-                    // Literal set of comparisons to use.
-                    pair<bool,const char*> whitelistval = sessionProps->getString("relayStateWhitelist");
-                    if (whitelistval.first) {
-                        string dup(whitelistval.second);
-                        split(whitelist, dup, is_space(), algorithm::token_compress_on);
-                    }
-                }
-                else {
-                    log.warn("unrecognized relayStateLimit policy (%s), blocked redirect to (%s)", relayStateLimit.second, relayState);
-                    throw opensaml::SecurityPolicyException("Unrecognized relayStateLimit setting.");
-                }
-
-                static bool (*startsWithI)(const char*,const char*) = XMLString::startsWithI;
-                if (find_if(whitelist.begin(), whitelist.end(),
-                        boost::bind(startsWithI, relayState, boost::bind(&string::c_str, _1))) == whitelist.end()) {
-                    log.warn("relayStateLimit policy (%s), blocked redirect to (%s)", relayStateLimit.second, relayState);
-                    throw opensaml::SecurityPolicyException("Blocked unacceptable redirect location.");
-                }
-            }
-        }
-    }
 };
 
 void SHIBSP_API shibsp::registerHandlers()
index 111ef9e..20a242f 100644 (file)
@@ -168,7 +168,7 @@ pair<bool,long> AssertionConsumerService::processMessage(
         DDF postData = recoverPostData(application, httpRequest, httpResponse, relayState.c_str());
         DDFJanitor postjan(postData);
         recoverRelayState(application, httpRequest, httpResponse, relayState);
-        limitRelayState(m_log, application, httpRequest, relayState.c_str());
+        application.limitRedirect(httpRequest, relayState.c_str());
         implementProtocol(application, httpRequest, httpResponse, *policy, nullptr, *msg);
 
         auto_ptr_char issuer(policy->getIssuer() ? policy->getIssuer()->getName() : nullptr);
index aba50f9..52ed0d0 100644 (file)
@@ -197,7 +197,13 @@ pair<bool,long> LocalLogoutInitiator::doRequest(
     // Route back to return location specified, or use the local template.
     const char* dest = httpRequest.getParameter("return");
     if (dest) {
-        limitRelayState(m_log, application, httpRequest, dest);
+        // Relative URLs get promoted, absolutes get validated.
+        if (*dest == '/') {
+            string d(dest);
+            httpRequest.absolutize(d);
+            return make_pair(true, httpResponse.sendRedirect(d.c_str()));
+        }
+        application.limitRedirect(httpRequest, dest);
         return make_pair(true, httpResponse.sendRedirect(dest));
     }
     return sendLogoutPage(application, httpRequest, httpResponse, "local");
index 144b540..8aa8632 100644 (file)
@@ -568,9 +568,7 @@ pair<bool,long> SAML2Logout::doRequest(const Application& application, const HTT
     if (logoutResponse) {
         if (!policy->isAuthenticated()) {
             SecurityPolicyException ex("Security of LogoutResponse not established.");
-            if (policy->getIssuerMetadata())
-                annotateException(&ex, policy->getIssuerMetadata()); // throws it
-            ex.raise();
+            annotateException(&ex, policy->getIssuerMetadata()); // throws it
         }
  
         if (logout_event) {
@@ -610,7 +608,7 @@ pair<bool,long> SAML2Logout::doRequest(const Application& application, const HTT
         }
 
         if (!relayState.empty()) {
-            limitRelayState(m_log, application, request, relayState.c_str());
+            application.limitRedirect(request, relayState.c_str());
             return make_pair(true, response.sendRedirect(relayState.c_str()));
         }
 
index 9896d3a..e8ab2da 100644 (file)
@@ -410,11 +410,21 @@ pair<bool,long> SAML2LogoutInitiator::doRequest(
                 else {
                     const char* returnloc = httpRequest.getParameter("return");
                     if (returnloc) {
-                        limitRelayState(m_log, application, httpRequest, returnloc);
-                        ret.second = httpResponse.sendRedirect(returnloc);
+                        // Relative URLs get promoted, absolutes get validated.
+                        if (*returnloc == '/') {
+                            string loc(returnloc);
+                            httpRequest.absolutize(loc);
+                            ret.second = httpResponse.sendRedirect(loc.c_str());
+                        }
+                        else {
+                            application.limitRedirect(httpRequest, returnloc);
+                            ret.second = httpResponse.sendRedirect(returnloc);
+                        }
                         ret.first = true;
                     }
-                    ret = sendLogoutPage(application, httpRequest, httpResponse, "global");
+                    else {
+                        ret = sendLogoutPage(application, httpRequest, httpResponse, "global");
+                    }
                 }
             }
 
@@ -431,8 +441,9 @@ pair<bool,long> SAML2LogoutInitiator::doRequest(
         string relayState;
         const char* returnloc = httpRequest.getParameter("return");
         if (returnloc) {
-            limitRelayState(m_log, application, httpRequest, returnloc);
+            application.limitRedirect(httpRequest, returnloc);
             relayState = returnloc;
+            httpRequest.absolutize(relayState);
             preserveRelayState(application, httpResponse, relayState);
         }
 
index f2e91b2..4d1bf2c 100644 (file)
@@ -447,9 +447,7 @@ pair<bool,long> SAML2NameIDMgmt::doRequest(const Application& application, const
     */
 
     FatalProfileException ex("Incoming message was not a samlp:ManageNameIDRequest.");
-    if (policy->getIssuerMetadata())
-        annotateException(&ex, policy->getIssuerMetadata()); // throws it
-    ex.raise();
+    annotateException(&ex, policy->getIssuerMetadata()); // throws it
     return make_pair(false, 0L);  // never happen, satisfies compiler
 #else
     throw ConfigurationException("Cannot process NameID mgmt message using lite version of shibsp library.");
index a3377e8..d12c7f2 100644 (file)
@@ -263,7 +263,7 @@ pair<bool,long> SAML2SessionInitiator::run(SPRequest& request, string& entityID,
 
         // Always need to recover target URL to compute handler below.
         recoverRelayState(app, request, request, target, false);
-        limitRelayState(m_log, app, request, target.c_str());
+        app.limitRedirect(request, target.c_str());
 
         pair<bool,bool> flag = getBool("isPassive", request);
         isPassive = (flag.first && flag.second);
index 7a7d61d..1674b66 100644 (file)
@@ -146,7 +146,7 @@ pair<bool,long> Shib1SessionInitiator::run(SPRequest& request, string& entityID,
         // Since we're passing the ACS by value, we need to compute the return URL,
         // so we'll need the target resource for real.
         recoverRelayState(app, request, request, target, false);
-        limitRelayState(m_log, app, request, target.c_str());
+        app.limitRedirect(request, target.c_str());
     }
     else {
         // Check for a hardwired target value in the map or handler.
index 879e54e..88ab294 100644 (file)
@@ -116,7 +116,7 @@ pair<bool,long> WAYFSessionInitiator::run(SPRequest& request, string& entityID,
         // Since we're passing the ACS by value, we need to compute the return URL,
         // so we'll need the target resource for real.
         recoverRelayState(request.getApplication(), request, request, target, false);
-        limitRelayState(m_log, request.getApplication(), request, target.c_str());
+        request.getApplication().limitRedirect(request, target.c_str());
 
         prop.second = request.getParameter("discoveryURL");
         if (prop.second && *prop.second)
index 96f90f8..a0cb779 100644 (file)
@@ -50,6 +50,7 @@
 #endif
 #include <algorithm>
 #include <boost/bind.hpp>
+#include <boost/lexical_cast.hpp>
 #include <boost/scoped_ptr.hpp>
 #include <boost/shared_ptr.hpp>
 #include <boost/algorithm/string.hpp>
@@ -70,8 +71,9 @@
 # include "attribute/resolver/AttributeResolver.h"
 # include "security/PKIXTrustEngine.h"
 # include "security/SecurityPolicyProvider.h"
-# include <saml/SAMLConfig.h>
+# include <saml/exceptions.h>
 # include <saml/version.h>
+# include <saml/SAMLConfig.h>
 # include <saml/binding/ArtifactMap.h>
 # include <saml/binding/SAMLArtifact.h>
 # include <saml/saml1/core/Assertions.h>
@@ -184,6 +186,7 @@ namespace {
         const vector<const Handler*>& getAssertionConsumerServicesByBinding(const XMLCh* binding) const;
         const Handler* getHandler(const char* path) const;
         void getHandlers(vector<const Handler*>& handlers) const;
+        void limitRedirect(const GenericRequest& request, const char* url) const;
 
         void receive(DDF& in, ostream& out) {
             // Only current function is to return the headers to clear.
@@ -272,6 +275,17 @@ namespace {
             if (m_artifactResolutionDefault) return m_artifactResolutionDefault->getInt("index");
             return m_base ? m_base->getArtifactEndpointIndex() : make_pair(false,0);
         }
+
+        enum {
+            REDIRECT_LIMIT_INHERIT,
+            REDIRECT_LIMIT_NONE,
+            REDIRECT_LIMIT_EXACT,
+            REDIRECT_LIMIT_HOST,
+            REDIRECT_LIMIT_WHITELIST,
+            REDIRECT_LIMIT_EXACT_WHITELIST,
+            REDIRECT_LIMIT_HOST_WHITELIST
+        } m_redirectLimit;
+        vector<string> m_redirectWhitelist;
     };
 
     // Top-level configuration implementation
@@ -527,10 +541,51 @@ XMLApplication::XMLApplication(
 #ifdef _DEBUG
     xmltooling::NDC ndc("XMLApplication");
 #endif
-    Category& log=Category::getInstance(SHIBSP_LOGCAT".Application");
+    Category& log = Category::getInstance(SHIBSP_LOGCAT".Application");
 
     // First load any property sets.
-    load(e, nullptr, this);
+    map<string,string> remapper;
+    remapper["relayStateLimit"] = "redirectLimit";
+    remapper["relayStateWhitelist"] = "redirectWhitelist";
+    load(e, nullptr, this, &remapper);
+
+    // Process redirect limit policy. Do this before assigning the parent pointer
+    // to ensure we get only our Sessions element.
+    const PropertySet* sessionProps = getPropertySet("Sessions");
+    if (sessionProps) {
+        pair<bool,const char*> redirectLimit = sessionProps->getString("redirectLimit");
+        if (redirectLimit.first) {
+            if (!strcmp(redirectLimit.second, "none"))
+                m_redirectLimit = REDIRECT_LIMIT_NONE;
+            else if (!strcmp(redirectLimit.second, "exact"))
+                m_redirectLimit = REDIRECT_LIMIT_EXACT;
+            else if (!strcmp(redirectLimit.second, "host"))
+                m_redirectLimit = REDIRECT_LIMIT_HOST;
+            else {
+                if (!strcmp(redirectLimit.second, "exact+whitelist"))
+                    m_redirectLimit = REDIRECT_LIMIT_EXACT_WHITELIST;
+                else if (!strcmp(redirectLimit.second, "exact+host"))
+                    m_redirectLimit = REDIRECT_LIMIT_HOST_WHITELIST;
+                else if (!strcmp(redirectLimit.second, "exact+host"))
+                    m_redirectLimit = REDIRECT_LIMIT_WHITELIST;
+                else
+                    throw ConfigurationException("Unrecognized redirectLimit setting ($1)", params(1, redirectLimit.second));
+                redirectLimit = sessionProps->getString("redirectWhitelist");
+                if (redirectLimit.first) {
+                    string dup(redirectLimit.second);
+                    split(m_redirectWhitelist, dup, is_space(), algorithm::token_compress_on);
+                }
+            }
+        }
+        else {
+            m_redirectLimit = base ? REDIRECT_LIMIT_INHERIT : REDIRECT_LIMIT_NONE;
+        }
+    }
+    else {
+        m_redirectLimit = base ? REDIRECT_LIMIT_INHERIT : REDIRECT_LIMIT_NONE;
+    }
+
+    // Assign parent.
     if (base)
         setParent(base);
 
@@ -1603,6 +1658,43 @@ void XMLApplication::getHandlers(vector<const Handler*>& handlers) const
     }
 }
 
+void XMLApplication::limitRedirect(const GenericRequest& request, const char* url) const
+{
+    if (!url || *url == '/')
+        return;
+    if (m_redirectLimit == REDIRECT_LIMIT_INHERIT)
+        return m_base->limitRedirect(request, url);
+    if (m_redirectLimit != REDIRECT_LIMIT_NONE) {
+        vector<string> whitelist;
+        if (m_redirectLimit == REDIRECT_LIMIT_EXACT || m_redirectLimit == REDIRECT_LIMIT_EXACT_WHITELIST) {
+            // Scheme and hostname have to match.
+            if (request.isDefaultPort()) {
+                whitelist.push_back(string(request.getScheme()) + "://" + request.getHostname() + '/');
+            }
+            whitelist.push_back(string(request.getScheme()) + "://" + request.getHostname() + ':' + lexical_cast<string>(request.getPort()) + '/');
+        }
+        else if (m_redirectLimit == REDIRECT_LIMIT_HOST || m_redirectLimit == REDIRECT_LIMIT_HOST_WHITELIST) {
+            // Allow any scheme or port.
+            whitelist.push_back(string("https://") + request.getHostname() + '/');
+            whitelist.push_back(string("http://") + request.getHostname() + '/');
+            whitelist.push_back(string("https://") + request.getHostname() + ':');
+            whitelist.push_back(string("http://") + request.getHostname() + ':');
+        }
+
+        static bool (*startsWithI)(const char*,const char*) = XMLString::startsWithI;
+        if (!whitelist.empty() && find_if(whitelist.begin(), whitelist.end(),
+                boost::bind(startsWithI, url, boost::bind(&string::c_str, _1))) != whitelist.end()) {
+            return;
+        }
+        else if (!m_redirectWhitelist.empty() && find_if(m_redirectWhitelist.begin(), m_redirectWhitelist.end(),
+                boost::bind(startsWithI, url, boost::bind(&string::c_str, _1))) != m_redirectWhitelist.end()) {
+            return;
+        }
+        Category::getInstance(SHIBSP_LOGCAT".Application").warn("redirectLimit policy enforced, blocked redirect to (%s)", url);
+        throw opensaml::SecurityPolicyException("Blocked unacceptable redirect location.");
+    }
+}
+
 #ifdef SHIBSP_XERCESC_SHORT_ACCEPTNODE
 short
 #else
index cb211a5..c35cccf 100644 (file)
 using namespace xmltooling::logging;
 using namespace xercesc;
 
-namespace shibsp {
-    void SHIBSP_DLLLOCAL limitRelayState(
-        xmltooling::logging::Category& log,
-        const Application& application,
-        const xmltooling::HTTPRequest& httpRequest,
-        const char* relayState
-        );
-};
-
 #endif /* __shibsp_internal_h__ */