[Openvpn-devel] [PATCH 1/2] In auth-pam plugin clear the password after use

[Openvpn-devel] [PATCH 1/2] In auth-pam plugin clear the password after use

[selva.nair]

From: Selva Nair <selva.nair@...277...>

This adds a minimal secure_memzero()

Signed-off-by: Selva Nair <selva.nair@...277...>
---
 src/plugins/auth-pam/auth-pam.c |  2 ++
 src/plugins/auth-pam/utils.h    | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index d3e2c89..10622fd 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -799,8 +799,10 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *
                         command);
                 goto done;
         }
+        secure_memzero(up.password, sizeof(up.password));
     }
 done:
+    secure_memzero(up.password, sizeof(up.password));
 
 #ifdef USE_PAM_DLOPEN
     dlclose_pam();
diff --git a/src/plugins/auth-pam/utils.h b/src/plugins/auth-pam/utils.h
index fbc9705..c1fa3ee 100644
--- a/src/plugins/auth-pam/utils.h
+++ b/src/plugins/auth-pam/utils.h
@@ -63,4 +63,20 @@ get_env(const char *name, const char *envp[]);
 int
 string_array_len(const char *array[]);
 
+/**
+ * Securely zero memory without letting optimized away by the compiler
+ *
+ * @param data  Pointer to data to fill with zero
+ * @param len   Length of data, in bytes.
+ */
+inline void
+secure_memzero(void *data, size_t len)
+{
+    volatile char *p = (volatile char *) data;
+    while (len--)
+    {
+        *p++ = 0;
+    }
+}
+
 #endif
-- 
2.1.4

[Openvpn-devel] [PATCH 2/2] Parse static challenge response in auth-pam plugin

[selva.nair]

From: Selva Nair <selva.nair@...277...>

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@...277...>
---
 src/plugins/auth-pam/Makefile.am     |  3 ++
 src/plugins/auth-pam/README.auth-pam | 15 +++++---
 src/plugins/auth-pam/auth-pam.c      | 75 ++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 5 deletions(-)

diff --git a/src/plugins/auth-pam/Makefile.am b/src/plugins/auth-pam/Makefile.am
index e6dc27e..8563fd4 100644
--- a/src/plugins/auth-pam/Makefile.am
+++ b/src/plugins/auth-pam/Makefile.am
@@ -9,6 +9,8 @@ MAINTAINERCLEANFILES = \
 
 AM_CFLAGS = \
 	-I$(top_srcdir)/include \
+	-I$(top_srcdir)/src/openvpn \
+	-I$(top_srcdir)/src/compat \
 	$(PLUGIN_AUTH_PAM_CFLAGS) \
 	$(OPTIONAL_CRYPTO_CFLAGS)
 
@@ -21,6 +23,7 @@ openvpn_plugin_auth_pam_la_SOURCES = \
 	utils.c \
 	auth-pam.c \
 	pamdl.c  pamdl.h \
+	$(top_srcdir)/src/openvpn/base64.c $(top_srcdir)/src/openvpn/base64.h \
 	auth-pam.exports
 openvpn_plugin_auth_pam_la_LIBADD = \
 	$(PLUGIN_AUTH_PAM_LIBS)
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 10622fd..f037eef 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -48,6 +48,7 @@
 #include <signal.h>
 #include <syslog.h>
 #include "utils.h"
+#include <base64.h>
 
 #include <openvpn-plugin.h>
 
@@ -85,6 +86,7 @@ struct auth_pam_context
  *  "USERNAME" -- substitute client-supplied username
  *  "PASSWORD" -- substitute client-specified password
  *  "COMMONNAME" -- substitute client certificate common name
+ *  "OTP" -- substitute static challenege response if available
  */
 
 #define N_NAME_VALUE 16
@@ -109,6 +111,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;
 };
@@ -274,6 +277,69 @@ 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 = sizeof("SCRV1") - 1;
+    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 ret;
+    }
+
+    char *pass = tmp + skip;
+    char *resp = strchr(pass, ':');
+    if (!resp) /* string not in SCRV1:xx:yy format */
+    {
+        goto ret;
+    }
+    *resp++ = '\0';
+
+    int n = openvpn_base64_decode(pass, up->password, sizeof(up->password)-1);
+    if (n > 0)
+    {
+        up->password[n] = '\0';
+        n = openvpn_base64_decode(resp, up->response, sizeof(up->response)-1);
+        if (n > 0)
+        {
+            up->response[n] = '\0';
+        }
+    }
+    else
+    {
+        /* decode error: reinstate original value of up->password and return */
+        secure_memzero(up->password, sizeof(up->password));
+        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");
+        goto ret;
+    }
+
+    if (DEBUG(up->verb))
+    {
+        fprintf(stderr, "AUTH-PAM: BACKGROUND: parsed static challenge password\n");
+    }
+
+ret:
+    if (tmp)
+    {
+        secure_memzero(tmp, strlen(tmp));
+        free(tmp);
+    }
+}
+
 OPENVPN_EXPORT openvpn_plugin_handle_t
 openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *envp[])
 {
@@ -581,6 +647,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);
@@ -769,6 +839,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)
@@ -800,9 +873,11 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *
                 goto done;
         }
         secure_memzero(up.password, sizeof(up.password));
