From 088be07a9e5aae54379a7f98e9e4615cd4451501 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek 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 (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