Skip to contentSkip to conversation

Fix CR prompt with --auth-user-pass + R from a file

PATCH In review Wayne Davison 3 participants 17 messages Dec 8, 2015 – Dec 17, 2015

lore
Share & tools
lore

Wayne Davison

author
#0 lore raw

cover Fix CR prompt with --auth-user-pass + R from a file

There is a bug in the challenge/response code when the username & password is read from a file -- the response is never prompted for. This bug affects older versions, including 2.3.8. The following patchset applies to master, and uses a similar bool idiom as what has been added for user & pass.

I also added code to read the response out of the --auth-user-pass file if there is a 3rd line present. This is particularly useful for an 2FA setup where the response is unchanging (in my case it is always "push" for a Duo Push).

The master branch already supports treating an empty password in the file as indicating that the password should be prompted from the user (2.3.8 used to complain), so I also amended the manpage to mention that the password can be empty in addition to its line not being present. This allows me to use a file that contains user / empty-pass / "push" and have openvpn just prompt me for my password.

I've broken the patch up into 2 parts for clarity, since it re-indents a large section of code. The 2nd patch also fixes an off-by-one indentation that is in the same file. Feel free to merge the patches together, as you see fit.

author
#1 lore raw

PATCH 1/2 Fix CR prompt with --auth-user-pass & support reading response.

Fixes a bug where there is no prompt for a dynamic (or static) challenge response when combining "--auth-retry interact" (or --static-challenge) with --auth-user-pass and the password is read from the file. It also extends the reading from the file to allow a response to be provided on the line after the password (which is nice for "push", "phone", etc.).

NOTE: indentation left too deep on one big section so that changes are clearly seen.

---
 doc/openvpn.8      |  8 +++++++-
 src/openvpn/misc.c | 31 ++++++++++++++++++++++---------
 2 files changed, 29 insertions(+), 10 deletions(-)
doc/openvpn.8 +7 -1
index 1b9dcae..ffe81c5 100644
@@ -3800,7 +3800,13 @@ over the client's routing table.
 Authenticate with server using username/password.
 .B up
 is a file containing username/password on 2 lines. If the
-password line is missing, OpenVPN will prompt for one.
+password line is missing or empty, OpenVPN will prompt for one.
+The third line of the file can be used to enter a response to
+a challenge, such as "push", "phone", etc., which can be used
+to eliminate an extra prompt for an unchanging text if your
+challenge/response setup uses push/callback two-factor auth
+(and can be combined with an empty password line to still be
+prompted for your password).
 
 If
 .B up
src/openvpn/misc.c +22 -9
index 5713d2e..a6915ac 100644
@@ -1038,6 +1038,10 @@ get_user_pass_cr (struct user_pass *up,
       bool from_authfile = (auth_file && !streq (auth_file, "stdin"));
       bool username_from_stdin = false;
       bool password_from_stdin = false;
+      bool response_from_stdin = false;
+#ifdef ENABLE_CLIENT_CR
+      char response_buf[USER_PASS_LEN] = { '\0' };
+#endif
 
       if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
 	msg (M_WARN, "Note: previous '%s' credentials failed", prefix);

@@ -1111,6 +1115,10 @@ get_user_pass_cr (struct user_pass *up,
           if (fgets (password_buf, USER_PASS_LEN, fp) != NULL)
             {
               chomp (password_buf);
+#ifdef ENABLE_CLIENT_CR
+              if (!(flags & GET_USER_PASS_PASSWORD_ONLY) && fgets (response_buf, USER_PASS_LEN, fp) != NULL)
+                chomp (response_buf);
+#endif
             }
 
           if (flags & GET_USER_PASS_PASSWORD_ONLY && !password_buf[0])

@@ -1119,7 +1127,7 @@ get_user_pass_cr (struct user_pass *up,
           if (password_buf[0])
             strncpy(up->password, password_buf, USER_PASS_LEN);
           else
-            password_from_stdin = 1;
+            password_from_stdin = true;
 
           fclose (fp);
 

@@ -1132,16 +1140,22 @@ get_user_pass_cr (struct user_pass *up,
           password_from_stdin = true;
         }
 
+#ifdef ENABLE_CLIENT_CR
+      if (auth_challenge && (flags & (GET_USER_PASS_DYNAMIC_CHALLENGE|GET_USER_PASS_STATIC_CHALLENGE)) && !response_buf[0])
+	response_from_stdin = true;
+#endif
+
       /*
        * Get username/password from standard input?
        */
-      if (username_from_stdin || password_from_stdin)
+      if (username_from_stdin || password_from_stdin || response_from_stdin)
 	{
 #ifndef WIN32
 	  /* did we --daemon'ize before asking for passwords? */
 	  if ( !isatty(0) && !isatty(2) )
 	    { msg(M_FATAL, "neither stdin nor stderr are a tty device, can't ask for %s password.  If you used --daemon, you need to use --askpass to make passphrase-protected keys work, and you can not use --auth-nocache.", prefix ); }
 #endif
+	}
 
 #ifdef ENABLE_CLIENT_CR
 	  if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))

@@ -1149,15 +1163,14 @@ get_user_pass_cr (struct user_pass *up,
 	      struct auth_challenge_info *ac = get_auth_challenge (auth_challenge, &gc);
 	      if (ac)
 		{
-		  char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
 		  struct buffer packed_resp;
 
 		  buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
 		  msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", ac->challenge_text);
-		  if (!get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response, USER_PASS_LEN))
+		  if (response_from_stdin && !get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response_buf, USER_PASS_LEN))
 		    msg (M_FATAL, "ERROR: could not read challenge response from stdin");
 		  strncpynt (up->username, ac->user, USER_PASS_LEN);
-		  buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response);
+		  buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response_buf);
 		}
 	      else
 		{

@@ -1187,15 +1200,16 @@ get_user_pass_cr (struct user_pass *up,
 #ifdef ENABLE_CLIENT_CR
 	      if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
 		{
-		  char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
 		  struct buffer packed_resp;
 		  char *pw64=NULL, *resp64=NULL;
 
 		  msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", auth_challenge);
-		  if (!get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response, USER_PASS_LEN))
+		  if (response_from_stdin && !get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response_buf, USER_PASS_LEN))
 		    msg (M_FATAL, "ERROR: could not read static challenge response from stdin");
+                  else if (!response_from_stdin && (flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO))
+                    msg (M_INFO|M_NOPREFIX, "Response: %s", response_buf);
 		  if (openvpn_base64_encode(up->password, strlen(up->password), &pw64) == -1
-		      || openvpn_base64_encode(response, strlen(response), &resp64) == -1)
+		      || openvpn_base64_encode(response_buf, strlen(response_buf), &resp64) == -1)
 		    msg (M_FATAL, "ERROR: could not base64-encode password/static_response");
 		  buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
 		  buf_printf (&packed_resp, "SCRV1:%s:%s", pw64, resp64);

