From bd5889c893a1fa956269f9acba4771af043bfea4 Mon Sep 17 00:00:00 2001
From: ale <ale@incal.net>
Date: Wed, 18 Jan 2017 22:19:51 +0000
Subject: [PATCH] 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 | 36 ++++++++
src/sso/sso.h | 2 +
src/sso/test/sso_unittest.cc | 162 +++++++++++++++++++++--------------
3 files changed, 136 insertions(+), 64 deletions(-)
diff --git a/src/sso/sso.c b/src/sso/sso.c
index 297a608..506afa9 100644
--- a/src/sso/sso.c
+++ b/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";
}
diff --git a/src/sso/sso.h b/src/sso/sso.h
index ba93ff7..9d339c4 100644
--- a/src/sso/sso.h
+++ b/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
diff --git a/src/sso/test/sso_unittest.cc b/src/sso/test/sso_unittest.cc
index 3afa09e..c684a46 100644
--- a/src/sso/test/sso_unittest.cc
+++ b/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,33 +114,36 @@ 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", "", "", NULL, 7200)), 0},
-
- {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},
-
- {strdup("not a base64-encoded string"), SSO_ERR_DECODE64},
- {strdup("\377\377\377"), SSO_ERR_DECODE64},
- {strdup("dGVzdHM="), SSO_ERR_BAD_SIGNATURE},
- {strdup(""), SSO_ERR_BAD_SIGNATURE},
-
- {NULL, 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("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},
+
+ {strdup("not a base64-encoded string"), SSO_ERR_DECODE64},
+ {strdup("\377\377\377"), SSO_ERR_DECODE64},
+ {strdup("dGVzdHM="), SSO_ERR_BAD_SIGNATURE},
+ {strdup(""), SSO_ERR_BAD_SIGNATURE},
+
+ {NULL, 0},
};
for (struct open_testdata *tdp = td; tdp->ticket_str; tdp++) {
sso_ticket_t t2 = NULL;
EXPECT_EQ(tdp->expected_result,
sso_ticket_open(&t2, tdp->ticket_str, public_key))
- << "Failed decoding ticket: " << tdp->ticket_str;
+ << "Failed decoding ticket: " << tdp->ticket_str;
if (tdp->expected_result == 0) {
EXPECT_TRUE(t2);
} else {
@@ -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)));
@@ -152,7 +179,7 @@ TEST_F(SSO, InternalSerializationBufferFull) {
// Ticket with a very large list of groups.
int ng = 255;
- const char *groups[ng+1];
+ const char *groups[ng + 1];
for (int i = 0; i < ng; i++)
groups[i] = "aaaaaaaaaaaaaaaa";
groups[ng] = NULL;
@@ -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;
@@ -198,36 +224,44 @@ TEST_F(SSO, Validation) {
const char *groups_fail[] = {"users", "others", NULL};
struct validate_testcontext with_groups = {
- "service/", "domain", allowed_groups,
+ "service/", "domain", allowed_groups,
};
struct validate_testcontext without_groups = {
- "service/", "domain", NULL,
+ "service/", "domain", NULL,
};
struct validate_testdata td[] = {
- {"ticket_with_groups, validate_groups",
- 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},
- {"ticket_without_groups, validate_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},
- {"ticket_with_bad_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},
- {"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},
- {NULL, NULL, NULL, 0},
+ {"ticket_with_groups, validate_groups",
+ 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},
+ {"ticket_without_groups, validate_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},
+ {"ticket_with_bad_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},
+ {"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},
+ {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))
- << "Validation failed: " << tdp->descr;
+ sso_validate(tdp->t, tdp->c->service, tdp->c->domain,
+ tdp->c->ok_groups))
+ << "Validation failed: " << tdp->descr;
sso_ticket_free(tdp->t);
}
}
--
GitLab