KCM: Fix off-by-one error in secrets key parsing

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>

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 40b64861c..8199bc613 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 8eff2f501..108eaf556 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. */