[Openvpn-devel,v2,1/8] Make management password check constant time

Message ID 20221220140458.2666637-1-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel,v2,1/8] Make management password check constant time | expand

Commit Message

Arne Schwabe Dec. 20, 2022, 2:04 p.m. UTC
This changes the password check on the management interface to be constant
time. Normally the management port should not be exposed in a way that allows
an attacker to even interact with it but making the check constant time as
an additional layer of security is always good.

Patch v2: include NUL byte in comparison

Reported-by: Connor Edwards <cedw@pm.me>
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
 src/openvpn/manage.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


Gert Doering Dec. 20, 2022, 3:30 p.m. UTC | #1
Acked-by: Gert Doering <gert@greenie.muc.de>

Works.  Password entered must be the right one now, not shorter or longer :-)

Your patch has been applied to the master and release/2.6 branch.

I think it would also be appropriate for 2.5, but there is no min_uint()
(came as part of commit 3f8fb2b2c), so the patch "as is" breaks
compilation on older branches.

commit e567f34262b0670fd51cbbcb6c6866b046454cee (master)
commit 6a82929076ed97f6c37161de266d3dcbe14d55c8 (release/2.6)
Author: Arne Schwabe
Date:   Tue Dec 20 15:04:58 2022 +0100

     Make management password check constant time

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20221220140458.2666637-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25784.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>

kind regards,

Gert Doering


diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index b11de224d..5465b7e9b 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -198,7 +198,12 @@  man_check_password(struct management *man, const char *line)
     if (man_password_needed(man))
-        if (streq(line, man->settings.up.password))
+        /* This comparison is not fixed time but since strlen(time) is based on
+         * the attacker choice, it should not give any indication of the real
+         * password length, use + 1 to include the NUL byte that terminates the
+         * string*/
+        size_t compare_len = min_uint(strlen(line) + 1, sizeof(man->settings.up.password));
+        if (memcmp_constant_time(line, man->settings.up.password, compare_len) == 0)
             man->connection.password_verified = true;
             msg(M_CLIENT, "SUCCESS: password is correct");