[Openvpn-devel,v3] Parse static challenge response in auth-pam plugin

Message ID 1532486093-24793-1-git-send-email-selva.nair@gmail.com
State Accepted
Headers show
Series
  • [Openvpn-devel,v3] Parse static challenge response in auth-pam plugin
Related show

Commit Message

Selva Nair July 25, 2018, 2:34 a.m.
From: Selva Nair <selva.nair@gmail.com>

If static challenge is in use, the password passed to the plugin by openvpn
is of the form "SCRV1:base64-pass:base64-response". Parse this string to
separate it into password and response and use them to respond to queries
in the pam conversation function.

On the plugin parameters line the substitution keyword for the static
challenge response is "OTP". For example, for pam config named "test" that
prompts for "user", "password" and "pin", use

plugin openvpn-auth-pam.so "test user USERNAME password PASSWORD pin OTP"

Signed-off-by: Selva Nair <selva.nair@gmail.com>

---
v2: Depends on the base64 export patch
v3: match password string with "SCRV1:" instead of "SCRV1"
(pointed out by Joe Bell <joeaintexas@gmail.com>)

 src/plugins/auth-pam/README.auth-pam | 15 +++++---
 src/plugins/auth-pam/auth-pam.c      | 75 +++++++++++++++++++++++++++++++++++-
 2 files changed, 84 insertions(+), 6 deletions(-)

Comments

