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