Re: [Openvpn-devel] [PATCH v2] Fix potential double-free() in Interactive Service (CVE-2018-9336)

2018-04-24 Thread Gert Doering
Hi,

On Sat, Apr 14, 2018 at 09:26:17AM +0200, Gert Doering wrote:
> Malformed input data on the service pipe towards the OpenVPN interactive
> service (normally used by the OpenVPN GUI to request openvpn instances
> from the service) can result in a double free() in the error handling code.
[..]

Due to the sensitive nature of the patch, it was held under embargo rules
- that is "only sent to the security@ list, privately ACKed, commits not
announced to the public list right away".

I do have an ACK for this from Selva, and have my usually "it went it
with commit ID..." mail, but for whatever reason I cannot re-send my own
mail to the -devel list, and I think SPF is stopping me from bouncing
Selva's mail...  which is a bit of an annoyance.

So I just forward the e-mails below, PGP sign all this (and the commits
are PGP-signed as well), and leave verification of "is the commit the
same as in the repo?  is it what Selva ACKed?" to the interested reader.


-
From: Selva Nair 
Date: Sat, 14 Apr 2018 12:44:57 -0400
Message-ID: 
Subject: Re: [PATCH v2] Fix potential double-free() in Interactive Service 
(CVE-2018-9336)
To: Gert Doering 
Cc: secur...@openvpn.net, jbai...@tenable.com

On Sat, Apr 14, 2018 at 3:26 AM, Gert Doering  wrote:

> Malformed input data on the service pipe towards the OpenVPN interactive
> service (normally used by the OpenVPN GUI to request openvpn instances
> from the service) can result in a double free() in the error handling code.
>
> This usually only leads to a process crash (DoS by an unprivileged local
> account) but since it could possibly lead to memory corruption if
> happening while multiple other threads are active at the same time,
> CVE-2018-9336 has been assigned to acknowledge this risk.
>
> Fix by ensuring that sud->directory is set to NULL in GetStartUpData()
> for all error cases (thus not being free()ed in FreeStartupData()).
>
> Rewrite control flow to use explicit error label for error exit.
>
> Discovered and reported by Jacob Baines .
>
> CVE: 2018-9336
>
> Signed-off-by: Gert Doering 
>
> --
> v2: reword commit message, no code changes


Just for completeness: all good so ACK again.

Selva

-
Date: Thu, 19 Apr 2018 17:24:49 +0200 (CEST)
Message-Id: <201804191524.w3jfongd007...@chekov.greenie.muc.de>
From: Gert Doering 
To: Gert Doering 
Cc: openvpn-devel@lists.sourceforge.net
Subject: [PATCH applied] Re: Fix potential double-free() in Interactive Service 
(CVE-2018-9336)

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

commit 1394192b210cb3c6624a7419bcf3ff966742e79b (master)
commit da242af8d3750a231bfd687d0a92cf2004dae988 (release/2.4)
Author: Gert Doering
Date:   Sat Apr 14 09:26:17 2018 +0200

 Fix potential double-free() in Interactive Service (CVE-2018-9336)

 Signed-off-by: Gert Doering 
 Acked-by: Selva Nair 
 Message-Id: <20180414072617.25075-1-g...@greenie.muc.de>
 URL: 
https://www.mail-archive.com/search?l=mid&q=20180414072617.25075-1-g...@greenie.muc.de
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering


-- 
"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 g...@greenie.muc.de


signature.asc
Description: PGP signature
--
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


[Openvpn-devel] [PATCH applied] Re: Fix potential double-free() in Interactive Service (CVE-2018-9336)

2018-04-24 Thread Gert Doering
Your patch has been applied to the master and release/2.4 branch.

commit 1394192b210cb3c6624a7419bcf3ff966742e79b (master)
commit da242af8d3750a231bfd687d0a92cf2004dae988 (release/2.4)
Author: Gert Doering
Date:   Sat Apr 14 09:26:17 2018 +0200

 Fix potential double-free() in Interactive Service (CVE-2018-9336)

 Signed-off-by: Gert Doering 
 Acked-by: Selva Nair 
 Message-Id: <20180414072617.25075-1-g...@greenie.muc.de>
 URL: 
https://www.mail-archive.com/search?l=mid&q=20180414072617.25075-1-g...@greenie.muc.de
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering


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


Re: [Openvpn-devel] [PATCH tap-windows6] Have the driver version display as major.minor.revision.build

2018-04-24 Thread Gert Doering
Hi,