Joe Bell July 25, 2018, 9:31 p.m. | #1
I don't know if it is appropriate to reply to this post in this manner, but
Selva's static challenge response in the PAM plugin would be a great
addition; I've applied this and the base64 patch and can successfully use
the implementation with Tunnelblick (which is supporting static-challenge
as of 3.7.7beta03, as well as the current version of the Windows OpenVPN
client (I'm using Google Authenticator for the OTP).

If there is anything I can do to provide a list of testcases, write-up on
its configuration and usage, etc., in aide of getting the patch merged and
released, please let me know.

Many thanks,
Joe


On Tue, Jul 24, 2018 at 9:34 PM, <selva.nair@gmail.com> wrote:

> From: Selva Nair <selva.nair@gmail.com>
>
> If static challenge is in use, the password passed to the plugin by openvpn
> is of the form "SCRV1:base64-pass:base64-response". Parse this string to
> separate it into password and response and use them to respond to queries
> in the pam conversation function.
>
> On the plugin parameters line the substitution keyword for the static
> challenge response is "OTP". For example, for pam config named "test" that
> prompts for "user", "password" and "pin", use
>
> plugin openvpn-auth-pam.so "test user USERNAME password PASSWORD pin OTP"
>
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
>
> ---
> v2: Depends on the base64 export patch
> v3: match password string with "SCRV1:" instead of "SCRV1"
> (pointed out by Joe Bell <joeaintexas@gmail.com>)
>
>  src/plugins/auth-pam/README.auth-pam | 15 +++++---
>  src/plugins/auth-pam/auth-pam.c      | 75 ++++++++++++++++++++++++++++++
> +++++-
>  2 files changed, 84 insertions(+), 6 deletions(-)
>
> diff --git a/src/plugins/auth-pam/README.auth-pam
> b/src/plugins/auth-pam/README.auth-pam
> index e123690..9081565 100644
> --- a/src/plugins/auth-pam/README.auth-pam
> +++ b/src/plugins/auth-pam/README.auth-pam
> @@ -36,19 +36,20 @@ pairs to answer PAM module queries.
>
>  For example:
>
> -  plugin openvpn-auth-pam.so "login login USERNAME password PASSWORD"
> +  plugin openvpn-auth-pam.so "login login USERNAME password PASSWORD pin
> OTP"
>
>  tells auth-pam to (a) use the "login" PAM module, (b) answer a
> -"login" query with the username given by the OpenVPN client, and
> -(c) answer a "password" query with the password given by the
> -OpenVPN client.  This provides flexibility in dealing with the different
> +"login" query with the username given by the OpenVPN client,
> +(c) answer a "password" query with the password, and (d) answer a
> +"pin" query with the OTP given by the OpenVPN client.
> +This provides flexibility in dealing with different
>  types of query strings which different PAM modules might generate.
>  For example, suppose you were using a PAM module called
>  "test" which queried for "name" rather than "login":
>
>    plugin openvpn-auth-pam.so "test name USERNAME password PASSWORD"
>
> -While "USERNAME" "COMMONNAME" and "PASSWORD" are special strings which
> substitute
> +While "USERNAME" "COMMONNAME" "PASSWORD" and "OTP" are special strings
> which substitute
>  to client-supplied values, it is also possible to name literal values
>  to use as PAM module query responses.  For example, suppose that the
>  login module queried for a third parameter, "domain" which
> @@ -61,6 +62,10 @@ the operation of this plugin:
>
>    client-cert-not-required
>    username-as-common-name
> +  static-challenge
> +
> +Use of --static challenege is required to pass a pin (represented by
> "OTP" in
> +parameter substituion) or a second password.
>
>  Run OpenVPN with --verb 7 or higher to get debugging output from
>  this plugin, including the list of queries presented by the
> diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-
> pam.c
> index 26b0eeb..e22ce5f 100644
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -6,6 +6,7 @@
>   *             packet compression.
>   *
>   *  Copyright (C) 2002-2018 OpenVPN Inc <sales@openvpn.net>
> + *  Copyright (C) 2016-2018 Selva Nair <selva.nair@gmail.com>
>   *
>   *  This program is free software; you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License version 2
> @@ -64,6 +65,7 @@
>
>  /* Pointers to functions exported from openvpn */
>  static plugin_secure_memzero_t plugin_secure_memzero = NULL;
> +static plugin_base64_decode_t plugin_base64_decode = NULL;
>
>  /*
>   * Plugin state, used by foreground
> @@ -87,6 +89,7 @@ struct auth_pam_context
>   *  "USERNAME" -- substitute client-supplied username
>   *  "PASSWORD" -- substitute client-specified password
>   *  "COMMONNAME" -- substitute client certificate common name
> + *  "OTP" -- substitute static challenge response if available
>   */
>
>  #define N_NAME_VALUE 16
> @@ -111,6 +114,7 @@ struct user_pass {
>      char username[128];
>      char password[128];
>      char common_name[128];
> +    char response[128];
>
>      const struct name_value_list *name_value_list;
>  };
> @@ -276,6 +280,66 @@ name_value_match(const char *query, const char *match)
>      return strncasecmp(match, query, strlen(match)) == 0;
>  }
>
> +/*
> + * Split and decode up->password in the form SCRV1:base64_pass:base64_
> response
> + * into pass and response and save in up->password and up->response.
> + * If the password is not in the expected format, input is not changed.
> + */
> +static void
> +split_scrv1_password(struct user_pass *up)
> +{
> +    const int skip = strlen("SCRV1:");
> +    if (strncmp(up->password, "SCRV1:", skip) != 0)
> +    {
> +        return;
> +    }
> +
> +    char *tmp = strdup(up->password);
> +    if (!tmp)
> +    {
> +        fprintf(stderr, "AUTH-PAM: out of memory parsing static challenge
> password\n");
> +        goto out;
> +    }
> +
> +    char *pass = tmp + skip;
> +    char *resp = strchr(pass, ':');
> +    if (!resp) /* string not in SCRV1:xx:yy format */
> +    {
> +        goto out;
> +    }
> +    *resp++ = '\0';
> +
> +    int n = plugin_base64_decode(pass, up->password,
> sizeof(up->password)-1);
> +    if (n > 0)
> +    {
> +        up->password[n] = '\0';
> +        n = plugin_base64_decode(resp, up->response,
> sizeof(up->response)-1);
> +        if (n > 0)
> +        {
> +            up->response[n] = '\0';
> +            if (DEBUG(up->verb))
> +            {
> +                fprintf(stderr, "AUTH-PAM: BACKGROUND: parsed static
> challenge password\n");
> +            }
> +            goto out;
> +        }
> +    }
> +
> +    /* decode error: reinstate original value of up->password and return
> */
> +    plugin_secure_memzero(up->password, sizeof(up->password));
> +    plugin_secure_memzero(up->response, sizeof(up->response));
> +    strcpy(up->password, tmp); /* tmp is guaranteed to fit in
> up->password */
> +
> +    fprintf(stderr, "AUTH-PAM: base64 decode error while parsing static
> challenge password\n");
> +
> +out:
> +    if (tmp)
> +    {
> +        plugin_secure_memzero(tmp, strlen(tmp));
> +        free(tmp);
> +    }
> +}
> +
>  OPENVPN_EXPORT int
>  openvpn_plugin_open_v3(const int v3structver,
>                         struct openvpn_plugin_args_open_in const *args,
> @@ -316,6 +380,7 @@ openvpn_plugin_open_v3(const int v3structver,
>
>      /* Save global pointers to functions exported from openvpn */
>      plugin_secure_memzero = args->callbacks->plugin_secure_memzero;
> +    plugin_base64_decode = args->callbacks->plugin_base64_decode;
>
>      /*
>       * Make sure we have two string arguments: the first is the .so name,
> @@ -599,6 +664,10 @@ my_conv(int n, const struct pam_message **msg_array,
>                      {
>                          aresp[i].resp = searchandreplace(match_value,
> "COMMONNAME", up->common_name);
>                      }
> +                    else if (strstr(match_value, "OTP"))
> +                    {
> +                        aresp[i].resp = searchandreplace(match_value,
> "OTP", up->response);
> +                    }
>                      else
>                      {
>                          aresp[i].resp = strdup(match_value);
> @@ -787,6 +856,9 @@ pam_server(int fd, const char *service, int verb,
> const struct name_value_list *
>  #endif
>                  }
>
> +                /* If password is of the form SCRV1:base64:base64 split
> it up */
> +                split_scrv1_password(&up);
> +
>                  if (pam_auth(service, &up)) /* Succeeded */
>                  {
>                      if (send_control(fd, RESPONSE_VERIFY_SUCCEEDED) == -1)
> @@ -818,10 +890,11 @@ pam_server(int fd, const char *service, int verb,
> const struct name_value_list *
>                          command);
>                  goto done;
>          }
> +        plugin_secure_memzero(up.response, sizeof(up.response));
>      }
>  done:
> -
>      plugin_secure_memzero(up.password, sizeof(up.password));
> +    plugin_secure_memzero(up.response, sizeof(up.response));
>  #ifdef USE_PAM_DLOPEN
>      dlclose_pam();
>  #endif
> --
> 2.1.4
>
>
> ------------------------------------------------------------
> ------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
<div dir="ltr">I don&#39;t know if it is appropriate to reply to this post in this manner, but Selva&#39;s static challenge response in the PAM plugin would be a great addition; I&#39;ve applied this and the base64 patch and can successfully use the implementation with Tunnelblick (which is supporting static-challenge as of 3.7.7beta03, as well as the current version of the Windows OpenVPN client (I&#39;m using Google Authenticator for the OTP).<div><br></div><div>If there is anything I can do to provide a list of testcases, write-up on its configuration and usage, etc., in aide of getting the patch merged and released, please let me know.</div><div><br></div><div>Many thanks,</div><div>Joe</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 24, 2018 at 9:34 PM,  <span dir="ltr">&lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt;<br>
<br>
If static challenge is in use, the password passed to the plugin by openvpn<br>
is of the form &quot;SCRV1:base64-pass:base64-<wbr>response&quot;. Parse this string to<br>
separate it into password and response and use them to respond to queries<br>
in the pam conversation function.<br>
<br>
On the plugin parameters line the substitution keyword for the static<br>
challenge response is &quot;OTP&quot;. For example, for pam config named &quot;test&quot; that<br>
prompts for &quot;user&quot;, &quot;password&quot; and &quot;pin&quot;, use<br>
<br>
plugin openvpn-auth-pam.so &quot;test user USERNAME password PASSWORD pin OTP&quot;<br>
<br>
Signed-off-by: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt;<br>
<br>
---<br>
v2: Depends on the base64 export patch<br>
v3: match password string with &quot;SCRV1:&quot; instead of &quot;SCRV1&quot;<br>
(pointed out by Joe Bell &lt;<a href="mailto:joeaintexas@gmail.com">joeaintexas@gmail.com</a>&gt;)<br>
<br>
 src/plugins/auth-pam/README.<wbr>auth-pam | 15 +++++---<br>
 src/plugins/auth-pam/auth-pam.<wbr>c      | 75 ++++++++++++++++++++++++++++++<wbr>+++++-<br>
 2 files changed, 84 insertions(+), 6 deletions(-)<br>
<br>
diff --git a/src/plugins/auth-pam/README.<wbr>auth-pam b/src/plugins/auth-pam/README.<wbr>auth-pam<br>
index e123690..9081565 100644<br>
--- a/src/plugins/auth-pam/README.<wbr>auth-pam<br>
+++ b/src/plugins/auth-pam/README.<wbr>auth-pam<br>
@@ -36,19 +36,20 @@ pairs to answer PAM module queries.<br>
<br>
 For example:<br>
<br>
-  plugin openvpn-auth-pam.so &quot;login login USERNAME password PASSWORD&quot;<br>
+  plugin openvpn-auth-pam.so &quot;login login USERNAME password PASSWORD pin OTP&quot;<br>
<br>
 tells auth-pam to (a) use the &quot;login&quot; PAM module, (b) answer a<br>
-&quot;login&quot; query with the username given by the OpenVPN client, and<br>
-(c) answer a &quot;password&quot; query with the password given by the<br>
-OpenVPN client.  This provides flexibility in dealing with the different<br>
+&quot;login&quot; query with the username given by the OpenVPN client,<br>
+(c) answer a &quot;password&quot; query with the password, and (d) answer a<br>
+&quot;pin&quot; query with the OTP given by the OpenVPN client.<br>
+This provides flexibility in dealing with different<br>
 types of query strings which different PAM modules might generate.<br>
 For example, suppose you were using a PAM module called<br>
 &quot;test&quot; which queried for &quot;name&quot; rather than &quot;login&quot;:<br>
<br>
   plugin openvpn-auth-pam.so &quot;test name USERNAME password PASSWORD&quot;<br>
<br>
-While &quot;USERNAME&quot; &quot;COMMONNAME&quot; and &quot;PASSWORD&quot; are special strings which substitute<br>
+While &quot;USERNAME&quot; &quot;COMMONNAME&quot; &quot;PASSWORD&quot; and &quot;OTP&quot; are special strings which substitute<br>
 to client-supplied values, it is also possible to name literal values<br>
 to use as PAM module query responses.  For example, suppose that the<br>
 login module queried for a third parameter, &quot;domain&quot; which<br>
@@ -61,6 +62,10 @@ the operation of this plugin:<br>
<br>
   client-cert-not-required<br>
   username-as-common-name<br>
+  static-challenge<br>
+<br>
+Use of --static challenege is required to pass a pin (represented by &quot;OTP&quot; in<br>
+parameter substituion) or a second password.<br>
<br>
 Run OpenVPN with --verb 7 or higher to get debugging output from<br>
 this plugin, including the list of queries presented by the<br>
diff --git a/src/plugins/auth-pam/auth-<wbr>pam.c b/src/plugins/auth-pam/auth-<wbr>pam.c<br>
index 26b0eeb..e22ce5f 100644<br>
--- a/src/plugins/auth-pam/auth-<wbr>pam.c<br>
+++ b/src/plugins/auth-pam/auth-<wbr>pam.c<br>
@@ -6,6 +6,7 @@<br>
  *             packet compression.<br>
  *<br>
  *  Copyright (C) 2002-2018 OpenVPN Inc &lt;<a href="mailto:sales@openvpn.net">sales@openvpn.net</a>&gt;<br>
+ *  Copyright (C) 2016-2018 Selva Nair &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt;<br>
  *<br>
  *  This program is free software; you can redistribute it and/or modify<br>
  *  it under the terms of the GNU General Public License version 2<br>
@@ -64,6 +65,7 @@<br>
<br>
 /* Pointers to functions exported from openvpn */<br>
 static plugin_secure_memzero_t plugin_secure_memzero = NULL;<br>
+static plugin_base64_decode_t plugin_base64_decode = NULL;<br>
<br>
 /*<br>
  * Plugin state, used by foreground<br>
@@ -87,6 +89,7 @@ struct auth_pam_context<br>
  *  &quot;USERNAME&quot; -- substitute client-supplied username<br>
  *  &quot;PASSWORD&quot; -- substitute client-specified password<br>
  *  &quot;COMMONNAME&quot; -- substitute client certificate common name<br>
+ *  &quot;OTP&quot; -- substitute static challenge response if available<br>
  */<br>
<br>
 #define N_NAME_VALUE 16<br>
@@ -111,6 +114,7 @@ struct user_pass {<br>
     char username[128];<br>
     char password[128];<br>
     char common_name[128];<br>
+    char response[128];<br>
<br>
     const struct name_value_list *name_value_list;<br>
 };<br>
@@ -276,6 +280,66 @@ name_value_match(const char *query, const char *match)<br>
     return strncasecmp(match, query, strlen(match)) == 0;<br>
 }<br>
<br>
+/*<br>
+ * Split and decode up-&gt;password in the form SCRV1:base64_pass:base64_<wbr>response<br>
+ * into pass and response and save in up-&gt;password and up-&gt;response.<br>
+ * If the password is not in the expected format, input is not changed.<br>
+ */<br>
+static void<br>
+split_scrv1_password(struct user_pass *up)<br>
+{<br>
+    const int skip = strlen(&quot;SCRV1:&quot;);<br>
+    if (strncmp(up-&gt;password, &quot;SCRV1:&quot;, skip) != 0)<br>
+    {<br>
+        return;<br>
+    }<br>
+<br>
+    char *tmp = strdup(up-&gt;password);<br>
+    if (!tmp)<br>
+    {<br>
+        fprintf(stderr, &quot;AUTH-PAM: out of memory parsing static challenge password\n&quot;);<br>
+        goto out;<br>
+    }<br>
+<br>
+    char *pass = tmp + skip;<br>
+    char *resp = strchr(pass, &#39;:&#39;);<br>
+    if (!resp) /* string not in SCRV1:xx:yy format */<br>
+    {<br>
+        goto out;<br>
+    }<br>
+    *resp++ = &#39;\0&#39;;<br>
+<br>
+    int n = plugin_base64_decode(pass, up-&gt;password, sizeof(up-&gt;password)-1);<br>
+    if (n &gt; 0)<br>
+    {<br>
+        up-&gt;password[n] = &#39;\0&#39;;<br>
+        n = plugin_base64_decode(resp, up-&gt;response, sizeof(up-&gt;response)-1);<br>
+        if (n &gt; 0)<br>
+        {<br>
+            up-&gt;response[n] = &#39;\0&#39;;<br>
+            if (DEBUG(up-&gt;verb))<br>
+            {<br>
+                fprintf(stderr, &quot;AUTH-PAM: BACKGROUND: parsed static challenge password\n&quot;);<br>
+            }<br>
+            goto out;<br>
+        }<br>
+    }<br>
+<br>
+    /* decode error: reinstate original value of up-&gt;password and return */<br>
+    plugin_secure_memzero(up-&gt;<wbr>password, sizeof(up-&gt;password));<br>
+    plugin_secure_memzero(up-&gt;<wbr>response, sizeof(up-&gt;response));<br>
+    strcpy(up-&gt;password, tmp); /* tmp is guaranteed to fit in up-&gt;password */<br>
+<br>
+    fprintf(stderr, &quot;AUTH-PAM: base64 decode error while parsing static challenge password\n&quot;);<br>
+<br>
+out:<br>
+    if (tmp)<br>
+    {<br>
+        plugin_secure_memzero(tmp, strlen(tmp));<br>
+        free(tmp);<br>
+    }<br>
+}<br>
+<br>
 OPENVPN_EXPORT int<br>
 openvpn_plugin_open_v3(const int v3structver,<br>
                        struct openvpn_plugin_args_open_in const *args,<br>
@@ -316,6 +380,7 @@ openvpn_plugin_open_v3(const int v3structver,<br>
<br>
     /* Save global pointers to functions exported from openvpn */<br>
     plugin_secure_memzero = args-&gt;callbacks-&gt;plugin_<wbr>secure_memzero;<br>
+    plugin_base64_decode = args-&gt;callbacks-&gt;plugin_<wbr>base64_decode;<br>
<br>
     /*<br>
      * Make sure we have two string arguments: the first is the .so name,<br>
@@ -599,6 +664,10 @@ my_conv(int n, const struct pam_message **msg_array,<br>
                     {<br>
                         aresp[i].resp = searchandreplace(match_value, &quot;COMMONNAME&quot;, up-&gt;common_name);<br>
                     }<br>
+                    else if (strstr(match_value, &quot;OTP&quot;))<br>
+                    {<br>
+                        aresp[i].resp = searchandreplace(match_value, &quot;OTP&quot;, up-&gt;response);<br>
+                    }<br>
                     else<br>
                     {<br>
                         aresp[i].resp = strdup(match_value);<br>
@@ -787,6 +856,9 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *<br>
 #endif<br>
                 }<br>
<br>
+                /* If password is of the form SCRV1:base64:base64 split it up */<br>
+                split_scrv1_password(&amp;up);<br>
+<br>
                 if (pam_auth(service, &amp;up)) /* Succeeded */<br>
                 {<br>
                     if (send_control(fd, RESPONSE_VERIFY_SUCCEEDED) == -1)<br>
@@ -818,10 +890,11 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *<br>
                         command);<br>
                 goto done;<br>
         }<br>
+        plugin_secure_memzero(up.<wbr>response, sizeof(up.response));<br>
     }<br>
 done:<br>
-<br>
     plugin_secure_memzero(up.<wbr>password, sizeof(up.password));<br>
+    plugin_secure_memzero(up.<wbr>response, sizeof(up.response));<br>
 #ifdef USE_PAM_DLOPEN<br>
     dlclose_pam();<br>
 #endif<br>
<span class="HOEnZb"><font color="#888888">-- <br>
2.1.4<br>
<br>
<br>
------------------------------<wbr>------------------------------<wbr>------------------<br>
Check out the vibrant tech community on one of the world&#39;s most<br>
engaging tech sites, Slashdot.org! <a href="http://sdm.link/slashdot" rel="noreferrer" target="_blank">http://sdm.link/slashdot</a><br>
______________________________<wbr>_________________<br>
Openvpn-devel mailing list<br>
<a href="mailto:Openvpn-devel@lists.sourceforge.net">Openvpn-devel@lists.<wbr>sourceforge.net</a><br>
<a href="https://lists.sourceforge.net/lists/listinfo/openvpn-devel" rel="noreferrer" target="_blank">https://lists.sourceforge.net/<wbr>lists/listinfo/openvpn-devel</a><br>
</font></span></blockquote></div><br></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering July 25, 2018, 9:43 p.m. | #2
Hi,

On Wed, Jul 25, 2018 at 04:31:05PM -0500, Joe Bell wrote:
> I don't know if it is appropriate to reply to this post in this manner, 

It is and it helps :-)

We want test reports, as in "I am using this, because it is a useful
feature for me, and it works fine!" - not as strong as a full code
review, but an indication that something is beneficial at least.

[..]
> If there is anything I can do to provide a list of testcases, write-up on
> its configuration and usage, etc., in aide of getting the patch merged and
> released, please let me know.

If you could find a few spare weeks for me, that would help quite a bit :-)

Jokes aside, a bit of writeup how this looks in the client config, how
the client prompts will then look, and how you configure the server would
be nice - just here on the list, so google can find it.

Actually, this is something useful for $payingcustomer, so I hope to
find time to review and test it myself "soonish"...  unfortunately, 
$otherpayingcustomers are screaming more loudly for me to get other stuff
done first, with a hard deadline next Tuesday...

gert
Joe Bell July 25, 2018, 10:33 p.m. | #3
On Wed, Jul 25, 2018 at 4:43 PM, Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Wed, Jul 25, 2018 at 04:31:05PM -0500, Joe Bell wrote:
> > I don't know if it is appropriate to reply to this post in this manner,
>
> It is and it helps :-)
>
> We want test reports, as in "I am using this, because it is a useful
> feature for me, and it works fine!" - not as strong as a full code
> review, but an indication that something is beneficial at least.
>

Terrific.  Indeed, I am using this now, and quite successfully in a staging
environment.


>
> [..]
> > If there is anything I can do to provide a list of testcases, write-up on
> > its configuration and usage, etc., in aide of getting the patch merged
> and
> > released, please let me know.
>
> If you could find a few spare weeks for me, that would help quite a bit :-)
>
> Jokes aside, a bit of writeup how this looks in the client config, how
> the client prompts will then look, and how you configure the server would
> be nice - just here on the list, so google can find it.
>
>
I will do a full writeup, but the magic comes from

plugin /usr/lib/openvpn/plugins/openvpn-plugin-auth-pam.so "openvpn login:
USERNAME Password: PASSWORD Verification OTP"

in the OpenVPN server.conf, as well as:

auth required /lib/security/pam_google_authenticator.so
secret=/etc/openvpn/google-authenticator/${USER} user=gauth
auth required pam_ldap.so

for the contents of /etc/pam.d/openvpn. (Note that I'm using LDAP for
username/password, this should be replaced by the appropriate PAM module
for other types.)

Then, on the client configuration (.ovpn):

auth-user-pass
static-challenge "Enter Google Authenticator Token" 1

Now obviously there are a lot of details i've left out (setting up Google
Authenticator, etc.) but once I compiled OpenVPN with the base64 patch and
the plugin patch, it was smiles all around.  I will work to put together a
full write-up of zero to sixty with screenshots, etc.

