From 6948aea62660e7406b7afbd9fe1c8e90e7f13c12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Helleu?= Date: Sat, 30 May 2026 19:00:59 +0200 Subject: [PATCH] relay: fix timing attack on password authentication (GHSA-vhv8-g2r9-cwcc) The relay authentication used non-constant-time comparisons (strcasecmp, strcmp) to verify password hashes and plaintext passwords, allowing an attacker to derive the expected hash byte-by-byte from response timing and then authenticate without knowing the password. - SHA/PBKDF2 hex hash comparisons: normalize the client-supplied hash to uppercase and compare in constant time over the fixed expected length. - Plaintext password comparison: HMAC-SHA256 both passwords with a fresh per-call random key and compare the fixed-size MACs in constant time, hiding both per-byte timing and the password length. Add string_memcmp_constant_time helper in core, exposed via the plugin API. Bump WEECHAT_PLUGIN_API_VERSION accordingly. --- CHANGELOG.md | 1 + src/core/core-string.c | 37 ++++++++++++ src/core/core-string.h | 2 + src/plugins/plugin.c | 1 + src/plugins/relay/relay-auth.c | 89 ++++++++++++++++++++++++---- src/plugins/weechat-plugin.h | 7 ++- tests/unit/core/test-core-string.cpp | 32 ++++++++++ 7 files changed, 158 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 516be7e2f..6cf2adc67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ SPDX-License-Identifier: GPL-3.0-or-later - irc: fix tag in message with list of names when joining a channel - fset: remove error displayed in core buffer when clicking with the mouse below the last option displayed - relay: limit size of decompressed websocket frame with permessage-deflate to prevent memory exhaustion ([GHSA-v2v4-45wm-5cr3](https://github.com/weechat/weechat/security/advisories/GHSA-v2v4-45wm-5cr3)) +- relay: fix timing attack on password authentication ([GHSA-vhv8-g2r9-cwcc](https://github.com/weechat/weechat/security/advisories/GHSA-vhv8-g2r9-cwcc)) ## Version 4.9.0 (2026-03-29) diff --git a/src/core/core-string.c b/src/core/core-string.c index f1926141b..586e6305c 100644 --- a/src/core/core-string.c +++ b/src/core/core-string.c @@ -923,6 +923,43 @@ string_strcmp_ignore_chars (const char *string1, const char *string2, string_charcasecmp (string1, string2); } +/* + * Compare two memory areas of the same size in constant time. + * + * Use to compare secrets (e.g. password hashes, MACs) without leaking + * information through the comparison's running time. The loop always + * walks the full "size" bytes and uses only bitwise operations on the + * data, so the execution time depends on "size" alone, not on the + * position of the first differing byte. + * + * If either pointer is NULL, the areas are considered different (the + * NULL check itself is not constant time but does not look at any + * secret content). + * + * Return: + * 0: areas are equal + * 1: areas differ + */ + +int +string_memcmp_constant_time (const void *area1, const void *area2, size_t size) +{ + const unsigned char *p1, *p2; + unsigned char diff; + size_t i; + + if (!area1 || !area2) + return (area1 == area2) ? 0 : 1; + + p1 = (const unsigned char *)area1; + p2 = (const unsigned char *)area2; + diff = 0; + for (i = 0; i < size; i++) + diff |= p1[i] ^ p2[i]; + + return (diff == 0) ? 0 : 1; +} + /* * Search for a string in another string (locale and case independent). * diff --git a/src/core/core-string.h b/src/core/core-string.h index 1be00011b..74a870e63 100644 --- a/src/core/core-string.h +++ b/src/core/core-string.h @@ -69,6 +69,8 @@ extern int string_strcmp_ignore_chars (const char *string1, const char *string2, const char *chars_ignored, int case_sensitive); +extern int string_memcmp_constant_time (const void *area1, const void *area2, + size_t size); extern const char *string_strcasestr (const char *string, const char *search); extern int string_match (const char *string, const char *mask, int case_sensitive); diff --git a/src/plugins/plugin.c b/src/plugins/plugin.c index d25661bfa..91f4a1110 100644 --- a/src/plugins/plugin.c +++ b/src/plugins/plugin.c @@ -624,6 +624,7 @@ plugin_load (const char *filename, int init_plugin, int argc, char **argv) new_plugin->strncasecmp = &string_strncasecmp; new_plugin->strncasecmp_range = &string_strncasecmp_range; new_plugin->strcmp_ignore_chars = &string_strcmp_ignore_chars; + new_plugin->string_memcmp_constant_time = &string_memcmp_constant_time; new_plugin->strcasestr = &string_strcasestr; new_plugin->strlen_screen = &gui_chat_strlen_screen; new_plugin->string_match = &string_match; diff --git a/src/plugins/relay/relay-auth.c b/src/plugins/relay/relay-auth.c index c08bb8866..5abe1e6a5 100644 --- a/src/plugins/relay/relay-auth.c +++ b/src/plugins/relay/relay-auth.c @@ -116,6 +116,12 @@ relay_auth_check_password_plain (struct t_relay_client *client, const char *password, const char *relay_password) { + unsigned char key[32]; + char hmac_password[64], hmac_relay[64]; + char *buf_password, *buf_relay; + int buf_password_size, buf_relay_size; + int hmac_password_size, hmac_relay_size, rc; + if (!client || !password || !relay_password) return -2; @@ -127,7 +133,47 @@ relay_auth_check_password_plain (struct t_relay_client *client, return -1; } - return (strcmp (password, relay_password) == 0) ? 0 : -2; + /* + * Compare passwords in constant time to defeat timing attacks: HMAC + * both sides with a fresh random key, then compare the fixed-size + * MACs. This hides both the per-byte comparison and the password + * length from the attacker. + * + * Both messages are prefixed with a zero byte so that empty + * passwords still produce a valid HMAC (the underlying crypto API + * rejects zero-length messages); the prefix is identical on both + * sides so equal inputs still yield equal MACs. + */ + rc = -2; + buf_password_size = strlen (password) + 1; + buf_relay_size = strlen (relay_password) + 1; + buf_password = malloc (buf_password_size); + buf_relay = malloc (buf_relay_size); + if (buf_password && buf_relay) + { + buf_password[0] = 0; + memcpy (buf_password + 1, password, buf_password_size - 1); + buf_relay[0] = 0; + memcpy (buf_relay + 1, relay_password, buf_relay_size - 1); + gcry_create_nonce (key, sizeof (key)); + if (weechat_crypto_hmac (key, sizeof (key), + buf_password, buf_password_size, + "sha256", + hmac_password, &hmac_password_size) + && weechat_crypto_hmac (key, sizeof (key), + buf_relay, buf_relay_size, + "sha256", + hmac_relay, &hmac_relay_size) + && (hmac_password_size == hmac_relay_size) + && (weechat_string_memcmp_constant_time ( + hmac_password, hmac_relay, hmac_password_size) == 0)) + { + rc = 0; + } + } + free (buf_password); + free (buf_relay); + return rc; } /* @@ -347,8 +393,9 @@ relay_auth_check_hash_sha (const char *hash_algo, const char *hash_sha, const char *relay_password) { - char *salt_password, hash[512 / 8], hash_hexa[((512 / 8) * 2) + 1]; - int rc, length, hash_size; + char *salt_password, *hash_sha_upper, hash[512 / 8]; + char hash_hexa[((512 / 8) * 2) + 1]; + int rc, length, hash_size, hash_hexa_len; rc = 0; @@ -366,10 +413,23 @@ relay_auth_check_hash_sha (const char *hash_algo, hash_algo, hash, &hash_size)) { - weechat_string_base_encode ("16", hash, hash_size, - hash_hexa); - if (weechat_strcasecmp (hash_hexa, hash_sha) == 0) + hash_hexa_len = weechat_string_base_encode ("16", hash, hash_size, + hash_hexa); + /* + * Compare in constant time to defeat timing attacks: the + * client-supplied hash is normalized to uppercase to match + * the output of base16 encoding, then compared byte-for-byte + * with no early exit. + */ + hash_sha_upper = weechat_string_toupper (hash_sha); + if (hash_sha_upper + && ((int)strlen (hash_sha_upper) == hash_hexa_len) + && (weechat_string_memcmp_constant_time ( + hash_hexa, hash_sha_upper, hash_hexa_len) == 0)) + { rc = 1; + } + free (hash_sha_upper); } free (salt_password); } @@ -393,8 +453,8 @@ relay_auth_check_hash_pbkdf2 (const char *hash_pbkdf2_algo, const char *hash_pbkdf2, const char *relay_password) { - char hash[512 / 8], hash_hexa[((512 / 8) * 2) + 1]; - int rc, hash_size; + char *hash_pbkdf2_upper, hash[512 / 8], hash_hexa[((512 / 8) * 2) + 1]; + int rc, hash_size, hash_hexa_len; rc = 0; @@ -407,9 +467,18 @@ relay_auth_check_hash_pbkdf2 (const char *hash_pbkdf2_algo, iterations, hash, &hash_size)) { - weechat_string_base_encode ("16", hash, hash_size, hash_hexa); - if (weechat_strcasecmp (hash_hexa, hash_pbkdf2) == 0) + hash_hexa_len = weechat_string_base_encode ("16", hash, hash_size, + hash_hexa); + /* see relay_auth_check_hash_sha for rationale */ + hash_pbkdf2_upper = weechat_string_toupper (hash_pbkdf2); + if (hash_pbkdf2_upper + && ((int)strlen (hash_pbkdf2_upper) == hash_hexa_len) + && (weechat_string_memcmp_constant_time ( + hash_hexa, hash_pbkdf2_upper, hash_hexa_len) == 0)) + { rc = 1; + } + free (hash_pbkdf2_upper); } } diff --git a/src/plugins/weechat-plugin.h b/src/plugins/weechat-plugin.h index dcbadeb57..7d644a657 100644 --- a/src/plugins/weechat-plugin.h +++ b/src/plugins/weechat-plugin.h @@ -76,7 +76,7 @@ struct t_weelist_item; * please change the date with current one; for a second change at same * date, increment the 01, otherwise please keep 01. */ -#define WEECHAT_PLUGIN_API_VERSION "20251112-01" +#define WEECHAT_PLUGIN_API_VERSION "20260530-01" /* macros for defining plugin infos */ #define WEECHAT_PLUGIN_NAME(__name) \ @@ -348,6 +348,8 @@ struct t_weechat_plugin int max, int range); int (*strcmp_ignore_chars) (const char *string1, const char *string2, const char *chars_ignored, int case_sensitive); + int (*string_memcmp_constant_time) (const void *area1, const void *area2, + size_t size); const char *(*strcasestr) (const char *string, const char *search); int (*strlen_screen) (const char *string); int (*string_match) (const char *string, const char *mask, @@ -1348,6 +1350,9 @@ extern int weechat_plugin_end (struct t_weechat_plugin *plugin); (weechat_plugin->strcmp_ignore_chars)(__string1, __string2, \ __chars_ignored, \ __case_sensitive) +#define weechat_string_memcmp_constant_time(__area1, __area2, __size) \ + (weechat_plugin->string_memcmp_constant_time)(__area1, __area2, \ + __size) #define weechat_strcasestr(__string, __search) \ (weechat_plugin->strcasestr)(__string, __search) #define weechat_strlen_screen(__string) \ diff --git a/tests/unit/core/test-core-string.cpp b/tests/unit/core/test-core-string.cpp index aad2756eb..2f9578edf 100644 --- a/tests/unit/core/test-core-string.cpp +++ b/tests/unit/core/test-core-string.cpp @@ -800,6 +800,38 @@ TEST(CoreString, StringComparison) LONGS_EQUAL(-2, string_strcmp_ignore_chars ("è", "ê", "", 1)); } +/* + * Test functions: + * string_memcmp_constant_time + */ + +TEST(CoreString, MemcmpConstantTime) +{ + /* NULL handling */ + LONGS_EQUAL(0, string_memcmp_constant_time (NULL, NULL, 0)); + LONGS_EQUAL(0, string_memcmp_constant_time (NULL, NULL, 4)); + LONGS_EQUAL(1, string_memcmp_constant_time (NULL, "abcd", 4)); + LONGS_EQUAL(1, string_memcmp_constant_time ("abcd", NULL, 4)); + + /* zero-size compare always equal */ + LONGS_EQUAL(0, string_memcmp_constant_time ("", "", 0)); + LONGS_EQUAL(0, string_memcmp_constant_time ("abc", "xyz", 0)); + + /* equal areas */ + LONGS_EQUAL(0, string_memcmp_constant_time ("abcd", "abcd", 4)); + LONGS_EQUAL(0, string_memcmp_constant_time ("\x00\x01\x02\xff", + "\x00\x01\x02\xff", 4)); + + /* differing areas (first / middle / last byte) */ + LONGS_EQUAL(1, string_memcmp_constant_time ("Xbcd", "abcd", 4)); + LONGS_EQUAL(1, string_memcmp_constant_time ("aXcd", "abcd", 4)); + LONGS_EQUAL(1, string_memcmp_constant_time ("abcX", "abcd", 4)); + LONGS_EQUAL(1, string_memcmp_constant_time ("abcd", "abce", 4)); + + /* only compares "size" bytes, ignores trailing content */ + LONGS_EQUAL(0, string_memcmp_constant_time ("abcd", "abcz", 3)); +} + /* * Test functions: * string_strcasestr