[Openvpn-devel] Refuse PUSH_REQUEST as client/refactor process_incoming_push_request

Message ID 20200725235143.22736-1-arne@rfc2549.org
State Awaiting Upstream
Headers show
Series
  • [Openvpn-devel] Refuse PUSH_REQUEST as client/refactor process_incoming_push_request
Related show

Commit Message

Arne Schwabe July 25, 2020, 11:51 p.m.
When a server sends a client a push request, the client will reply
with a push reply. The reply is bogus and almost empty since almost
all the options that are normally set (remote ip etc) are unset.

I checked 2.4 and master and this does not have any security implications
or other bugs but it is still better to refuse this.

This code also refactors the method to get rid of the ret variable to
make the code flow easier to understand.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/push.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

Comments

tincanteksup July 26, 2020, 1:05 p.m. | #1
Sanity check:

On 26/07/2020 00:51, Arne Schwabe wrote:
> When a server sends a client a push request, the client will reply
> with a push reply. The reply is bogus and almost empty since almost
> all the options that are normally set (remote ip etc) are unset.
> 
> I checked 2.4 and master and this does not have any security implications
> or other bugs but it is still better to refuse this.
> 
> This code also refactors the method to get rid of the ret variable to
> make the code flow easier to understand.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>   src/openvpn/push.c | 40 ++++++++++++++++++++++------------------
>   1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 1c4f2033..84193afe 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -733,13 +733,19 @@ push_remove_option(struct options *o, const char *p)
>   int
>   process_incoming_push_request(struct context *c)
>   {
> -    int ret = PUSH_MSG_ERROR;
> +    /* if we receive a message a client we do not want to reply to it
> +     * so limit this to multi server */

This sentence does not make sense, suggestion:

If a client receives a push request then ignore it.  Only multi server 
will reply to a push requests.

Something like that ..




> +    if (c->options.mode != MODE_SERVER)
> +    {
> +        return PUSH_MSG_ERROR;
> +    }
> +
>   
>       if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED || c->c2.context_auth == CAS_FAILED)
>       {
>           const char *client_reason = tls_client_reason(c->c2.tls_multi);
>           send_auth_failed(c, client_reason);
> -        ret = PUSH_MSG_AUTH_FAILURE;
> +        return PUSH_MSG_AUTH_FAILURE;
>       }
>       else if (c->c2.context_auth == CAS_SUCCEEDED)
>       {
> @@ -748,29 +754,27 @@ process_incoming_push_request(struct context *c)
>           openvpn_time(&now);
>           if (c->c2.sent_push_reply_expiry > now)
>           {
> -            ret = PUSH_MSG_ALREADY_REPLIED;
> +            return PUSH_MSG_ALREADY_REPLIED;
>           }
> -        else
> -        {
> -            /* per-client push options - peer-id, cipher, ifconfig, ipv6-ifconfig */
> -            struct push_list push_list = { 0 };
> -            struct gc_arena gc = gc_new();
>   
> -            if (prepare_push_reply(c, &gc, &push_list)
> -                && send_push_reply(c, &push_list))
> -            {
> -                ret = PUSH_MSG_REQUEST;
> -                c->c2.sent_push_reply_expiry = now + 30;
> -            }
> -            gc_free(&gc);
> +        int ret = PUSH_MSG_ERROR;
> +        /* per-client push options - peer-id, cipher, ifconfig, ipv6-ifconfig */
> +        struct push_list push_list = { 0 };
> +        struct gc_arena gc = gc_new();
> +
> +        if (prepare_push_reply(c, &gc, &push_list)
> +            && send_push_reply(c, &push_list))
> +        {
> +            ret = PUSH_MSG_REQUEST;
> +            c->c2.sent_push_reply_expiry = now + 30;
>           }
> +        gc_free(&gc);
> +        return ret;
>       }
>       else
>       {
> -        ret = PUSH_MSG_REQUEST_DEFERRED;
> +        return PUSH_MSG_REQUEST_DEFERRED;
>       }
> -
> -    return ret;
>   }
>   
>   static void
>
Arne Schwabe July 26, 2020, 4:44 p.m. | #2
Am 26.07.20 um 01:51 schrieb Arne Schwabe:
> When a server sends a client a push request, the client will reply
> with a push reply. The reply is bogus and almost empty since almost
> all the options that are normally set (remote ip etc) are unset.
> 
> I checked 2.4 and master and this does not have any security implications
> or other bugs but it is still better to refuse this.
> 
> This code also refactors the method to get rid of the ret variable to
> make the code flow easier to understand.

On further discussion on IRC, retract this patch. The
tls-server/tls-client pair as a p2p pair with one side (does not even
need to be the one with tls-server) can have multiple "push xy" in the
config.

Arne

Patch

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 1c4f2033..84193afe 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -733,13 +733,19 @@  push_remove_option(struct options *o, const char *p)
 int
 process_incoming_push_request(struct context *c)
 {
-    int ret = PUSH_MSG_ERROR;
+    /* if we receive a message a client we do not want to reply to it
+     * so limit this to multi server */
+    if (c->options.mode != MODE_SERVER)
+    {
+        return PUSH_MSG_ERROR;
+    }
+
 
     if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED || c->c2.context_auth == CAS_FAILED)
     {
         const char *client_reason = tls_client_reason(c->c2.tls_multi);
         send_auth_failed(c, client_reason);
-        ret = PUSH_MSG_AUTH_FAILURE;
+        return PUSH_MSG_AUTH_FAILURE;
     }
     else if (c->c2.context_auth == CAS_SUCCEEDED)
     {
@@ -748,29 +754,27 @@  process_incoming_push_request(struct context *c)
         openvpn_time(&now);
         if (c->c2.sent_push_reply_expiry > now)
         {
-            ret = PUSH_MSG_ALREADY_REPLIED;
+            return PUSH_MSG_ALREADY_REPLIED;
         }
-        else
-        {
-            /* per-client push options - peer-id, cipher, ifconfig, ipv6-ifconfig */
-            struct push_list push_list = { 0 };
-            struct gc_arena gc = gc_new();
 
-            if (prepare_push_reply(c, &gc, &push_list)
-                && send_push_reply(c, &push_list))
-            {
-                ret = PUSH_MSG_REQUEST;
-                c->c2.sent_push_reply_expiry = now + 30;
-            }
-            gc_free(&gc);
+        int ret = PUSH_MSG_ERROR;
+        /* per-client push options - peer-id, cipher, ifconfig, ipv6-ifconfig */
+        struct push_list push_list = { 0 };
+        struct gc_arena gc = gc_new();
+
+        if (prepare_push_reply(c, &gc, &push_list)
+            && send_push_reply(c, &push_list))
+        {
+            ret = PUSH_MSG_REQUEST;
+            c->c2.sent_push_reply_expiry = now + 30;
         }
+        gc_free(&gc);
+        return ret;
     }
     else
     {
-        ret = PUSH_MSG_REQUEST_DEFERRED;
+        return PUSH_MSG_REQUEST_DEFERRED;
     }
-
-    return ret;
 }
 
 static void