I've been delighted to cobble this together with my previous knowledge of
OpenVPN configurations, finding Selva's patches, and pointers on how to
pull this all together, but without a doubt it would have been delightful
to have it out-of-the-box.  Thanks for the feedback and encouragement to
push for this.

Joe
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 25, 2018 at 4:43 PM, Gert Doering <span dir="ltr">&lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<span class="gmail-"><br>
On Wed, Jul 25, 2018 at 04:31:05PM -0500, Joe Bell wrote:<br>
&gt; I don&#39;t know if it is appropriate to reply to this post in this manner, <br>
<br>
</span>It is and it helps :-)<br>
<br>
We want test reports, as in &quot;I am using this, because it is a useful<br>
feature for me, and it works fine!&quot; - not as strong as a full code<br>
review, but an indication that something is beneficial at least.<br></blockquote><div><br></div><div>Terrific.  Indeed, I am using this now, and quite successfully in a staging environment.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[..]<br>
<span class="gmail-">&gt; If there is anything I can do to provide a list of testcases, write-up on<br>
&gt; its configuration and usage, etc., in aide of getting the patch merged and<br>
&gt; released, please let me know.<br>
<br>
</span>If you could find a few spare weeks for me, that would help quite a bit :-)<br>
<br>
Jokes aside, a bit of writeup how this looks in the client config, how<br>
the client prompts will then look, and how you configure the server would<br>
be nice - just here on the list, so google can find it.<br>
<br></blockquote><div><br></div><div>I will do a full writeup, but the magic comes from</div><div><br></div><div><font face="monospace, monospace">plugin /usr/lib/openvpn/plugins/openvpn-plugin-auth-pam.so &quot;openvpn login: USERNAME Password: PASSWORD Verification OTP&quot;</font></div><div><br></div><div>in the OpenVPN server.conf, as well as:</div><div><br></div><div><div><font face="monospace, monospace">auth required /lib/security/pam_google_authenticator.so secret=/etc/openvpn/google-authenticator/${USER} user=gauth</font></div><div><font face="monospace, monospace">auth required pam_ldap.so</font></div></div><div><br></div><div>for the contents of <span style="font-family:monospace,monospace;font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">/etc/pam.d/openvpn</span><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif">. (Note that I&#39;m using LDAP for username/password, this should be replaced by the appropriate PAM module for other types.)</font></span></div><div><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif"><br></font></span></div><div><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif">Then, on the client configuration (.ovpn):</font></span></div><div><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif"><br></font></span></div><div><span style="background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="monospace, monospace"><div style="">auth-user-pass</div><div style="">static-challenge &quot;Enter Google Authenticator Token&quot; 1</div></font></span></div><div><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif"><br></font></span></div><div><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif">Now obviously there are a lot of details i&#39;ve left out (setting up Google Authenticator, etc.) but once I compiled OpenVPN with the base64 patch and the plugin patch, it was smiles all around.  I will work to put together a full write-up of zero to sixty with screenshots, etc.</font></span></div><div><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif"><br></font></span></div><div><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif">I&#39;ve been delighted to cobble this together with my previous knowledge of OpenVPN configurations, finding Selva&#39;s patches, and pointers on how to pull this all together, but without a doubt it would have been delightful to have it out-of-the-box.  Thanks for the feedback and encouragement to push for this.</font></span></div><div><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif"><br></font></span></div><div><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif">Joe</font></span></div><div><span style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><font face="arial, helvetica, sans-serif"><br></font></span></div></div></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering July 29, 2018, 7:34 p.m. | #4
Hi,