@@ -1206,7 +1220,6 @@ get_user_pass_cr (struct user_pass *up,
 		}
 #endif
 	    }
-	}
 
       string_mod (up->username, CC_PRINT, CC_CRLF, 0);
       string_mod (up->password, CC_PRINT, CC_CRLF, 0);
Signature · 1 line
1.9.1
author
#16 lore raw

PATCH 2/2 Fix indentation of just-tweaked section plus an off-by-one section.

In reply to Wayne Davison
---
 src/openvpn/misc.c | 120 ++++++++++++++++++++++++++---------------------------
 1 file changed, 60 insertions(+), 60 deletions(-)
src/openvpn/misc.c +60 -60
index a6915ac..af64cb0 100644
@@ -1105,11 +1105,11 @@ get_user_pass_cr (struct user_pass *up,
           if ((flags & GET_USER_PASS_PASSWORD_ONLY) == 0)
             {
               /* Read username first */
-               if (fgets (up->username, USER_PASS_LEN, fp) == NULL)
-                 msg (M_FATAL, "Error reading username from %s authfile: %s",
-                      prefix,
-                      auth_file);
-             }
+              if (fgets (up->username, USER_PASS_LEN, fp) == NULL)
+                msg (M_FATAL, "Error reading username from %s authfile: %s",
+                     prefix,
+                     auth_file);
+            }
           chomp (up->username);
 
           if (fgets (password_buf, USER_PASS_LEN, fp) != NULL)

@@ -1158,68 +1158,68 @@ get_user_pass_cr (struct user_pass *up,
 	}
 
 #ifdef ENABLE_CLIENT_CR
-	  if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
-	    {
-	      struct auth_challenge_info *ac = get_auth_challenge (auth_challenge, &gc);
-	      if (ac)
-		{
-		  struct buffer packed_resp;
-
-		  buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
-		  msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", ac->challenge_text);
-		  if (response_from_stdin && !get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response_buf, USER_PASS_LEN))
-		    msg (M_FATAL, "ERROR: could not read challenge response from stdin");
-		  strncpynt (up->username, ac->user, USER_PASS_LEN);
-		  buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response_buf);
-		}
-	      else
-		{
-		  msg (M_FATAL, "ERROR: received malformed challenge request from server");
-		}
-	    }
-	  else
+      if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
+        {
+          struct auth_challenge_info *ac = get_auth_challenge (auth_challenge, &gc);
+          if (ac)
+            {
+              struct buffer packed_resp;
+
+              buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
+              msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", ac->challenge_text);
+              if (response_from_stdin && !get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response_buf, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not read challenge response from stdin");
+              strncpynt (up->username, ac->user, USER_PASS_LEN);
+              buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response_buf);
+            }
+          else
+            {
+              msg (M_FATAL, "ERROR: received malformed challenge request from server");
+            }
+        }
+      else
 #endif