On Tue, Apr 24, 2018 at 10:25:20PM -0400, selva.n...@gmail.com wrote:
> From: Selva Nair 
> 
> The driver version (taken from the INF) and the file version of
> tap0901.sys file (from its resource) will now display like
> 9.22.1.601.
> 
> The driver details tab will continue to show the text form of
> the version as "major.minor.revision (major/minor)".
> Eg., "9.22.1 (9/22)"
> 
> Signed-off-by: Selva Nair 
> ---
> 
> Not sure this should be submitted as a PR  or like this..

Not sure either (tap-windows6 has seen so little development that we
never defined a proper process, I think).  

So I just ACK this here on the basis of "we did not know how to do it, 
but this is what we wanted to achieve" - it *looks* reasonable.

So if Samuli can be convinced to build a driver with this merged and
a signature, I'm happy to give it a test run and see if everything is
happy...

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 g...@greenie.muc.de


signature.asc
Description: PGP signature
--
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


Re: [Openvpn-devel] [PATCH v2] Fix potential double-free() in Interactive Service (CVE-2018-9336)

2018-04-24 Thread Selva Nair
On Sat, Apr 14, 2018 at 3:26 AM, Gert Doering  wrote:

> Malformed input data on the service pipe towards the OpenVPN interactive
> service (normally used by the OpenVPN GUI to request openvpn instances
> from the service) can result in a double free() in the error handling code.
>
> This usually only leads to a process crash (DoS by an unprivileged local
> account) but since it could possibly lead to memory corruption if
> happening while multiple other threads are active at the same time,
> CVE-2018-9336 has been assigned to acknowledge this risk.
>
> Fix by ensuring that sud->directory is set to NULL in GetStartUpData()
> for all error cases (thus not being free()ed in FreeStartupData()).
>
> Rewrite control flow to use explicit error label for error exit.
>
> Discovered and reported by Jacob Baines .
>
> CVE: 2018-9336
>
> Signed-off-by: Gert Doering 
>
> --
> v2: reword commit message, no code changes


Just for completeness: all good so ACK again.

Selva
--
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


[Openvpn-devel] [PATCH v2] Fix potential double-free() in Interactive Service (CVE-2018-9336)

2018-04-24 Thread Gert Doering
Malformed input data on the service pipe towards the OpenVPN interactive
service (normally used by the OpenVPN GUI to request openvpn instances
from the service) can result in a double free() in the error handling code.

This usually only leads to a process crash (DoS by an unprivileged local
account) but since it could possibly lead to memory corruption if
happening while multiple other threads are active at the same time,
CVE-2018-9336 has been assigned to acknowledge this risk.

Fix by ensuring that sud->directory is set to NULL in GetStartUpData()
for all error cases (thus not being free()ed in FreeStartupData()).

Rewrite control flow to use explicit error label for error exit.

Discovered and reported by Jacob Baines .

CVE: 2018-9336

Signed-off-by: Gert Doering 

--
v2: reword commit message, no code changes
---
 src/openvpnserv/interactive.c | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index fbc32f90..861f5e70 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -453,7 +453,6 @@ static BOOL
 GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
 {
 size_t size, len;
-BOOL ret = FALSE;
 WCHAR *data = NULL;
 DWORD bytes, read;
 
@@ -462,7 +461,7 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
 {
 MsgToEventLog(M_SYSERR, TEXT("PeekNamedPipeAsync failed"));
 ReturnLastError(pipe, L"PeekNamedPipeAsync");
-goto out;
+goto err;
 }
 
 size = bytes / sizeof(*data);
@@ -470,7 +469,7 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
 {
 MsgToEventLog(M_SYSERR, TEXT("malformed startup data: 1 byte 
received"));
 ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, 
&exit_event);
-goto out;
+goto err;
 }
 
 data = malloc(bytes);
@@ -478,7 +477,7 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
 {
 MsgToEventLog(M_SYSERR, TEXT("malloc failed"));
 ReturnLastError(pipe, L"malloc");
-goto out;
+goto err;
 }
 
 read = ReadPipeAsync(pipe, data, bytes, 1, &exit_event);
@@ -486,14 +485,14 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
 {
 MsgToEventLog(M_SYSERR, TEXT("ReadPipeAsync failed"));
 ReturnLastError(pipe, L"ReadPipeAsync");
-goto out;
+goto err;
 }
 
 if (data[size - 1] != 0)
 {
 MsgToEventLog(M_ERR, TEXT("Startup data is not NULL terminated"));
 ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, 
&exit_event);
-goto out;
+goto err;
 }
 
 sud->directory = data;
@@ -503,7 +502,7 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
 {
 MsgToEventLog(M_ERR, TEXT("Startup data ends at working directory"));
 ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, 
&exit_event);
-goto out;
+goto err;
 }
 
 sud->options = sud->directory + len;