On Tue, Jul 24, 2018 at 10:34:53PM -0400, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
> 
> If static challenge is in use, the password passed to the plugin by openvpn
> is of the form "SCRV1:base64-pass:base64-response". Parse this string to
> separate it into password and response and use them to respond to queries
> in the pam conversation function.
> 
> On the plugin parameters line the substitution keyword for the static
> challenge response is "OTP". For example, for pam config named "test" that
> prompts for "user", "password" and "pin", use
> 
> plugin openvpn-auth-pam.so "test user USERNAME password PASSWORD pin OTP"
> 
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> 
> ---
> v2: Depends on the base64 export patch
> v3: match password string with "SCRV1:" instead of "SCRV1"
> (pointed out by Joe Bell <joeaintexas@gmail.com>)

I'm a bit confused about the prerequisites for this - if I found the
right "base64 export patch", this is still pending some reworking
because you and David disagreed on details, and then David got sucked
into "Client for OpenVPN 3 on Linux" and had no time working on the
plugin API for v2.x anymore.

Or is there a different base64 export patch which is "just" pending
review and merge?

I want *this* (so, feature-ACK, haven't stared at the code yet or 
tested it) - so I want all the prerequisites too :-)

gert
Selva Nair July 29, 2018, 8:16 p.m. | #5
Hi,

