From c737373d17070035b77acf21a01f21eabfb0e0fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Helleu?= Date: Sat, 30 May 2026 20:46:52 +0200 Subject: [PATCH] relay/irc: fix timing attack on PASS command (GHSA-vhv8-g2r9-cwcc) The IRC relay protocol's PASS handler compared the server password with the client-supplied value using strcmp, leaking the password byte-by-byte via response timing. This is the same class of bug fixed for the api and weechat protocols, on a separate code path that did not go through relay_auth_check_password_plain. Extract the HMAC-then-constant-time-compare logic from relay_auth_check_password_plain into relay_auth_password_equals, then use it in both the plain-auth wrapper and the IRC PASS handler. --- src/plugins/relay/irc/relay-irc.c | 3 +- src/plugins/relay/relay-auth.c | 108 +++++++++++-------- src/plugins/relay/relay-auth.h | 2 + tests/unit/plugins/relay/test-relay-auth.cpp | 25 +++++ 4 files changed, 90 insertions(+), 48 deletions(-) diff --git a/src/plugins/relay/irc/relay-irc.c b/src/plugins/relay/irc/relay-irc.c index 185915281..2b6746853 100644 --- a/src/plugins/relay/irc/relay-irc.c +++ b/src/plugins/relay/irc/relay-irc.c @@ -33,6 +33,7 @@ #include "../../weechat-plugin.h" #include "../relay.h" #include "relay-irc.h" +#include "../relay-auth.h" #include "../relay-buffer.h" #include "../relay-client.h" #include "../relay-config.h" @@ -1701,7 +1702,7 @@ relay_irc_recv (struct t_relay_client *client, const char *data) NULL, NULL, NULL); if (password) { - if (strcmp (password, pos_password) == 0) + if (relay_auth_password_equals (password, pos_password)) { RELAY_IRC_DATA(client, password_ok) = 1; weechat_hook_signal_send ("relay_client_auth_ok", diff --git a/src/plugins/relay/relay-auth.c b/src/plugins/relay/relay-auth.c index 5abe1e6a5..42536af81 100644 --- a/src/plugins/relay/relay-auth.c +++ b/src/plugins/relay/relay-auth.c @@ -102,6 +102,66 @@ relay_auth_generate_nonce (int size) return nonce_hexa; } +/* + * Compare two passwords for equality in constant time. + * + * 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 a timing-side-channel observer. + * + * Both messages are prefixed with a zero byte so 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. + * + * Return: + * 1: passwords are equal + * 0: passwords are not equal (or an error occurred) + */ + +int +relay_auth_password_equals (const char *password1, const char *password2) +{ + unsigned char key[32]; + char hmac1[64], hmac2[64]; + char *buf1, *buf2; + int buf1_size, buf2_size, hmac1_size, hmac2_size, rc; + + if (!password1 || !password2) + return 0; + + rc = 0; + buf1_size = strlen (password1) + 1; + buf2_size = strlen (password2) + 1; + buf1 = malloc (buf1_size); + buf2 = malloc (buf2_size); + if (buf1 && buf2) + { + buf1[0] = 0; + memcpy (buf1 + 1, password1, buf1_size - 1); + buf2[0] = 0; + memcpy (buf2 + 1, password2, buf2_size - 1); + gcry_create_nonce (key, sizeof (key)); + if (weechat_crypto_hmac (key, sizeof (key), + buf1, buf1_size, + "sha256", + hmac1, &hmac1_size) + && weechat_crypto_hmac (key, sizeof (key), + buf2, buf2_size, + "sha256", + hmac2, &hmac2_size) + && (hmac1_size == hmac2_size) + && (weechat_string_memcmp_constant_time ( + hmac1, hmac2, hmac1_size) == 0)) + { + rc = 1; + } + } + free (buf1); + free (buf2); + return rc; +} + /* * Check if password received as plain text is valid. * @@ -116,12 +176,6 @@ 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; @@ -133,47 +187,7 @@ relay_auth_check_password_plain (struct t_relay_client *client, return -1; } - /* - * 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; + return relay_auth_password_equals (password, relay_password) ? 0 : -2; } /* diff --git a/src/plugins/relay/relay-auth.h b/src/plugins/relay/relay-auth.h index 44bdd5f63..be1b35336 100644 --- a/src/plugins/relay/relay-auth.h +++ b/src/plugins/relay/relay-auth.h @@ -39,6 +39,8 @@ extern char *relay_auth_password_hash_algo_name[]; extern int relay_auth_password_hash_algo_search (const char *name); extern char *relay_auth_generate_nonce (int size); +extern int relay_auth_password_equals (const char *password1, + const char *password2); extern int relay_auth_check_password_plain (struct t_relay_client *client, const char *password, const char *relay_password); diff --git a/tests/unit/plugins/relay/test-relay-auth.cpp b/tests/unit/plugins/relay/test-relay-auth.cpp index 782927a36..4798099c9 100644 --- a/tests/unit/plugins/relay/test-relay-auth.cpp +++ b/tests/unit/plugins/relay/test-relay-auth.cpp @@ -109,6 +109,31 @@ TEST(RelayAuth, GenerateNonce) free (nonce); } +/* + * Test functions: + * relay_auth_password_equals + */ + +TEST(RelayAuth, PasswordEquals) +{ + /* invalid arguments */ + LONGS_EQUAL(0, relay_auth_password_equals (NULL, NULL)); + LONGS_EQUAL(0, relay_auth_password_equals ("abcd", NULL)); + LONGS_EQUAL(0, relay_auth_password_equals (NULL, "abcd")); + + /* different passwords */ + LONGS_EQUAL(0, relay_auth_password_equals ("test", "password")); + LONGS_EQUAL(0, relay_auth_password_equals ("Password", "password")); + LONGS_EQUAL(0, relay_auth_password_equals ("", "password")); + LONGS_EQUAL(0, relay_auth_password_equals ("password", "")); + + /* equal passwords */ + LONGS_EQUAL(1, relay_auth_password_equals ("", "")); + LONGS_EQUAL(1, relay_auth_password_equals ("password", "password")); + LONGS_EQUAL(1, relay_auth_password_equals ("a really long password with spaces", + "a really long password with spaces")); +} + /* * Test functions: * relay_auth_check_password_plain