+        secure_memzero(up.response, sizeof(up.response));
     }
 done:
     secure_memzero(up.password, sizeof(up.password));
+    secure_memzero(up.response, sizeof(up.response));
 
 #ifdef USE_PAM_DLOPEN
     dlclose_pam();
-- 
2.1.4

Re: [Openvpn-devel] [PATCH 2/2] Parse static challenge response in auth-pam plugin

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

Hi,

wading through my heap of mails that did not get proper attention...

On Fri, May 05, 2017 at 02:24:02PM -0400, selva.nair@...277... wrote:
> From: Selva Nair <selva.nair@...277...>
> 
> 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"

What is the status of this one?  Does it need updating after 1/2 got
changed to v2 and v3 are these independent enough that 2/2 is standalone?

Re: [Openvpn-devel] [PATCH 2/2] Parse static challenge response in auth-pam plugin

[David Sommerseth]

[-- Attachment #1.1: Type: text/plain, Size: 1016 bytes --]

On 12/06/17 20:14, Gert Doering wrote:
> 
> What is the status of this one?  Does it need updating after 1/2 got
> changed to v2 and v3 are these independent enough that 2/2 is standalone?
> 
> From a cursory glance, it calls secure_memzero() which is now, I think,
> plugin_secure_memzero() - right?

Yes, that seems to be correct to me.  A v2 is needed .... but ...

> I also seem to remember discussions between you and David regarding
> base64 function exporting - what's the state on this?

... I have managed to inadvertently managed to drop this ball.  Yes, we
should export the base64 functions.  I had a patch out, which Selva
reviewed and he discovered a few issues with the plug-in v3 API.  And I
remember Selva sent some patches fixing this too - but I've lost track
on those patches too.

Regardless, we should get the API fixes and the base64 export in place,
then a v2 of this patch makes far more sense.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Openvpn-devel] [PATCH 2/2] Parse static challenge response in auth-pam plugin

[Selva Nair]

[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]

On Mon, Jun 12, 2017 at 2:14 PM, Gert Doering <gert@...1296...> wrote:

> Hi,
>
> wading through my heap of mails that did not get proper attention...
>
> On Fri, May 05, 2017 at 02:24:02PM -0400, selva.nair@...277... wrote:
> > From: Selva Nair <selva.nair@...277...>
> >
> > 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"
>
> What is the status of this one?  Does it need updating after 1/2 got
> changed to v2 and v3 are these independent enough that 2/2 is standalone?
>
> From a cursory glance, it calls secure_memzero() which is now, I think,
> plugin_secure_memzero() - right?
>
>
> I also seem to remember discussions between you and David regarding
> base64 function exporting - what's the state on this?
>


I have a version 2 that uses exported base64 but waiting on the following
patch to get an NAK or ACK to finalize it.

   https://www.mail-archive.com/openvpn-devel@...1357...
rge.net/msg14655.html

That would also help David's base64 export patch revised, reviewed and
merged. For background, see the thread

https://www.mail-archive.com/openvpn-devel@...1199...
sourceforge.net/msg14577.html

Selva

[-- Attachment #2: Type: text/html, Size: 2767 bytes --]

Re: [Openvpn-devel] [PATCH 2/2] Parse static challenge response in auth-pam plugin

[Selva Nair]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On Mon, Jun 12, 2017 at 2:28 PM, Selva Nair <selva.nair@...277...> wrote:

> On Mon, Jun 12, 2017 at 2:14 PM, Gert Doering <gert@...1296...> wrote:
>
>> Hi,
>>
>> wading through my heap of mails that did not get proper attention...
>>
>> On Fri, May 05, 2017 at 02:24:02PM -0400, selva.nair@...277... wrote:
>> > From: Selva Nair <selva.nair@...277...>
>> >
>> > 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"
>>
>> What is the status of this one?  Does it need updating after 1/2 got
>> changed to v2 and v3 are these independent enough that 2/2 is standalone?
>>
>> From a cursory glance, it calls secure_memzero() which is now, I think,
>> plugin_secure_memzero() - right?
>>
>>
>> I also seem to remember discussions between you and David regarding
>> base64 function exporting - what's the state on this?
>>
>
>
> I have a version 2 that uses exported base64 but waiting on the following
> patch to get an NAK or ACK to finalize it.
>
>    https://www.mail-archive.com/openvpn-devel@...1357...
> rge.net/msg14655.html
>
> That would also help David's base64 export patch revised, reviewed and
> merged. For background, see the thread
>
> https://www.mail-archive.com/openvpn-devel@...282...
> .net/msg14577.html
>

bump :)