On Sun, Jul 29, 2018 at 3:34 PM, Gert Doering <gert@greenie.muc.de> wrote:
> Hi,
>
> On Tue, Jul 24, 2018 at 10:34:53PM -0400, selva.nair@gmail.com wrote:
>> From: Selva Nair <selva.nair@gmail.com>
>>
>> If static challenge is in use, the password passed to the plugin by openvpn
>> is of the form "SCRV1:base64-pass:base64-response". Parse this string to
>> separate it into password and response and use them to respond to queries
>> in the pam conversation function.
>>
>> On the plugin parameters line the substitution keyword for the static
>> challenge response is "OTP". For example, for pam config named "test" that
>> prompts for "user", "password" and "pin", use
>>
>> plugin openvpn-auth-pam.so "test user USERNAME password PASSWORD pin OTP"
>>
>> Signed-off-by: Selva Nair <selva.nair@gmail.com>
>>
>> ---
>> v2: Depends on the base64 export patch
>> v3: match password string with "SCRV1:" instead of "SCRV1"
>> (pch ointed out by Joe Bell <joeaintexas@gmail.com>)
>
> I'm a bit confused about the prerequisites for this - if I found the
> right "base64 export patch", this is still pending some reworking
> because you and David disagreed on details, and then David got sucked
> into "Client for OpenVPN 3 on Linux" and had no time working on the
> plugin API for v2.x anymore.

Yes that's the base64 patch. What is stopping it is not the
disagreement on that patch but an "error" [*] in the plugin header
that I had discovered.  David wants to fix that before this one, but
it seems he is too busy with other things.

And there is a pending patch to fix that :
https://patchwork.openvpn.net/patch/87/

Actually we can get base64 export merged without "fixing" the API
(header) as the only place it is referred to in David's patch is in
the accompanying sample that shows how to use the exported function.

If David is okay with it I can volunteer to split his patch into two
and get the base64 export merged sooner as its generally useful.

Selva

[*] A function signature uses a pointer to an opaque handle (a void *)
while it should be just the handle. It generates no warning as it is
void * vs void ** and all existing codes out there must be correctly
passing the pointer (handle) ignoring the signature in the header --
else they wont work. I wanted the header to be fixed and David seems
to agree with that.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Antonio Quartulli July 30, 2018, 2:31 p.m. | #6
Hi,

On 30/07/18 04:16, Selva Nair wrote:
> Yes that's the base64 patch. What is stopping it is not the
> disagreement on that patch but an "error" [*] in the plugin header
> that I had discovered.  David wants to fix that before this one, but
> it seems he is too busy with other things.
> 
> And there is a pending patch to fix that :
> https://patchwork.openvpn.net/patch/87/
> 

[CUT]


> 
> [*] A function signature uses a pointer to an opaque handle (a void *)
> while it should be just the handle. It generates no warning as it is
> void * vs void ** and all existing codes out there must be correctly
> passing the pointer (handle) ignoring the signature in the header --
> else they wont work. I wanted the header to be fixed and David seems
> to agree with that.
> 

I remember discussing this patch with David as well and the general
feeling was that the patch was correct.

Maybe David wanted to spend some more time on this patch, but it slipped
off the plate.

As Selva said, if users of that function were following the header they
would see a lot of explosions, while this is not the case.
Therefore it should be happily applied with no risk.


Cheers,
Selva Nair July 30, 2018, 2:58 p.m. | #7
Hi,

On Mon, Jul 30, 2018 at 10:31 AM, Antonio Quartulli <a@unstable.cc> wrote:
> Hi,
>
> On 30/07/18 04:16, Selva Nair wrote:
>> Yes that's the base64 patch. What is stopping it is not the
>> disagreement on that patch but an "error" [*] in the plugin header
>> that I had discovered.  David wants to fix that before this one, but
>> it seems he is too busy with other things.
>>
>> And there is a pending patch to fix that :
>> https://patchwork.openvpn.net/patch/87/
>>
>
> [CUT]
>
>
>>
>> [*] A function signature uses a pointer to an opaque handle (a void *)
>> while it should be just the handle. It generates no warning as it is
>> void * vs void ** and all existing codes out there must be correctly
>> passing the pointer (handle) ignoring the signature in the header --
>> else they wont work. I wanted the header to be fixed and David seems
>> to agree with that.
>>
>
> I remember discussing this patch with David as well and the general
> feeling was that the patch was correct.
>
> Maybe David wanted to spend some more time on this patch, but it slipped
> off the plate.
>
> As Selva said, if users of that function were following the header they
> would see a lot of explosions, while this is not the case.
> Therefore it should be happily applied with no risk.

Antonio, thanks for recalling your discussion and for the reassurance.

My description of this in the previous mail was not entirely correct:
it has been a long time, and I was a bit rusty on the details -- the
pointer in question is a member of "struct openvpn_plugin_args_open_return"
where its wrongly declared.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
David Sommerseth July 31, 2018, 7:07 a.m. | #8
On 30/07/18 16:58, Selva Nair wrote:
> Hi,
> 
> On Mon, Jul 30, 2018 at 10:31 AM, Antonio Quartulli <a@unstable.cc> wrote:
>> Hi,
>>
>> On 30/07/18 04:16, Selva Nair wrote:
>>> Yes that's the base64 patch. What is stopping it is not the
>>> disagreement on that patch but an "error" [*] in the plugin header
>>> that I had discovered.  David wants to fix that before this one, but
>>> it seems he is too busy with other things.
>>>
>>> And there is a pending patch to fix that :
>>> https://patchwork.openvpn.net/patch/87/
>>>
>>
>> [CUT]
>>
>>
>>>
>>> [*] A function signature uses a pointer to an opaque handle (a void *)
>>> while it should be just the handle. It generates no warning as it is
>>> void * vs void ** and all existing codes out there must be correctly
>>> passing the pointer (handle) ignoring the signature in the header --
>>> else they wont work. I wanted the header to be fixed and David seems
>>> to agree with that.
>>>
>>
>> I remember discussing this patch with David as well and the general
>> feeling was that the patch was correct.
>>
>> Maybe David wanted to spend some more time on this patch, but it slipped
>> off the plate.
>>
>> As Selva said, if users of that function were following the header they
>> would see a lot of explosions, while this is not the case.
>> Therefore it should be happily applied with no risk.
> 
> Antonio, thanks for recalling your discussion and for the reassurance.
> 
> My description of this in the previous mail was not entirely correct:
> it has been a long time, and I was a bit rusty on the details -- the
> pointer in question is a member of "struct openvpn_plugin_args_open_return"
> where its wrongly declared.
> 