@@ -513,16 +512,16 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
 {
 MsgToEventLog(M_ERR, TEXT("Startup data ends at command line 
options"));
 ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, 
&exit_event);
-goto out;
+goto err;
 }
 
 sud->std_input = sud->options + len;
-data = NULL; /* don't free data */
-ret = TRUE;
+return TRUE;
 
-out:
+err:
+sud->directory = NULL; /* caller must not free() */
 free(data);
-return ret;
+return FALSE;
 }
 
 
-- 
2.16.1


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


[Openvpn-devel] [PATCH tap-windows6] Have the driver version display as major.minor.revision.build

2018-04-24 Thread selva . nair
From: Selva Nair 

The driver version (taken from the INF) and the file version of
tap0901.sys file (from its resource) will now display like
9.22.1.601.

The driver details tab will continue to show the text form of
the version as "major.minor.revision (major/minor)".
Eg., "9.22.1 (9/22)"

Signed-off-by: Selva Nair 
---

Not sure this should be submitted as a PR  or like this..

An image showing the version is here
https://user-images.githubusercontent.com/3981391/39222715-29500c5a-480c-11e8-823a-73328baf69dd.PNG
But I would like to test again using a properly signed version 

 src/OemVista.inf.in | 2 +-
 src/config.h.in | 2 ++
 src/resource.rc | 2 +-
 version.m4  | 4 ++--
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/OemVista.inf.in b/src/OemVista.inf.in
index 004ed62..26152f5 100644
--- a/src/OemVista.inf.in
+++ b/src/OemVista.inf.in
@@ -55,7 +55,7 @@
 
 ; This version number should match the version
 ; number given in SOURCES.
-   
DriverVer=@PRODUCT_TAP_WIN_RELDATE@,@PRODUCT_TAP_WIN_MAJOR@.00.00.@PRODUCT_TAP_WIN_MINOR@
+   
DriverVer=@PRODUCT_TAP_WIN_RELDATE@,@PRODUCT_TAP_WIN_MAJOR@.@PRODUCT_TAP_WIN_MINOR@.@PRODUCT_TAP_WIN_REVISION@.@PRODUCT_TAP_WIN_BUILD@
 
 [Strings]
DeviceDescription = "@PRODUCT_TAP_WIN_DEVICE_DESCRIPTION@"
diff --git a/src/config.h.in b/src/config.h.in
index 322afa8..c013348 100644
--- a/src/config.h.in
+++ b/src/config.h.in
@@ -7,3 +7,5 @@
 #define PRODUCT_TAP_WIN_PROVIDER   "@PRODUCT_TAP_WIN_PROVIDER@"
 #define PRODUCT_TAP_WIN_DEVICE_DESCRIPTION 
"@PRODUCT_TAP_WIN_DEVICE_DESCRIPTION@"
 #define PRODUCT_TAP_WIN_RELDATE"@PRODUCT_TAP_WIN_RELDATE@"
+#define PRODUCT_TAP_WIN_REVISION   @PRODUCT_TAP_WIN_REVISION@
+#define PRODUCT_TAP_WIN_BUILD  @PRODUCT_TAP_WIN_BUILD@
diff --git a/src/resource.rc b/src/resource.rc
index 3c40d03..229e437 100644
--- a/src/resource.rc
+++ b/src/resource.rc
@@ -44,7 +44,7 @@
 
 
 #define VER_PRODUCTNAME_STR VER_FILEDESCRIPTION_STR
-#define VER_PRODUCTVERSION 
PRODUCT_TAP_WIN_MAJOR,00,00,PRODUCT_TAP_WIN_MINOR
+#define VER_PRODUCTVERSION 
PRODUCT_TAP_WIN_MAJOR,PRODUCT_TAP_WIN_MINOR,PRODUCT_TAP_WIN_REVISION,PRODUCT_TAP_WIN_BUILD
 
 #define XSTR(s) STR(s)
 #define STR(s) #s
