[Openvpn-devel,v2] Stop using deprecated getpass()

Message ID 20210328171151.12056-1-toivol@gmail.com
State Accepted
Headers show
Series
  • [Openvpn-devel,v2] Stop using deprecated getpass()
Related show

Commit Message

Tõivo Leedjärv March 28, 2021, 5:11 p.m.
The getpass() function is present in SUSv2, but marked LEGACY. It is
removed in POSIX.1-2001. Additionally, on Solaris getpass() returns
maximum 9 bytes. This will make longer passwords fail with no
possibility for user to know what is happening.

This patch removes usage of getpass() completely and replaces it with
direct implementation of what getpass() does: opens tty (existing code),
outputs the prompt (existing code), turns off echoing (new code), reads
one line (existing code shared with echoed mode), restores tty state
(new code) and closes tty (existing code).

Patch v2: incorporate review feedback, incl. style fixes, merge
          termios.h check in configure.ac with an existing
          AC_CHECK_HEADERS, add error check and logging after
          tcsettattr() when restoring tty settings

Signed-off-by: Tõivo Leedjärv <toivol@gmail.com>
---
 configure.ac                  |  4 +--
 src/openvpn/console_builtin.c | 60 +++++++++++++++++++++--------------
 2 files changed, 39 insertions(+), 25 deletions(-)

Comments

Antonio Quartulli March 31, 2021, 9:46 p.m. | #1
Hi,

Thanks a lot for considering my suggestions.

On 28/03/2021 19:11, Tõivo Leedjärv wrote:
> The getpass() function is present in SUSv2, but marked LEGACY. It is
> removed in POSIX.1-2001. Additionally, on Solaris getpass() returns
> maximum 9 bytes. This will make longer passwords fail with no
> possibility for user to know what is happening.
> 
> This patch removes usage of getpass() completely and replaces it with
> direct implementation of what getpass() does: opens tty (existing code),
> outputs the prompt (existing code), turns off echoing (new code), reads
> one line (existing code shared with echoed mode), restores tty state
> (new code) and closes tty (existing code).
> 
> Patch v2: incorporate review feedback, incl. style fixes, merge
>           termios.h check in configure.ac with an existing
>           AC_CHECK_HEADERS, add error check and logging after
>           tcsettattr() when restoring tty settings
> 
> Signed-off-by: Tõivo Leedjärv <toivol@gmail.com>

The code looks good and it actually simplifies our get_console_input()
function.

It's agreed that termios is available on most (all?) systems supporting
tun interfaces, therefore it is a valid replacement for getpass(),
without making us worry about old platforms.


I tested this patch with a simple client-server setup:
* Client was configured with "--auth-user-pass" and was sending some
username/password.
* Server was configured with "--auth-user-pass-verify script via-env",
where script was just printing its full env.

I then verified that the input on the client reflected the output on the
server, also with some unicode chars, to be sure the new logic would
still handle characters only Davids may use in his passwords.

password=ciao@@@@
password=ééëëff
password=test²³¤€’¥×~~
password=Ð


Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering April 1, 2021, 6:23 a.m. | #2
Your patch has been applied to the master branch.

I have compile-tested this across all our supported platforms to be
sure that we do not hit undefined symbols (like, "ECHOK", which was
unknown to me before) - but hat is all OK.  I have given it minimal
testing on my corp VPN profile on Linux (works).

Interesting enough, my corp VPN "password" is 2FA + PIN, which makes
it longer than 8 characters :-) - I've never used that from a Solaris
*client*, otherwise I might have noticed the issue myself...

Thanks for your work!

commit 76ccc62d4884721b6ecc11078abef747ea60d8d0
Author: Tõivo Leedjärv
Date:   Sun Mar 28 17:11:51 2021 +0000

     Stop using deprecated getpass()

     Signed-off-by: Tõivo Leedjärv <toivol@gmail.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210328171151.12056-1-toivol@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21889.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/configure.ac b/configure.ac
index 428bebed..6668bbcf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -438,7 +438,7 @@  AC_CHECK_HEADERS([ \
 	sys/time.h sys/ioctl.h sys/stat.h \
 	sys/mman.h sys/file.h sys/wait.h \
 	unistd.h signal.h libgen.h stropts.h \
-	syslog.h pwd.h grp.h \
+	syslog.h pwd.h grp.h termios.h \
 	sys/sockio.h sys/uio.h linux/sockios.h \
 	linux/types.h poll.h sys/epoll.h err.h \
 ])
