Validate ticket fields before signing

It's probably wise to verify that the ticket that we're about to sign is
syntactically correct. Required fields must be present, and values
should not contain separator characters.

src/sso/sso.c

@@ -251,6 +251,33 @@ int sso_generate_keys(unsigned char *publicp, unsigned char *secretp) {
   return crypto_sign_keypair(publicp, secretp);
 }
 
+static int sso_ticket_validate_fields(sso_ticket_t t) {
+  char **gp;
+
+  // Ensure that required fields are set.
+  if ((t->user == NULL) || (t->service == NULL) || (t->domain == NULL)) {
+    return SSO_ERR_MISSING_REQUIRED_FIELD;
+  }
+
+  // Check the syntax of the fields. The only requirement is that they
+  // do not contain the separator character, besides that anything
+  // will do. The (better) alternative would be to escape the values.
+  if ((strchr(t->user, FIELD_SEP_CH) != NULL) ||
+      (strchr(t->service, FIELD_SEP_CH) != NULL) ||
+      (strchr(t->domain, FIELD_SEP_CH) != NULL)) {
+    return SSO_ERR_INVALID_FIELD;
+  }
+  if (t->groups != NULL) {
+    for (gp = t->groups; *gp; gp++) {
+      if ((strchr(*gp, FIELD_SEP_CH) != NULL) ||
+          strchr(*gp, GROUP_SEP_CH) != NULL) {
+        return SSO_ERR_INVALID_FIELD;
+      }
+    }
+  }
+  return SSO_OK;
+}
+
 int sso_ticket_sign(sso_ticket_t t, const unsigned char *secret_key, char *out,
                     size_t out_size) {
   char *serialized;
@@ -259,6 +286,11 @@ int sso_ticket_sign(sso_ticket_t t, const unsigned char *secret_key, char *out,
   unsigned long long signed_size;
   int r;
 
+  r = sso_ticket_validate_fields(t);
+  if (r != SSO_OK) {
+    return r;
+  }
+
   serialized = sso_ticket_serialize(t);
   if (serialized == NULL) {
     return SSO_ERR_SERIALIZATION;
@@ -360,6 +392,10 @@ const char *sso_strerror(int err) {
     return "invalid ticket (bad domain)";
   case SSO_ERR_NO_MATCHING_GROUPS:
     return "no matching groups";
+  case SSO_ERR_MISSING_REQUIRED_FIELD:
+    return "missing a required field";
+  case SSO_ERR_INVALID_FIELD:
+    return "a field has invalid syntax";
   default:
     return "unknown error";
   }

src/sso/sso.h

@@ -21,6 +21,8 @@ extern "C" {
 #define SSO_ERR_BAD_DOMAIN -9
 #define SSO_ERR_NO_MATCHING_GROUPS -10
 #define SSO_ERR_DECODE64 -11
+#define SSO_ERR_MISSING_REQUIRED_FIELD -13
+#define SSO_ERR_INVALID_FIELD -14
 
 #define SSO_PUBLIC_KEY_SIZE 32
 #define SSO_SECRET_KEY_SIZE 64

src/sso/test/sso_unittest.cc

@@ -12,7 +12,8 @@ TEST(Base64, EncodeOk) {
   const char *in = "tests";
   char out[128] = {0};
   size_t sz = sizeof(out);
-  EXPECT_EQ(0, sso_base64_encode((unsigned char *)out, &sz, (unsigned char *)in, (size_t)strlen(in)));
+  EXPECT_EQ(0, sso_base64_encode((unsigned char *)out, &sz, (unsigned char *)in,
+                                 (size_t)strlen(in)));
   EXPECT_EQ(8, int(sso_base64_encode_size((size_t)strlen(in))));
   EXPECT_EQ(8, int(sz));
   EXPECT_EQ("dGVzdHM=", std::string(out));
@@ -22,23 +23,20 @@ TEST(Base64, DecodeOk) {
   char in[] = "dGVzdHM=";
   char out[128] = {0};
   size_t sz = sizeof(out);
-  EXPECT_EQ(0, sso_base64_decode((unsigned char*)out, &sz, (unsigned char *)in, (size_t)strlen(in)));
+  EXPECT_EQ(0, sso_base64_decode((unsigned char *)out, &sz, (unsigned char *)in,
+                                 (size_t)strlen(in)));
   EXPECT_EQ(5, int(sz));
   EXPECT_EQ("tests", std::string(out));
 }
 
 class SSO : public testing::Test {
 protected:
-
-  virtual void SetUp() {
-    sso_generate_keys(public_key, secret_key);
-  }
+  virtual void SetUp() { sso_generate_keys(public_key, secret_key); }
 
   // Return a signed ticket, for test data generation.
   char *sign_ticket(sso_ticket_t t) {
     char buf[1024];
-    EXPECT_EQ(0, sso_ticket_sign(t, secret_key,
-                                 buf, sizeof(buf)));
+    EXPECT_EQ(0, sso_ticket_sign(t, secret_key, buf, sizeof(buf)));
     // No further use for the original ticket.
     sso_ticket_free(t);
     return strdup(buf);
@@ -46,12 +44,10 @@ protected:
 
   // Sign a ticket with a random secret key.
   char *sign_ticket_with_random_key(sso_ticket_t t) {
-    unsigned char pk[SSO_PUBLIC_KEY_SIZE],
-      sk[SSO_SECRET_KEY_SIZE];
+    unsigned char pk[SSO_PUBLIC_KEY_SIZE], sk[SSO_SECRET_KEY_SIZE];
     sso_generate_keys(pk, sk);
     char buf[1024];
-    EXPECT_EQ(0, sso_ticket_sign(t, sk,
-                                 buf, sizeof(buf)));
+    EXPECT_EQ(0, sso_ticket_sign(t, sk, buf, sizeof(buf)));
     // No further use for the original ticket.
     sso_ticket_free(t);
     return strdup(buf);
@@ -64,12 +60,10 @@ protected:
     size_t sz = sizeof(buf);
     unsigned long long encsz = sizeof(encbuf);
 
-    crypto_sign(encbuf, &encsz,
-                (const unsigned char *)contents,
+    crypto_sign(encbuf, &encsz, (const unsigned char *)contents,
                 strlen(contents), secret_key);
 
-    sso_base64_encode((unsigned char *)buf, &sz,
-                      encbuf, encsz);
+    sso_base64_encode((unsigned char *)buf, &sz, encbuf, encsz);
     return strdup((const char *)buf);
   }
 
@@ -77,11 +71,41 @@ protected:
   unsigned char secret_key[SSO_SECRET_KEY_SIZE];
 };
 
+struct sign_testdata {
+  sso_ticket_t tkt;
+  int expected_result;
+};
+
 TEST_F(SSO, Sign) {
-  sso_ticket_t t = sso_ticket_new("user", "service/", "domain", NULL, 7200);
+  struct sign_testdata td[] = {
+      {sso_ticket_new("user", "service/", "domain", NULL, 7200), 0},
+
+      {sso_ticket_new(NULL, NULL, NULL, NULL, 7200),
+       SSO_ERR_MISSING_REQUIRED_FIELD},
+      {sso_ticket_new(NULL, "service", "domain", NULL, 7200),
+       SSO_ERR_MISSING_REQUIRED_FIELD},
+      {sso_ticket_new("user", NULL, "domain", NULL, 7200),
+       SSO_ERR_MISSING_REQUIRED_FIELD},
+      {sso_ticket_new("user", "service", NULL, NULL, 7200),
+       SSO_ERR_MISSING_REQUIRED_FIELD},
+
+      {sso_ticket_new("u|ser", "service/", "domain", NULL, 7200),
+       SSO_ERR_INVALID_FIELD},
+      {sso_ticket_new("user", "s|ervice/", "domain", NULL, 7200),
+       SSO_ERR_INVALID_FIELD},
+      {sso_ticket_new("user", "service/", "d|omain", NULL, 7200),
+       SSO_ERR_INVALID_FIELD},
+
+      {NULL, 0},
+  };
   char buf[1024];
-  EXPECT_EQ(0, sso_ticket_sign(t, secret_key, buf, sizeof(buf)));
-  sso_ticket_free(t);
+
+  for (struct sign_testdata *tdp = td; tdp->tkt; tdp++) {
+    EXPECT_EQ(tdp->expected_result,
+              sso_ticket_sign(tdp->tkt, secret_key, buf, sizeof(buf)))
+        << "Failed test: #" << (tdp - td);
+    sso_ticket_free(tdp->tkt);
+  }
 }
 
 struct open_testdata {
@@ -90,19 +114,22 @@ struct open_testdata {
 };
 
 TEST_F(SSO, Open) {
-  const char *groups[] = {
-    "users", "wheel", "daemon", NULL
-  };
+  const char *groups[] = {"users", "wheel", "daemon", NULL};
   struct open_testdata td[] = {
-    {sign_ticket(sso_ticket_new("user", "service/", "domain", NULL, 7200)), 0},
-    {sign_ticket(sso_ticket_new("user", "service/", "domain", groups, 7200)), 0},
+      {sign_ticket(sso_ticket_new("user", "service/", "domain", NULL, 7200)),
+       0},
+      {sign_ticket(sso_ticket_new("user", "service/", "domain", groups, 7200)),
+       0},
       {sign_ticket(sso_ticket_new("user", "", "", NULL, 7200)), 0},
 
-    {sign_string("4|user|service/|domain|1414402999|"), SSO_ERR_UNSUPPORTED_VERSION},
+      {sign_string("4|user|service/|domain|1414402999|"),
+       SSO_ERR_UNSUPPORTED_VERSION},
       {sign_string("3|definitely not a ticket"), SSO_ERR_DESERIALIZATION},
       {sign_string("3|||||"), 0},
 
-    {sign_ticket_with_random_key(sso_ticket_new("user", "service/", "domain", NULL, 7200)), SSO_ERR_BAD_SIGNATURE},
+      {sign_ticket_with_random_key(
+           sso_ticket_new("user", "service/", "domain", NULL, 7200)),
+       SSO_ERR_BAD_SIGNATURE},
 
       {strdup("not a base64-encoded string"), SSO_ERR_DECODE64},
       {strdup("\377\377\377"), SSO_ERR_DECODE64},
@@ -144,7 +171,7 @@ TEST_F(SSO, InternalSerializationBufferFull) {
   // Ticket with a very big field that should exceed the size of the
   // internal serialization buffer.
   char *big = big_string();
-  sso_ticket_t t = sso_ticket_new(big, NULL, NULL, NULL, 7200);
+  sso_ticket_t t = sso_ticket_new(big, "service", "domain", NULL, 7200);
   free(big);
   EXPECT_EQ(SSO_ERR_SERIALIZATION,
             sso_ticket_sign(t, secret_key, buf, sizeof(buf)));
@@ -169,8 +196,7 @@ TEST_F(SSO, SerializationOutputBufferFull) {
   char buf[1024];
   memset(buf, 42, sizeof(buf));
 
-  EXPECT_EQ(SSO_ERR_BUFFER_TOO_SMALL,
-            sso_ticket_sign(t, secret_key, buf, 16));
+  EXPECT_EQ(SSO_ERR_BUFFER_TOO_SMALL, sso_ticket_sign(t, secret_key, buf, 16));
 
   for (int i = 16; i < 1024; i++) {
     EXPECT_EQ(42, buf[i]) << "Detected overflow at pos " << i;
@@ -206,27 +232,35 @@ TEST_F(SSO, Validation) {
 
   struct validate_testdata td[] = {
       {"ticket_with_groups, validate_groups",
-     sso_ticket_new("user", "service/", "domain", groups_ok, 7200), &with_groups, 0},
+       sso_ticket_new("user", "service/", "domain", groups_ok, 7200),
+       &with_groups, 0},
       {"ticket_with_groups, validate_no_groups",
-     sso_ticket_new("user", "service/", "domain", groups_ok, 7200), &without_groups, 0},
+       sso_ticket_new("user", "service/", "domain", groups_ok, 7200),
+       &without_groups, 0},
       {"ticket_without_groups, validate_groups",
-     sso_ticket_new("user", "service/", "domain", NULL, 7200), &with_groups, SSO_ERR_NO_MATCHING_GROUPS},
+       sso_ticket_new("user", "service/", "domain", NULL, 7200), &with_groups,
+       SSO_ERR_NO_MATCHING_GROUPS},
       {"ticket_without_groups, validate_no_groups",
-     sso_ticket_new("user", "service/", "domain", NULL, 7200), &without_groups, 0},
+       sso_ticket_new("user", "service/", "domain", NULL, 7200),
+       &without_groups, 0},
       {"ticket_with_bad_groups",
-     sso_ticket_new("user", "service/", "domain", groups_fail, 7200), &with_groups, SSO_ERR_NO_MATCHING_GROUPS},
+       sso_ticket_new("user", "service/", "domain", groups_fail, 7200),
+       &with_groups, SSO_ERR_NO_MATCHING_GROUPS},
       {"bad_domain",
-     sso_ticket_new("user", "service/", "other", groups_ok, 7200), &with_groups, SSO_ERR_BAD_DOMAIN},
+       sso_ticket_new("user", "service/", "other", groups_ok, 7200),
+       &with_groups, SSO_ERR_BAD_DOMAIN},
       {"bad_service",
-     sso_ticket_new("user", "other/", "domain", groups_ok, 7200), &with_groups, SSO_ERR_BAD_SERVICE},
-    {"expired",
-     sso_ticket_new("user", "service/", "domain", NULL, -1000), &without_groups, SSO_ERR_EXPIRED},
+       sso_ticket_new("user", "other/", "domain", groups_ok, 7200),
+       &with_groups, SSO_ERR_BAD_SERVICE},
+      {"expired", sso_ticket_new("user", "service/", "domain", NULL, -1000),
+       &without_groups, SSO_ERR_EXPIRED},
       {NULL, NULL, NULL, 0},
   };
 
   for (struct validate_testdata *tdp = td; tdp->t; tdp++) {
     EXPECT_EQ(tdp->expected_result,
-              sso_validate(tdp->t, tdp->c->service, tdp->c->domain, tdp->c->ok_groups))
+              sso_validate(tdp->t, tdp->c->service, tdp->c->domain,
+                           tdp->c->ok_groups))
         << "Validation failed: " << tdp->descr;
     sso_ticket_free(tdp->t);
   }