[Openvpn-devel] Stop using deprecated getpass()

  The getpass() function is present in SUSv2, but marked LEGACY. It is
removed in POSIX.1-2001. Additionally, on Solaris getpass() returns
maximum 8 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).

Signed-off-by: Tõivo Leedjärv <69477666+tleedjarv@users.noreply.github.com>
---
 configure.ac                  |  4 ++-
 src/openvpn/console_builtin.c | 63 +++++++++++++++++++++--------------
 2 files changed, 41 insertions(+), 26 deletions(-)

  * 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)
+    int restore_tty = 0;
+    struct termios tty_a, 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,40 @@ get_console_input(const char *prompt, const bool
echo, char *input, const int ca
         close(fd);
     }

-    if (echo)
-    {
-        FILE *fp;
+    FILE *fp;

-        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);
-        if (fgets(input, capacity, fp) != NULL)
-        {
-            chomp(input);
-            ret = true;
-        }
-        close_tty(fp);
+    fp = open_tty(false);
+
+    if (!echo && (tcgetattr(fileno(fp), &tty_a) == 0))
+    {
+        tty_save = tty_a;
+        tty_a.c_lflag &= ~(ECHO | ECHOE | ECHOK | ECHONL | ISIG);
+        restore_tty = (tcsetattr(fileno(fp), TCSAFLUSH, &tty_a) == 0);
     }
-    else
+
+    if (fgets(input, capacity, fp) != NULL)
     {
-        char *gp = getpass(prompt);
-        if (gp)
-        {
-            strncpynt(input, gp, capacity);
-            secure_memzero(gp, strlen(gp));
-            ret = true;
-        }
+        chomp(input);
+        ret = true;
     }