[-- Attachment #2: Type: text/html, Size: 3287 bytes --]

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

[selva.nair]

From: Selva Nair <selva.nair@...277...>

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@...277...>

v2: Depends on the base64 export patch

v3: BUMP++
---
 src/plugins/auth-pam/README.auth-pam | 15 +++++---
 src/plugins/auth-pam/auth-pam.c      | 74 +++++++++++++++++++++++++++++++++++-
 2 files changed, 83 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 ae514d7..2d25af3 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -64,6 +64,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 +88,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 +113,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 +279,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 = sizeof("SCRV1") - 1;
+    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 +379,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 +663,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 +855,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 +889,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

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

[selva.nair]

From: Selva Nair <selva.nair@...277...>

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@...277...>

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

 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@...515...>
+ *  Copyright (C) 2016-2018 Selva Nair <selva.nair@...277...>
  *
  *  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

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Joe Bell]

[-- Attachment #1: Type: text/plain, Size: 9434 bytes --]

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@...277...> wrote:

> From: Selva Nair <selva.nair@...277...>
>
> 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@...277...>
>
> ---
> v2: Depends on the base64 export patch
> v3: match password string with "SCRV1:" instead of "SCRV1"
> (pointed out by Joe Bell <joeaintexas@...277...>)
>
>  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@...515...>
> + *  Copyright (C) 2016-2018 Selva Nair <selva.nair@...277...>
>   *
>   *  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
>

[-- Attachment #2: Type: text/html, Size: 12122 bytes --]

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1505 bytes --]

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
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Joe Bell]

[-- Attachment #1: Type: text/plain, Size: 2267 bytes --]

On Wed, Jul 25, 2018 at 4:43 PM, Gert Doering <gert@...1296...> 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

[-- Attachment #2: Type: text/html, Size: 6003 bytes --]

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]

Hi,

On Tue, Jul 24, 2018 at 10:34:53PM -0400, selva.nair@...277... wrote:
> From: Selva Nair <selva.nair@...277...>
> 
> 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@...277...>
> 
> ---
> v2: Depends on the base64 export patch
> v3: match password string with "SCRV1:" instead of "SCRV1"
> (pointed out by Joe Bell <joeaintexas@...277...>)

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
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Selva Nair]

Hi,

On Sun, Jul 29, 2018 at 3:34 PM, Gert Doering <gert@...1296...> wrote:
> Hi,
>
> On Tue, Jul 24, 2018 at 10:34:53PM -0400, selva.nair@...277... wrote:
>> From: Selva Nair <selva.nair@...277...>
>>
>> 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@...277...>
>>
>> ---
>> v2: Depends on the base64 export patch
>> v3: match password string with "SCRV1:" instead of "SCRV1"
>> (pch ointed out by Joe Bell <joeaintexas@...277...>)
>
> 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.

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Antonio Quartulli]

[-- Attachment #1.1: Type: text/plain, Size: 1233 bytes --]

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,

-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Selva Nair]

Hi,

On Mon, Jul 30, 2018 at 10:31 AM, Antonio Quartulli <a@...2181...> 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

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[David Sommerseth]

On 30/07/18 16:58, Selva Nair wrote:
> Hi,
> 
> On Mon, Jul 30, 2018 at 10:31 AM, Antonio Quartulli <a@...2181...> 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).


-- 
kind regards,

David Sommerseth
OpenVPN Inc

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Selva Nair]

HI

On Tue, Jul 31, 2018 at 3:07 AM, David Sommerseth
<openvpn@...2080...> wrote:
> On 30/07/18 16:58, Selva Nair wrote:
>> Hi,
>>
>> On Mon, Jul 30, 2018 at 10:31 AM, Antonio Quartulli <a@...2181...> 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

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 3317 bytes --]

Hi,

On Tue, Jul 24, 2018 at 10:34:53PM -0400, selva.nair@...277... wrote:
> From: Selva Nair <selva.nair@...277...>
> 
> 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@...277...>
> 
> ---
> v2: Depends on the base64 export patch
> v3: match password string with "SCRV1:" instead of "SCRV1"
> (pointed out by Joe Bell <joeaintexas@...277...>)

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

Acked-By: Gert Doering <gert@...1296...>


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

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

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@...277...>)
> 
> 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
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Selva Nair]

Hi,

On Tue, Aug 7, 2018 at 3:07 PM, Gert Doering <gert@...1296...> 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@...277...>)
>>
>> 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

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Selva Nair]

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

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]

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
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Selva Nair]

Hi

On Tue, Aug 7, 2018 at 5:01 PM, Gert Doering <gert@...1296...> 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

Re: [Openvpn-devel] [PATCH v3] Parse static challenge response in auth-pam plugin

