Privileges not being dropped if the first connection is not successful

Author: Lukasz Kutyla <[email protected]...>

Interruption in first connection will prevent further privilege dropping

OpenVPN does not drop privileges (UID/GID/chroot) as requested according to the configuration file and/or passed arguments if the first connection is not established successfully, this also includes applying SELinux context. Signals and restarts are processed after "context.first_time" is set to "false" which results in omitting entire block in do_uid_gid_chroot() when another (successful) connection is made, everything is initialized correctly and said function called.

Reproducing is easy, one of the ways is to use the same host with different ports (first one unused, so it fails):

  # reduce timeout to trigger reset faster
  keepalive 10 30
  # unused PORT (to trigger reset when it fails)
  remote VPN_IP 29999
  # correct PORT
  remote VPN_IP 1194
Any later connections made will be associated with no attempts to drop
privileges and the process will keep the ones it has been started with.

A lot of versions seem to be affected by that (including the newest).

Suggested patch removes "c->first_time" from initial checking and additionally moves it to memstat (which isn't affected by "no_delay", so the code could be executed twice). "c0 && !c0->uid_gid_set" expression can handle the job alone and properly even when UID/GID are not specified ("context_0.uid_gid_set" is set to "true" one way or another).

---
 src/openvpn/init.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index fe00918..21cafb5 100644
@@ -948,7 +948,7 @@ 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_set)
     {
       /* chroot if requested */
       if (c->options.chroot_dir)

@@ -972,7 +972,7 @@ do_uid_gid_chroot (struct context *c, bool no_delay)
 	}
 
 #ifdef ENABLE_MEMSTATS
-      if (c->options.memstats_fn)
+      if (c->first_time && c->options.memstats_fn)
 	mstats_open(c->options.memstats_fn);
 #endif
 

--

Lukasz

Hi,

Which OpenVPN version is this about? 2.3.x up to 2.3.5, or 2.3.6/.7/.8, or git master?

(Init sequence and connection attempts have changed quite a bit, and this might be yet another daemon() fallout)

gert

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

Hello,

Affected versions that I've checked so far:
2.1.3, 2.2.1, 2.3.4, 2.3.6, 2.3.7, 2.3.8, git (OpenVPN 2.3_git [git:master/291c227d2ccecaa9+])

I guess it's safe to assume at this point that other releases with similar code present are going to have the same issue.

I forgot to mention previously that suggested patch is NOT going to get rid off "NOTE:" messages for each reconnect (for the sake of being explicit, so this is intentional).

Improved patch below which introduces additional checks using "c->first_time" for notice messages will make sure that the user is only presented with "NOTE:" once, if this is the correct course of action.

---
 src/openvpn/init.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index fe00918..403d2d9 100644
@@ -948,14 +948,14 @@ 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_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);
}

@@ -966,13 +966,13 @@ do_uid_gid_chroot (struct context *c, bool no_delay) platform_user_set (&c0->platform_state_user);
c0->uid_gid_set = true;
}
- else if (c0->uid_gid_specified)
+ else if (c->first_time && c0->uid_gid_specified)
{
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,7 +991,7 @@ 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

--

Lukasz K.

Hi,

[..]

Thanks, this is way more than I expected :-) - and it seems that we did not fiddle with this part of the code yet, so it's time to do so.

