Improve cookie handling code

Pull reference code out of apache's own util_cookies.c (which is
unfortunately not part of libapr so we can't link to it in our tests).

Also use libapr for url-escaping, getting rid of some ugly code.

src/mod_sso/mod_sso.c

@@ -432,7 +432,7 @@ static int mod_sso_method_handler(request_rec *r) {
   ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
                "sso: logout? \"%s\" \"%s\"", sso_logout_path, uri);
   if (!strcmp(uri, sso_logout_path)) {
-    modsso_del_cookie(r, sso_cookie_name);
+    modsso_del_cookie(r, sso_cookie_name, service_path);
     return http_sendstring(r, "OK");
   }
 
@@ -443,7 +443,8 @@ static int mod_sso_method_handler(request_rec *r) {
   if (!strcmp(uri, sso_login_path)) {
     struct modsso_params params;
     sso_ticket_t t;
-    char *redir, *unique_id = NULL;
+    const char *redir;
+    char *unique_id = NULL;
     int err;
 
     if (!r->args) {
@@ -463,7 +464,7 @@ static int mod_sso_method_handler(request_rec *r) {
     // Only do this if a session key is set (sessions are enabled).
     if (s_cfg->session_key != NULL) {
       if (modsso_session_read(r, s_cfg->session_key, s_cfg->session_key_len,
-                              &unique_id) < 0) {
+                              &unique_id, sso_login_path) < 0) {
         ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
                      "sso: could not read session cookie");
         return HTTP_BAD_REQUEST;

src/mod_sso/mod_sso.h

@@ -50,8 +50,8 @@ struct modsso_params {
 typedef struct modsso_params *modsso_params_t;
 
 // sso_utils.c
-char *modsso_url_decode(apr_pool_t *p, const char *str);
-char *modsso_url_encode(apr_pool_t *p, const char *str);
+const char *modsso_url_decode(apr_pool_t *p, const char *str);
+const char *modsso_url_encode(apr_pool_t *p, const char *str);
 
 int modsso_parse_query_string(apr_pool_t *p, const char *str,
                               modsso_params_t params);
@@ -61,7 +61,8 @@ char *modsso_get_cookie(request_rec *r, const char *cookie_name);
 void modsso_set_cookie(request_rec *r, const char *cookie_name,
                        const char *value, const char *path);
 
-void modsso_del_cookie(request_rec *r, const char *cookie_name);
+void modsso_del_cookie(request_rec *r, const char *cookie_name,
+                       const char *path);
 
 // session.c
 int modsso_session_read_key_from_file(apr_pool_t *pool, const char *path,
@@ -75,7 +76,7 @@ int modsso_session_serialize(apr_pool_t *pool, const unsigned char *key,
                              size_t key_len, const char *value,
                              size_t value_len, char **output);
 int modsso_session_read(request_rec *r, const unsigned char *key,
-                        size_t key_len, char **value);
+                        size_t key_len, char **value, const char *cookie_path);
 int modsso_session_save(request_rec *r, const unsigned char *key,
                         size_t key_len, const char *unique_id,
                         const char *path);

src/mod_sso/session.c

@@ -178,8 +178,9 @@ int modsso_session_serialize(apr_pool_t *pool, const unsigned char *key,
 }
 
 int modsso_session_read(request_rec *r, const unsigned char *key,
-                        size_t key_len, char **value) {
-  char *serialized;
+                        size_t key_len, char **value,
+                        const char *cookie_path) {
+  const char *serialized;
   size_t value_len;
   int ret = -1;
 
@@ -197,7 +198,7 @@ int modsso_session_read(request_rec *r, const unsigned char *key,
   ret = 0;
 
  fail:
-  modsso_del_cookie(r, session_cookie_name);
+  modsso_del_cookie(r, session_cookie_name, cookie_path);
   return ret;
 }
 

src/mod_sso/sso_utils.c

@@ -22,20 +22,26 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include <ctype.h>
+
+#include "ap_config.h"
+#include "apr_lib.h"
+#include "apr_escape.h"
+#include "apr_strings.h"
 #include "httpd.h"
+
 #include "http_config.h"
 #include "http_core.h"
 #include "http_log.h"
-#include "apr_strings.h"
 #include "http_protocol.h"
 #include "http_main.h"
-#include "ap_config.h"
+
 #include "mod_sso.h"
-#include <ctype.h>
 
 // Ugly implementation of url decoding. Returns a newly allocated
-// string (using malloc).
-char *modsso_url_decode(apr_pool_t *p, const char *i) {
+// string (from the pool). We're using this because
+// apr_punescape_url() segfaults on certain invalid inputs.
+const char *modsso_url_decode(apr_pool_t *p, const char *i) {
   char *o, *output = (char *)apr_palloc(p, 1 + strlen(i));
   char state = 'N';
   char buf[3], obuf[2];
@@ -87,22 +93,8 @@ char *modsso_url_decode(apr_pool_t *p, const char *i) {
   return output;
 }
 
-// ugly implementation of url encoding
-char *modsso_url_encode(apr_pool_t *p, const char *s) {
-  static const char *hex = "0123456789ABCDEF";
-  char *out = (char *)apr_palloc(p, strlen(s) * 3 + 1), *optr;
-  for (optr = out; *s; s++) {
-    unsigned char c = *s;
-    if (!isalnum(c) && (c != '.') && (c != '-') && (c != '_')) {
-      *optr++ = '%';
-      *optr++ = hex[(c & 0xf0) >> 4];
-      *optr++ = hex[c & 0x0f];
-    } else {
-      *optr++ = c;
-    }
-  }
-  *optr = '\0';
-  return out;
+const char *modsso_url_encode(apr_pool_t *p, const char *s) {
+  return apr_pescape_urlencoded(p, s);
 }
 
 int modsso_parse_query_string(apr_pool_t *p, const char *str, modsso_params_t params) {
@@ -116,7 +108,8 @@ int modsso_parse_query_string(apr_pool_t *p, const char *str, modsso_params_t pa
   }
 
   while (1) {
-    char *token, *value, *sep;
+    const char *token;
+    char *value, *sep;
     token = strtok_r(tmp, "&", &strptr);
     if (token == NULL) {
       break;
@@ -147,81 +140,94 @@ int modsso_parse_query_string(apr_pool_t *p, const char *str, modsso_params_t pa
   return 0;
 }
 
-static char *make_cookie_value(request_rec *r,
-                               const char *name,
-                               const char *session_id,
-                               const char *path, 
-                               int cookie_lifespan)
+struct modsso_cookie_do {
+  request_rec *r;
+  const char *name;
+  char *value;
+};
+
+/* Iterate through the cookies, isolate our cookie and then remove it.
+ *
+ * If our cookie appears two or more times, but with different values,
+ * remove it twice and set the duplicated flag to true. Remove any
+ * $path or other attributes following our cookie if present. If we end
+ * up with an empty cookie, remove the whole header.
+ */
+static int extract_cookie_line(void *varg, const char *key, const char *val)
 {
-  size_t sz = 1024;
-  char *cookie_value = (char *)apr_palloc(r->pool, sz);
-
-  if (cookie_lifespan == 0) {
-    
-    snprintf(cookie_value, sz,
-             "%s=%s; path=%s; secure; httpOnly;",
-             name, session_id, path);
-  } else {
-    char expires[200];
-    time_t t;
-    struct tm *tmp;
-    t = time(NULL) + cookie_lifespan;
-    tmp = gmtime(&t);
-    strftime(expires, sizeof(expires), "%a, %d-%b-%Y %H:%M:%S GMT", tmp);
-    snprintf(cookie_value, sz,
-             "%s=%s; expires=%s; path=%s; secure; httpOnly;",
-             name, session_id, expires, path);
+  struct modsso_cookie_do *v = varg;
+  char *last1, *last2;
+  char *cookie = apr_pstrdup(v->r->pool, val);
+  const char *name = apr_pstrcat(v->r->pool, v->name, "=", NULL);
+  apr_size_t len = strlen(name);
+  const char *comma = ",";
+  char *next1;
+  const char *semi = ";";
+  char *next2;
+
+  /* find the cookie called name */
+  next1 = apr_strtok(cookie, comma, &last1);
+  while (next1) {
+    next2 = apr_strtok(next1, semi, &last2);
+    while (next2) {
+      char *trim = next2;
+      while (apr_isspace(*trim)) {
+        trim++;
+      }
+      if (!strncmp(trim, name, len)) {
+        v->value = apr_pstrdup(v->r->pool, trim + len);
+      }
+      next2 = apr_strtok(NULL, semi, &last2);
+    }
+
+    next1 = apr_strtok(NULL, comma, &last1);
   }
-  return cookie_value;
+
+  return 1;
 }
 
 /**
  * Read the value of a cookie.
  *
+ * Almost a copy of util_cookies.c:ap_cookie_read, but we can't link
+ * to that one in testing, so we reproduce most of its functionality.
+ *
  * @param r Pointer to the request_rec structure.
  * @param cookie_name Name of the cookie to retrieve.
  * @return the cookie value, or an empty string if no cookie was found.
  */
 char *modsso_get_cookie(request_rec *r, const char *cookie_name) {
-  char *rv = NULL, *cookies, *cookie, *tokenizerCtx = NULL;
-  int cookie_name_len = strlen(cookie_name);
-  
-  const char *cookies_c = apr_table_get(r->headers_in, "Cookie");
-  if (cookies_c == NULL) {
-    return NULL;
-  }
-
-  // Make a copy of the Cookie header so that we can modify it
-  // (tokenization is destructive).
-  cookies = apr_pstrdup(r->pool, cookies_c);
-  cookie = apr_strtok(cookies, ";", &tokenizerCtx);
-  while (cookie) {
-    while (*cookie == ' ')
-      cookie++;
-    if (strncmp(cookie, cookie_name, cookie_name_len) == 0 && cookie[cookie_name_len] == '=') {
-      cookie += (cookie_name_len + 1);
-      rv = apr_pstrdup(r->pool, cookie);
-      break;
-    }
-    cookie = apr_strtok(NULL, ";", &tokenizerCtx);
-  }
-  return rv;
+  struct modsso_cookie_do v;
+
+  v.r = r;
+  v.value = NULL;
+  v.name = cookie_name;
+  apr_table_do(extract_cookie_line, &v, r->headers_in,
+               "Cookie", "Cookie2", NULL);
+  return v.value;
 }
 
+/**
+ * Set a cookie.
+ *
+ * Almost a copy of util_cookies.c:ap_cookie_write, but we can't link
+ * to that one in testing, so we reproduce most of its functionality.
+ */
 void modsso_set_cookie(request_rec *r, const char *cookie_name, 
                        const char *value, const char *path)
 {
-  char *cookie_value;
-  cookie_value = make_cookie_value(r, cookie_name, value, path, 0);
-  apr_table_setn(r->err_headers_out, "Set-Cookie", 
-                 apr_pstrdup(r->pool, cookie_value));
+  const char *rfc2109;
+
+  rfc2109 = apr_pstrcat(r->pool, cookie_name, "=", value, ";Path=", path, ";HttpOnly;Secure;Version=1", NULL);
+  apr_table_addn(r->headers_out, "Set-Cookie", rfc2109);
+  apr_table_addn(r->err_headers_out, "Set-Cookie", rfc2109);
 }
 
-void modsso_del_cookie(request_rec *r, const char *cookie_name)
+void modsso_del_cookie(request_rec *r, const char *cookie_name, const char *path)
 {
-  char cookie_value[512];
-  sprintf(cookie_value, "%s=; expires=Mar, 01-01-1971 00:00:00 GMT",
-          cookie_name);
-  apr_table_setn(r->err_headers_out, "Set-Cookie",
-                 apr_pstrdup(r->pool, cookie_value));
+  const char *rfc2109;
+
+  rfc2109 = apr_pstrcat(r->pool, cookie_name, "=;Path=", path, ";Version=1;Expires=Thu, 01 Jan 1970 00:00:00 GMT", NULL);
+  apr_table_addn(r->headers_out, "Set-Cookie", rfc2109);
+  apr_table_addn(r->err_headers_out, "Set-Cookie", rfc2109);
 }

src/mod_sso/test/httpd_integration_test.py

@@ -172,7 +172,7 @@ class HttpdIntegrationTestBase(unittest.TestCase):
             ("index -> redirect",
              {"url": "/index.html", 
               "status": 302, 
-              "location": "https://login.example.com/?s=service.example.com%2F&d=https%3A%2F%2Fservice.example.com%2Findex.html" + extra_groups}),
+              "location": "https://login.example.com/?s=service.example.com%2f&d=https%3a%2f%2fservice.example.com%2findex.html" + extra_groups}),
             ("index with cookie -> ok",
              {"url": "/index.html",
               "cookie": mkcookie(self._ticket()),
@@ -182,12 +182,12 @@ class HttpdIntegrationTestBase(unittest.TestCase):
              {"url": "/index.html",
               "cookie": mkcookie('blahblah' * 8),
               "status": 302,
-              "location": "https://login.example.com/?s=service.example.com%2F&d=https%3A%2F%2Fservice.example.com%2Findex.html" + extra_groups}),
+              "location": "https://login.example.com/?s=service.example.com%2f&d=https%3a%2f%2fservice.example.com%2findex.html" + extra_groups}),
 
             ("protected-user -> redirect",
              {"url": "/protected-user/index.html", 
               "status": 302, 
-              "location": "https://login.example.com/?s=service.example.com%2F&d=https%3A%2F%2Fservice.example.com%2Fprotected-user%2Findex.html" + extra_groups}),
+              "location": "https://login.example.com/?s=service.example.com%2f&d=https%3a%2f%2fservice.example.com%2fprotected-user%2findex.html" + extra_groups}),
             ("protected-user with cookie -> ok",
              {"url": "/protected-user/index.html",
               "cookie": mkcookie(self._ticket()),
@@ -201,7 +201,7 @@ class HttpdIntegrationTestBase(unittest.TestCase):
             ("protected-group -> redirect",
              {"url": "/protected-group/index.html", 
               "status": 302, 
-              "location": "https://login.example.com/?s=service.example.com%2F&d=https%3A%2F%2Fservice.example.com%2Fprotected-group%2Findex.html" + (extra_groups if extra_groups else "&g=group1")}),
+              "location": "https://login.example.com/?s=service.example.com%2f&d=https%3a%2f%2fservice.example.com%2fprotected-group%2findex.html" + (extra_groups if extra_groups else "&g=group1")}),
             ("protected-group with cookie -> ok",
              {"url": "/protected-group/index.html",
               "cookie": mkcookie(self._ticket()),
@@ -215,18 +215,18 @@ class HttpdIntegrationTestBase(unittest.TestCase):
              {"url": "/protected-group/index.html",
               "cookie": mkcookie(self._ticket(group="group2")),
               "status": 302,
-              "location": "https://login.example.com/?s=service.example.com%2F&d=https%3A%2F%2Fservice.example.com%2Fprotected-group%2Findex.html" + (extra_groups if extra_groups else "&g=group1")}),
+              "location": "https://login.example.com/?s=service.example.com%2f&d=https%3a%2f%2fservice.example.com%2fprotected-group%2findex.html" + (extra_groups if extra_groups else "&g=group1")}),
 
             ("other-service -> redirect",
              {"url": "/other-service/index.html",
               "status": 302,
               "http_host": "testhost.example.com",
-              "location": "https://login.example.com/?s=testhost.example.com%2Fother-service%2F&d=https%3A%2F%2Ftesthost.example.com%2Fother-service%2Findex.html" + extra_groups}),
+              "location": "https://login.example.com/?s=testhost.example.com%2fother-service%2f&d=https%3a%2f%2ftesthost.example.com%2fother-service%2findex.html" + extra_groups}),
 
             ("protected-htaccess -> redirect",
              {"url": "/protected-htaccess/index.html",
               "status": 302,
-              "location": "https://login.example.com/?s=service.example.com%2Fprotected-htaccess%2F&d=https%3A%2F%2Fservice.example.com%2Fprotected-htaccess%2Findex.html" + extra_groups}),
+              "location": "https://login.example.com/?s=service.example.com%2fprotected-htaccess%2f&d=https%3a%2f%2fservice.example.com%2fprotected-htaccess%2findex.html" + extra_groups}),
             ("protected-htaccess with cookie -> ok",
              {"url": "/protected-htaccess/index.html",
               "cookie": mkcookie(self._ticket(service="service.example.com/protected-htaccess/")),

src/mod_sso/test/session_unittest.cc

@@ -26,21 +26,36 @@ protected:
 };
 
 TEST_F(SessionTest, Serialize) {
-  string value = "test value";
+  const char *values[] = {
+    "a",
+    "ab",
+    "abc",
+    "abcd",
+    "abcde",
+    "abcdef",
+    "abcdefg",
+    "abcdefgh",
+    "abcdefghi",
+    "WokecMCoCgsAAAATnNUAAABA",
+    NULL,
+  };
   char *output = NULL;
-  ASSERT_EQ(0, modsso_session_serialize(pool_, key_, key_len_, value.c_str(),
-                                        value.size(), &output));
-  ASSERT_STRNE(NULL, output);
-  ASSERT_STRNE(NULL, strchr(output, '.'));
-
-  char *value2 = NULL;
-  size_t value2_len = 0;
-  ASSERT_EQ(0, modsso_session_deserialize(pool_, key_, key_len_, output,
-                                          &value2, &value2_len));
-  ASSERT_STRNE(NULL, value2);
-  ASSERT_NE((size_t)0, value2_len);
-
-  ASSERT_STREQ(value2, value.c_str());
+
+  for (int i = 0; values[i]; i++) {
+    ASSERT_EQ(0, modsso_session_serialize(pool_, key_, key_len_, values[i],
+                                          strlen(values[i]), &output));
+    ASSERT_STRNE(NULL, output);
+    ASSERT_STRNE(NULL, strchr(output, '.'));
+
+    char *value2 = NULL;
+    size_t value2_len = 0;
+    ASSERT_EQ(0, modsso_session_deserialize(pool_, key_, key_len_, output,
+                                            &value2, &value2_len));
+    ASSERT_STRNE(NULL, value2);
+    ASSERT_NE((size_t)0, value2_len);
+
+    ASSERT_STREQ(value2, values[i]);
+  }
 }
 
 } // namespace

src/mod_sso/test/sso_utils_unittest.cc

@@ -39,7 +39,7 @@ TEST_F(SSOUtilsTest, UrlDecode) {
 
 TEST_F(SSOUtilsTest, UrlEncode) {
   string sample("http://example.com"),
-    reference("http%3A%2F%2Fexample.com");
+    reference("http%3a%2f%2fexample.com");
   string result = modsso_url_encode(pool_, sample.c_str());
   EXPECT_EQ(reference, result);
 }
@@ -70,7 +70,7 @@ TEST_F(SSOUtilsTest, GetCookieEasy) {
   for (i = 0; i < (int)(sizeof(headers) / sizeof(char *)); i++) {
     apr_table_set(r.headers_in, "Cookie", headers[i]);
 
-    char *cookie = modsso_get_cookie(&r, "SSO");
+    const char *cookie = modsso_get_cookie(&r, "SSO");
     ASSERT_TRUE(cookie) << "cookie not found in: " << headers[i];
     ASSERT_EQ("1234", string(cookie))
       << "bad cookie parsed from " << headers[i] << ": " << cookie;