From 088be07a9e5aae54379a7f98e9e4615cd4451501 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek@redhat.com>
Date: Wed, 29 Mar 2017 22:49:09 +0200
Subject: [PATCH 73/90] KCM: Fix off-by-one error in secrets key parsing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When parsing the secrets key, the code tried to protect against malformed keys
or keys that are too short, but it did an error - the UUID stringified
form is 36 bytes long, so the UUID_STR_SIZE is 37 because UUID_STR_SIZE
accounts for the null terminator.
But the code, that was trying to assert that there are two characters after
the UUID string (separator and at least a single character for the name)
didn't take the NULL terminator (which strlen() doesn't return) into
account and ended up rejecting all ccaches whose name is only a single
character.
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>
(cherry picked from commit 7d73049884e3a96ca3b00b5bd4104f4edd6287ab)
---
src/responder/kcm/kcmsrv_ccache_json.c | 43 +++++++++-------
src/tests/cmocka/test_kcm_json_marshalling.c | 75 ++++++++++++++++++++++++++++
2 files changed, 101 insertions(+), 17 deletions(-)
diff --git a/src/responder/kcm/kcmsrv_ccache_json.c b/src/responder/kcm/kcmsrv_ccache_json.c
index 40b64861c209206d6f60ccd0843857edee24a844..8199bc613e4204859438e1cd820f3f4b2123dd7e 100644
--- a/src/responder/kcm/kcmsrv_ccache_json.c
+++ b/src/responder/kcm/kcmsrv_ccache_json.c
@@ -109,6 +109,28 @@ static const char *sec_key_create(TALLOC_CTX *mem_ctx,
"%s%c%s", uuid_str, SEC_KEY_SEPARATOR, name);
}
+static bool sec_key_valid(const char *sec_key)
+{
+ if (sec_key == NULL) {
+ return false;
+ }
+
+ if (strlen(sec_key) < UUID_STR_SIZE + 1) {
+ /* One char for separator (at UUID_STR_SIZE, because strlen doesn't
+ * include the '\0', but UUID_STR_SIZE does) and at least one for
+ * the name */
+ DEBUG(SSSDBG_CRIT_FAILURE, "Key %s is too short\n", sec_key);
+ return false;
+ }
+
+ if (sec_key[UUID_STR_SIZE - 1] != SEC_KEY_SEPARATOR) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Key doesn't contain the separator\n");
+ return false;
+ }
+
+ return true;
+}
+
static errno_t sec_key_parse(TALLOC_CTX *mem_ctx,
const char *sec_key,
const char **_name,
@@ -116,9 +138,7 @@ static errno_t sec_key_parse(TALLOC_CTX *mem_ctx,
{
char uuid_str[UUID_STR_SIZE];
- if (strlen(sec_key) < UUID_STR_SIZE + 2) {
- /* One char for separator and at least one for the name */
- DEBUG(SSSDBG_CRIT_FAILURE, "Key %s is too short\n", sec_key);
+ if (!sec_key_valid(sec_key)) {
return EINVAL;
}
@@ -143,14 +163,7 @@ errno_t sec_key_get_uuid(const char *sec_key,
{
char uuid_str[UUID_STR_SIZE];
- if (strlen(sec_key) < UUID_STR_SIZE + 2) {
- /* One char for separator and at least one for the name */
- DEBUG(SSSDBG_CRIT_FAILURE, "Key %s is too short\n", sec_key);
- return EINVAL;
- }
-
- if (sec_key[UUID_STR_SIZE-1] != SEC_KEY_SEPARATOR) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Key doesn't contain the separator\n");
+ if (!sec_key_valid(sec_key)) {
return EINVAL;
}
@@ -162,9 +175,7 @@ errno_t sec_key_get_uuid(const char *sec_key,
const char *sec_key_get_name(const char *sec_key)
{
- if (strlen(sec_key) < UUID_STR_SIZE + 2) {
- /* One char for separator and at least one for the name */
- DEBUG(SSSDBG_CRIT_FAILURE, "Key %s is too short\n", sec_key);
+ if (!sec_key_valid(sec_key)) {
return NULL;
}
@@ -174,9 +185,7 @@ const char *sec_key_get_name(const char *sec_key)
bool sec_key_match_name(const char *sec_key,
const char *name)
{
- if (strlen(sec_key) < UUID_STR_SIZE + 2) {
- /* One char for separator and at least one for the name */
- DEBUG(SSSDBG_MINOR_FAILURE, "Key %s is too short\n", sec_key);
+ if (!sec_key_valid(sec_key) || name == NULL) {
return false;
}
diff --git a/src/tests/cmocka/test_kcm_json_marshalling.c b/src/tests/cmocka/test_kcm_json_marshalling.c
index 8eff2f501066c70a8730cd3d4dc41b92d7a03e4c..108eaf55628029a6de8c23cd6486bdccc42c0364 100644
--- a/src/tests/cmocka/test_kcm_json_marshalling.c
+++ b/src/tests/cmocka/test_kcm_json_marshalling.c
@@ -32,6 +32,12 @@
#define TEST_CREDS "TESTCREDS"
+#define TEST_UUID_STR "5f8f296b-02be-4e86-9235-500e82354186"
+#define TEST_SEC_KEY_ONEDIGIT TEST_UUID_STR"-0"
+#define TEST_SEC_KEY_MULTIDIGITS TEST_UUID_STR"-123456"
+
+#define TEST_SEC_KEY_NOSEP TEST_UUID_STR"+0"
+
const struct kcm_ccdb_ops ccdb_mem_ops;
const struct kcm_ccdb_ops ccdb_sec_ops;
@@ -188,6 +194,72 @@ static void test_kcm_ccache_marshall_unmarshall(void **state)
assert_int_equal(ret, EOK);
assert_cc_equal(cc, cc2);
+
+ /* This key is exactly one byte shorter than it should be */
+ ret = sec_kv_to_ccache(test_ctx,
+ TEST_UUID_STR"-",
+ (const char *) data,
+ &owner,
+ &cc2);
+ assert_int_equal(ret, EINVAL);
+}
+
+void test_sec_key_get_uuid(void **state)
+{
+ errno_t ret;
+ uuid_t uuid;
+ char str_uuid[UUID_STR_SIZE];
+
+ uuid_clear(uuid);
+ ret = sec_key_get_uuid(TEST_SEC_KEY_ONEDIGIT, uuid);
+ assert_int_equal(ret, EOK);
+ uuid_unparse(uuid, str_uuid);
+ assert_string_equal(TEST_UUID_STR, str_uuid);
+
+ ret = sec_key_get_uuid(TEST_SEC_KEY_NOSEP, uuid);
+ assert_int_equal(ret, EINVAL);
+
+ ret = sec_key_get_uuid(TEST_UUID_STR, uuid);
+ assert_int_equal(ret, EINVAL);
+
+ ret = sec_key_get_uuid(NULL, uuid);
+ assert_int_equal(ret, EINVAL);
+}
+
+void test_sec_key_get_name(void **state)
+{
+ const char *name;
+
+ name = sec_key_get_name(TEST_SEC_KEY_ONEDIGIT);
+ assert_non_null(name);
+ assert_string_equal(name, "0");
+
+ name = sec_key_get_name(TEST_SEC_KEY_MULTIDIGITS);
+ assert_non_null(name);
+ assert_string_equal(name, "123456");
+
+ name = sec_key_get_name(TEST_UUID_STR);
+ assert_null(name);
+
+ name = sec_key_get_name(TEST_SEC_KEY_NOSEP);
+ assert_null(name);
+
+ name = sec_key_get_name(NULL);
+ assert_null(name);
+}
+
+void test_sec_key_match_name(void **state)
+{
+ assert_true(sec_key_match_name(TEST_SEC_KEY_ONEDIGIT, "0"));
+ assert_true(sec_key_match_name(TEST_SEC_KEY_MULTIDIGITS, "123456"));
+
+ assert_false(sec_key_match_name(TEST_SEC_KEY_MULTIDIGITS, "0"));
+ assert_false(sec_key_match_name(TEST_SEC_KEY_ONEDIGIT, "123456"));
+
+ assert_false(sec_key_match_name(TEST_UUID_STR, "0"));
+ assert_false(sec_key_match_name(TEST_SEC_KEY_NOSEP, "0"));
+ assert_false(sec_key_match_name(TEST_SEC_KEY_ONEDIGIT, NULL));
+ assert_false(sec_key_match_name(NULL, "0"));
}
int main(int argc, const char *argv[])
@@ -205,6 +277,9 @@ int main(int argc, const char *argv[])
cmocka_unit_test_setup_teardown(test_kcm_ccache_marshall_unmarshall,
setup_kcm_marshalling,
teardown_kcm_marshalling),
+ cmocka_unit_test(test_sec_key_get_uuid),
+ cmocka_unit_test(test_sec_key_get_name),
+ cmocka_unit_test(test_sec_key_match_name),
};
/* Set debug level to invalid value so we can deside if -d 0 was used. */
--
2.9.3