@@ -652,7 +652,7 @@  AC_FUNC_FORK
 
 AC_CHECK_FUNCS([ \
 	daemon chroot getpwnam setuid nice system getpid dup dup2 \
-	getpass syslog openlog mlockall getrlimit getgrnam setgid \
+	syslog openlog mlockall getrlimit getgrnam setgid \
 	setgroups stat flock readv writev time gettimeofday \
 	ctime memset vsnprintf strdup \
 	setsid chdir putenv getpeername unlink \
diff --git a/src/openvpn/console_builtin.c b/src/openvpn/console_builtin.c
index 445928bf..22ac9f79 100644
--- a/src/openvpn/console_builtin.c
+++ b/src/openvpn/console_builtin.c
@@ -40,6 +40,10 @@ 
 #include "buffer.h"
 #include "misc.h"
 
+#ifdef HAVE_TERMIOS_H
+#include <termios.h>
+#endif
+
 #ifdef _WIN32
 
 #include "win32.h"
@@ -138,7 +142,7 @@  get_console_input_win32(const char *prompt, const bool echo, char *input, const
 #endif   /* _WIN32 */
 
 
-#ifdef HAVE_GETPASS
+#ifdef HAVE_TERMIOS_H
 
 /**
  * Open the current console TTY for read/write operations
@@ -177,7 +181,7 @@  close_tty(FILE *fp)
     }
 }
 
-#endif   /* HAVE_GETPASS */
+#endif   /* HAVE_TERMIOS_H */
 
 
 /**
@@ -201,7 +205,9 @@  get_console_input(const char *prompt, const bool echo, char *input, const int ca
 
 #if defined(_WIN32)
     return get_console_input_win32(prompt, echo, input, capacity);
-#elif defined(HAVE_GETPASS)
+#elif defined(HAVE_TERMIOS_H)
+    bool restore_tty = false;
+    struct termios tty_tmp, tty_save;
 
     /* did we --daemon'ize before asking for passwords?
      * (in which case neither stdin or stderr are connected to a tty and
@@ -220,33 +226,41 @@  get_console_input(const char *prompt, const bool echo, char *input, const int ca
         close(fd);
     }
 
-    if (echo)
-    {
-        FILE *fp;
+    FILE *fp = open_tty(true);
+    fprintf(fp, "%s", prompt);
+    fflush(fp);
+    close_tty(fp);
 
-        fp = open_tty(true);
-        fprintf(fp, "%s", prompt);
-        fflush(fp);
-        close_tty(fp);
+    fp = open_tty(false);
 
-        fp = open_tty(false);
-        if (fgets(input, capacity, fp) != NULL)
-        {
-            chomp(input);
-            ret = true;
-        }
-        close_tty(fp);
+    if (!echo && (tcgetattr(fileno(fp), &tty_tmp) == 0))
+    {
+        tty_save = tty_tmp;
+        tty_tmp.c_lflag &= ~(ECHO | ECHOE | ECHOK | ECHONL | ISIG);
+        restore_tty = (tcsetattr(fileno(fp), TCSAFLUSH, &tty_tmp) == 0);
     }
-    else
+
+    if (fgets(input, capacity, fp) != NULL)
+    {
+        chomp(input);
+        ret = true;
+    }
+
+    if (restore_tty)
     {
-        char *gp = getpass(prompt);
-        if (gp)
+        if (tcsetattr(fileno(fp), TCSAFLUSH, &tty_save) == -1)
         {
-            strncpynt(input, gp, capacity);
-            secure_memzero(gp, strlen(gp));
-            ret = true;
+            msg(M_WARN | M_ERRNO, "tcsetattr() failed to restore tty settings");
         }
+
+        /* Echo the non-echoed newline */
+        close_tty(fp);
+        fp = open_tty(true);
+        fprintf(fp, "\n");
+        fflush(fp);
     }
+
+    close_tty(fp);
 #else  /* if defined(_WIN32) */
     msg(M_FATAL, "Sorry, but I can't get console input on this OS (%s)", prompt);
 #endif /* if defined(_WIN32) */