diff --git a/version.m4 b/version.m4
index 1d7f92e..955b418 100644
--- a/version.m4
+++ b/version.m4
@@ -2,7 +2,7 @@ dnl define the TAP version
 define([PRODUCT_NAME], [TAP-Windows])
 define([PRODUCT_PUBLISHER], [OpenVPN Technologies, Inc.])
 define([PRODUCT_VERSION], [9.22.1])
-define([PRODUCT_VERSION_RESOURCE], [9,0,0,22])
+define([PRODUCT_VERSION_RESOURCE], [9,22,1,601])
 define([PRODUCT_TAP_WIN_COMPONENT_ID], [tap0901])
 define([PRODUCT_TAP_WIN_MAJOR], [9])
 define([PRODUCT_TAP_WIN_MINOR], [22])
@@ -11,4 +11,4 @@ define([PRODUCT_TAP_WIN_BUILD], [601])
 define([PRODUCT_TAP_WIN_PROVIDER], [TAP-Windows Provider V9])
 define([PRODUCT_TAP_WIN_CHARACTERISTICS], [0x81])
 define([PRODUCT_TAP_WIN_DEVICE_DESCRIPTION], [TAP-Windows Adapter V9])
-define([PRODUCT_TAP_WIN_RELDATE], [04/15/2018])
+define([PRODUCT_TAP_WIN_RELDATE], [04/24/2018])
-- 
2.6.2


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


Re: [Openvpn-devel] [PATCH 1/1] systemd: run openvpn with dedicated user

2018-04-24 Thread Christian Hesse
Antonio Quartulli  on Tue, 2018/04/24 23:08:
> OTOH I understand that there are people that don't care about having a
> working tunnel reconfiguration and are fine with starting openvpn as
> root (and then dropping privileges).
> 
> For these people, adding the above capabilities results in giving the
> openvpn process more power than before.
> 
> Maybe users willing to adopt this stricter behaviour should have a knob
> somewhere that will enable the usual
> run-as-root-and-then-drop-priv-with-no-caps?

NAK. :-p

I think the solution for this dilemma is pretty easy: I should strip the part
from my patch that disables user switching when started from systemd. We can
start as user "openvpn" any way - as long as the process has capabilities
CAP_SETGID and CAP_SETUID it still can switch user context and drop
privileges.

So users have two options: keep the process running as user "openvpn" with
capabilities for more flexibility or switch to unprivileged user for more
security.

No need to have root involved. Sounds good?
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgpCuVux2kjDG.pgp
Description: OpenPGP digital signature
--
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


Re: [Openvpn-devel] [PATCH 1/1] systemd: run openvpn with dedicated user

2018-04-24 Thread Antonio Quartulli
Hi,

On 24/04/18 21:08, Simon Ruderich wrote:
>> I do not agree that the process is running with root privileges. It has some
>> extra capabilities, but it can not kill processes, fork away and change
>> cgroups, etc.
>> IMHO that is what we want to achieve.
> 
> I disagree. A process with CAP_DAC_OVERRIDE can read/write
> _every_ file on the system (man 7 capabilities)! This equals root
> privileges. Even CAP_NET_ADMIN is very powerful as it allows
> modifying the firewall which might give access to sensitive
> services which are normally not exposed.
> 
>> For this patch I took the current set of capabilities and stripped CAP_SETGID
>> and CAP_SETUID for the netlink version. Whether or not the other capabilities
>> are required should be discussed independently. Wondering why we have
>> CAP_DAC_OVERRIDE in our capability capability set... That looks suspicious
>> indeed.
> 
> Even with CAP_DAC_OVERRIDE stripped this change keeps openvpn
> running with (much) more privileges than before. Is this
> desirable?

I think it depends on your perspective.

What Christian says is that with this patch:

1) you can start openvpn as non-root directly (impossible right now)

2) you have full support for tunnel reconfiguration even when running as
non-root (people willing to achieve this now must start and keep openvpn
running as root)

I consider both points above steps forward towards a better security
model for OpenVPN.


OTOH I understand that there are people that don't care about having a
working tunnel reconfiguration and are fine with starting openvpn as
root (and then dropping privileges).

For these people, adding the above capabilities results in giving the
openvpn process more power than before.

Maybe users willing to adopt this stricter behaviour should have a knob
somewhere that will enable the usual
run-as-root-and-then-drop-priv-with-no-caps?

Generally speaking I believe that openvpn, as a VPN and partly routing
daemon, should be allowed to run with CAP_NET_ADMIN set as it enables
more features (tunnel reconfiguration to start with).

Maybe clients should run with the caps by default while servers should
be launched with the original behaviour? Not sure, honestly.