Hi all, and sorry for letting this one (with many others) fall through the
cracks.  And quickly responding from a holiday now (planning to be mostly
disconnected until mid-August; first real holiday in 2 years now).  I remember
patch was fine but never got around to fix the header file.

So if we get the header file fixed,  all should be good for getting the base64
stuff added; including this patch.  I have only glared at the code quickly of
this last v3 patch, so it just needs to be well tested before we add it to git
master.

Next cool thing to get added would be dynamic challenge, but that will require
quite some extensions in the plug-in API as well.  Use case: systems which
uses sssd to do the authentication via PAM, where OTP may or may not be
activated for some user accounts - or temporarily disabled if the backend sssd
depends on is unavailable (where sssd can do a local off-line password auth only).
Selva Nair July 31, 2018, 2:07 p.m. | #9
HI

On Tue, Jul 31, 2018 at 3:07 AM, David Sommerseth
<openvpn@sf.lists.topphemmelig.net> wrote:
> On 30/07/18 16:58, Selva Nair wrote:
>> Hi,
>>
>> On Mon, Jul 30, 2018 at 10:31 AM, Antonio Quartulli <a@unstable.cc> wrote:
>>> Hi,
>>>
>>> On 30/07/18 04:16, Selva Nair wrote:
>>>> Yes that's the base64 patch. What is stopping it is not the
>>>> disagreement on that patch but an "error" [*] in the plugin header
>>>> that I had discovered.  David wants to fix that before this one, but
>>>> it seems he is too busy with other things.
>>>>

snip

>>> I remember discussing this patch with David as well and the general
>>> feeling was that the patch was correct.
>>>
>>> Maybe David wanted to spend some more time on this patch, but it slipped
>>> off the plate.
>>>
>>> As Selva said, if users of that function were following the header they
>>> would see a lot of explosions, while this is not the case.
>>> Therefore it should be happily applied with no risk.
>>
>> Antonio, thanks for recalling your discussion and for the reassurance.
>>

snip

>
> Hi all, and sorry for letting this one (with many others) fall through the
> cracks.  And quickly responding from a holiday now (planning to be mostly
> disconnected until mid-August; first real holiday in 2 years now).  I remember
> patch was fine but never got around to fix the header file.

Agreed :)

The header patch is short and benign, and it seems more than a couple
of pairs of
eyes have viewed and "approved" it. So can we get an ACK?

Here is a link to the patch.
https://patchwork.openvpn.net/patch/87/

Once this is accepted, the rest should be straightforward.

> Next cool thing to get added would be dynamic challenge, but that will require
> quite some extensions in the plug-in API as well.

What is stopping this is really the same old problem with sending an AUTH_FAILED
reason back to the client -- once that is solved easy to support this
with minimal
changes to the plugin code.

I have some ideas on how to solve the former without too much
refactoring, but let's
get the static challenge support in first.

Thanks,

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Aug. 7, 2018, 6:59 p.m. | #10
Hi,

On Tue, Jul 24, 2018 at 10:34:53PM -0400, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
> 
> If static challenge is in use, the password passed to the plugin by openvpn
> is of the form "SCRV1:base64-pass:base64-response". Parse this string to
> separate it into password and response and use them to respond to queries
> in the pam conversation function.
> 
> On the plugin parameters line the substitution keyword for the static
> challenge response is "OTP". For example, for pam config named "test" that
> prompts for "user", "password" and "pin", use
> 
> plugin openvpn-auth-pam.so "test user USERNAME password PASSWORD pin OTP"
> 
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> 
> ---
> v2: Depends on the base64 export patch
> v3: match password string with "SCRV1:" instead of "SCRV1"
> (pointed out by Joe Bell <joeaintexas@gmail.com>)

Nicely works and does what it says on the lid.  So...

Acked-By: Gert Doering <gert@greenie.muc.de>


I've tried to make it work for our use case, but in combination with
pam_auth_linotp, it actually doesn't work - the latter wants 
pin+otp to be "one string" (to be parsed by the server), so there
is no separate "OTP" challenge.

I've made it work by hacking a "combine password+otp together" 
substitution

+                    else if (strstr(match_value, "PASS+OTP"))
+                    {
+                       char temp[100];
+                       snprintf( temp, sizeof(temp), "%s%s", up->password, up->response );
+                        aresp[i].resp = searchandreplace(match_value, "PASS+OTP", temp);
+                   }

... but that's not exactly pretty.  It works, though :-)


Plugin logging is also something we should improve.  The current approach
"just log to stderr" is failing if the main openvpn process is started
by init/rc and stderr is not available...  we have logging functions
in the v3 api nowadays, so maybe we should use them :-)

 *
 * plugin_log
 * plugin_vlog : Use these functions to add information to the OpenVPN log file.
 *               Messages will only be displayed if the plugin_name parameter
 *               is set. PLOG_DEBUG messages will only be displayed with plug-in
 *               debug log verbosity (at the time of writing that's verb >= 7).



There's another catch which we might want to at least document: if you
build this plugin and run it from a slightly older openvpn binary which 
doesn't export the base64 functions, it will core dump most ungraciously

Aug  7 20:17:19 openvpn-tcp kernel: pid 49459 (openvpn), uid 0: exited on signal 11 (core dumped)

(unsurprisingly, jumping over an uninitialized function pointer)


So, shall we bump the plugin API version "with base64" to 5, and change
this plugin to actually check it?

(Applying it anyway, because this can all be addressed in a subsequent
patch)

gert
Gert Doering Aug. 7, 2018, 7:01 p.m. | #11
Your patch has been applied to the master branch.

(I'm a bit undecided about release/2.4 - this is in "new feature!" land,
and all the challenge stuff is "master" territory.  OTOH, it's not openvpn
main code, and the code is sane enough - so if folks think it should be
in release/2.4, tell me)

commit 7369d01bf360bcfa02f26c05b86dde5496d120f6 (master)
Author: Selva Nair
Date:   Tue Jul 24 22:34:53 2018 -0400

     Parse static challenge response in auth-pam plugin

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <1532486093-24793-1-git-send-email-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17307.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Aug. 7, 2018, 7:07 p.m. | #12
Hi,

On Tue, Aug 07, 2018 at 08:59:37PM +0200, Gert Doering wrote:
> > v2: Depends on the base64 export patch
> > v3: match password string with "SCRV1:" instead of "SCRV1"
> > (pointed out by Joe Bell <joeaintexas@gmail.com>)
> 
> Nicely works and does what it says on the lid.  So...

Talking to myself a lot, lately...

Found an interesting caveat which should be addressed, I think.

Our system (LinOTP) knows "PIN+OTP" or "PIN" as valid input, the
latter leading to "send me a token by SMS/e-mail/...".

If I press return at the challenge prompt, it seems the SCRV1: string
is not formed the way the plugin wants it, and I end up with

  pass=SCRV1%3AMTE5NQ%3D%3D

in the LinOTP URL - so, it didn't decode it, because the second ':'
was missing (if I put a blank in there, I get pass=mypin%20).

Is this intentional?  Should it be that way?

gert
Selva Nair Aug. 7, 2018, 7:15 p.m. | #13
Hi,

On Tue, Aug 7, 2018 at 2:59 PM, Gert Doering <gert@greenie.muc.de> wrote:

...some good comments snipped...

>
> There's another catch which we might want to at least document: if you
> build this plugin and run it from a slightly older openvpn binary which
> doesn't export the base64 functions, it will core dump most ungraciously
>
> Aug  7 20:17:19 openvpn-tcp kernel: pid 49459 (openvpn), uid 0: exited on signal 11 (core dumped)
>
> (unsurprisingly, jumping over an uninitialized function pointer)
>
>
> So, shall we bump the plugin API version "with base64" to 5, and change
> this plugin to actually check it?
>
> (Applying it anyway, because this can all be addressed in a subsequent
> patch)

Oh, the version bump was overlooked... David's export patch was
obviously written
assuming it will get merged soon after the secure_memzero export patch
but we took
a year and some to get there :)

