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

[Lukasz K.]

Hello,

On Sun, 30 Aug 2015 13:54:44 +0200
Gert Doering <g...@greenie.muc.de> wrote:

> Hi,
> 
> On Sun, Aug 30, 2015 at 12:23:36AM +0200, Lukasz K. wrote:
> > Author: Lukasz Kutyla <movrax-...@cryptolab.net>
> > 
> > Interruption in first connection will prevent further privilege
> > dropping
> 
> 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

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.

Signed-off-by: Lukasz Kutyla <movrax-...@cryptolab.net>
---
 src/openvpn/init.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index fe00918..403d2d9 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -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.

pgpXhKFfBqNPt.pgp
Description: OpenPGP digital signature