On a first glance this v2 version looks good to me, but I need to look a bit more into details at the surrounding code to check whether it might have interesting side effects (we're good at that).

gert

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

Hi,

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

Hello,

As I've mentioned in my first message:
"[...] even when UID/GID are not specified ("context_0.uid_gid_set" is set to "true" one way or another) [...]"

To see this it's necessary to take a better look at "do_uid_gid_chroot()" code.
This is original, unmodified part of code from git (I have stripped out original comments and added my own in places worth to look at with explanations):
---
[...]
/* "c0->uid_gid_set" is controlling whether to enter this block
* regardless of "c->first_time" presence
*/
if (c->first_time && c0 && !c0->uid_gid_set)

    {
      if (c->options.chroot_dir)
	{
	  if (no_delay)
	    platform_chroot (c->options.chroot_dir);
	  else
	    msg (M_INFO, "NOTE: chroot %s", why_not);
	}

      /* There's no explicit check for UID/GID presence, so it will
       * always enter this block when everything has been set and
       * "no_delay" is passed as "true", even when there's no
       * UID/GID change (only chroot etc.).
       */
      if (no_delay)
	{
	  /* "platform_*_set()" is internally checking on it's own
	   * whether UID/GID has been specified.
	   */
	  platform_group_set (&c0->platform_state_group);
	  platform_user_set (&c0->platform_state_user);
	  /* "c0->uid_gid_set" is flipped to "true", which means that
	   * from now on "c0 && !c0->uid_gid_set" expression from the
	   * beginning will return "false", which in turn will omit the
	   * entire block next time like "c->first_time == false" would
	   */
	  c0->uid_gid_set = true;
	}
      else if (c0->uid_gid_specified)
	{
	  msg (M_INFO, "NOTE: UID/GID downgrade %s", why_not);
	}

[...]
--

External calls are made only when "no_delay" is passed as "true", and when this happens "c0->uid_gid_set" is always set to "true". The only exception is memstat (where it's not bound to "no_delay" checks) and call to "msg()" with "NOTE:":
---
[...]

#ifdef ENABLE_MEMSTATS
      if (c->options.memstats_fn)
	mstats_open(c->options.memstats_fn);

#endif
[...]
--
That's why it gets a "first_time" check as this is something that could be called more than once.

User will also keep receiving appropriate "NOTE:" messages (depending on which privileges are going to be dropped) on each reconnect until "no_delay" is passed as "true" (everything is initialized and ready to actually do privilege dropping if requested, and where "c0->uid_gid_set" is set to "true"):
---
[...]

	  if (no_delay)
	    platform_chroot (c->options.chroot_dir);
	  else
	    msg (M_INFO, "NOTE: chroot %s", why_not);
[...]
      if (no_delay)
	{
	  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) {
	    if (-1 == setcon (c->options.selinux_context))
	      msg (M_ERR, "setcon to '%s' failed; is /proc
accessible?", c->options.selinux_context);
	    else
	      msg (M_INFO, "setcon to '%s' succeeded",
c->options.selinux_context);
	  }
	  else
	    msg (M_INFO, "NOTE: setcon %s", why_not);

[...]
--
And this is something that second patch also targets (with the use of "first_time").

This logic hasn't changed since 2.1.3 (this is the oldest release I've tested).
In 2.1.3/2.2.1 there's no memstat section though, it looks like it has been added in 2.3.x. There are some minor naming changes between those versions too.

It's worth to note that the use of "context_0.uid_gid_set" seems to be limited to said function only.

Lukasz K.

"context_0.uid_gid_set" has already been since 2.1.3 (and probably earlier versions) serving as a dedicated way of guarding "do_uid_gid_chroot()" from executing privilege dropping area twice (if you take "context.first_time" out of the way first and put "first_time" checks to calls [the ones outside of "no_delay" checks] that could be executed more than once, before "uid_gid_set" is flipped to "true"). Unlike "context.first_time" it also only prevents entering entire block *after* initialization is done (connection successful etc.) and the process has already dropped privileges (if requested).

"uid_gid_set" name is actually causing a small confusion here since it might indicate that it's related *only* to UID/GID dropping (maybe that was the initial intention), but the way it is used (and has been) is more "do_uid_gid_chroot()" oriented overall - this is not a bad thing, because it allows us to fix the issue.

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).

Lukasz K.

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+]).

---
 src/openvpn/init.c    | 31 ++++++++++++++++++-------------
 src/openvpn/openvpn.h |  6 ++++--
 2 files changed, 22 insertions(+), 15 deletions(-)

index fe00918..e17a43f 100644
@@ -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;
     }

}

index 1c2a80b..b13ff50 100644
@@ -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.

Hi,

Thanks, and thanks for the explanations - as you could see, I'm not very well acquainted with that particular section of the code (I'm more the packet guy...). We'll try to discuss this at the upcoming IRC meeting next monday (#openvpn-devel on freenode.net) and then decide which version we pick :-) - quite obviously it's a bug that needs fixing.

gert

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

Thanks. This version looks good to me, but the patch won't apply. Most likely your MUA messed up line wrapping. Could you resend the patch using 'git send-email', or alternatively as an attachment?

-Steffan

Hi,

You're right, my mail client is wrapping lines once they go past 72 characters, so it broke the patch.

I'm really surprised I haven't noticed that.

This time I've used 'git format-patch', and included resulting file as a separate attachment (as you requested).