Let's bump the plugin struct version and add a check for it in this plugin.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Selva Nair Aug. 7, 2018, 7:38 p.m. | #14
Hi,

On Tue, Aug 7, 2018 at 3:07 PM, Gert Doering <gert@greenie.muc.de> wrote:
> Hi,
>
> On Tue, Aug 07, 2018 at 08:59:37PM +0200, Gert Doering wrote:
>> > v2: Depends on the base64 export patch
>> > v3: match password string with "SCRV1:" instead of "SCRV1"
>> > (pointed out by Joe Bell <joeaintexas@gmail.com>)
>>
>> Nicely works and does what it says on the lid.  So...
>
> Talking to myself a lot, lately...
>
> Found an interesting caveat which should be addressed, I think.
>
> Our system (LinOTP) knows "PIN+OTP" or "PIN" as valid input, the
> latter leading to "send me a token by SMS/e-mail/...".
>
> If I press return at the challenge prompt, it seems the SCRV1: string
> is not formed the way the plugin wants it, and I end up with
>
>   pass=SCRV1%3AMTE5NQ%3D%3D

How to format this if response is empty is not clearly documented
but my impression was that the second ':' (%3A) is required.

management-notes.txt specifies the format as

password "Auth" "SCRV1:<BASE64_PASSWORD>:<BASE64_RESPONSE>"

The Windows GUI prints it using a template "SCRV1:%s:%s", so the second
colon will be present if response is empty -- if that's not happening
I would consider
that a bug in the GUI.

When password is read from stdin, its formatted as (from misc.c line 358)

buf_printf(&packed_resp, "SCRV1:%s:%s", pw64, resp64);

So that should also contain the second colon.

>
> in the LinOTP URL - so, it didn't decode it, because the second ':'
> was missing (if I put a blank in there, I get pass=mypin%20).
>
> Is this intentional?  Should it be that way?

If you are constructing the SCRV1: line using a custom UI,
I would suggest to add the second colon. If using Windows-GUI or running
OpenVPN from command line we'll need to fix this one place
or the other.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Selva Nair Aug. 7, 2018, 8:33 p.m. | #15
Hi,

Correcting myself...

>> Found an interesting caveat which should be addressed, I think.
>>
>> Our system (LinOTP) knows "PIN+OTP" or "PIN" as valid input, the
>> latter leading to "send me a token by SMS/e-mail/...".
>>
>> If I press return at the challenge prompt, it seems the SCRV1: string
>> is not formed the way the plugin wants it, and I end up with
>>
>>   pass=SCRV1%3AMTE5NQ%3D%3D
>
> How to format this if response is empty is not clearly documented
> but my impression was that the second ':' (%3A) is required.
>
> management-notes.txt specifies the format as
>
> password "Auth" "SCRV1:<BASE64_PASSWORD>:<BASE64_RESPONSE>"
>
> The Windows GUI prints it using a template "SCRV1:%s:%s", so the second
> colon will be present if response is empty -- if that's not happening
> I would consider that a bug in the GUI.
>
> When password is read from stdin, its formatted as (from misc.c line 358)
>
> buf_printf(&packed_resp, "SCRV1:%s:%s", pw64, resp64);
>
> So that should also contain the second colon.

However. just realized that my plugin patch assumes a non-empty
password and response after base64 decode. Else it gives up on
unpacking and treats it as a regular password.

E.g., in auth-pamc.c (line 316)

   n = plugin_base64_decode(resp, up->response, sizeof(up->response)-1);
   if (n > 0)

I think we could and should accept empty strings for both -- let's fix it.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Aug. 7, 2018, 9:01 p.m. | #16
Hi,

On Tue, Aug 07, 2018 at 03:38:43PM -0400, Selva Nair wrote:
> > If I press return at the challenge prompt, it seems the SCRV1: string
> > is not formed the way the plugin wants it, and I end up with
> >
> >   pass=SCRV1%3AMTE5NQ%3D%3D
> 
> How to format this if response is empty is not clearly documented
> but my impression was that the second ':' (%3A) is required.
> 
> management-notes.txt specifies the format as
> 
> password "Auth" "SCRV1:<BASE64_PASSWORD>:<BASE64_RESPONSE>"

OK, so something is bugged, and it seems it's not the plugin.


> When password is read from stdin, its formatted as (from misc.c line 358)
> 
> buf_printf(&packed_resp, "SCRV1:%s:%s", pw64, resp64);
> 
> So that should also contain the second colon.

This is a bit surprising.  So "something" is eating it between
"openvpn command line client", "openvpn server" and "plugin-auth-pam".

Interesting.

> > in the LinOTP URL - so, it didn't decode it, because the second ':'
> > was missing (if I put a blank in there, I get pass=mypin%20).
> >
> > Is this intentional?  Should it be that way?
> 
> If you are constructing the SCRV1: line using a custom UI,
> I would suggest to add the second colon. If using Windows-GUI or running
> OpenVPN from command line we'll need to fix this one place
> or the other.

Command line client (git:master/5961250e776194a4, what I happened to 
have lying around), run with a config file that has

  auth-user-pass
  auth-nocache
  auth-retry interact
  static-challenge "token value: " 1

in it, and pressing <return> at the

CHALLENGE: token value: _

prompt.

gert
Selva Nair Aug. 8, 2018, 3:16 a.m. | #17
Hi

On Tue, Aug 7, 2018 at 5:01 PM, Gert Doering <gert@greenie.muc.de> wrote:
>
>> > in the LinOTP URL - so, it didn't decode it, because the second ':'
>> > was missing (if I put a blank in there, I get pass=mypin%20).
>> >
>> > Is this intentional?  Should it be that way?
>>
>> If you are constructing the SCRV1: line using a custom UI,
>> I would suggest to add the second colon. If using Windows-GUI or running
>> OpenVPN from command line we'll need to fix this one place
>> or the other.
>
> Command line client (git:master/5961250e776194a4, what I happened to
> have lying around), run with a config file that has
>
>   auth-user-pass
>   auth-nocache
>   auth-retry interact
>   static-challenge "token value: " 1
>
> in it, and pressing <return> at the
>
> CHALLENGE: token value: _
>
> prompt.

