Opened 3 years ago

Closed 2 months ago

Last modified 2 months ago

#225 closed Bug / Defect (fixed)

'--auth-user-pass FILE' and '--auth-nocache' problem

Reported by: ye_olde_iron Owned by: syzzer
Priority: minor Milestone: release 2.3.7
Component: Configuration Version: 2.2.2
Severity: Not set (if unsure, select this one) Keywords: auth-user-pass auth-nocache
Cc:

Description

If you specify that username/password information should be read from a file ('--auth-user-pass FILE'), but that this information should NOT be cached ('--auth-nocache') then initially all works well, with the username and password being read from the file. However, when the renegotiation timeout occurs on the client ('--reneg-sec nnn'), an attempt is made to read the username and password FROM STDIN instead of from the specified file!

[The log gives the error "ERROR: could not read Auth username from stdin"]

If you have specified that the information should be read from a file, then the nocache should cause the information to be re-read from the file, not stdin.

(In this case I wish the file to contain a password that changes over time, so I do not want it to be cached).

The same problem may to have been reported under "Ticket #60 : --auth-user-pass <file> does not work after TLS soft reset disconnect", but '--auth-nocache' is not mentioned.

Change History (14)

comment:1 Changed 3 years ago by ye_olde_iron

Additional information ...

The problems appears to be caused by the fact that once the renegotiation timeout occurs auth_user_pass_setup() is being called with a 'NULL' auth_file pointer (and auth_user_pass.defined is false) - this causes get_user_pass_cr() to try to prompt from stdin.

I believe the initial sequence is as follows (very simplified) :-

  ssl_set_auth_nocache()
    {on exit: auth_user_pass.defined=0 , auth_user_pass.nocache=1}
  context_init_1( ... )
    init_query_passwords( ... )
      auth_user_pass_setup( auth_file=<somepath> )
        get_user_pass_cr( ... , auth_file=<somepath> , ... )
          ... from_stdin=0 ...
          {the username and password are read from <somepath>}
          {on exit: up->defined=1 , up->nocache=1}
  tls_process( ... )
    key_method_2_write( ... )
      auth_user_pass_setup( auth_file=NULL )  <-- second call to this routine, but now with a NULL auth_file
        {does not call get_user_pass_cr() as auth_user_pass.defined=1}
      purge_user_pass( auth_user_pas , false )
      {on exit: auth_user_pass.defined=0 , auth_user_pass.nocache=1}

At this stage the tunnel is running, but auth_user_pass.defined is now false so the program
will need to read the username/password data again.

The renegotiation timeout occurs and tls_process() gets underway again :-

  tls_process( ... )
    key_method_2_write( ... )
      auth_user_pass_setup( auth_file=NULL )  <-- third call to this routine, again with a NULL auth_file
        get_user_pass_cr( ... , '''auth_file=NULL''' , ... )
          ... from_stdin=1 ...
          "ERROR: could not read Auth username from stdin"

key_method_2_write() always makes the call "auth_user_pass_setup(NULL)" - this means that once purge_user_pass() purges the username and password from memory (as expected with nocache) the routine get_user_pass_cr() is not being given the name of the auth_file.

key_method_2_write() needs to pass the name of the auth_file if defined in the config, or perhaps the name could be saved in the auth_user_pass structure (and not cleared unless it changes).

comment:2 follow-up: Changed 3 years ago by ye_olde_iron

Potential solution ... add a variable to "struct tls_options" that will save the auth-user-pass value for a connection and then use this variable rather than NULL when key_method_2_write() calls auth_user_pass_setup().

Changes to ssl.h to add in a new variable 'auth_user_pass_file' :-

--- ssl.h_ORIG	2011-12-14 05:58:56.000000000 +1300
+++ ssl.h	2012-08-21 08:00:11.000000000 +1200
@@ -477,6 +477,8 @@
   const char *auth_user_pass_verify_script;
   bool auth_user_pass_verify_script_via_file;
   const char *tmp_dir;
+  /* ... and at the client end ... */
+  const char *auth_user_pass_file;
 
   /* use the client-config-dir as a positive authenticator */
   const char *client_config_dir_exclusive;

Changes to init.c (in "do_init_crypto_tls()") to initialise the 'auth_user_pass_file' variable :-

--- init.c_ORIG	2011-12-14 05:58:56.000000000 +1300
+++ init.c	2012-08-21 08:08:38.000000000 +1200
@@ -2049,6 +2049,7 @@
   if (options->ccd_exclusive)
     to.client_config_dir_exclusive = options->client_config_dir;
 #endif
+  to.auth_user_pass_file = options->auth_user_pass_file;
 
   /* TLS handshake authentication (--tls-auth) */
   if (options->tls_auth_file)

Changes to ssl.c (in "key_method_2_write()") to use the new variable when calling "auth_user_pass_setup()" :-

