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>

1 files changed, 26 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;
     }