I cannot reproduce this but the plugin was not able to handle an empty
challenge response. The patch sent to the list should fix that.

With that just pressing <return> at the challenge prompt behaves as
expected (on unpack up->password gets the user input, up->response
stays empty). I haven't tested this using LinOTP, but should work.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/src/plugins/auth-pam/README.auth-pam b/src/plugins/auth-pam/README.auth-pam
index e123690..9081565 100644
--- a/src/plugins/auth-pam/README.auth-pam
+++ b/src/plugins/auth-pam/README.auth-pam
@@ -36,19 +36,20 @@  pairs to answer PAM module queries.
 
 For example:
 
-  plugin openvpn-auth-pam.so "login login USERNAME password PASSWORD"
+  plugin openvpn-auth-pam.so "login login USERNAME password PASSWORD pin OTP"
 
 tells auth-pam to (a) use the "login" PAM module, (b) answer a
-"login" query with the username given by the OpenVPN client, and
-(c) answer a "password" query with the password given by the
-OpenVPN client.  This provides flexibility in dealing with the different
+"login" query with the username given by the OpenVPN client,
+(c) answer a "password" query with the password, and (d) answer a
+"pin" query with the OTP given by the OpenVPN client.
+This provides flexibility in dealing with different
 types of query strings which different PAM modules might generate.
 For example, suppose you were using a PAM module called
 "test" which queried for "name" rather than "login":
 
   plugin openvpn-auth-pam.so "test name USERNAME password PASSWORD"
 
-While "USERNAME" "COMMONNAME" and "PASSWORD" are special strings which substitute
+While "USERNAME" "COMMONNAME" "PASSWORD" and "OTP" are special strings which substitute
 to client-supplied values, it is also possible to name literal values
 to use as PAM module query responses.  For example, suppose that the
 login module queried for a third parameter, "domain" which
@@ -61,6 +62,10 @@  the operation of this plugin:
 
   client-cert-not-required
   username-as-common-name
+  static-challenge
+
+Use of --static challenege is required to pass a pin (represented by "OTP" in
+parameter substituion) or a second password.
 
 Run OpenVPN with --verb 7 or higher to get debugging output from
 this plugin, including the list of queries presented by the
diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index 26b0eeb..e22ce5f 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -6,6 +6,7 @@ 
  *             packet compression.
  *
  *  Copyright (C) 2002-2018 OpenVPN Inc <sales@openvpn.net>
+ *  Copyright (C) 2016-2018 Selva Nair <selva.nair@gmail.com>
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2
@@ -64,6 +65,7 @@ 
 
 /* Pointers to functions exported from openvpn */
 static plugin_secure_memzero_t plugin_secure_memzero = NULL;
+static plugin_base64_decode_t plugin_base64_decode = NULL;
 
 /*
  * Plugin state, used by foreground
@@ -87,6 +89,7 @@  struct auth_pam_context
  *  "USERNAME" -- substitute client-supplied username
  *  "PASSWORD" -- substitute client-specified password
  *  "COMMONNAME" -- substitute client certificate common name
+ *  "OTP" -- substitute static challenge response if available
  */
 
 #define N_NAME_VALUE 16
@@ -111,6 +114,7 @@  struct user_pass {
     char username[128];
     char password[128];
     char common_name[128];
+    char response[128];
 
     const struct name_value_list *name_value_list;
 };
@@ -276,6 +280,66 @@  name_value_match(const char *query, const char *match)
     return strncasecmp(match, query, strlen(match)) == 0;
 }
 
+/*
+ * Split and decode up->password in the form SCRV1:base64_pass:base64_response
+ * into pass and response and save in up->password and up->response.
+ * If the password is not in the expected format, input is not changed.
+ */
+static void
+split_scrv1_password(struct user_pass *up)
+{
+    const int skip = strlen("SCRV1:");
+    if (strncmp(up->password, "SCRV1:", skip) != 0)
+    {
+        return;
+    }
+
+    char *tmp = strdup(up->password);
+    if (!tmp)
+    {
+        fprintf(stderr, "AUTH-PAM: out of memory parsing static challenge password\n");
+        goto out;
+    }
+
+    char *pass = tmp + skip;
+    char *resp = strchr(pass, ':');
+    if (!resp) /* string not in SCRV1:xx:yy format */
+    {
+        goto out;
+    }
+    *resp++ = '\0';
+
+    int n = plugin_base64_decode(pass, up->password, sizeof(up->password)-1);
+    if (n > 0)
+    {
+        up->password[n] = '\0';
+        n = plugin_base64_decode(resp, up->response, sizeof(up->response)-1);
+        if (n > 0)
+        {
+            up->response[n] = '\0';
+            if (DEBUG(up->verb))
+            {
+                fprintf(stderr, "AUTH-PAM: BACKGROUND: parsed static challenge password\n");
+            }
+            goto out;
+        }
+    }
+
+    /* decode error: reinstate original value of up->password and return */
+    plugin_secure_memzero(up->password, sizeof(up->password));
+    plugin_secure_memzero(up->response, sizeof(up->response));
+    strcpy(up->password, tmp); /* tmp is guaranteed to fit in up->password */
+
+    fprintf(stderr, "AUTH-PAM: base64 decode error while parsing static challenge password\n");
+
+out:
+    if (tmp)
+    {
+        plugin_secure_memzero(tmp, strlen(tmp));
+        free(tmp);
+    }
+}
+
 OPENVPN_EXPORT int
 openvpn_plugin_open_v3(const int v3structver,
                        struct openvpn_plugin_args_open_in const *args,
@@ -316,6 +380,7 @@  openvpn_plugin_open_v3(const int v3structver,
 
     /* Save global pointers to functions exported from openvpn */
     plugin_secure_memzero = args->callbacks->plugin_secure_memzero;
+    plugin_base64_decode = args->callbacks->plugin_base64_decode;
 
     /*
      * Make sure we have two string arguments: the first is the .so name,
@@ -599,6 +664,10 @@  my_conv(int n, const struct pam_message **msg_array,
                     {
                         aresp[i].resp = searchandreplace(match_value, "COMMONNAME", up->common_name);
                     }
+                    else if (strstr(match_value, "OTP"))
+                    {
+                        aresp[i].resp = searchandreplace(match_value, "OTP", up->response);
+                    }
                     else
                     {
                         aresp[i].resp = strdup(match_value);
@@ -787,6 +856,9 @@  pam_server(int fd, const char *service, int verb, const struct name_value_list *
 #endif
                 }
 
+                /* If password is of the form SCRV1:base64:base64 split it up */
+                split_scrv1_password(&up);
+
                 if (pam_auth(service, &up)) /* Succeeded */
                 {
                     if (send_control(fd, RESPONSE_VERIFY_SUCCEEDED) == -1)
@@ -818,10 +890,11 @@  pam_server(int fd, const char *service, int verb, const struct name_value_list *
                         command);
                 goto done;
         }
+        plugin_secure_memzero(up.response, sizeof(up.response));
     }
 done:
-
     plugin_secure_memzero(up.password, sizeof(up.password));
+    plugin_secure_memzero(up.response, sizeof(up.response));
 #ifdef USE_PAM_DLOPEN
     dlclose_pam();
 #endif