Re: [Openvpn-devel] [PATCH] Privileges not being dropped if the first connection is not successful

[Lukasz K.]

> While the patch is designed to only fix the problem I was thinking if
> it wouldn't be perhaps better to rename "context_0.uid_gid_set" to
> something more function related like "context_0.uid_gid_chroot_set",
> to make it more obvious for people who would look at the code in the
> future that this member is actually meant for the function in general,
> not only a certain part of it (UID/GID).

With above in mind I decided to introduce this change to the patch.

I also included few other "improvements" to make the function appear
more consistent and more obvious (like "c0->uid_gid_chroot_set = true"
separated and put at the end of the block).

At this point I'm leaving it up to you guys, this is just my another
suggestion, compiled & tested with latest git ([2.3_git
[git:master/291c227d2ccecaa9+]).

Signed-off-by: Lukasz Kutyla <movrax-...@cryptolab.net>
---
 src/openvpn/init.c    | 31 ++++++++++++++++++-------------
 src/openvpn/openvpn.h |  6 ++++--
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index fe00918..e17a43f 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -948,31 +948,30 @@ do_uid_gid_chroot (struct context *c, bool
no_delay) static const char why_not[] = "will be delayed because of
--client, --pull, or --up-delay"; struct context_0 *c0 = c->c0;
 
-  if (c->first_time && c0 && !c0->uid_gid_set)
+  if (c0 && !c0->uid_gid_chroot_set)
     {
       /* chroot if requested */
       if (c->options.chroot_dir)
        {
          if (no_delay)
            platform_chroot (c->options.chroot_dir);
-         else
+         else if (c->first_time)
            msg (M_INFO, "NOTE: chroot %s", why_not);
        }
 
-      /* set user and/or group that we want to setuid/setgid to */
-      if (no_delay)
+      /* set user and/or group if we want to setuid/setgid */
+      if (c0->uid_gid_specified)
        {
-         platform_group_set (&c0->platform_state_group);
-         platform_user_set (&c0->platform_state_user);
-         c0->uid_gid_set = true;
-       }
-      else if (c0->uid_gid_specified)
-       {
-         msg (M_INFO, "NOTE: UID/GID downgrade %s", why_not);
+         if (no_delay) {
+           platform_group_set (&c0->platform_state_group);
+           platform_user_set (&c0->platform_state_user);
+         }
+         else if (c->first_time)
+           msg (M_INFO, "NOTE: UID/GID downgrade %s", why_not);
        }
 
 #ifdef ENABLE_MEMSTATS
-      if (c->options.memstats_fn)
+      if (c->first_time && c->options.memstats_fn)
        mstats_open(c->options.memstats_fn);
 #endif
 
@@ -991,10 +990,16 @@ do_uid_gid_chroot (struct context *c, bool
no_delay) else
              msg (M_INFO, "setcon to '%s' succeeded",
c->options.selinux_context); }
-         else
+         else if (c->first_time)
            msg (M_INFO, "NOTE: setcon %s", why_not);
        }
 #endif
+
+      /* Privileges are going to be dropped by now (if requested), be
sure
+       * to prevent any future privilege dropping attempts from now on.
+       */
+      if (no_delay)
+       c0->uid_gid_chroot_set = true;
     }
 }
 
diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index 1c2a80b..b13ff50 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -130,13 +130,15 @@ struct context_persist
  *
  * Level 0 state is initialized once at program startup, and then
remains
  * throughout the lifetime of the OpenVPN process.  This structure
- * contains information related to the process's PID, user, and group.
+ * contains information related to the process's PID, user, group, and
+ * privileges.
  */
 struct context_0
 {
   /* workspace for --user/--group */
   bool uid_gid_specified;
-  bool uid_gid_set;
+  /* helper to tell us whether we should keep trying to drop
privileges */
+  bool uid_gid_chroot_set;
   struct platform_state_user platform_state_user;
   struct platform_state_group platform_state_group;
 };

--

Lukasz K.

pgpTkIF66sVH0.pgp
Description: OpenPGP digital signature