--- ssl.c_ORIG	2011-12-14 05:58:56.000000000 +1300
+++ ssl.c	2012-08-21 09:20:37.000000000 +1200
@@ -3623,7 +3623,8 @@
   /* write username/password if specified */
   if (auth_user_pass_enabled)
     {
-      auth_user_pass_setup (NULL);
+      /* auth_user_pass_setup (NULL); */
+      auth_user_pass_setup (session->opt->auth_user_pass_file);
       if (!write_string (buf, auth_user_pass.username, -1))
 	goto error;
       if (!write_string (buf, auth_user_pass.password, -1))

comment:3 Changed 3 years ago by dazo

  • Component changed from Generic / unclassified to Configuration
  • Keywords stdin removed
  • Priority changed from major to minor

I'll grant you that this is a bug. But your fix is not the right fix, IMO.

The problem here is that mixing --auth-nocache with --auth-user-pass FILE is moot. That really doesn't make sense. If you don't mind having the password stored on a disk, why would you care if the password is temporarily saved in memory or not?

So the right fix is to make --auth-nocache and --auth-user-pass FILE mutually exclusive. Make the user choose one of them, not both.

comment:4 Changed 2 years ago by ye_olde_iron

... That really doesn't make sense. If you don't mind having the password stored on a disk, why would you care if the password is temporarily saved in memory or not?

Actually it does make sense in my situation. I am *updating* the password that is stored on disk every few hours to keep it fresh (with a current timestamp etc.) - this is done by an entirely separate program to OpenVPN. I want OpenVPN to pick up the updates over time, not use a previously cached one!

I would appreciate it if the patch could go through, there is a sensible use for it :-)

Many thanks

comment:5 Changed 2 years ago by lfarkas

i have to agree. eg. we change the on disk auth file each time when the network settings change (eg. dhcp gives a new ip). so we need both --auth-nocache AND --auth-user-pass.

is there any current version which fix this verion?

comment:6 Changed 2 years ago by cron2

Actually, I think from a first glance, the patch looks reasonable. The code surrounding this is a bit complicated, so it needs a more close look. Time...

comment:7 Changed 19 months ago by cron2

This seems to be a duplicate of #60.

Checked the patch from Davide Brini, and it does not fix the issue, but at least tell us more clearly about it...

+ msg (M_USAGE, "Cannot use --auth-nocache with credentials from file");

... so this needs independent observation, based on the patch and description here.

comment:8 Changed 15 months ago by cron2

  • Milestone set to release 2.3.5

Review before 3.2.5 release, decide if this should go in 3.2.5 or is too intrusive, then reclassify to "master". Sorry for the delay in working on this.

comment:9 Changed 7 months ago by cron2

  • Milestone changed from release 2.3.5 to release 2.3.7

comment:10 Changed 3 months ago by cron2

  • Owner set to syzzer
  • Status changed from new to assigned

Found a victim, uh, volunteer :-)

Last edited 3 months ago by cron2 (previous) (diff)

comment:11 in reply to: ↑ 2 Changed 2 months ago by syzzer

The changes proposed by ye_olde_iron indeed do look reasonable. There is however a problem: when used with --daemon or --cd, using relative paths will either fail during init (because the user/pass file is read once before any call to chdir() or daemon()) or during connection (because the file is re-read, but chdir()/daemon() will have changed the working directory).

I propose the following:

  1. For release/2.3, apply the changes as suggested by ye_olde_iron. It fixes this bug and introduces the above described (less severe) bug, but at least it is possible for users to have a correct functioning setup by using absolute paths. Since this particular setup would not work before anyway, it will not break any currently functioning setups.
  2. For master, only read the file during (re)connect, and make sure to add a notice to the change log that the relavite path of --auth-user-pass is now influenced by --cd and --daemon. This fixes both bugs but will probably break some user configs.

comment:12 Changed 2 months ago by cron2

commit ac1cb5bfbb9e09e79fd737bc57999d968d77c5ad (master)
commit 6f789d2ec6b6aacb46ab27f1482222c6981faab6 (release/2.3)

Author: Steffan Karger
Date: Sat May 23 15:02:25 2015 +0200

Re-read auth-user-pass file on (re)connect if required

... which also includes a new text in openvpn.8 to explain that using --auth-user-pass file + --auth-nocache + --daemon will bite you regarding absolute/relative path names. Working around this (like, determine absolute path name and remember that one) would have been possible, but we did not consider this to be crucial for a somewhat niche application. "You know what you're getting yourself into" ;-)

comment:13 Changed 2 months ago by syzzer

  • Resolution set to fixed
  • Status changed from assigned to closed

... which means 'fixed'!

comment:14 Changed 2 months ago by cron2

#308 seems to be a duplicate of this one (using slighly different option combinations, but it smells very similar)

Note: See TracTickets for help on using tickets.