-	    {
-	      struct buffer user_prompt = alloc_buf_gc (128, &gc);
-	      struct buffer pass_prompt = alloc_buf_gc (128, &gc);
+        {
+          struct buffer user_prompt = alloc_buf_gc (128, &gc);
+          struct buffer pass_prompt = alloc_buf_gc (128, &gc);
 
-	      buf_printf (&user_prompt, "Enter %s Username:", prefix);
-	      buf_printf (&pass_prompt, "Enter %s Password:", prefix);
+          buf_printf (&user_prompt, "Enter %s Username:", prefix);
+          buf_printf (&pass_prompt, "Enter %s Password:", prefix);
 
-	      if (username_from_stdin && !(flags & GET_USER_PASS_PASSWORD_ONLY))
-		{
-		  if (!get_console_input (BSTR (&user_prompt), true, up->username, USER_PASS_LEN))
-		    msg (M_FATAL, "ERROR: could not read %s username from stdin", prefix);
-		  if (strlen (up->username) == 0)
-		    msg (M_FATAL, "ERROR: %s username is empty", prefix);
-		}
+          if (username_from_stdin && !(flags & GET_USER_PASS_PASSWORD_ONLY))
+            {
+              if (!get_console_input (BSTR (&user_prompt), true, up->username, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not read %s username from stdin", prefix);
+              if (strlen (up->username) == 0)
+                msg (M_FATAL, "ERROR: %s username is empty", prefix);
+            }
 
-	      if (password_from_stdin && !get_console_input (BSTR (&pass_prompt), false, up->password, USER_PASS_LEN))
-		msg (M_FATAL, "ERROR: could not not read %s password from stdin", prefix);
+          if (password_from_stdin && !get_console_input (BSTR (&pass_prompt), false, up->password, USER_PASS_LEN))
+            msg (M_FATAL, "ERROR: could not not read %s password from stdin", prefix);
 
 #ifdef ENABLE_CLIENT_CR
-	      if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
-		{
-		  struct buffer packed_resp;
-		  char *pw64=NULL, *resp64=NULL;
-
-		  msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", auth_challenge);
-		  if (response_from_stdin && !get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response_buf, USER_PASS_LEN))
-		    msg (M_FATAL, "ERROR: could not read static challenge response from stdin");
-                  else if (!response_from_stdin && (flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO))
-                    msg (M_INFO|M_NOPREFIX, "Response: %s", response_buf);
-		  if (openvpn_base64_encode(up->password, strlen(up->password), &pw64) == -1
-		      || openvpn_base64_encode(response_buf, strlen(response_buf), &resp64) == -1)
-		    msg (M_FATAL, "ERROR: could not base64-encode password/static_response");
-		  buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
-		  buf_printf (&packed_resp, "SCRV1:%s:%s", pw64, resp64);
-		  string_clear(pw64);
-		  free(pw64);
-		  string_clear(resp64);
-		  free(resp64);
-		}
+          if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
+            {
+              struct buffer packed_resp;
+              char *pw64=NULL, *resp64=NULL;
+
+              msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", auth_challenge);
+              if (response_from_stdin && !get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response_buf, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not read static challenge response from stdin");
+              else if (!response_from_stdin && (flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO))
+                msg (M_INFO|M_NOPREFIX, "Response: %s", response_buf);
+              if (openvpn_base64_encode(up->password, strlen(up->password), &pw64) == -1
+                  || openvpn_base64_encode(response_buf, strlen(response_buf), &resp64) == -1)
+                msg (M_FATAL, "ERROR: could not base64-encode password/static_response");
+              buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
+              buf_printf (&packed_resp, "SCRV1:%s:%s", pw64, resp64);
+              string_clear(pw64);
+              free(pw64);
+              string_clear(resp64);
+              free(resp64);
+            }
 #endif
-	    }
+        }
 
       string_mod (up->username, CC_PRINT, CC_CRLF, 0);
       string_mod (up->password, CC_PRINT, CC_CRLF, 0);
Signature · 1 line
1.9.1
#2 lore raw

Re: [Openvpn-devel] [PATCH 1/2] Fix CR prompt with --auth-user-pass & support reading response.

In reply to Wayne Davison

Am 08.12.15 um 19:28 schrieb Wayne Davison:

Quoted 8 lines

Fixes a bug where there is no prompt for a dynamic (or static) challenge response when combining "--auth-retry interact" (or --static-challenge) with --auth-user-pass and the password is read from the file. It also extends the reading from the file to allow a response to be provided on the line after the password (which is nice for "push", "phone", etc.).

NOTE: indentation left too deep on one big section so that changes are clearly seen.

Could you describe the scenario this patch fixes? The dynamic and static challenge protocol are enabled if also the management interface is enabled.

The scenario read the password from file and then ask the password per console seems a bit strange.

Arne

author
#3 lore raw

Re: [Openvpn-devel] [PATCH 1/2] Fix CR prompt with --auth-user-pass & support reading response.

Quoted 3 lines · Arne Schwabe

The scenario read the password from file and then ask the password per console seems a bit strange.

You mean read the password from a file and then ask for the CR from the console. If someone combines a server that issues a challenge with the "--auth-user-pass FILE" option, neither dynamic nor static CR prompts ever happen, and auth always fails. This is because those CR sections are inside the block that is only entered if username_from_stdin || password_from_stdin is true, and they are false when a file supplies the values. I'll post a patch that separates out just this fix and make it on the change that moves the isatty() checks out of misc.c. It is also improved a bit from my prior patch set in that it moves the user_prompt and pass_prompt vars into code blocks where they are used (so they don't get set when they aren't needed).

..wayne..

author
#4 lore raw

PATCH 1/1 Improve stdin prompting section, fixing CR prompting.

---
 src/openvpn/misc.c | 119 +++++++++++++++++++++++++----------------------------
 1 file changed, 57 insertions(+), 62 deletions(-)
src/openvpn/misc.c +57 -62
index bc411bf..83e10f7 100644
@@ -1132,74 +1132,69 @@ get_user_pass_cr (struct user_pass *up,
           password_from_stdin = true;
         }
 
-      /*
-       * Get username/password from standard input?
-       */
-      if (username_from_stdin || password_from_stdin)
-	{
 #ifdef ENABLE_CLIENT_CR
-	  if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
-	    {
-	      struct auth_challenge_info *ac = get_auth_challenge (auth_challenge, &gc);
-	      if (ac)
-		{
-		  char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
-		  struct buffer packed_resp;
-
-		  buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
-		  msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", ac->challenge_text);
-		  if (!get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response, USER_PASS_LEN))
-		    msg (M_FATAL, "ERROR: could not read challenge response from stdin");
-		  strncpynt (up->username, ac->user, USER_PASS_LEN);
-		  buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response);
-		}
-	      else
-		{
-		  msg (M_FATAL, "ERROR: received malformed challenge request from server");
-		}
-	    }
-	  else
+      if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
+        {
+          struct auth_challenge_info *ac = get_auth_challenge (auth_challenge, &gc);
+          if (ac)
+            {
+              char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
+              struct buffer packed_resp;
+
+              buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
+              msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", ac->challenge_text);
+              if (!get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not read challenge response from stdin");
+              strncpynt (up->username, ac->user, USER_PASS_LEN);
+              buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response);
+            }
+          else
+            {
+              msg (M_FATAL, "ERROR: received malformed challenge request from server");
+            }
+        }
+      else
 #endif
-	    {
-	      struct buffer user_prompt = alloc_buf_gc (128, &gc);
-	      struct buffer pass_prompt = alloc_buf_gc (128, &gc);
-
-	      buf_printf (&user_prompt, "Enter %s Username:", prefix);
-	      buf_printf (&pass_prompt, "Enter %s Password:", prefix);
-
-	      if (username_from_stdin && !(flags & GET_USER_PASS_PASSWORD_ONLY))
-		{
-		  if (!get_console_input (BSTR (&user_prompt), true, up->username, USER_PASS_LEN))
-		    msg (M_FATAL, "ERROR: could not read %s username from stdin", prefix);
-		  if (strlen (up->username) == 0)
-		    msg (M_FATAL, "ERROR: %s username is empty", prefix);
-		}
+        {
+          if (username_from_stdin && !(flags & GET_USER_PASS_PASSWORD_ONLY))
+            {
+              struct buffer user_prompt = alloc_buf_gc (128, &gc);
+              buf_printf (&user_prompt, "Enter %s Username:", prefix);
+              if (!get_console_input (BSTR (&user_prompt), true, up->username, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not read %s username from stdin", prefix);
+              if (strlen (up->username) == 0)
+                msg (M_FATAL, "ERROR: %s username is empty", prefix);
+            }
 
-	      if (password_from_stdin && !get_console_input (BSTR (&pass_prompt), false, up->password, USER_PASS_LEN))
-		msg (M_FATAL, "ERROR: could not not read %s password from stdin", prefix);
+          if (password_from_stdin)
+            {
+              struct buffer pass_prompt = alloc_buf_gc (128, &gc);
+              buf_printf (&pass_prompt, "Enter %s Password:", prefix);
+              if (!get_console_input (BSTR (&pass_prompt), false, up->password, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not not read %s password from stdin", prefix);
+            }
 
 #ifdef ENABLE_CLIENT_CR
-	      if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
-		{
-		  char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
-		  struct buffer packed_resp;
-		  char *pw64=NULL, *resp64=NULL;
-
-		  msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", auth_challenge);
-		  if (!get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response, USER_PASS_LEN))
-		    msg (M_FATAL, "ERROR: could not read static challenge response from stdin");
-		  if (openvpn_base64_encode(up->password, strlen(up->password), &pw64) == -1
-		      || openvpn_base64_encode(response, strlen(response), &resp64) == -1)
-		    msg (M_FATAL, "ERROR: could not base64-encode password/static_response");
-		  buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
-		  buf_printf (&packed_resp, "SCRV1:%s:%s", pw64, resp64);
-		  string_clear(pw64);
-		  free(pw64);
-		  string_clear(resp64);
-		  free(resp64);
-		}
-#endif
+          if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
+            {
+              char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
+              struct buffer packed_resp;
+              char *pw64=NULL, *resp64=NULL;
+
+              msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", auth_challenge);
+              if (!get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not read static challenge response from stdin");
+              if (openvpn_base64_encode(up->password, strlen(up->password), &pw64) == -1
+                  || openvpn_base64_encode(response, strlen(response), &resp64) == -1)
+                msg (M_FATAL, "ERROR: could not base64-encode password/static_response");
+              buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
+              buf_printf (&packed_resp, "SCRV1:%s:%s", pw64, resp64);
+              string_clear(pw64);
+              free(pw64);
+              string_clear(resp64);
+              free(resp64);
 	    }
+#endif
 	}
 
       string_mod (up->username, CC_PRINT, CC_CRLF, 0);
Signature · 1 line
1.9.1
author
#5 lore raw

PATCH 1/1 Allow challenge/response to be read from --auth-user-pass file.

Must be applied after the patch to fix CR prompting.

---
 doc/openvpn.8      |  8 +++++++-
 src/openvpn/misc.c | 19 +++++++++++++------
 2 files changed, 20 insertions(+), 7 deletions(-)
doc/openvpn.8 +7 -1
index 1b9dcae..e806930 100644
@@ -3800,7 +3800,13 @@ over the client's routing table.
 Authenticate with server using username/password.
 .B up
 is a file containing username/password on 2 lines. If the
-password line is missing, OpenVPN will prompt for one.
+password line is missing or empty, OpenVPN will prompt for one.
+The third line of the file can be used to enter a response to
+a challenge, such as "push", "phone", etc., which can be used
+to eliminate an extra prompt for an unchanging text if your
+challenge/response setup uses push/callback two-factor auth
+(and can also be combined with an empty password line to still
+be prompted for your password).
 
 If
 .B up
src/openvpn/misc.c +13 -6
index 83e10f7..4de4a17 100644
@@ -1038,6 +1038,9 @@ get_user_pass_cr (struct user_pass *up,
       bool from_authfile = (auth_file && !streq (auth_file, "stdin"));
       bool username_from_stdin = false;
       bool password_from_stdin = false;
+#ifdef ENABLE_CLIENT_CR
+      char response_buf[USER_PASS_LEN] = { '\0' };
+#endif
 
       if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
 	msg (M_WARN, "Note: previous '%s' credentials failed", prefix);

@@ -1111,6 +1114,10 @@ get_user_pass_cr (struct user_pass *up,
           if (fgets (password_buf, USER_PASS_LEN, fp) != NULL)
             {
               chomp (password_buf);
+#ifdef ENABLE_CLIENT_CR
+              if (!(flags & GET_USER_PASS_PASSWORD_ONLY) && fgets (response_buf, USER_PASS_LEN, fp) != NULL)
+                chomp (response_buf);
+#endif
             }
 
           if (flags & GET_USER_PASS_PASSWORD_ONLY && !password_buf[0])

@@ -1138,15 +1145,14 @@ get_user_pass_cr (struct user_pass *up,
           struct auth_challenge_info *ac = get_auth_challenge (auth_challenge, &gc);
           if (ac)
             {
-              char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
               struct buffer packed_resp;
 
               buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
               msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", ac->challenge_text);
-              if (!get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response, USER_PASS_LEN))
+              if (!response_buf[0] && !get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response_buf, USER_PASS_LEN))
                 msg (M_FATAL, "ERROR: could not read challenge response from stdin");
               strncpynt (up->username, ac->user, USER_PASS_LEN);
-              buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response);
+              buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response_buf);
             }
           else
             {

@@ -1177,15 +1183,16 @@ get_user_pass_cr (struct user_pass *up,
 #ifdef ENABLE_CLIENT_CR
           if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
             {
-              char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
               struct buffer packed_resp;
               char *pw64=NULL, *resp64=NULL;
 
               msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", auth_challenge);
-              if (!get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response, USER_PASS_LEN))
+              if (!response_buf[0] && !get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response_buf, USER_PASS_LEN))
                 msg (M_FATAL, "ERROR: could not read static challenge response from stdin");
+              else if (!response_buf[0] && (flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO))
+                msg (M_INFO|M_NOPREFIX, "Response: %s", response_buf);
               if (openvpn_base64_encode(up->password, strlen(up->password), &pw64) == -1
-                  || openvpn_base64_encode(response, strlen(response), &resp64) == -1)
+                  || openvpn_base64_encode(response_buf, strlen(response_buf), &resp64) == -1)
                 msg (M_FATAL, "ERROR: could not base64-encode password/static_response");
               buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
               buf_printf (&packed_resp, "SCRV1:%s:%s", pw64, resp64);
Signature · 1 line
1.9.1
author
#6 lore raw

Re: [Openvpn-devel] [PATCH] Improve stdin prompting section, fixing CR prompting.

In reply to Wayne Davison
Quoted 4 lines · Wayne Davison
 src/openvpn/misc.c | 119
+++++++++++++++++++++++++----------------------------
 1 file changed, 57 insertions(+), 62 deletions(-)

Any questions I can answer about this patch? This is such a straight-forward bug with a simple fix that I'd hope that it makes it into the upcoming release. (The patch is mainly re-indenting, which bloats it a good bit.)

..wayne..

#7 lore raw

Re: [Openvpn-devel] [PATCH] Improve stdin prompting section, fixing CR prompting.

Hi,

Quoted 9 lines · Wayne Davison
Quoted 4 lines · Wayne Davison
 src/openvpn/misc.c | 119
+++++++++++++++++++++++++----------------------------
 1 file changed, 57 insertions(+), 62 deletions(-)
Quoted 5 lines

Any questions I can answer about this patch? This is such a straight-forward bug with a simple fix that I'd hope that it makes it into the upcoming release. (The patch is mainly re-indenting, which bloats it a good bit.)

I took a quick look and it seems a simplified patch that addresses the most critical-sounding issue (challenge/reponse not prompted for from stdin) may be more useful.

From the cover-letter to the patches:

There is a bug in the challenge/response code when the username & password

Quoted 4 lines

is read from a file -- the response is never prompted for. This bug affects
older versions, including 2.3.8.

A patch that fixes that and only that is easier to review.

I also added code to read the response out of the --auth-user-pass file if

Quoted 4 lines

there is a 3rd line present. This is particularly useful for an 2FA setup where the response is unchanging (in my case it is always "push" for a Duo Push).

I see no compelling reason to read response to a challenge from a file. If the response is unchanging, its a misuse of challenge/response. Implemet the check on the server such that it doesn't have to hear "push" every time from the client. In case of, duo it could be an out-of-band way to add user preferences on the sever saying user1 prefers push, user2 phone, user3 sms etc. Or send it attached to the username or password and parse it in the server-side user-pass-verify script or in a custom pam_module, for example. This is a custom use case trying to exploit CR for something else. Nothing wrong with that, but may be done without adding a new "feature" into OpenVPN.

Selva

author
#8 lore raw

Re: [Openvpn-devel] [PATCH] Improve stdin prompting section, fixing CR prompting.

Quoted 4 lines · Selva Nair

I took a quick look and it seems a simplified patch that addresses the most critical-sounding issue (challenge/reponse not prompted for from stdin) may be more useful.

That's exactly what that patch is.

From the cover-letter to the patches:

Quoted 1 line

That's the older patch set. The newer one (based on newer master changes in misc.c) separates the fix from the feature.

If the response is unchanging, its a misuse of challenge/response.

Quoted 1 line

It's only mostly unchanging. The user can respond "push", "sms", "phone", or enter a number from the generator app as they see fit. For me, I use "push" most of the time. I figure if the app is going to support reading the prompts from a file, it should support all the prompts the same way for consistency.

..wayne..

#9 lore raw

Re: [Openvpn-devel] [PATCH] Improve stdin prompting section, fixing CR prompting.

In reply to Wayne Davison

Hi,

Quoted 9 lines · Selva Nair
Quoted 5 lines

I took a quick look and it seems a simplified patch that addresses the most critical-sounding issue (challenge/reponse not prompted for from stdin) may be more useful.

That's exactly what that patch is.

Ok, now looking at the newer, version:

Quoted 5 lines · Wayne Davison
---
 src/openvpn/misc.c | 119
+++++++++++++++++++++++++----------------------------
 1 file changed, 57 insertions(+), 62 deletions(-)

A commit message that explains the "why" and "how" will be helpful.

Quoted 64 lines
src/openvpn/misc.c +14 -26
index bc411bf..83e10f7 100644
@@ -1132,74 +1132,69 @@ get_user_pass_cr (struct user_pass *up,
           password_from_stdin = true;
         }
 
-      /*
-       * Get username/password from standard input?
-       */
-      if (username_from_stdin || password_from_stdin)
-       {
 #ifdef ENABLE_CLIENT_CR
-         if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
-           {
-             struct auth_challenge_info *ac = get_auth_challenge
 (auth_challenge, &gc);
-             if (ac)
-               {
-                 char *response = (char *) gc_malloc (USER_PASS_LEN,
 false, &gc);
-                 struct buffer packed_resp;
-
-                 buf_set_write (&packed_resp, (uint8_t*)up->password,
 USER_PASS_LEN);
-                 msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s",
 ac->challenge_text);
-                 if (!get_console_input ("Response:",
 BOOL_CAST(ac->flags&CR_ECHO), response, USER_PASS_LEN))
-                   msg (M_FATAL, "ERROR: could not read challenge
 response from stdin");
-                 strncpynt (up->username, ac->user, USER_PASS_LEN);
-                 buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id,
 response);
-               }
-             else
-               {
-                 msg (M_FATAL, "ERROR: received malformed challenge
 request from server");
-               }
-           }
-         else
+      if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
+        {
+          struct auth_challenge_info *ac = get_auth_challenge
 (auth_challenge, &gc);
+          if (ac)
+            {
+              char *response = (char *) gc_malloc (USER_PASS_LEN, false,
 &gc);
+              struct buffer packed_resp;
+
+              buf_set_write (&packed_resp, (uint8_t*)up->password,
 USER_PASS_LEN);
+              msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s",
 ac->challenge_text);
+              if (!get_console_input ("Response:",
 BOOL_CAST(ac->flags&CR_ECHO), response, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not read challenge response
 from stdin");
+              strncpynt (up->username, ac->user, USER_PASS_LEN);
+              buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id,

src/openvpn/misc.c get_user_pass_cr (struct user_pass *up, · line 1173

Quoted 10 lines
src/openvpn/misc.c +7 -0
 response);
+            }
+          else
+            {
+              msg (M_FATAL, "ERROR: received malformed challenge request
 from server");
+            }
+        }
+      else

I would either avoid excessive indentation changes or leave it for a white-space-only patch (which will likely get ignored :). Same goes for rest of the patch, which only moves around variable declarations, or change whitespace.

This could be a small ~2 line patch -- easier to review and test.

Selva

author
#10 lore raw

Re: [Openvpn-devel] [PATCH] Improve stdin prompting section, fixing CR prompting.

Quoted 2 lines · Selva Nair

This could be a small ~2 line patch -- easier to review and test.

I'll post a 3-part patch as a reply to this email. They are: (1) the "if" removal that fixes the bug, (2) the prompt buffer moves, and (3) the reindentation. If you end up wanting to consider the CR-from-a-file feature, that patch would be unchanged from the last one I posted.

..wayne..

author
#11 lore raw

PATCH 1/3 Fix CR prompting when user & pass are read from a file.

The code that reads the challenge response (both dynamic & static) will not prompt the user if the username and password information was read from a file. In the latest code this can be fixed by simply removing the "if (username_from_stdin || password_from_stdin)" condition because all the deeper code is already conditionalized adequately to only prompt for the missing information.

NOTE: indentation left too deep so that changes are clearly seen.

---
 src/openvpn/misc.c | 3 ---
 1 file changed, 3 deletions(-)
src/openvpn/misc.c +0 -3
index 05ed073..a9259f0 100644
@@ -1141,8 +1141,6 @@ get_user_pass_cr (struct user_pass *up,
       /*
        * Get username/password from standard input?
        */
-      if (username_from_stdin || password_from_stdin)
-	{
 #ifdef ENABLE_CLIENT_CR
 	  if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
 	    {

@@ -1206,7 +1204,6 @@ get_user_pass_cr (struct user_pass *up,
 		}
 #endif
 	    }
-	}
 
       string_mod (up->username, CC_PRINT, CC_CRLF, 0);
       string_mod (up->password, CC_PRINT, CC_CRLF, 0);
Signature · 1 line
1.9.1
author
#12 lore raw

PATCH 2/3 Move 2 prompt buffers into deeper blocks.

Two buffers used for username/password prompting can be moved into a deeper block so that they don't get set if they are not going to be used.

---
 src/openvpn/misc.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
src/openvpn/misc.c +9 -8
index a9259f0..e0aa5f9 100644
@@ -1165,22 +1165,23 @@ get_user_pass_cr (struct user_pass *up,
 	  else
 #endif
 	    {
-	      struct buffer user_prompt = alloc_buf_gc (128, &gc);
-	      struct buffer pass_prompt = alloc_buf_gc (128, &gc);
-
-	      buf_printf (&user_prompt, "Enter %s Username:", prefix);
-	      buf_printf (&pass_prompt, "Enter %s Password:", prefix);
-
 	      if (username_from_stdin && !(flags & GET_USER_PASS_PASSWORD_ONLY))
 		{
+		  struct buffer user_prompt = alloc_buf_gc (128, &gc);
+		  buf_printf (&user_prompt, "Enter %s Username:", prefix);
 		  if (!get_console_input (BSTR (&user_prompt), true, up->username, USER_PASS_LEN))
 		    msg (M_FATAL, "ERROR: could not read %s username from stdin", prefix);
 		  if (strlen (up->username) == 0)
 		    msg (M_FATAL, "ERROR: %s username is empty", prefix);
 		}
 
-	      if (password_from_stdin && !get_console_input (BSTR (&pass_prompt), false, up->password, USER_PASS_LEN))
-		msg (M_FATAL, "ERROR: could not not read %s password from stdin", prefix);
+              if (password_from_stdin)
+                {
+		  struct buffer pass_prompt = alloc_buf_gc (128, &gc);
+		  buf_printf (&pass_prompt, "Enter %s Password:", prefix);
+		  if (!get_console_input (BSTR (&pass_prompt), false, up->password, USER_PASS_LEN))
+                    msg (M_FATAL, "ERROR: could not not read %s password from stdin", prefix);
+                }
 
 #ifdef ENABLE_CLIENT_CR
 	      if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
Signature · 1 line
1.9.1
author
#13 lore raw

PATCH 3/3 Fix too-deep indentation.

In reply to Wayne Davison

Fix the indentation on the code block that got moved out of an unneeded "if".

---
 src/openvpn/misc.c | 116 ++++++++++++++++++++++++++---------------------------
 1 file changed, 58 insertions(+), 58 deletions(-)
src/openvpn/misc.c +58 -58
index e0aa5f9..517a2eb 100644
@@ -1142,69 +1142,69 @@ get_user_pass_cr (struct user_pass *up,
        * Get username/password from standard input?
        */
 #ifdef ENABLE_CLIENT_CR
-	  if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
-	    {
-	      struct auth_challenge_info *ac = get_auth_challenge (auth_challenge, &gc);
-	      if (ac)
-		{
-		  char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
-		  struct buffer packed_resp;
-
-		  buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
-		  msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", ac->challenge_text);
-		  if (!get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response, USER_PASS_LEN))
-		    msg (M_FATAL, "ERROR: could not read challenge response from stdin");
-		  strncpynt (up->username, ac->user, USER_PASS_LEN);
-		  buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response);
-		}
-	      else
-		{
-		  msg (M_FATAL, "ERROR: received malformed challenge request from server");
-		}
-	    }
-	  else
+      if (auth_challenge && (flags & GET_USER_PASS_DYNAMIC_CHALLENGE))
+        {
+          struct auth_challenge_info *ac = get_auth_challenge (auth_challenge, &gc);
+          if (ac)
+            {
+              char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
+              struct buffer packed_resp;
+
+              buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
+              msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", ac->challenge_text);
+              if (!get_console_input ("Response:", BOOL_CAST(ac->flags&CR_ECHO), response, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not read challenge response from stdin");
+              strncpynt (up->username, ac->user, USER_PASS_LEN);
+              buf_printf (&packed_resp, "CRV1::%s::%s", ac->state_id, response);
+            }
+          else
+            {
+              msg (M_FATAL, "ERROR: received malformed challenge request from server");
+            }
+        }
+      else
 #endif
-	    {
-	      if (username_from_stdin && !(flags & GET_USER_PASS_PASSWORD_ONLY))
-		{
-		  struct buffer user_prompt = alloc_buf_gc (128, &gc);
-		  buf_printf (&user_prompt, "Enter %s Username:", prefix);
-		  if (!get_console_input (BSTR (&user_prompt), true, up->username, USER_PASS_LEN))
-		    msg (M_FATAL, "ERROR: could not read %s username from stdin", prefix);
-		  if (strlen (up->username) == 0)
-		    msg (M_FATAL, "ERROR: %s username is empty", prefix);
-		}
+        {
+          if (username_from_stdin && !(flags & GET_USER_PASS_PASSWORD_ONLY))
+            {
+              struct buffer user_prompt = alloc_buf_gc (128, &gc);
+              buf_printf (&user_prompt, "Enter %s Username:", prefix);
+              if (!get_console_input (BSTR (&user_prompt), true, up->username, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not read %s username from stdin", prefix);
+              if (strlen (up->username) == 0)
+                msg (M_FATAL, "ERROR: %s username is empty", prefix);
+            }
 
-              if (password_from_stdin)
-                {
-		  struct buffer pass_prompt = alloc_buf_gc (128, &gc);
-		  buf_printf (&pass_prompt, "Enter %s Password:", prefix);
-		  if (!get_console_input (BSTR (&pass_prompt), false, up->password, USER_PASS_LEN))
-                    msg (M_FATAL, "ERROR: could not not read %s password from stdin", prefix);
-                }
+          if (password_from_stdin)
+            {
+              struct buffer pass_prompt = alloc_buf_gc (128, &gc);
+              buf_printf (&pass_prompt, "Enter %s Password:", prefix);
+              if (!get_console_input (BSTR (&pass_prompt), false, up->password, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not not read %s password from stdin", prefix);
+            }
 
 #ifdef ENABLE_CLIENT_CR
-	      if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
-		{
-		  char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
-		  struct buffer packed_resp;
-		  char *pw64=NULL, *resp64=NULL;
-
-		  msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", auth_challenge);
-		  if (!get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response, USER_PASS_LEN))
-		    msg (M_FATAL, "ERROR: could not read static challenge response from stdin");
-		  if (openvpn_base64_encode(up->password, strlen(up->password), &pw64) == -1
-		      || openvpn_base64_encode(response, strlen(response), &resp64) == -1)
-		    msg (M_FATAL, "ERROR: could not base64-encode password/static_response");
-		  buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
-		  buf_printf (&packed_resp, "SCRV1:%s:%s", pw64, resp64);
-		  string_clear(pw64);
-		  free(pw64);
-		  string_clear(resp64);
-		  free(resp64);
-		}
-#endif
+          if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
+            {
+              char *response = (char *) gc_malloc (USER_PASS_LEN, false, &gc);
+              struct buffer packed_resp;
+              char *pw64=NULL, *resp64=NULL;
+
+              msg (M_INFO|M_NOPREFIX, "CHALLENGE: %s", auth_challenge);
+              if (!get_console_input ("Response:", BOOL_CAST(flags & GET_USER_PASS_STATIC_CHALLENGE_ECHO), response, USER_PASS_LEN))
+                msg (M_FATAL, "ERROR: could not read static challenge response from stdin");
+              if (openvpn_base64_encode(up->password, strlen(up->password), &pw64) == -1
+                  || openvpn_base64_encode(response, strlen(response), &resp64) == -1)
+                msg (M_FATAL, "ERROR: could not base64-encode password/static_response");
+              buf_set_write (&packed_resp, (uint8_t*)up->password, USER_PASS_LEN);
+              buf_printf (&packed_resp, "SCRV1:%s:%s", pw64, resp64);
+              string_clear(pw64);
+              free(pw64);
+              string_clear(resp64);
+              free(resp64);
 	    }
+#endif
+        }
 
       string_mod (up->username, CC_PRINT, CC_CRLF, 0);
       string_mod (up->password, CC_PRINT, CC_CRLF, 0);
Signature · 1 line
1.9.1
#14 lore raw

Re: [Openvpn-devel] [PATCH 1/3] Fix CR prompting when user & pass are read from a file.

In reply to Wayne Davison

Hi,

Quoted 4 lines · Wayne Davison

The code that reads the challenge response (both dynamic & static) will not prompt the user if the username and password information was read from a file.

Agreed this is not good and should be fixed. But the patch is broken

Quoted 5 lines

In the latest code this can be fixed by simply removing
the "if (username_from_stdin || password_from_stdin)" condition because all the deeper code is already conditionalized adequately to only prompt for the missing information.

This doesn't work: it messes up any response already read from the management interface.

A simple way of doing this is to define a new variable, say, CR_from_stdin = true if not read from management. Then add "CR_from_stdin == true" to those two if (auth_challenge && ..) lines. The patch will still remain only a few lines and no changes in indentation/whitespace needed. The logic will stay the same as what its now too.

Thanks,

Selva

author
#15 lore raw

Re: [Openvpn-devel] [PATCH 1/3] Fix CR prompting when user & pass are read from a file.

Quoted 3 lines · Selva Nair

This doesn't work: it messes up any response already read from the management interface.

Given what options? I tested --management with --management-query-passwords without issue. Are you combining that with --auth-user-pass FILENAME? I can't get that combo to work even without my changes. I guess I'll let you guys fix it up the right way.

..wayne..