[Selva Nair]

Hi,

On Tue, Aug 7, 2018 at 2:59 PM, Gert Doering <gert@...1296...> 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

[Openvpn-devel] [PATCH applied] Re: Parse static challenge response in auth-pam plugin

[Gert Doering]

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@...277...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <1532486093-24793-1-git-send-email-selva.nair@...277...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17307.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH applied] Re: Parse static challenge response in auth-pam plugin

[Selva Nair]

Hi,

On Tue, Aug 7, 2018 at 3:01 PM Gert Doering <gert@...1296...> wrote:
>
> 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)

I failed to respond to this that time.

Although this is a new feature, I too think its safe and useful to
include this in 2.4.
If we do, we'll need this one (commit
7369d01bf360bcfa02f26c05b86dde5496d120f6) and the followup one
7a8109023f4c345fe12f23421c5fa7e88e1ea85b

Both should cherry-pick without conflicts.

See also Trac #1275 https://community.openvpn.net/openvpn/ticket/1275

Thanks,

Selva

Re: [Openvpn-devel] [PATCH applied] Re: Parse static challenge response in auth-pam plugin

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

Hi,

On Thu, Apr 23, 2020 at 10:02:10AM -0400, Selva Nair wrote:
> On Tue, Aug 7, 2018 at 3:01 PM Gert Doering <gert@...1296...> wrote:
> > 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)
> 
> I failed to respond to this that time.
> 
> Although this is a new feature, I too think its safe and useful to
> include this in 2.4.
> If we do, we'll need this one (commit
> 7369d01bf360bcfa02f26c05b86dde5496d120f6) and the followup one
> 7a8109023f4c345fe12f23421c5fa7e88e1ea85b
> 
> Both should cherry-pick without conflicts.

If you tell me we should have it, I'm with you :-)

(Given that this is only affecting the auth-pam plugin, and that this is
a very useful addition, I think it makes sense)


commit cab48ad43eaba51c54fa23e55b0b2eb436dd921f (HEAD -> release/2.4)
Author: Selva Nair <selva.nair@...277...>
Date:   Tue Aug 7 22:44:31 2018 -0400

    Accept empty password and/or response in auth-pam plugin
    (cherry picked from commit 7a8109023f4c345fe12f23421c5fa7e88e1ea85b)

commit b89e48b015e581a4a0f5c306e2ab20da34c862ea
Author: Selva Nair <selva.nair@...277...>
Date:   Tue Jul 24 22:34:53 2018 -0400

    Parse static challenge response in auth-pam plugin
    (cherry picked from commit 7369d01bf360bcfa02f26c05b86dde5496d120f6)


I've only done very limited testing - quick look at the changes (nothing
conflicting or "funky" here), and a test build.

> See also Trac #1275 https://community.openvpn.net/openvpn/ticket/1275

Noted!

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH 1/2] In auth-pam plugin clear the password after use

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

Hi,

On Fri, May 05, 2017 at 02:24:01PM -0400, selva.nair@...277... wrote:
> From: Selva Nair <selva.nair@...277...>
> 
> This adds a minimal secure_memzero()
> 
> Signed-off-by: Selva Nair <selva.nair@...277...>

Feature-ACK, Code-NAK, but just because David is planning to export
secure_memzero() to plugins from OpenVPN proper - mentioned just today
on IRC

17:09 <@dazo> syzzer: just thinking aloud (remembered our discussion on wiping 
              passwords securely in plug-ins) ... what do you think about 
              exposing secure_memzero() to plug-ins, similar to what we do with 
              plugin_{log,vlog}()?
17:09 <@dazo> to avoid each plug-in needing to re-implement this
17:09 <@syzzer> yeah, sounds useful
17:10 <@dazo> I'll send a patch doing that too (should be quick to solve)


So I'd postpone this until David's patch plus instructions for plugin
authors show up.

Good timing :-)

gert

-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             gert@...1296...
fax: +49-89-35655025                        gert@...1297...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH 1/2] In auth-pam plugin clear the password after use

[David Sommerseth]

[-- Attachment #1.1: Type: text/plain, Size: 1993 bytes --]

On 05/05/17 20:28, Gert Doering wrote:
> Hi,
> 
> On Fri, May 05, 2017 at 02:24:01PM -0400, selva.nair@...277... wrote:
>> From: Selva Nair <selva.nair@...277...>
>>
>> This adds a minimal secure_memzero()
>>
>> Signed-off-by: Selva Nair <selva.nair@...277...>
> 
> Feature-ACK, Code-NAK, but just because David is planning to export
> secure_memzero() to plugins from OpenVPN proper - mentioned just today
> on IRC
> 
> 17:09 <@dazo> syzzer: just thinking aloud (remembered our discussion on wiping 
>               passwords securely in plug-ins) ... what do you think about 
>               exposing secure_memzero() to plug-ins, similar to what we do with 
>               plugin_{log,vlog}()?
> 17:09 <@dazo> to avoid each plug-in needing to re-implement this
> 17:09 <@syzzer> yeah, sounds useful
> 17:10 <@dazo> I'll send a patch doing that too (should be quick to solve)
> 
> 
> So I'd postpone this until David's patch plus instructions for plugin
> authors show up.
> 
> Good timing :-)