Cheers,



-- 
Antonio Quartulli



signature.asc
Description: OpenPGP digital signature
--
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


Re: [Openvpn-devel] [PATCH 1/1] systemd: run openvpn with dedicated user

2018-04-24 Thread Simon Ruderich
On Tue, Apr 24, 2018 at 12:03:37PM +0200, Christian Hesse wrote:
> The above snippet holds code for both, netlink and iproute2 versions.
>
> The iproute2 version (that is what is used currently) uses systemd option
> "CapabilityBoundingSet" to limit the capabilities to the given set. If
> configured openvpn will drop privileges after setup.
>
> With netlink and my patch on top we go the other way: The process runs (and
> is started) with user "openvpn". To grant required privileges we use
> systemd option "AmbientCapabilities" and give capabilities to the process.
> The process keeps these capabilities, but that's a benefit: The process
> survives a reconnect that requires configuration changes and shuts down
> cleanly (takes down routes and addresses).

Hello Christian,

Thanks for the confirmation, that's what I assumed.

> I do not agree that the process is running with root privileges. It has some
> extra capabilities, but it can not kill processes, fork away and change
> cgroups, etc.
> IMHO that is what we want to achieve.

I disagree. A process with CAP_DAC_OVERRIDE can read/write
_every_ file on the system (man 7 capabilities)! This equals root
privileges. Even CAP_NET_ADMIN is very powerful as it allows
modifying the firewall which might give access to sensitive
services which are normally not exposed.

> For this patch I took the current set of capabilities and stripped CAP_SETGID
> and CAP_SETUID for the netlink version. Whether or not the other capabilities
> are required should be discussed independently. Wondering why we have
> CAP_DAC_OVERRIDE in our capability capability set... That looks suspicious
> indeed.

Even with CAP_DAC_OVERRIDE stripped this change keeps openvpn
running with (much) more privileges than before. Is this
desirable?

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9


signature.asc
Description: PGP signature
--
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


Re: [Openvpn-devel] Summary of the community meeting (Wed, 18th Apr 2018)

2018-04-24 Thread Gert Doering
Hi,

On Tue, Apr 24, 2018 at 11:33:19AM +0200, Simon Matter wrote:
> I'm just wondering what happened to the proposed 2.4.6 release? Will it
> come anytime soon?

Windows driver signing did not work as planned.

Right now it looks like "release will happen today", stay tuned :-)

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 g...@greenie.muc.de


signature.asc
Description: PGP signature
--
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


Re: [Openvpn-devel] [PATCH 1/1] systemd: run openvpn with dedicated user

2018-04-24 Thread Christian Hesse
Simon Ruderich  on Tue, 2018/04/24 10:38:
> I haven't followed the netlink conversion in detail, so please
> tell me if the following was already discussed and I've just
> missed it.

No, it has not been discussed and needs a review.

> On Mon, Apr 23, 2018 at 11:28:13AM +0200, Christian Hesse wrote:
> >  if ENABLE_SYSTEMD
> > +if ENABLE_IPROUTE
> > +SYSTEMD_USER=root
> > +SYSTEMD_CAPS_OPTION=CapabilityBoundingSet
> > +SYSTEMD_CAPS_VALUES=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE
> > CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE +else
> > +SYSTEMD_USER=openvpn
> > +SYSTEMD_CAPS_OPTION=AmbientCapabilities
> > +SYSTEMD_CAPS_VALUES=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE
> > CAP_NET_RAW CAP_SYS_CHROOT CAP_DAC_OVERRIDE  
> 
> Are those capabilities dropped after initialization? If they are
> not this sounds like a serious issue as the process is basically
> running as root even if it's using another user (CAP_NET_ADMIN
> and CAP_DAC_OVERRIDE). Or am I missing something here?
>
> Regarding the netlink change in general: From what I understand
> it means that openvpn will always run with CAP_NET_ADMIN
> capabilities. Is this correct? If so, this sounds like it
> requires much more privileges than before for the normal
> operation (unless I misunderstand the current setup - to my
> knowledge it only requires a normal user after setup and no
> further capabilities or privileges once setup/connected).

The above snippet holds code for both, netlink and iproute2 versions.

The iproute2 version (that is what is used currently) uses systemd option
"CapabilityBoundingSet" to limit the capabilities to the given set. If
configured openvpn will drop privileges after setup.