I'm sorry for the delay, I was unavailable lately.

Lukasz K.

From 35d268234fc65b75239b2dff8f9eb15c8c4a642a Mon Sep 17 00:00:00 2001
From: Lukasz Kutyla <[email protected]...>
Date: Sat, 17 Oct 2015 21:15:15 +0200
Subject: [PATCH] Fix privilege drop if first connection attempt fails

OpenVPN does not drop privileges (UID/GID/chroot) as requested according to the configuration file and/or passed arguments if the first connection attempt is not established successfully, this also includes applying SELinux context.
Signals and restarts are processed after "context.first_time" is set to "false", which results in omitting entire privilege dropping block in "do_uid_gid_chroot()" when successful connection is finally made (everything is initialized correctly and said function is called), since "context.first_time" is used as block entry condition.

We modify "do_uid_gid_chroot()" in such a way that allows us to drop privileges even when first connection attempt was unsuccessful.

---
 src/openvpn/init.c    | 31 ++++++++++++++++++-------------
 src/openvpn/openvpn.h |  6 ++++--
 2 files changed, 22 insertions(+), 15 deletions(-)

index 5dd8781..c5c0ab6 100644
@@ -950,31 +950,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
 

@@ -993,10 +992,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;
     }
 }
 

index 4fab00b..3f1df6e 100644
@@ -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 which tells 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;
 };

2.6.1

I'm resending my previous message (as a quote) with attachment, using different headers and no signing, as it appears it has been incorrectly handled by mailman.

Lukasz K.

From 35d268234fc65b75239b2dff8f9eb15c8c4a642a Mon Sep 17 00:00:00 2001
From: Lukasz Kutyla <[email protected]>
Date: Sat, 17 Oct 2015 21:15:15 +0200
Subject: [PATCH] Fix privilege drop if first connection attempt fails

OpenVPN does not drop privileges (UID/GID/chroot) as requested according to the configuration file and/or passed arguments if the first connection attempt is not established successfully, this also includes applying SELinux context.
Signals and restarts are processed after "context.first_time" is set to "false", which results in omitting entire privilege dropping block in "do_uid_gid_chroot()" when successful connection is finally made (everything is initialized correctly and said function is called), since "context.first_time" is used as block entry condition.

We modify "do_uid_gid_chroot()" in such a way that allows us to drop privileges even when first connection attempt was unsuccessful.

---
 src/openvpn/init.c    | 31 ++++++++++++++++++-------------
 src/openvpn/openvpn.h |  6 ++++--
 2 files changed, 22 insertions(+), 15 deletions(-)

index 5dd8781..c5c0ab6 100644
@@ -950,31 +950,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
 

@@ -993,10 +992,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;
     }
 }
 

index 4fab00b..3f1df6e 100644
@@ -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 which tells 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;
 };

2.6.1

Hi,

ACK. Code (still) looks good, patch applies just fine, and testing confirms this does fix the bug.

I think most of the delay has been on our side actually ;) Thanks for the patch, and for following up.

-Steffan

Hi,

The way that really works best is "git send-email ..." - it can handle remote servers (smtp+auth) if needed, or just feed to local MTA, and it will always get the formatting and whitespaceing and mail headers and everything right :-)

Your e-mail, for example, was again a bit weird, as the original mail was just "included" and not "attached" (message/rfc822 attachment), plus, it had CR+NL line endings in the "attachment" part - so "git am" had a hard time extracting the actual patch as it stopped searching when it hit the second "From " line...

Anyway, since it wasn't whitespace-garbled, it applied, but really and truly, git send-email is what everyone wants :-)

Merging right now...

gert

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

[email protected]...>
References: <[email protected]... [email protected]...>

Thanks.

Your patch has been applied to the master and release/2.3 branch.

commit 825b3272acb353e04b37f38299d4df7e63e87d9e (master) commit 6c6280ae1672ba10aa009ba8d3bf923f68272de4 (release/2.3)

Author: Lukasz Kutyla
Date:   Sat Oct 17 21:15:15 2015 +0200

     Fix privilege drop if first connection attempt fails

     Signed-off-by: Lukasz Kutyla <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10301
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10301
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

wooops... "this is what I meant with 'please use git send-email'..." - I had to hand-massage the mail a bit, and then it picked up the message ID twice...

gert

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