+
+    if (!echo && restore_tty)
+    {
+        (void) tcsetattr(fileno(fp), TCSAFLUSH, &tty_save);
+
+        /* 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) */
  

Hi,

sorry if it took this long to get to this patch.

Unfortunately this patch should be rebased on master, because right now
it does not apply and I cannot test.
Can you please rebase and resubmit it (with v2 in the subject)?

However, I have some comments below.

On 13/01/2021 20:19, 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 8 bytes. This will make longer passwords fail with no
> possibility for user to know what is happening.
> 

Indeed the manpage clearly says that this function is obsolete, and
users should look into termios.

I believe termios.h should normally be available on any *nix system,
therefore it should be ok to switch to it.

> 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).
> 
> Signed-off-by: Tõivo Leedjärv <69477666+tleedjarv@users.noreply.github.com>
> ---
>  configure.ac                  |  4 ++-
>  src/openvpn/console_builtin.c | 63 +++++++++++++++++++++--------------
>  2 files changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 1ab8fe59..2c094da7 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -645,7 +645,7 @@ AC_FUNC_FORK
> 
>  AC_CHECK_FUNCS([ \
>         daemon chroot getpwnam setuid nice system getpid dup dup2 \
> -       getpass syslog openlog mlockall getgrnam setgid \
> +       syslog openlog mlockall getgrnam setgid \
>         setgroups stat flock readv writev time gettimeofday \
>         ctime memset vsnprintf strdup \
>         setsid chdir putenv getpeername unlink \
> @@ -653,6 +653,8 @@ AC_CHECK_FUNCS([ \
>         epoll_create strsep \
>  ])
> 
> +AC_CHECK_HEADERS([termios.h])

We already have a big list of headers that we check. May it make sense
to append termios.h there?

> +
>  AC_CHECK_LIB(
>         [dl],
>         [dlopen],
> diff --git a/src/openvpn/console_builtin.c b/src/openvpn/console_builtin.c
> index 445928bf..f1d91b32 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)
> +    int restore_tty = 0;

shouldn't this be bool?

> +    struct termios tty_a, tty_save;

how about tty_tmp instead of tty_a ?

> 
>      /* did we --daemon'ize before asking for passwords?
>       * (in which case neither stdin or stderr are connected to a tty and
> @@ -220,33 +226,40 @@ get_console_input(const char *prompt, const bool
> echo, char *input, const int ca
>          close(fd);
>      }
> 
> -    if (echo)
> -    {
> -        FILE *fp;
> +    FILE *fp;
> 
> -        fp = open_tty(true);
> -        fprintf(fp, "%s", prompt);
> -        fflush(fp);
> -        close_tty(fp);
> +    fp = open_tty(true);

since you are touching this code, please move the initialization inline
with the declaration.

> +    fprintf(fp, "%s", prompt);
> +    fflush(fp);
> +    close_tty(fp);
> 
> -        fp = open_tty(false);
> -        if (fgets(input, capacity, fp) != NULL)
> -        {
> -            chomp(input);
> -            ret = true;
> -        }
> -        close_tty(fp);
> +    fp = open_tty(false);
> +
> +    if (!echo && (tcgetattr(fileno(fp), &tty_a) == 0))
> +    {
> +        tty_save = tty_a;
> +        tty_a.c_lflag &= ~(ECHO | ECHOE | ECHOK | ECHONL | ISIG);
> +        restore_tty = (tcsetattr(fileno(fp), TCSAFLUSH, &tty_a) == 0);
>      }
> -    else
> +
> +    if (fgets(input, capacity, fp) != NULL)
>      {
> -        char *gp = getpass(prompt);
> -        if (gp)
> -        {
> -            strncpynt(input, gp, capacity);
> -            secure_memzero(gp, strlen(gp));
> -            ret = true;
> -        }
> +        chomp(input);
> +        ret = true;
>      }
> +
> +    if (!echo && restore_tty)

I guess we can simply check restore_tty being true?

> +    {
> +        (void) tcsetattr(fileno(fp), TCSAFLUSH, &tty_save);

We should not need to cast to void - I don't think we have warnings set
for non-checked return values.

> +
> +        /* 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) */
> 

The rest looks good to me and I believe it is a valid patch.

Once we have a patch that can be committed on master I will perform some
tests.

Best Regards,
  

Hi,

On Sun, Mar 28, 2021 at 01:52:51AM +0100, Antonio Quartulli wrote:
> I believe termios.h should normally be available on any *nix system,
> therefore it should be ok to switch to it.

Well, on everything that is recent enough to have tun/tap and sockets,
termios.h will exist.

On the necessity for the patch, I have mixed feelings - I do not see
any OS remove getpass() any time soon, as it would break applications...

But I need to have a close look at the patch again - give that we already
do the /dev/tty handling etc., it might effectively make the code easier.

[..]
> > +    {
> > +        (void) tcsetattr(fileno(fp), TCSAFLUSH, &tty_save);
> 
> We should not need to cast to void - I don't think we have warnings set
> for non-checked return values.

Actually we should catch errors here and log them.  If tcsetattr() fails, 
the tty will be in a messed-up state - so we want to know that it happened,
and possibly also the "why".

gert
  

Hi,

On 28/03/2021 10:52, Gert Doering wrote:
> [..]
>>> +    {
>>> +        (void) tcsetattr(fileno(fp), TCSAFLUSH, &tty_save);
>>
>> We should not need to cast to void - I don't think we have warnings set
>> for non-checked return values.
> 
> Actually we should catch errors here and log them.  If tcsetattr() fails, 
> the tty will be in a messed-up state - so we want to know that it happened,
> and possibly also the "why".

True. I totally agree here.

Cheers,
  

Hi,

Thank you both for the review and feedback.

On Sun, Mar 28, 2021 at 10:52 AM Gert Doering <gert@greenie.muc.de> wrote:
>
> On the necessity for the patch, I have mixed feelings - I do not see
> any OS remove getpass() any time soon, as it would break applications...

The specific issue I ran into was getpass() having 8 character limit
on Solaris/illumos.

Still, there is no inherent need to go the way of removing getpass().
There is a workaround for the length limit because Solaris/illumos
have getpassphrase() for which the man page declares "The
getpassphrase() function is identical to getpass(), except that it
reads and returns a string of up to 257 characters in length."

If you don't mind the code being guarded by an #ifdef TARGET_SOLARIS
then a simple alternative patch would look something like this:
-        char *gp = getpass(prompt);
+#ifndef TARGET_SOLARIS
+        char *gp = getpass(prompt); /* On Solaris, returns up to 9 bytes */
+#else
+        char *gp = getpassphrase(prompt); /* Returns up to 257 bytes */
+#endif

or even go one further and do a #define getpass(x) getpassphrase(x)
for TARGET_SOLARIS.

> > > +        (void) tcsetattr(fileno(fp), TCSAFLUSH, &tty_save);
> >
> > We should not need to cast to void - I don't think we have warnings set
> > for non-checked return values.
>
> Actually we should catch errors here and log them.  If tcsetattr() fails,
> the tty will be in a messed-up state - so we want to know that it happened,
> and possibly also the "why".

Right. What you have in mind, would that look like msg(M_ERR,
"...explanation...") or perhaps an M_WARN?
  

Hi,

On Sun, Mar 28, 2021 at 02:03:37PM +0200, Tõivo Leedjärv wrote:
> The specific issue I ran into was getpass() having 8 character limit
> on Solaris/illumos.

Ah, thanks for that reminder.  That is, indeed, a strong argument (and
who knows which other platforms might do this and we don't know about
it yet).

> If you don't mind the code being guarded by an #ifdef TARGET_SOLARIS
> then a simple alternative patch would look something like this:
> -        char *gp = getpass(prompt);
> +#ifndef TARGET_SOLARIS
> +        char *gp = getpass(prompt); /* On Solaris, returns up to 9 bytes */
> +#else
> +        char *gp = getpassphrase(prompt); /* Returns up to 257 bytes */
> +#endif

No extra #ifdef please - let's go for a v2 of your patch, with the
error checking.

> > Actually we should catch errors here and log them.  If tcsetattr() fails,
> > the tty will be in a messed-up state - so we want to know that it happened,
> > and possibly also the "why".
> 
> Right. What you have in mind, would that look like msg(M_ERR,
> "...explanation...") or perhaps an M_WARN?

msg(M_WARN | M_ERRNO, "tcsetattr() failed to restore tty settings")

(or some other wording to that extent) should do.

(Some parts of the code uses the M_* macros differently, but half of that 
is due to unclear documentation how they should be used, and lack of 
stringent commit regime :) )

gert