Indeed :)

So the patch exporting secure_memzero() is on the way to the mailing
list now.

To use this, you need to switch the openvpn_plugin_open_v1() to
openvpn_plugin_open_v3().  The API on this function is quite different,
but you shouldn't need to tweak too much.  All the pointers are
available in the new struct pointers the _v3 function uses.

Then you basically can declare a global variable like this:

  plugin_secure_memzero_t ovpn_secure_memzero = NULL;

And in the the openvpn_plugin_open_v3() function, you should add an API
check:

    if (v3structver < 4)
    {
         /* printf() some noise to the log */
         return OPENVPN_PLUGIN_FUNC_ERROR;
    }

This ensures that the OpenVPN version is new enough to export the
secure_memzero() function.

Then you just assign the ovpn_secure_memzero pointer:

     /* Hook into exported plug-in functions */
     ovpn_secure_memzero = args->callbacks->plugin_secure_memzero;

Re: [Openvpn-devel] [PATCH 1/2] In auth-pam plugin clear the password after use

[Selva Nair]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

On Fri, May 5, 2017 at 3:01 PM, David Sommerseth <
openvpn@...2080...> wrote:

> On 05/05/17 20:28, Gert Doering wrote:
> > Hi,
> >
> > On Fri, May 05, 2017 at 02:24:01PM -0400, selva.nair@...277... wrote:
> >> From: Selva Nair <selva.nair@...277...>
> >>
> >> This adds a minimal secure_memzero()
> >>
> >> Signed-off-by: Selva Nair <selva.nair@...277...>
> >
> > Feature-ACK, Code-NAK, but just because David is planning to export
> > secure_memzero() to plugins from OpenVPN proper - mentioned just today
> > on IRC
> >
> > 17:09 <@dazo> syzzer: just thinking aloud (remembered our discussion on
> wiping
> >               passwords securely in plug-ins) ... what do you think about
> >               exposing secure_memzero() to plug-ins, similar to what we
> do with
> >               plugin_{log,vlog}()?
> > 17:09 <@dazo> to avoid each plug-in needing to re-implement this
> > 17:09 <@syzzer> yeah, sounds useful
> > 17:10 <@dazo> I'll send a patch doing that too (should be quick to solve)
> >
> >
> > So I'd postpone this until David's patch plus instructions for plugin
> > authors show up.
> >
> > Good timing :-)
>
> Indeed :)
>

Good to know.. This happened to be a dependency for the second patch I sent
(parsing static challenge in auth-pam).


>
> So the patch exporting secure_memzero() is on the way to the mailing
> list now.
>
> To use this, you need to switch the openvpn_plugin_open_v1() to
> openvpn_plugin_open_v3().  The API on this function is quite different,
> but you shouldn't need to tweak too much.  All the pointers are
> available in the new struct pointers the _v3 function uses.
>
> Then you basically can declare a global variable like this:
>
>   plugin_secure_memzero_t ovpn_secure_memzero = NULL;


OK, I'll change this to use v3.

For the static-challenge patch I call openvpn_base64_decode() by compiling
base64.c into the plugin. Instead, can we also get base64_encode/decode
available as callbacks?

Selva

[-- Attachment #2: Type: text/html, Size: 3671 bytes --]

[Openvpn-devel] [PATCH 1/2 v2] In auth-pam plugin clear the password after use

[selva.nair]

From: Selva Nair <selva.nair@...277...>

v2: Change the plugin open to use v3 API so that
  openvpn_secure_memzero() exported from OpenVPN can be used.

Note: context is cast as (openvpn_plugin_handle_t *) for consistency
with the current plugin header. If/when the header is fixed, change
this cast as well.

Signed-off-by: Selva Nair <selva.nair@...277...>
---
 src/plugins/auth-pam/auth-pam.c       | 31 ++++++++++++++++++++++++++-----
 src/plugins/auth-pam/auth-pam.exports |  2 +-
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index d3e2c89..f3566c7 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -63,6 +63,9 @@
 #define RESPONSE_VERIFY_SUCCEEDED 12
 #define RESPONSE_VERIFY_FAILED    13
 
+/* Pointers to functions exported from openvpn */
+static plugin_secure_memzero_t plugin_secure_memzero = NULL;
+
 /*
  * Plugin state, used by foreground
  */
@@ -274,8 +277,10 @@ name_value_match(const char *query, const char *match)
     return strncasecmp(match, query, strlen(match)) == 0;
 }
 