With netlink and my patch on top we go the other way: The process runs (and
is started) with user "openvpn". To grant required privileges we use
systemd option "AmbientCapabilities" and give capabilities to the process.
The process keeps these capabilities, but that's a benefit: The process
survives a reconnect that requires configuration changes and shuts down
cleanly (takes down routes and addresses).

I do not agree that the process is running with root privileges. It has some
extra capabilities, but it can not kill processes, fork away and change
cgroups, etc.
IMHO that is what we want to achieve.

For this patch I took the current set of capabilities and stripped CAP_SETGID
and CAP_SETUID for the netlink version. Whether or not the other capabilities
are required should be discussed independently. Wondering why we have
CAP_DAC_OVERRIDE in our capability capability set... That looks suspicious
indeed.
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgpOWT9rvHVUt.pgp
Description: OpenPGP digital signature
--
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


Re: [Openvpn-devel] Summary of the community meeting (Wed, 18th Apr 2018)

2018-04-24 Thread Simon Matter
Hi,

I'm just wondering what happened to the proposed 2.4.6 release? Will it
come anytime soon?

Regards,
Simon

> Hi,
>
> Here's the summary of the IRC meeting.
> ---
>
> COMMUNITY MEETING
>
> Place: #openvpn-meeting on irc.freenode.net
> Date: Wednesday 18th Apr 2018
> Time: 11:30 CET (10:30 UTC)
>
> Planned meeting topics for this meeting were here:
>
> 
>
> The next meeting has not been scheduled yet.
>
> Your local meeting time is easy to check from services such as
>
> 
>
> SUMMARY
>
> cron2, dazo, mattock, ordex, plaisthos and syzzer participated in
> this meeting.
>
> --
>
> Discussed upcoming OpenVPN 2.4.6 release. It will contain what
> release/2.4 has now, plus one security fix which is under embargo. The
> tree will be pushed to buildbot tomorrow afternoon after which the
> release machinery starts for good.
>
> The 2.4.6 release will include an updated tap-windows6 driver with a
> small security fix. The fix will not get a CVE as exploitation requires
> local admin privileges and is not remotely exploitable. It was agreed
> that we should update PRODUCT_VERSION in tap-windows6 from 9,0,0,21 to
> 9,22,1,601. This means it will be in-line with the real version
> (9.22.1). The old, confusing PRODUCT_VERSION string seems to be just
> historic baggage brought over from tap-windows (i.e. the old NDIS 5
> driver).
>
> --
>
> Discussed unit testing the netlink code and in particular the msg()
> function. Noted that we have mock_msg.c already which does minimal
> logging, which renders it more suitable for unit testing.
>
> --
>
> Discussed the location of the next hackathon. One possibility is Lviv,
> Ukraine, where OpenVPN Inc. has a rather large team. It is easily
> accessible for EU citizens as no visa is required.
>
> ---
>
> Full chatlog attached.
>
> --
> Samuli Seppänen
> Community Manager
> OpenVPN Technologies, Inc
>
> irc freenode net: mattock
>
>
> --
> 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
>



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


Re: [Openvpn-devel] [PATCH 1/1] systemd: run openvpn with dedicated user

2018-04-24 Thread Simon Ruderich
Hello,

I haven't followed the netlink conversion in detail, so please
tell me if the following was already discussed and I've just
missed it.

On Mon, Apr 23, 2018 at 11:28:13AM +0200, Christian Hesse wrote:
>  if ENABLE_SYSTEMD
> +if ENABLE_IPROUTE
> +SYSTEMD_USER=root
> +SYSTEMD_CAPS_OPTION=CapabilityBoundingSet
> +SYSTEMD_CAPS_VALUES=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE 
> CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
> +else
> +SYSTEMD_USER=openvpn
> +SYSTEMD_CAPS_OPTION=AmbientCapabilities
> +SYSTEMD_CAPS_VALUES=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE 
> CAP_NET_RAW CAP_SYS_CHROOT CAP_DAC_OVERRIDE

Are those capabilities dropped after initialization? If they are
not this sounds like a serious issue as the process is basically
running as root even if it's using another user (CAP_NET_ADMIN
and CAP_DAC_OVERRIDE). Or am I missing something here?

Regarding the netlink change in general: From what I understand
it means that openvpn will always run with CAP_NET_ADMIN
capabilities. Is this correct? If so, this sounds like it
requires much more privileges than before for the normal
operation (unless I misunderstand the current setup - to my
knowledge it only requires a normal user after setup and no
further capabilities or privileges once setup/connected).

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9


signature.asc
Description: PGP signature
--
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