diff options
author | Jakub Hrozek <jhrozek@redhat.com> | 2017-03-29 22:49:09 +0200 |
---|---|---|
committer | Lukas Slebodnik <lslebodn@redhat.com> | 2017-03-30 18:20:19 +0200 |
commit | 7d73049884e3a96ca3b00b5bd4104f4edd6287ab (patch) | |
tree | 828845f30539759c7d1e0ac0a9eb9565df88511c /src | |
parent | 861ab44e8148208425b67c4711bc8fade10fd3ed (diff) | |
download | sssd-7d73049884e3a96ca3b00b5bd4104f4edd6287ab.tar.gz sssd-7d73049884e3a96ca3b00b5bd4104f4edd6287ab.tar.xz sssd-7d73049884e3a96ca3b00b5bd4104f4edd6287ab.zip |
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>
Diffstat (limited to 'src')
-rw-r--r-- | src/responder/kcm/kcmsrv_ccache_json.c | 43 | ||||
-rw-r--r-- | 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 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. */ |