-OPENVPN_EXPORT openvpn_plugin_handle_t
-openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *envp[])
+OPENVPN_EXPORT int
+openvpn_plugin_open_v3(const int v3structver,
+                       struct openvpn_plugin_args_open_in const *args,
+                       struct openvpn_plugin_args_open_return *ret)
 {
     pid_t pid;
     int fd[2];
@@ -285,6 +290,16 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *
 
     const int base_parms = 2;
 
+    const char **argv = args->argv;
+    const char **envp = args->envp;
+
+    /* Check API compatibility */
+    if (v3structver != OPENVPN_PLUGINv3_STRUCTVER)
+    {
+        fprintf(stderr, "AUTH-PAM: This plugin is incompatible with the running version of OpenVPN\n");
+        return OPENVPN_PLUGIN_FUNC_ERROR;
+    }
+
     /*
      * Allocate our context
      */
@@ -298,7 +313,10 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *
     /*
      * Intercept the --auth-user-pass-verify callback.
      */
-    *type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY);
+    ret->type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY);
+
+    /* Save global pointers to functions exported from openvpn */
+    plugin_secure_memzero = args->callbacks->plugin_secure_memzero;
 
     /*
      * Make sure we have two string arguments: the first is the .so name,
@@ -386,7 +404,8 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *
         if (status == RESPONSE_INIT_SUCCEEDED)
         {
             context->foreground_fd = fd[0];
-            return (openvpn_plugin_handle_t) context;
+            ret->handle = (openvpn_plugin_handle_t *) context;
+            return OPENVPN_PLUGIN_FUNC_SUCCESS;
         }
     }
     else
@@ -420,7 +439,7 @@ error:
     {
         free(context);
     }
-    return NULL;
+    return OPENVPN_PLUGIN_FUNC_ERROR;
 }
 
 OPENVPN_EXPORT int
@@ -785,6 +804,7 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *
                         goto done;
                     }
                 }
+                plugin_secure_memzero(up.password, sizeof(up.password));
                 break;
 
             case COMMAND_EXIT:
@@ -802,6 +822,7 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *
     }
 done:
 
+    plugin_secure_memzero(up.password, sizeof(up.password));
 #ifdef USE_PAM_DLOPEN
     dlclose_pam();
 #endif
diff --git a/src/plugins/auth-pam/auth-pam.exports b/src/plugins/auth-pam/auth-pam.exports
index b07937c..597e33f 100644
--- a/src/plugins/auth-pam/auth-pam.exports
+++ b/src/plugins/auth-pam/auth-pam.exports
@@ -1,4 +1,4 @@
-openvpn_plugin_open_v1
+openvpn_plugin_open_v3
 openvpn_plugin_func_v1
 openvpn_plugin_close_v1
 openvpn_plugin_abort_v1
-- 
2.1.4

Re: [Openvpn-devel] [PATCH 1/2 v2] In auth-pam plugin clear the password after use

[David Sommerseth]

[-- Attachment #1.1: Type: text/plain, Size: 3847 bytes --]

On 09/05/17 16:52, selva.nair@...277... wrote:
> From: Selva Nair <selva.nair@...277...>
> 
> v2: Change the plugin open to use v3 API so that
>   openvpn_secure_memzero() exported from OpenVPN can be used.
> 
> Note: context is cast as (openvpn_plugin_handle_t *) for consistency
> with the current plugin header. If/when the header is fixed, change
> this cast as well.
> 
> Signed-off-by: Selva Nair <selva.nair@...277...>
> ---
>  src/plugins/auth-pam/auth-pam.c       | 31 ++++++++++++++++++++++++++-----
>  src/plugins/auth-pam/auth-pam.exports |  2 +-
>  2 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
> index d3e2c89..f3566c7 100644
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -63,6 +63,9 @@
>  #define RESPONSE_VERIFY_SUCCEEDED 12
>  #define RESPONSE_VERIFY_FAILED    13
>  
> +/* Pointers to functions exported from openvpn */
> +static plugin_secure_memzero_t plugin_secure_memzero = NULL;
> +
>  /*
>   * Plugin state, used by foreground
>   */
> @@ -274,8 +277,10 @@ name_value_match(const char *query, const char *match)
>      return strncasecmp(match, query, strlen(match)) == 0;
>  }
>  
> -OPENVPN_EXPORT openvpn_plugin_handle_t
> -openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *envp[])
> +OPENVPN_EXPORT int
> +openvpn_plugin_open_v3(const int v3structver,
> +                       struct openvpn_plugin_args_open_in const *args,
> +                       struct openvpn_plugin_args_open_return *ret)
>  {
>      pid_t pid;
>      int fd[2];
> @@ -285,6 +290,16 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *
>  
>      const int base_parms = 2;
>  
> +    const char **argv = args->argv;
> +    const char **envp = args->envp;
> +
> +    /* Check API compatibility */
> +    if (v3structver != OPENVPN_PLUGINv3_STRUCTVER)

This generally looks good.  Going to run some tests.  But I think the
if() line above needs a reconsideration.

If the plug-in built and packaged separately and that build is not tied
to OpenVPN itself, this can make this plug-in fail without any
particular real reason if the OpenVPN binary gets updated independently.

Even though not explicitly documented, we should never remove any
information from the plug-in v3 structs, only append to and only append
at the end of the existing structs.  This should help ensure backwards
compatibility.  So if a plug-in is built against
OPENVPN_PLUGINv3_STRUCTVER being 3, that plug-in binary should work
out-of-the-box even if OpenVPN is updating to OPENVPN_PLUGINv3_STRUCTVER
set to 4.

The sample plug-ins do have a strict requirement (v3structver !=
OPENVPN_PLUGINv3_STRUCTVER) but that is more meant to be a kind of
sanity check for us when testing the plug-ins during OpenVPN
development.  That tells is instantly if we've remembered to rebuild the
sample plug-in.

The plug-ins in src/plugins should use, which ensures forward
compatibility against the OpenVPN binary:

   if (v3structver < OPENVPN_PLUGINv3_STRUCTVER)

So if OpenVPN gets upgraded the plug-in will still work, new features
exists but the plug-in isn't written to make use of them.  If OpenVPN
gets downgraded with a lesser plug-in API, it will complain as features
are missing.

In fact, if a plug-in only needs the feature-set of
OPENVPN_PLUGINv3_STRUCTVER 2 or 1, it is possible replace the macro
constant with the appropriate integer value.  That can ensure the
plug-in works regardless of which version of the openvpn-plugin.h it is
built against.

(And yes, I do realise we need more proper plug-in documentation)


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc





[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Openvpn-devel] [PATCH 1/2 v2] In auth-pam plugin clear the password after use

[Selva Nair]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

Hi,

On Tue, May 9, 2017 at 1:34 PM, David Sommerseth <openvpn@...2142...
topphemmelig.net> wrote:

> If the plug-in built and packaged separately and that build is not tied
> to OpenVPN itself, this can make this plug-in fail without any
> particular real reason if the OpenVPN binary gets updated independently.
>
> Even though not explicitly documented, we should never remove any
> information from the plug-in v3 structs, only append to and only append
> at the end of the existing structs.
>

I just assumed these "official" plugins will always be built and
distributed with openvpn core.
But if future revisions is guaranteed not to reorder/remove items in the
structs, a less strict check
is definitely better.


> The plug-ins in src/plugins should use, which ensures forward
> compatibility against the OpenVPN binary:
>
>    if (v3structver < OPENVPN_PLUGINv3_STRUCTVER)
>

Yes, will change to this.

Selva

[-- Attachment #2: Type: text/html, Size: 1835 bytes --]

[Openvpn-devel] [PATCH 1/2 v3] In auth-pam plugin clear the password after use

[selva.nair]

From: Selva Nair <selva.nair@...277...>

v2: Change the plugin open to use v3 API so that secure_memzero()
    exported from OpenVPN can be used.
v3: Relaxe API compatibility check: struct version 4 or higher
    will have secure_memzero exported.

Note: context is cast as (openvpn_plugin_handle_t *) for consistency
with the current plugin header. If/when the header is fixed, change
this cast as well.

Signed-off-by: Selva Nair <selva.nair@...277...>
---
 src/plugins/auth-pam/auth-pam.c       | 31 ++++++++++++++++++++++++++-----
 src/plugins/auth-pam/auth-pam.exports |  2 +-
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index d3e2c89..54471a3 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -63,6 +63,9 @@
 #define RESPONSE_VERIFY_SUCCEEDED 12
 #define RESPONSE_VERIFY_FAILED    13
 
+/* Pointers to functions exported from openvpn */
+static plugin_secure_memzero_t plugin_secure_memzero = NULL;
+
 /*
  * Plugin state, used by foreground
  */
@@ -274,8 +277,10 @@ name_value_match(const char *query, const char *match)
     return strncasecmp(match, query, strlen(match)) == 0;
 }
 
-OPENVPN_EXPORT openvpn_plugin_handle_t
-openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *envp[])
+OPENVPN_EXPORT int
+openvpn_plugin_open_v3(const int v3structver,
+                       struct openvpn_plugin_args_open_in const *args,
+                       struct openvpn_plugin_args_open_return *ret)
 {
     pid_t pid;
     int fd[2];
@@ -285,6 +290,16 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *
 
     const int base_parms = 2;
 
+    const char **argv = args->argv;
+    const char **envp = args->envp;
+
+    /* Check API compatibility -- struct version 4 or higher needed */
+    if (v3structver < 4)
+    {
+        fprintf(stderr, "AUTH-PAM: This plugin is incompatible with the running version of OpenVPN\n");
+        return OPENVPN_PLUGIN_FUNC_ERROR;
+    }
+
     /*
      * Allocate our context
      */
@@ -298,7 +313,10 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *
     /*
      * Intercept the --auth-user-pass-verify callback.
      */
-    *type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY);
+    ret->type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY);
+
+    /* Save global pointers to functions exported from openvpn */
+    plugin_secure_memzero = args->callbacks->plugin_secure_memzero;
 
     /*
      * Make sure we have two string arguments: the first is the .so name,
@@ -386,7 +404,8 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *
         if (status == RESPONSE_INIT_SUCCEEDED)
         {
             context->foreground_fd = fd[0];
-            return (openvpn_plugin_handle_t) context;
+            ret->handle = (openvpn_plugin_handle_t *) context;
+            return OPENVPN_PLUGIN_FUNC_SUCCESS;
         }
     }
     else
@@ -420,7 +439,7 @@ error:
     {
         free(context);
     }
-    return NULL;
+    return OPENVPN_PLUGIN_FUNC_ERROR;
 }
 
 OPENVPN_EXPORT int
@@ -785,6 +804,7 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *
                         goto done;
                     }
                 }
+                plugin_secure_memzero(up.password, sizeof(up.password));
                 break;
 
             case COMMAND_EXIT:
@@ -802,6 +822,7 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *
     }
 done:
 
+    plugin_secure_memzero(up.password, sizeof(up.password));
 #ifdef USE_PAM_DLOPEN
     dlclose_pam();
 #endif
diff --git a/src/plugins/auth-pam/auth-pam.exports b/src/plugins/auth-pam/auth-pam.exports
index b07937c..597e33f 100644
--- a/src/plugins/auth-pam/auth-pam.exports
+++ b/src/plugins/auth-pam/auth-pam.exports
@@ -1,4 +1,4 @@
-openvpn_plugin_open_v1
+openvpn_plugin_open_v3
 openvpn_plugin_func_v1
 openvpn_plugin_close_v1
 openvpn_plugin_abort_v1
-- 
2.1.4

Re: [Openvpn-devel] [PATCH applied] In auth-pam plugin clear the password after use

[David Sommerseth]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK.  Looking good now.  Tests are good too, and the password wiping works
as expected.

Your patch has been applied to the following branches

commit f403b9a2bf93f0fa35ee8316c2d219f48638a3e5  (master)
commit 4702cf5a898f33693f4f705038e1b4f2568bde71  (release/2.4)
Author: Selva Nair
Date:   Tue May 9 15:44:29 2017 -0400

     In auth-pam plugin clear the password after use

     Signed-off-by: Selva Nair <selva.nair@...277...>
     Acked-by: David Sommerseth <davids@...515...>
     Message-Id: <1494359069-13824-1-git-send-email-selva.nair@...277...>
     URL: http://www.mail-archive.com/search?l=mid&q=1494359069-13824-1-git-send-email-selva.nair@...277...
     Signed-off-by: David Sommerseth <davids@...515...>


- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBCAAGBQJZEiDkAAoJEIbPlEyWcf3yqlgP/RtbbUqmtZ+4GOTSe7bZO4Hp
s07SovaN+xAfNgEa7Gr1UfrfDVkmYjkshtlBTn7CP4arC/dRyI7nKZzbdzg75m9/
krCIZgfELrrWKK4dnx+oe78LCxwM4aSA2F/bmKcf4gjbAPb+H/OK2J3OpbAy2/u8
fVTvt+uq0yzDTHIP77iCAuyC+Ss5248D4/Tc2s8ahyqi06ozKzsM3+FY0nuzVRk6
Qml8CyiwiBqCeO6RC4uVst/9vr9A5scyDb1te2tHuxU/Ue8AA7imxrF1OXqmCF0U
I/0x16+dWLhPBfDp1ISPQ072uejJynTrWvRStckiC5uvVrY5CHxDC1IhwwUg9cml
zYesKzNhXJfXp6BGTr7YOKSLrSOpSRLSmRDhzgCzQ4N2Ov877Yx46uNzfv5D+17D
GHpZXgGyfqKs8NMTr9U+fcAgKTe6cxE/q/NAtdK83L4Xi6L3hkm3vksy7zUQM/xf
mkF3fRkKew2gnx4AieGWNTXL3etzeb+yQUvkWwzScldgB2xJ9zNfyryJ2aa3lL7g
M9clMooBOxCP51heWiVse/YvcuWb4SuqzKff57RH/MgQBWUELokTJSJqL40Eege5
2m1RGBqo/u3HOjycrmVLm8nqMicAIR0QWBhfwrNQ4nnS6GBQv7Aj+2U4B5+871ch
3AsqMrSR3hIAWonO5x42
=23Zp
-----END PGP SIGNATURE-----