From 5030710ff22bba4681e031d41c820bb85530f9d7 Mon Sep 17 00:00:00 2001 From: Philipp Eder Date: Mon, 10 May 2021 15:10:16 +0200 Subject: [PATCH] When hash or password are NULL return INVALID rather then ERR To make it easier to handle hash or passqord unset cases for the caller gvm-libs passwordbasedauthentication should not threat hash or password is a nullpointer as an error case but rather as a invalid password. --- CHANGELOG.md | 1 + util/passwordbasedauthentication.c | 16 ++++++++++++---- util/passwordbasedauthentication_tests.c | 14 +++++++++++++- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 462c9edb2..3969fcf50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Changed Use a char pointer instead of an zero-lenght array as kb_redis struct member. [443](https://github.com/greenbone/gvm-libs/pull/443) +- pba verify returns INVALID instead of ERR when hash or password are null [496](https://github.com/greenbone/gvm-libs/pull/496) ### Fixed - Fixing [#434](https://github.com/greenbone/gvm-libs/pull/434) by removing the extra parentheses in `base/networking.c` [#437](https://github.com/greenbone/gvm-libs/pull/437) diff --git a/util/passwordbasedauthentication.c b/util/passwordbasedauthentication.c index bc9855127..3eade52fd 100644 --- a/util/passwordbasedauthentication.c +++ b/util/passwordbasedauthentication.c @@ -27,6 +27,11 @@ // this shouldn't affect other implementations #define __USE_GNU #include +// INVALID_HASH is used on verify when the given hash is a NULL pointer. +// This is done to not directly jump to exit with a INVALID_HASH result +// but rather keep calculating to make it a little bit harder to guess +// if a user exists or not based on timing. +#define INVALID_HASH "1234567890$" #ifndef CRYPT_GENSALT_OUTPUT_SIZE #define CRYPT_GENSALT_OUTPUT_SIZE 192 #endif @@ -163,7 +168,7 @@ pba_is_phc_compliant (const char *setting) { if (setting == NULL) { - return 0; + return 1; } return strlen (setting) > 1 && setting[0] == '$'; } @@ -223,7 +228,7 @@ pba_verify_hash (const struct PBASettings *setting, const char *hash, struct crypt_data *data = NULL; int i = 0; enum pba_rc result = ERR; - if (!setting || !hash || !password) + if (!setting ) goto exit; if (!is_prefix_supported (setting->prefix)) goto exit; @@ -232,7 +237,7 @@ pba_verify_hash (const struct PBASettings *setting, const char *hash, data = calloc (1, sizeof (struct crypt_data)); // manipulate hash to reapply pepper tmp = malloc ( CRYPT_OUTPUT_SIZE); - strncpy(tmp, hash, CRYPT_OUTPUT_SIZE); + strncpy(tmp, hash ? hash : INVALID_HASH, CRYPT_OUTPUT_SIZE); cmp = strrchr (tmp, '$'); for (i = MAX_PEPPER_SIZE - 1; i > -1; i--) { @@ -240,7 +245,10 @@ pba_verify_hash (const struct PBASettings *setting, const char *hash, if (setting->pepper[i] != 0) cmp[0] = setting->pepper[i]; } - cmp = crypt_r (password, tmp, data); + // some crypt_r implementations cannot handle if password is a + // NULL pointer and run into SEGMENTATION faults. + // Therefore we set it to "" + cmp = crypt_r (password ? password : "", tmp, data); if (strcmp (tmp, cmp) == 0) result = VALID; else diff --git a/util/passwordbasedauthentication_tests.c b/util/passwordbasedauthentication_tests.c index 2e6afc9d6..bddcba480 100644 --- a/util/passwordbasedauthentication_tests.c +++ b/util/passwordbasedauthentication_tests.c @@ -33,7 +33,6 @@ AfterEach (PBA) Ensure (PBA, returns_false_on_not_phc_compliant_setting) { - assert_false(pba_is_phc_compliant(NULL)); assert_false(pba_is_phc_compliant("$")); assert_false(pba_is_phc_compliant("password")); } @@ -76,6 +75,17 @@ Ensure (PBA, verify_hash) free(hash); } +Ensure (PBA, verify_hash_returns_invalid_on_np_hash_np_password) +{ + struct PBASettings setting = { "4242" , 20000, "$6$"}; + char *hash; + hash = pba_hash(&setting, "*password"); + assert_not_equal(hash, NULL); + assert_equal(pba_verify_hash(&setting, NULL, "*password"), INVALID); + assert_equal(pba_verify_hash(&setting, hash, NULL), INVALID); +} + + Ensure (PBA, defaults) { int i; @@ -131,6 +141,8 @@ main (int argc, char **argv) unique_hash_without_adding_used_pepper); add_test_with_context (suite, PBA, verify_hash); + add_test_with_context (suite, PBA, + verify_hash_returns_invalid_on_np_hash_np_password); add_test_with_context (suite, PBA, handle_md5_hash); add_test_with_context (suite, PBA,