Re: [Openvpn-devel] [Openvpn-devel/users] Debugging Windows based server scripts

2021-02-18 Thread Selva Nair
Hi,

On Wed, Feb 17, 2021 at 5:38 PM tincanteksup  wrote:

> Hi,
>
> due to not being allowed to have scripts "echo data" to the log file
> under Windows, debugging scripts is next to impossible.
>
> I presume there are no compile time options to enable "echo" under Windows
> ?
>
> Could anybody provide me with a patch to enable "echo" just for the
> purpose of debugging ?
>
> I would like the patch to work for Openvpn 2.5
>
> I understand the risks and I am not distributing OpenVPN binaries.
>
> As justification I make these points:
>
> * Any large distributor of Openvpn binaries could make the change to
> enable "echo" under Windows.
>
> * That distributor could then abuse it as they please.
>
> * I am simply asking for help for the purpose of debugging Open Source
> Software made for the community.


If it's for debugging, why not redirect the output of the scripts? There
are several ways of doing this like:

(i) replace the script by a wrapper

@echo off
rem this wrapper calls the actual up_script_orig.bat
call up_script_orig.bat >up_script.log 2>&1
exit /b

(ii) move the script to a function and call it, redirecting o/p

@echo off
call :do_work >up_script.log 2>&1
exit /b

:do_work
@echo on
@rem the original script follows..


@rem end of script
@echo off
exit /b

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] Quote the domain name argument passed to the wmic command

2021-02-16 Thread selva . nair
From: Selva Nair 

It appears wmic needs domain names containing hyphens to
be quoted.

Trac #1375

Signed-off-by: Selva Nair 
---
 src/openvpnserv/interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 65bb106..5d5cbfe 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1104,7 +1104,7 @@ wmic_nicconfig_cmd(const wchar_t *action, const 
NET_IFINDEX if_index,
 }
 else
 {
-   fmt = L"wmic nicconfig where (InterfaceIndex=%ld) call %s %s";
+   fmt = L"wmic nicconfig where (InterfaceIndex=%ld) call %s \"%s\"";
 }
 
 size_t ncmdline = wcslen(fmt) + 20 + wcslen(action) /* max 20 for ifindex 
*/
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] rfc: mingw and the interactive service code

2021-01-27 Thread Selva Nair
Hi,

Starting version 8, mingw has started automatically setting
__USE_MINGW_ANSI_STDIO
= 1 under some feature-set conditions: for example, when _GNU_SOURCE is
defined or -std=C99, both of which are true in our case.

See: release notes at http://mingw-w64.org/doku.php

This causes several stdio functions to use internal ISO-C compliant
implementations (instead of those in msvcrt), and to break some of our s
wprintf family calls.

I see two options:
(i) force __USE_MINGW_ANSI_STDIO = 0 in configure.
(ii) Change all stdio function calls to be compatible building with and
without this macro defined. AFICT, the only change required would be to
replace %s and %S by %ls and %hs in some places -- mostly in interactive
service, one instance in tun.c

Any thoughts? I'm leaning towards option (ii).

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v3] Document common uses of 'echo' directive, re-enable logging for 'echo'.

2021-01-18 Thread Selva Nair
Hi.

Looks good to me.

The white- "space police" may object to the formatting in the logging
code (see below). Could be fixed at commit time.

On Mon, Jan 18, 2021 at 11:31 AM Gert Doering  wrote:
>
> The 'echo' command can be used to signal information to an OpenVPN
> GUI driving the openvpn core via management interface.  Which commands
> exists and their syntax has so far been mostly undocumented.
>
> Condense the long and good discussion between Selva Nair and
> Jonathan K. Bullard into doc/gui-notes.txt (initial draft from
> Jonathan, comments from Selva and Arne), with a pointer added
> to doc/management-notes.txt.
>
> See:
>
> https://sourceforge.net/p/openvpn/mailman/openvpn-users/thread/CAEsd45T%2Bd6FUJ9Po0KHwtHjfuL9Q2D-poG8yFtY45Qyh%2BtHjkg%40mail.gmail.com/#msg36136236
>
> and
>
> https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/CAKuzo_jPThhvXTJAtzhqVUVOLPW1VGu6h2jQhVsHicY8P2WRqA%40mail.gmail.com/#msg36141193
>
> for the details.
>
> Re-enable logging of 'echo' statements, but only for the particular
> class of messages starting with 'echo msg...'.
>
> v2:
>   incorporate feedback from Selva Nair, correct >ECHO examples
>
> v3:
>   add "msg*" support status for Windows GUI (11.22.0) and Android (Planned)
>
> Signed-off-by: Gert Doering 
> ---
>  doc/Makefile.am  |   2 +-
>  doc/gui-notes.txt| 370 +++
>  doc/management-notes.txt |  10 ++
>  src/openvpn/options.c|  15 +-
>  4 files changed, 389 insertions(+), 8 deletions(-)
>  create mode 100644 doc/gui-notes.txt
>
> diff --git a/doc/Makefile.am b/doc/Makefile.am
> index df2f54a3..e411f5f9 100644
> --- a/doc/Makefile.am
> +++ b/doc/Makefile.am
> @@ -15,7 +15,7 @@ MAINTAINERCLEANFILES = \
>  SUBDIRS = doxygen
>
>  dist_doc_DATA = \
> -   management-notes.txt
> +   management-notes.txt gui-notes.txt
>
>  dist_noinst_DATA = \
> README.plugins interactive-service-notes.rst \
> diff --git a/doc/gui-notes.txt b/doc/gui-notes.txt
> new file mode 100644
> index ..08270f07
> --- /dev/null
> +++ b/doc/gui-notes.txt
> @@ -0,0 +1,370 @@
> +Management Interface "echo" protocol
> +
>

snipped...

+COMMAND -- setenv
+-
+
+Syntax: setenv name value
+
+Sets an environment variable that will be available to the scripts run
+by the GUI.
+
+This will set environment variable "OPENVPN_name" to value "value" for
+the scripts run by the GUI. "name" is changed to "OPENVPN_name" to
+prevent overwriting sensitive variables such as PATH. Variables are
+set in the order received, with later values replacing earlier ones
+for the same "name".
+
+Names may include only alphanumeric characters and underscores. A
+"setenv" command with an invalid name will be ignored.
+
+Android: ??
+
+Tunnelblick: Planned.
+
+Windows OpenVPN GUI: supported since release v2.4.7 (GUI version v11.12.0)
+The variables set by "setenv" are merged with those for the process
+environment.  In case of duplicate names the one in the setenv list is
+chosen.
+

The extra blank line at the end of file is unnecessary (whitespace
error according to git).

> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index ff3954d5..36009f4f 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -5306,13 +5306,14 @@ add_option(struct options *options,
>  }
>  if (good)
>  {
> -#if 0
> -/* removed for now since ECHO can potentially include
> - * security-sensitive strings */
> -msg(M_INFO, "%s:%s",
> -pull_mode ? "ECHO-PULL" : "ECHO",
> -BSTR());
> -#endif
> +/* only message-related ECHO are logged, since other ECHOs
> + * can potentially include security-sensitive strings */
> +if (strncmp(p[1],"msg",3) == 0)

Missing space between function arguments.

> +{
> +msg(M_INFO, "%s:%s",
> +pull_mode ? "ECHO-PULL" : "ECHO",
> +BSTR());
> +}
>  #ifdef ENABLE_MANAGEMENT
>  if (management)
>  {
> --
> 2.26.2

I have only compile tested as the code change looks sane and simple.

Acked by: selva.n...@gmail.com

Thanks,

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Document common uses of 'echo' directive, re-enable logging for 'echo'.

2021-01-18 Thread Selva Nair
Hi,

On Mon, Jan 18, 2021 at 8:17 AM Gert Doering  wrote:
>
> There will be a v3, as I just added "Android: Planned" to all the
> msg stuff.
>
> Selva, which GUI version will be "the one with msg support"?  So I can
> have this fixed as well.

GUI is at 11.21.0 right now, this will be in 11.22.0 to be released
with 2.4.11 and 2.5.1 onwards, I suppose.

Thanks,

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] is it possible to store saved password in tpm instead of registry ?

2021-01-13 Thread Selva Nair
Hi,

The blob stored in the registry is encrypted by DPAPI and requires access
to the user's session to decrypt. No matter where the blob is stored, if an
attacker has access to the session, anything the GUI can read can be read
by the attacker too.

That said, if there is a well-defined API for protecting data using a
non-exportable key in TPM, respecting security boundaries, we could use it.
The blob has to be still stored in registry or Windows password store
(which also stores it in registry). TPM is useful for storing
non-exportable private keys. For exportable data, if at all possible and
space permits, I see little point in putting it in TPM.

DPAPI supports an app-specific salt, and we could have it wrapped by TPM to
add some extra protection but I would be wary of inventing our own schemes
like that.

Storing the certificate private key in TPM makes sense.

Selva

On Wed, Jan 13, 2021 at 1:31 PM Илья Шипицин  wrote:

>
>
> ср, 13 янв. 2021 г. в 22:01, Jan Just Keijser :
>
>> Hi,
>>
>> On 13/01/21 17:20, Илья Шипицин wrote:
>> > Hello,
>> >
>> > if user save password, it might be stolen from well known location
>> > (there are popular password stealers).
>> >
>> > in theory, is it possible to keep password in tpm ? will it prevent
>> > password from being stolen ?
>> >
>> in theory, yes, but as always, it depends on the circumstances.
>>
>> With TPM 1.2 you can only store a very limited amount of data in the TPM
>> chip; the (open source) implementation I have seen (tss, trousers) store
>>
>
> I meant openvpn-gui + user/password authentication + password is kept in
> registry encrypted by data protection api (not clear text, but might be
> decrypted and stolen easily).
>
> trousers is linux, right ?
>
>
>> a key in the TPM to scramble other data with; thus, you can encrypt a
>> private key or password with a key stored on the TPM and only if you
>> have the TPM will you be able to decrypt it.
>> I've never been particularly impressed with the security of this setup,
>> however, as trousers seems to suggest to store the actualy decryption
>> key in an environment variable...
>>
>> With TPM 2.0 you can store more data in the chip, including a full
>> private key. This makes it behave more like a regular PKCS#11 device,
>> where you store the private key, not the user password on it. Of course,
>> it will/should also be possible to store a user password on it.
>>
>> cheers,
>>
>> JJK
>>
>> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Document common uses of 'echo' directive, re-enable logging for 'echo'.

2020-12-30 Thread Selva Nair
Hi,

Some small corrections to the proposed doc below

On Fri, Dec 25, 2020 at 2:28 PM Gert Doering  wrote:

> The 'echo' command can be used to signal information to an OpenVPN
> GUI driving the openvpn core via management interface.  Which commands
> exists and their syntax has so far been mostly undocumented.
>
> Condense the long and good discussion between Selva Nair and
> Jonathan K. Bullard into doc/gui-notes.txt (initial draft from
> Jonathan, comments from Selva and Arne), with a pointer added
> to doc/management-notes.txt.
>
> See:
>
>
> https://sourceforge.net/p/openvpn/mailman/openvpn-users/thread/CAEsd45T%2Bd6FUJ9Po0KHwtHjfuL9Q2D-poG8yFtY45Qyh%2BtHjkg%40mail.gmail.com/#msg36136236
>
> and
>
>
> https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/CAKuzo_jPThhvXTJAtzhqVUVOLPW1VGu6h2jQhVsHicY8P2WRqA%40mail.gmail.com/#msg36141193
>
> for the details.
>
> Re-enable logging of 'echo' statements, but only for the particular
> class of messages starting with 'echo msg...'.
>
> Signed-off-by: Gert Doering 
> ---
>  doc/Makefile.am  |   2 +-
>  doc/gui-notes.txt| 363 +++
>  doc/management-notes.txt |  10 ++
>  src/openvpn/options.c|  15 +-
>  4 files changed, 382 insertions(+), 8 deletions(-)
>  create mode 100644 doc/gui-notes.txt
>
> diff --git a/doc/Makefile.am b/doc/Makefile.am
> index df2f54a3..e411f5f9 100644
> --- a/doc/Makefile.am
> +++ b/doc/Makefile.am
> @@ -15,7 +15,7 @@ MAINTAINERCLEANFILES = \
>  SUBDIRS = doxygen
>
>  dist_doc_DATA = \
> -   management-notes.txt
> +   management-notes.txt gui-notes.txt
>
>  dist_noinst_DATA = \
> README.plugins interactive-service-notes.rst \
> diff --git a/doc/gui-notes.txt b/doc/gui-notes.txt
> new file mode 100644
> index ..9715e8f6
> --- /dev/null
> +++ b/doc/gui-notes.txt
> @@ -0,0 +1,363 @@
> +Management Interface "echo" protocol
> +
>
> +
> +THIS IS A PRELIMINARY VERSION OF THIS DOCUMENT. ALL INFORMATION IN IT
> +IS SUBJECT TO CHANGE.
>
> +
> +
> +
> +CONTENTS
> +THE OPENVPN --ECHO OPTION
> +ENVIRONMENT COMMAND
> +MESSSAGE COMMANDS
> +PASSWORD COMMANDS
> +QUOTING
> +COMMMAND DETAILS
> +
> +
>
>
...snipped..


> +===
> +QUOTING
> +===
> +
> + * In a configuration file, the rest of the line is parsed into
> +separate arguments  and then 'echo' and the arguments are passed, each
> +separated by a single space, through the management interface. For
> +example:
> +
> +echo argument1 argument2
> +echo" argument1  argument2"
> +
> +will be sent through the management interface as
> +
> +>ECHO:timestamp,argument1,argument2
> +>ECHO:timestamp, argument1, argument2
>

Should be
 >ECHO:timestamp,argument1 argument2
 >ECHO:timestamp, argument1 argument2

(Only a comma after timestamp, no commas added between words)

+
> + * In a command line option, the single argument following "--echo" is
> +parsed similarly, so
> +
> +--echo   "argument1 argument2"
>

Remove quotes to match with example above


> +--echo"'argument1 argument2'"
>

Extra single quotes above are probably not intended.


> +
> +will be sent through the management interface as
> +
> +>ECHO:timestamp,argument1,argument2


replace comma after argument1 by space


> +>ECHO:timestamp, argument1, argument2


remove comma after argument1


> +
> + * In a "push" option in a server configuration file, the single
> +option following "push" is parsed similarly, so
> +
> +push "echo argument1 argument2 argument3   argument4"
> +push echo "'argument1 argument2 argument3   argument4'"
>

The opening quote should move to before echo:

 push "echo 'argument1 argument2 argument3   argument4'"


> +will be sent through the management interface as
> +
> +>ECHO:timestamp,argument1,argument2,argument3 argument4
>

replace commas after first one by space


> +>ECHO:timestamp, argument1,argument2 argument3   argument4
>

replace comma after argument1 by space


.. snipped...


> +COMMAND -- msg
> +--
> +
> +Syntax: msg text
> +
> +The text is appended to any previous text from "msg" or "msg-n"
> +commands, and a newline is appended after that.
> +
> +A trailing newline will be re

Re: [Openvpn-devel] wanted: mechanism to send text messages to client

2020-12-25 Thread Selva Nair
Hi

Thanks for the comments.

On Fri, Dec 25, 2020 at 3:03 PM Gert Doering  wrote:

> Hi,
>
> On Sun, Dec 20, 2020 at 07:31:42PM -0500, Selva Nair wrote:
> > Here is the link again.
> > https://github.com/selvanair/openvpn-gui/releases/tag/v11-echo-msg
> > I got no feedback then nor now.
>
> I have stared at the code a bit, and it seems to make sense.  The part
> "store a digest + timestamp, and avoid repeating the same message for
>  hours" is a good idea.
>
> I can see that "echo_msg_clear()" has a bool parameter to clear the
> message history but I can not see a call with "true" - what did you
> have in mind?  A button to clear the messages history?  Or "if changing
> to a different profile"?
>

It has been a while...

The history needs to be cleared to free up memory when the thread is
terminated on disconnect. Looks like a bug if it's not done during cleanup.
That said, I see that I have newer versions of this echo-msg branch (upto
v4) in my local repo and the latest includes persisting the history in
registry and clearing before disconnect.


> I have not tested the binary.
>

Don't. While we are in this blessed "One True Echo" state, let me rebase
the latest version, do some quick tests and post it as a PR for review &
comments.

Have to do that before I again get stuck in procrastination and the desire
to add more glitter like urls in message text etc..

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] wanted: mechanism to send text messages to client

2020-12-25 Thread Selva Nair
Hi,

Merry Christmas!

On Wed, Dec 23, 2020 at 6:15 AM Jan Just Keijser  wrote:

> On 21/12/20 18:22, Selva Nair wrote:
>
>
>
> On Mon, Dec 21, 2020 at 2:04 AM Gert Doering  wrote:
>
>> Hi,
>>
>> On Sun, Dec 20, 2020 at 07:31:42PM -0500, Selva Nair wrote:
>> > I thought we already went through this when we discussed the proposed
>> "echo
>> > msg" in considerable detail 3 years ago.
>>
>> Yeah, sorry.  Seems I got distracted and forgot all about the discussed
>> "solution space", and just remembered the itch.
>>
>> I'm sure the thread is still sitting in my mailbox... will go looking for
>> it today.
>>
>
> For those who have lost the original threads:
>
>
> https://sourceforge.net/p/openvpn/mailman/openvpn-users/thread/CAEsd45T%2Bd6FUJ9Po0KHwtHjfuL9Q2D-poG8yFtY45Qyh%2BtHjkg%40mail.gmail.com/#msg36136236
>
>
> https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/CAKuzo_jPThhvXTJAtzhqVUVOLPW1VGu6h2jQhVsHicY8P2WRqA%40mail.gmail.com/#msg36141193
>
> That was in Nov-Dec 2017. Actually, I was also thinking of reviving this
> only the other day when intimating users about some updates came up..
> Somehow such itches re-surface at the end of the year :)
>
> sorry to chip on so late, but at some point we did have a way to push env
> vars to the client (back in v2.2 at least):
>
> inside a client-connect script you can/could do
>
> echo "push \"setenv-safe MSG1 'hello'\"" > $1
> echo "push \"setenv-safe MSG2 'download version 2.5 please'\"" >> $1
>

setenv-safe is not passed on to the UI/GUI, so not very useful for sending
messages to the user. In fact we already use echo to send env variables to
the UI -- OpenVPN-GUI interprets
push "echo setenv foo bar" and passes it in the env to scripts run by the
GUI.
See https://github.com/OpenVPN/openvpn-gui/pull/200
In one of our setups we use it to pass file server names, SMB shares to map
etc.

Among options that already exist, "echo" is the best candidate for this
purpose, and it's designed for sending "commands" (which could be messages)
from server to client side UI. It's use only requires an agreement on the
syntax so that all UI authors can code to interpret them consistently.

I find it hard to imagine why everyone seems to be avoiding "echo" and
trying to find alternatives.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] wanted: mechanism to send text messages to client

2020-12-21 Thread Selva Nair
Hi,

On Mon, Dec 21, 2020 at 3:27 PM Arne Schwabe  wrote:

> Am 21.12.20 um 20:11 schrieb Gert Doering:
> > Hi,
> >
> > On Mon, Dec 21, 2020 at 06:24:36PM +, Greg Cox wrote:
> >> My contention is, a VPN client has enough information from its own
> certs to
> >> know when its certs are expired and thus not going to work (Yes, there's
> >> plenty of OTHER reasons a connection can fail, but in a well designed
> >> setup, the user's certs will go stale long before the server).  It tells
> >> you this problem in the logs, which folks never read.
> >
> > We consciously decided to make this not more prominent (so, warning only,
> > not error) because the client's machine's time might be wrong - and
> > ultimately it's the server's notion of time that decides if the cert
> > is valid or not.  So this is a hint, but not a "IT WILL NOT WORK!" hard
> > error.
> >
> >> If the software were
> >> to contain a mechanism to make certain failure cases automatically more
> >> prominent, particularly for 'simple' users who have GUI clients, it'll
> be a
> >> big win for supportability on larger installs.
> >
> > This is indeed getting into philosophy... we do send different types of
> > AUTH_FAILED today (like, for token expired).  Maybe we could send an
> > "AUTH_FAILED,cert expired" and have the client display this?
> >
> > (I admit that I'm neither an expert on AUTH_FAILED message, nor on
> > "what is the client doing on variations of it", nor on "what *should*
> > be the expected outcome?".  Selva, Arne will know more).
>
> It is easy to add that message, however the question is if we want to.
> Sending different AUTH_FAILED message also leaks information. Especially
> with authentication you don't want to give an attacker an idea how they
> get before failing the authentication. I.e. if you send User disable,
> certificate expired, account not allow to use VPN etc. an attacker gets
> information about the account/profile he using to connect.
>
> So with these AUTH_FAILED codes you have to be very careful not to
> accidently leak information. I.e. AUTH_FAILED, cert expired happens only
> if user/pass is right/wrong, otherwise you get a normal AUTH_FAILED.
>
> HOWEVER, on the client side. We can transform a normal AUTH_FAILED into
> an AUTH_FAILED, server gave no reason, [client certificate is expired]
> or something like that.
>

We already warn on the client if the certificate has expired and warnings
show up in red at least in OpenvPN-GUI. Sure, UI's can add more bold-face
warnings and popups showing probable reasons, but better leave the core as
is.

To chime in with Arne, a client presenting an invalid cert (expired, in CRL
or otherwise bogus) has to be treated as rogue and should not be given any
additional feedback from the server on the reason for failure.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] wanted: mechanism to send text messages to client

2020-12-21 Thread Selva Nair
On Mon, Dec 21, 2020 at 2:04 AM Gert Doering  wrote:

> Hi,
>
> On Sun, Dec 20, 2020 at 07:31:42PM -0500, Selva Nair wrote:
> > I thought we already went through this when we discussed the proposed
> "echo
> > msg" in considerable detail 3 years ago.
>
> Yeah, sorry.  Seems I got distracted and forgot all about the discussed
> "solution space", and just remembered the itch.
>
> I'm sure the thread is still sitting in my mailbox... will go looking for
> it today.
>

For those who have lost the original threads:

https://sourceforge.net/p/openvpn/mailman/openvpn-users/thread/CAEsd45T%2Bd6FUJ9Po0KHwtHjfuL9Q2D-poG8yFtY45Qyh%2BtHjkg%40mail.gmail.com/#msg36136236

https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/CAKuzo_jPThhvXTJAtzhqVUVOLPW1VGu6h2jQhVsHicY8P2WRqA%40mail.gmail.com/#msg36141193

That was in Nov-Dec 2017. Actually, I was also thinking of reviving this
only the other day when intimating users about some updates came up..
Somehow such itches re-surface at the end of the year :)

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] wanted: mechanism to send text messages to client

2020-12-20 Thread Selva Nair
Hi,

On Sun, Dec 20, 2020 at 5:38 PM Gert Doering  wrote:

> Hi,
>
> On Sun, Dec 20, 2020 at 04:00:13PM +0100, Arne Schwabe wrote:
> > > ... and the client would then either print this on the console
> > > (if !management) or dump it to management, where the GUI/Tunnelblick
> > > could pick it up and create a popup window.
> > >
> > See --echo.  That is basically what you desribe as info-msg in your mail
>
> While echo sounds as if it would do what I want, it doesn't.
>
> Echo is "something magic" - it is never printed by the 2.x client, it
> is not even logged.  And it's not displayed by the GUIs either, but
> instead it can be used to "do things"...
>

The "not  getting logged" concern is easy to fix.

What echo does is to get a message from the server to the client-side UI.
Currently GUI's do not display them because so far the only meanings we
have assigned to echo commands are as directives like save-password or
forget-password.

But that doesn't mean we can't use them for messages.And, that's exactly
what we discussed 3 years ago -- see the draft implementation from Nov 2017
that I posted. Here is the link again.
https://github.com/selvanair/openvpn-gui/releases/tag/v11-echo-msg
I got no feedback then nor now.


> So less "echo to user" but "send this string to the management interface
> and make the mgmt client do something magic"...
>

A message to the user can be delivered in a useful fashion only if there is
a UI. The core itself can only write the message to log which may not be
seen in time, or to the console if one exists. And, IMO, any decent UI of
openvpn should use the management interface -- almost all do (except NM?).

I thought we already went through this when we discussed the proposed "echo
msg" in considerable detail 3 years ago.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] wanted: mechanism to send text messages to client

2020-12-20 Thread Selva Nair
Hi,

On Sun, Dec 20, 2020 at 5:55 AM Gert Doering  wrote:

> Hi,
>
> I find myself looking for a mechanism by which I could send informational
> messages ("your cert expires in two weeks, go refresh!" - "your openvpn
> client needs an upgrade") from the openvpn server to incoming clients.
>
> Of course this should work with all connecting clients, that is, "text
> clients", windows GUI, Tunnelblick, iOS Connect, Android.
>
> As far as I am aware, there is no such mechanism today.
>

We once had extensive discussions on using echo for this. As echo
commands are up to the UI to interpret, adding new one's does
not require any changes in the core.

I had posted a draft implementation for OpenVPN-GUI (not sure it still
works, though) along with some details of the message syntax:
https://github.com/selvanair/openvpn-gui/releases/tag/v11-echo-msg

IIRC, Jonathan had written up documentation for the proposed syntax.
I don't have links to the relevant mails at hand, but should be in the
archives.

It may be best to resurrect that effort.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Fix too early argv freeing when registering DNS

2020-12-15 Thread Selva Nair
Hi

On Tue, Dec 15, 2020 at 12:37 PM Gert Doering  wrote:

> Hi,
>
> On Tue, Dec 15, 2020 at 06:16:00PM +0100, Domagoj Pensa wrote:
> > When registering DNS on Windows, argv is freed after being used in first
> > ipconfig command (/flushdns).
> >
> > Then same argv is used uninitialized in next ipconfig command
> (/registerdns)
> > causing heap exception and subprocess crash.
> >
> > As a consequence second command is never executed and locked netcmd
> > semaphore is not cleanly released.
> >
> > Removing argv freeing between ipconfig calls solves the problem.
>
> Oh!  Yes, now with your patch, this is very obvious - there is a trac
> ticket (so when I merge this, I'll add the trac ticket number to the
> commit message) but it sort of puzzled Selva and me, because the
> code "looked sane".
>

Indeed, I went looking at wrong places.

Thanks,

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 2/3] netsh: Clear existing IPv6 DNS servers before configuring new ones

2020-09-28 Thread Selva Nair
Hi,

On Thu, Sep 24, 2020 at 4:57 AM Lev Stipakov  wrote:

> Hi,
>
> > When there are no IPv6 DNS published, the adapter state is not
> > sanitized and might contain IPv6 DNS server from a previous session.
>
> In this case, shouldn't the "set dns" call below overwrite the previous
> value?


> > netsh_ifconfig_options() clears DNS servers for IPv4 already.
>
> Agreed, let's do it for consistency.
>
> >   * The list of dns servers currently set on the interface
> >   * are cleared first.
>
> Either existing comment lied or "implicit clear" was assumed because
> of the "set dns" call.
>

The clearing happened only if at least one DNS address was specified using
--dhcp-option DNS.


> But now it is fine, since we have real clearing.
>

Now that this has been merged, should we make DNS settings using iservice
consistent with this? Currently, when using the service, we do not touch
any existing dns servers if none are specified using --dhcp-option. Same
approach was taken in my patch for setting the domain suffix. The original
logic was that any statically set values should not be overwritten unless
explicitly asked for. At the same time, we do delete all addresses for v4
(not v6) while closing tun.

But, if we want to ensure a clean state for the adapter, as argued here, we
should be clearing current values regardless of whether new one's are being
set or not.

Selva


> Stared and the code, built and tested on MSVC/Win10.
>
> Acked-by: Lev Stipakov 
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] Improve documentation of --username-as-common-name

2020-09-27 Thread selva . nair
From: Selva Nair 

Trac #1079

Signed-off-by: Selva Nair 
---
 doc/man-sections/server-options.rst | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/doc/man-sections/server-options.rst 
b/doc/man-sections/server-options.rst
index c0b22a5..4b649b1 100644
--- a/doc/man-sections/server-options.rst
+++ b/doc/man-sections/server-options.rst
@@ -668,9 +668,15 @@ fast hardware. SSL/TLS authentication must be used in this 
mode.
   ``--max-routes-per-client``
 
 --username-as-common-name
-  For ``--auth-user-pass-verify`` authentication, use the authenticated
-  username as the common name, rather than the common name from the client
-  cert.
+  Use the authenticated username as the common-name, rather than the
+  common-name from the client certificate. Requires that some form of
+  auth-user-pass verification is in effect. As the replacement happens after
+  auth-user-pass verification, the verification script or plugin will still
+  receive the common-name from the certificate.
+
+  The common_name environment variable passed to scripts and plugins invoked
+  after authentication (e.g, client-connect script) and file names parsed in
+  client-config directory will match the username.
 
 --verify-client-cert mode
   Specify whether the client is required to supply a valid certificate.
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2] Set DNS Domain using iservice

2020-09-25 Thread selva . nair
From: Selva Nair 

Use wmic instead of directly editing the registry
as the former does not take full effect unless the dns
client service is restarted.

Editing the registry appears to work erratically depending
on whether its followed with a dchp renew or ipconfig /registerdns
etc.

DOMAIN-SEARCH is not handled here as wmic only supports
setting the global search list which will over-ride all
interface specific values.  Editing the registry directly
combined with a wmic command to reset the global SearchList
is an option that could be considered in a separate patch.

Trac # 1209, 1331

v2 changes
- Separate DNS domain setting from DNS server setting and call
  only once either during IPv4 processing or IPv6 processing
  if the former is not active. (file changed: tun.c)
- Null terminate domain and interface_name received from the
  client. (file changed: interactive.c)
  Its done using a const cast-away of msg in a limited scope.
  Not pretty, but alternatives are no better.

Signed-off-by: Selva Nair 
---
Note that the design of interactive service is such that the
msg-channel client is always a designated (i.e. trusted) executable.
So strigs received from it can be trusted. The null-termination above
is out of an abundance of caution. This is unlike those received
on the service pipe which cannot be trusted.

 src/openvpn/tun.c |  68 
 src/openvpnserv/interactive.c | 140 +-
 2 files changed, 205 insertions(+), 3 deletions(-)

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 80ae695..9eeaed0 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -149,6 +149,61 @@ out:
 }
 
 static bool
+do_dns_domain_service(bool add, const struct tuntap *tt)
+{
+bool ret = false;
+ack_message_t ack;
+struct gc_arena gc = gc_new();
+HANDLE pipe = tt->options.msg_channel;
+
+if (!tt->options.domain) /* no  domain to add or delete */
+{
+return true;
+}
+
+/* Use dns_cfg_msg with addr_len = 0 for setting only the DOMAIN */
+dns_cfg_message_t dns = {
+.header = {
+(add ? msg_add_dns_cfg : msg_del_dns_cfg),
+sizeof(dns_cfg_message_t),
+0
+},
+.iface = { .index = tt->adapter_index, .name = "" },
+.domains = "",  /* set below */
+.family = AF_INET,  /* unused */
+.addr_len = 0   /* add/delete only the domain, not DNS servers */
+};
+
+strncpynt(dns.iface.name, tt->actual_name, sizeof(dns.iface.name));
+strncpynt(dns.domains, tt->options.domain, sizeof(dns.domains));
+/* truncation of domain name is not checked as it can't happen
+ * with 512 bytes room in dns.domains.
+ */
+
+msg(D_LOW, "%s dns domain on '%s' (if_index = %d) using service",
+(add ? "Setting" : "Deleting"), dns.iface.name, dns.iface.index);
+if (!send_msg_iservice(pipe, , sizeof(dns), , "TUN"))
+{
+goto out;
+}
+
+if (ack.error_number != NO_ERROR)
+{
+msg(M_WARN, "TUN: %s dns domain failed using service: %s [status=%u 
if_name=%s]",
+(add ? "adding" : "deleting"), strerror_win32(ack.error_number, 
),
+ack.error_number, dns.iface.name);
+goto out;
+}
+
+msg(M_INFO, "DNS domain %s using service", (add ? "set" : "deleted"));
+ret = true;
+
+out:
+gc_free();
+return ret;
+}
+
+static bool
 do_dns_service(bool add, const short family, const struct tuntap *tt)
 {
 bool ret = false;
@@ -164,6 +219,7 @@ do_dns_service(bool add, const short family, const struct 
tuntap *tt)
 return true;
 }
 
+/* Use dns_cfg_msg with domain = "" for setting only the DNS servers */
 dns_cfg_message_t dns = {
 .header = {
 (add ? msg_add_dns_cfg : msg_del_dns_cfg),
@@ -1100,6 +1156,11 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, 
int tun_mtu,
 }
 do_dns_service(true, AF_INET6, tt);
 do_set_mtu_service(tt, AF_INET6, tun_mtu);
+/* If IPv4 is not enabled, set DNS domain here */
+if (!tt->did_ifconfig_setup)
+{
+   do_dns_domain_service(true, tt);
+}
 }
 else
 {
@@ -1485,6 +1546,7 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, 
int tun_mtu,
 {
 do_address_service(true, AF_INET, tt);
 do_dns_service(true, AF_INET, tt);
+do_dns_domain_service(true, tt);
 }
 else if (tt->options.ip_win32_type == IPW32_SET_NETSH)
 {
@@ -6761,6 +6823,11 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
 }
 else if (tt->options.msg_channel)
 {
+/* If IPv4 is not enabled, delete DNS domain here */
+if (!tt->did_ifconfig_setup)
+{
+do_dns_domain_service(

Re: [Openvpn-devel] [PATCH] Set DNS Domain using iservice

2020-09-25 Thread Selva Nair
Hi

Thanks for the review.

On Fri, Sep 25, 2020 at 5:24 AM Lev Stipakov  wrote:

> Hi,
>
> > Note: this will set the domain twice if both v4 and v6 DNS
> > servers are defined. It cant hurt, but could be avoided by
> > making the domain setting a separate call from the DNS
> > server setting.
>
> I think we should do that - there is no reason to make expensive call
> twice:
>

Separating the two is easy, but not sure how hard it would be to
avoid calling it twice. With the ipv6-only feature, ipv4 setting is no
longer guaranteed and elsewhere in the code we do some tasks twice: once
for v4 and once for v6 instead of keeping a tab.

I'll take a closer look.


>
> $ time wmic nicconfig where \(InterfaceIndex=38\) call SetDNSDomain
> vpntest.lev
> Executing
> (\\LAPTOP-4L3N7KFS\ROOT\CIMV2:Win32_NetworkAdapterConfiguration.Index=17)->SetDNSDomain()
> Method execution successful.
> Out Parameters:
> instance of __PARAMETERS
> {
> ReturnValue = 0;
> };
>
> real0m0,236s
> user0m0,000s
> sys 0m0,015s

Besides, DOMAIN could also be set by DHCP - there is no point
> to do it via service in this case.
>

If DHCP is in use, do_dns_service() will not get called and the DOMAIN
setting will not be delegated to the service even with this patch.


> > +strncpy(dns.domains, tt->options.domain, _countof(dns.domains));
> > +   /* truncation of domain name is not checked as it cant happen
> > +* with 512 bytes room in dns.domains.
> > +*/
>
> Mix of tabs and whitespaces.
>

Will fix.


>
> > +/* comma separated list must be enclosed in parenthesis */
>
> We don't pass lists yet, is this for future SearchDomains support?
>

Yes, kind of...

I have been playing with this for a while and was motivated because wmic
does support SetDNSSuffixSearchOrder. But works only for the global
setting, not per interface  -- it errors if a where clause is added.

I left the code in there as it may get supported in future. Also it took
awhile for me to figure out what format is needed for lists vs single
objects from the command line.


>
> > +if (msg->domains[0]) {
> > +err = SetDNSDomain(wide_name, msg->domains, lists);
> > +}
>
> msg->domains comes from (unprivileged) client and might
> not be NULL terminated. Shall we do something like
>
> msg->domains[sizeof(msg->domains) - 1] = '\0';
>
> Same for interface_t::name. Or am I missing something?
>

My mistake. Will fix. Also our own safer strncpynt instead of strncpy in
do_dns_service().

Thanks,

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] Set DNS Domain using iservice

2020-09-24 Thread selva . nair
From: Selva Nair 

Use wmic instead of directly editing the registry
as the former does not take full effect unless the dns
client service is restarted.

Editing the registry appears to work erratically depending
on whether its followed with a dchp renew or ipconfig /registerdns
etc.

DOMAIN-SEARCH is not handled here as wmic only supports
setting the global search list which will over-ride all
interface specific values.  Editing the registry directly
combined with a wmic command to reset the global SearchList
is an option that could be considered in a separate patch.

Trac # 1209, 1331

Signed-off-by: Selva Nair 
---
Note: this will set the domain twice if both v4 and v6 DNS
servers are defined. It cant hurt, but could be avoided by
making the domain setting a separate call from the DNS
server setting.

 src/openvpn/tun.c |  21 +--
 src/openvpnserv/interactive.c | 133 +-
 2 files changed, 147 insertions(+), 7 deletions(-)

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 80ae695..b681905 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -159,7 +159,7 @@ do_dns_service(bool add, const short family, const struct 
tuntap *tt)
 int addr_len = add ? len : 0;
 const char *ip_proto_name = family == AF_INET6 ? "IPv6" : "IPv4";
 
-if (addr_len == 0 && add) /* no addresses to add */
+if (addr_len == 0 && !tt->options.domain && add) /* no addresses or domain 
to add */
 {
 return true;
 }
@@ -199,9 +199,21 @@ do_dns_service(bool add, const short family, const struct 
tuntap *tt)
 dns.addr[i].ipv4.s_addr = htonl(tt->options.dns[i]);
 }
 }
+if (tt->options.domain)
+{
+strncpy(dns.domains, tt->options.domain, _countof(dns.domains));
+   /* truncation of domain name is not checked as it cant happen
+* with 512 bytes room in dns.domains.
+*/
+msg(D_LOW, "%s dns domain on '%s' (if_index = %d) using service",
+(add ? "Setting" : "Deleting"), dns.iface.name, dns.iface.index);
+}
 
-msg(D_LOW, "%s %s dns servers on '%s' (if_index = %d) using service",
-(add ? "Setting" : "Deleting"), ip_proto_name, dns.iface.name, 
dns.iface.index);
+if (addr_len > 0 || !add)
+{
+msg(D_LOW, "%s %s dns servers on '%s' (if_index = %d) using service",
+(add ? "Setting" : "Deleting"), ip_proto_name, dns.iface.name, 
dns.iface.index);
+}
 
 if (!send_msg_iservice(pipe, , sizeof(dns), , "TUN"))
 {
@@ -216,7 +228,8 @@ do_dns_service(bool add, const short family, const struct 
tuntap *tt)
 goto out;
 }
 
-msg(M_INFO, "%s dns servers %s using service", ip_proto_name, (add ? "set" 
: "deleted"));
+msg(M_INFO, "%s dns servers %s%s using service", ip_proto_name,
+   (tt->options.domain? "and domain " : ""), (add ? "set" : 
"deleted"));
 ret = true;
 
 out:
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 207cc4a..b7f144d 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -91,6 +91,7 @@ typedef enum {
 block_dns,
 undo_dns4,
 undo_dns6,
+undo_domain,
 _undo_type_max
 } undo_type_t;
 typedef list_item_t *undo_lists_t[_undo_type_max];
@@ -564,6 +565,24 @@ InterfaceLuid(const char *iface_name, PNET_LUID luid)
 return status;
 }
 
+static DWORD
+ConvertInterfaceNameToIndex(const wchar_t *ifname, NET_IFINDEX *index)
+{
+   NET_LUID luid;
+   DWORD err;
+
+   err = ConvertInterfaceAliasToLuid(ifname, );
+   if (err == ERROR_SUCCESS)
+   {
+   err = ConvertInterfaceLuidToIndex(, index);
+   }
+   if (err != ERROR_SUCCESS)
+   {
+   MsgToEventLog(M_ERR, L"Failed to find interface index for <%s>", 
ifname);
+   }
+   return err;
+}
+
 static BOOL
 CmpAddress(LPVOID item, LPVOID address)
 {
@@ -1057,6 +1076,53 @@ out:
 return err;
 }
 
+/**
+ * Run command: wmic nicconfig (InterfaceIndex=$if_index) call $action ($data)
+ * @param  if_index"index of interface"
+ * @param  action  e.g., "SetDNSDomain"
+ * @param  datadata if required for action
+ * - a single word for SetDNSDomain, empty or NULL to 
delete
+ * - comma separated values for a list
+ */
+static DWORD
+wmic_nicconfig_cmd(const wchar_t *action, const NET_IFINDEX if_index,
+   const wchar_t *data)
+{
+DWORD err = 0;
+wchar_t argv0[MAX_PATH];
+wchar_t *cmdline = NULL;
+int timeout = 1; /* in msec */
+
+swprintf(argv0, _countof(argv0), L"%s\\%s", get_win_sys_path(), 
L"wbem\\wmic.exe");
+argv0[_countof(argv0) - 1] = L'\0';
+

Re: [Openvpn-devel] [PATCH] Allow --dhcp-option in config file when windows-driver is wintun

2020-09-15 Thread Selva Nair
Hi

On Tue, Sep 15, 2020 at 2:48 AM Lev Stipakov  wrote:

> Hi,
>
> > -msg(M_USAGE, "--dhcp-options requires --ip-win32 dynamic or
> adaptive");
> > +msg(M_USAGE, "--dhcp-option requires --ip-win32 dynamic or
> adaptive");
>
> Nice, this typo has been there since at least 2005.
>
> It looks like that warning is not quite correct - for example,
> DNS can be set via iservice or netsh, which means that
>
> --ip-win32 netsh
> --dhcp-option DNS 8.8.8.8
>
> is a valid combination regardless of driver type. On the other hand,
> NBDD/NTP are set via DHCP.
>
> It would be good to make the warning condition more fine-grained, but
> this patch is good enough as it is, since it fixes important case - specify
> DNS in the ovpn profile when using wintun.
>

We could deprecate --ip-win32 as a user option. Default to dynamic or
adaptive, automatically fail-over to alternate methods or change it
internally as required for wintun etc.

And work towards supporting more dhcp-options when dhcp is not possible --
using iservice, API, netsh etc.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] Allow --dhcp-option in config file when windows-driver is wintun

2020-09-14 Thread selva . nair
From: Selva Nair 

When wintun is in use we mutate ip_win32_type to NETSH
and then complain that ip-win32 option should be dynamic or adaptive
if any --dhcp-option directive is present in the config file. This
causes a fatal error.

How to reproduce: specify a --dhcp-option in the config and change the
--windows-driver to wintun.

Fix this behaviour. A typo in the message is also corrected.

Signed-off-by: Selva Nair 
---
 src/openvpn/options.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 8bf82c5..4b22d3d 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2181,10 +2181,11 @@ options_postprocess_verify_ce(const struct options 
*options, const struct connec
 }
 
 if (options->tuntap_options.dhcp_options
+&& options->windows_driver != WINDOWS_DRIVER_WINTUN
 && options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ
 && options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE)
 {
-msg(M_USAGE, "--dhcp-options requires --ip-win32 dynamic or adaptive");
+msg(M_USAGE, "--dhcp-option requires --ip-win32 dynamic or adaptive");
 }
 
 if (options->windows_driver == WINDOWS_DRIVER_WINTUN && dev != 
DEV_TYPE_TUN)
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] problem with beta3 and wintun

2020-09-11 Thread Selva Nair
Hi

On Fri, Sep 11, 2020 at 1:45 PM RafaeHil Gava  wrote:

> Hi Selva,
>
> I was wondering if it's possible to detect UAC during the installation.
> What do you think?
>

There are many ways of running the GUI as admin and all involve some
deliberate action on the part of the user. The best we can do is to warn
them that the GUI is running as admin and its not recommended. That will
also cover the initial launch of GUI on install with UAC disabled or from
an elevated cmdline.

Disabling UAC can be legit too if the admin knows what they are doing --
like, all users have limited privileges and admins login only to do tasks
requiring elevation etc.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] problem with beta3 and wintun

2020-09-11 Thread Selva Nair
Hi,

On Fri, Sep 11, 2020 at 1:58 AM Gert Doering  wrote:

> Hi,
>
> On Thu, Sep 10, 2020 at 06:10:17PM -0700, Marvin wrote:
> > To All 3,
> > Thank you with your help I found the issue. UAC was disabled in the
> > registry on this image.  IIRC we had trouble updating some software by
> > automated script and turning UAC off was required.
> >
> > After re-enabling UAC, wintun started normally.
>
> Cool, thanks for digging into this and reporting back.
>
> Selva, is there any reasonable way to detect this?  Or do we just go
> for "we always use the iservice if it is running, no matter what
> privs the GUI is running with"?


I think TokenElevationType will indicate whether using split token (UAC
enabled) or not.

Personally, I would like to show a warning when the GUI is started with
privileges but some users may not like it. And we could make iservice a
requirement for the GUI. The service is mature enough now and has become a
necessity with wintun support.

Although we don't have any reason not to connect to named pipes as admin in
post-vista systems, the idea that the GUI should be run without elevated
privileges appeals to me. Unless the user has taken deliberate steps to
force running it as admin, it just works that way. Disabling UAC and then
log in as admin for day to day work is like using root for browsing the
web.

That said, I won't object to a patch that removes the restriction when the
runtime version is Win7 and newer.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Fix client's poor man NCP fallback

2020-09-10 Thread Selva Nair
Hi

On Thu, Sep 10, 2020 at 3:10 AM Marvin Adeff  wrote:

> Selva,
>
> Please allow me to back up a moment and restate this:
> 1.  I installed the beta3 msi from the web site logged in as a user that
> has admin privileges.  But no elevation was used to install it, just
> double-click on the file.
> 2.  I only used the GUI as installed, with no elevation, to start OpenVPN.
> 3.  With TAP selected in my .ovpn config file, everything works normally.
> 4.  I am reporting that (from the same login) if I change the .ovpn to use
> wintun (all edits done through the GUI selection), it fails with the error
> I showed below.
>
> Is 4. what you are saying is not supported?
>

This use case is fully supported and should work. If it's not working, as
lev said, something is not right. Please share the full connection log with
verb=4 and we may spot something.


> In our use, as we have done for the past decade, the client boxes are used
> for M2M monitoring.  OpenVPN has to connect on bootup (.ovpn config file
> contains inline certificates) regardless if there is a user logged in or
> not as M2M monitoring occurs in the background.  And if a user does login,
> most often it is with credentials that have admin privileges.  I am trying
> to understand if what you’re telling me is that this will no longer work,
> or if we will need to do something different now?  My testing used the GUI
> to see how things will work with wintun so we can continue testing.
>

> Do I need to NOT use the GUI to get wintun to work?
>

Connections started at boot using OpenVPNService will also work with wintun
(2.5_beta3 and newer). You do not have to  use the GUI.

When an admin user logs in to Windows the elevated privileges in the token
are disabled by default , so the user starting any process including
OpenVPN-GUI will run in the *correct* unprivileged mode. Privileges are
acquired only when/if a UAC prompt appears and the user consents to it, or
when explicitly using run-as-admin. So, OpenVPN-GUi will run without
enabling privileges for all users and that is the right way to run the GUI.
So does a million other programs. This is the default behaviour of Windows
since Vista and is a good thing.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Fix client's poor man NCP fallback

2020-09-09 Thread Selva Nair
Hi,

On Thu, Sep 10, 2020 at 12:19 AM Marvin  wrote:

> Hi Selva,
>
> The GUI did not have this error unless run as administrator which you
>> should not and will never work.
>
> So you are saying that if OpenVPN is installed by a user who has admin
> privileges (as our case does) that v2.5 with wintun will not work?
>

You mean OpenVPN started using the GUI? If so, no, I'm not saying that. The
GUI just works even with beta1 or with beta3. On Windows user's processes
run with limited privileges even if the user is an "admin". At least that
is the default because of UAC.

Installation can be done when logged in as admin or as a limited user
(provided you know admin credentials) as the installer prompts for
elevation as required.

That qualifier I added was because: the GUI running with privileges (admin)
has been discouraged in 2.4 ever since we introduced the interactive
service. And that continues with 2.5. It will work for some things and not
for some other things. Same with running the GUI with interactive service
disabled. As such uses are not supported, don't complain about it :)

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Fix client's poor man NCP fallback

2020-09-09 Thread Selva Nair
Hi

On Wed, Sep 9, 2020 at 8:30 PM Marvin  wrote:

> Selva,
>
> Sorry for the wrong thread.  I was replying to an earlier thread about
> this same error on Beta1 and beta2.  So i am a bit confused by your
> statement that this error did not show up in earlier betas, because that's
> what started this thread.
>

The GUI did not have this error unless run as administrator which you
should not and will never work.

What was fixed was the error generated even when run as SYSTEM from the
automatic service or on command line using psexec or other tools that can
elevate to SYSTEM.

Selva

>
> Marvin
>
> On Wed, Sep 9, 2020 at 5:14 PM Selva Nair  wrote:
>
>> Hi Marvin,
>>
>> This is the wrong thread, but...
>>
>> On Wed, Sep 9, 2020 at 7:54 PM Marvin  wrote:
>>
>>> Hi Guys,
>>>
>>> I just tested beta3 on Win10.  I am getting the exact same error with
>>> wintun as before.  TAP works normally.  I tried with the GUI and by cli.
>>>
>>
>> The GUI never generated this error even before beta3, so not sure what
>> you are doing wrong. Ensure that the interactive service is running (that's
>> the default) and  GUI is not started with admin privileges (do not start
>> the GUI from an elevated command line or using run as admin).
>>
>> For cli you will get this error unless you use some utility like psexec
>> to elevate to SYSTEM.
>>
>> For connections started at boot using OpenVPNService (not the same as the
>> interactive service used by the GUI) beta3 should work. There was a bug
>> that affected this use case which was fixed in beta3.
>>
>>
>>> 2020-09-09 16:23:20 us=991306 ERROR:  Wintun requires SYSTEM privileges
>>> and therefore should be used with interactive service. If you want to use
>>> openvpn from command line, you need to do SYSTEM elevation yourself (for
>>> example with psexec).
>>> 2020-09-09 16:23:20 us=991306 Exiting due to fatal error
>>>
>>
>> If you got this error running from the command line as SYSTEM please
>> check the logs to be sure its beta3.
>>
>> Selva
>>
>>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Fix client's poor man NCP fallback

2020-09-09 Thread Selva Nair
Hi Marvin,

This is the wrong thread, but...

On Wed, Sep 9, 2020 at 7:54 PM Marvin  wrote:

> Hi Guys,
>
> I just tested beta3 on Win10.  I am getting the exact same error with
> wintun as before.  TAP works normally.  I tried with the GUI and by cli.
>

The GUI never generated this error even before beta3, so not sure what you
are doing wrong. Ensure that the interactive service is running (that's the
default) and  GUI is not started with admin privileges (do not start the
GUI from an elevated command line or using run as admin).

For cli you will get this error unless you use some utility like psexec to
elevate to SYSTEM.

For connections started at boot using OpenVPNService (not the same as the
interactive service used by the GUI) beta3 should work. There was a bug
that affected this use case which was fixed in beta3.


> 2020-09-09 16:23:20 us=991306 ERROR:  Wintun requires SYSTEM privileges
> and therefore should be used with interactive service. If you want to use
> openvpn from command line, you need to do SYSTEM elevation yourself (for
> example with psexec).
> 2020-09-09 16:23:20 us=991306 Exiting due to fatal error
>

If you got this error running from the command line as SYSTEM please check
the logs to be sure its beta3.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] Add a remark on dropping privileges when --mlock is used

2020-09-09 Thread selva . nair
From: Selva Nair 

trac #1059

Signed-off-by: Selva Nair 
---
 doc/man-sections/generic-options.rst | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/doc/man-sections/generic-options.rst 
b/doc/man-sections/generic-options.rst
index a07fe7e..d5f0883 100644
--- a/doc/man-sections/generic-options.rst
+++ b/doc/man-sections/generic-options.rst
@@ -230,6 +230,13 @@ which mode OpenVPN is configured as.
   The downside of using ``--mlock`` is that it will reduce the amount of
   physical memory available to other applications.
 
+  The limit on how much memory can be locked and how that limit
+  is enforced are OS-dependent. On Linux the default limit that an
+  unprivileged process may lock (RLIMIT_MEMLOCK) is low, and if
+  privileges are dropped later, future memory allocations will very
+  likely fail. The limit can be increased using ulimit or systemd
+  directives depending on how OpenVPN is started.
+
 --nice n
   Change process priority after initialization (``n`` greater than 0 is
   lower priority, ``n`` less than zero is higher priority).
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] In tap.c use DiInstallDevice to install the driver on a new adapter

2020-09-03 Thread selva . nair
From: Selva Nair 

As reported in Trac 1321, additional adapter instalaltion
by tapctl.exe fails to fully setup the device node (some registry
keys missing, error in setapi.dev.log etc.).
Although the exact cause of this failure is unclear,
letting the Plug and Play subsystem handle the instalaltion
by calling DiInsatllDevice() avoids it.

We let the system automatically choose the best driver
by passing NULL for driverinfo to DiInstallDevice().
This also eliminates the need for enumerating all drivers
in the Net class and selecting a matching one.

Somehow mingw-w64 fails to find  DiInstallDriver() in
newdev.lib although the header does define it. Use LoadLibrary()
to locate it at run time (available in Vista and above).

Built using mingw and tested both the msi installer (code shared
with libopenvpnmscia.dll) and tapctl.exe on Windows 10 64 bit.

Fixes: Trac #1321
Signed-off-by: Selva Nair 
---
 config-msvc.h|   1 +
 src/tapctl/tap.c | 231 +++
 2 files changed, 78 insertions(+), 154 deletions(-)

diff --git a/config-msvc.h b/config-msvc.h
index 8ef4897..f199bb2 100644
--- a/config-msvc.h
+++ b/config-msvc.h
@@ -112,6 +112,7 @@
 #define HAVE_EC_GROUP_ORDER_BITS 1
 #define OPENSSL_NO_EC 1
 #define HAVE_EVP_CIPHER_CTX_RESET 1
+#define HAVE_DIINSTALLDEVICE 1
 
 #define PATH_SEPARATOR '\\'
 #define PATH_SEPARATOR_STR "\\"
diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c
index 7cb3ded..e36d132 100644
--- a/src/tapctl/tap.c
+++ b/src/tapctl/tap.c
@@ -33,18 +33,69 @@
 #include 
 #include 
 #include 
+#include 
 
 #ifdef _MSC_VER
 #pragma comment(lib, "advapi32.lib")
 #pragma comment(lib, "ole32.lib")
 #pragma comment(lib, "setupapi.lib")
+#pragma comment(lib, "newdev.lib")
 #endif
 
+
 const static GUID GUID_DEVCLASS_NET = { 0x4d36e972L, 0xe325, 0x11ce, { 0xbf, 
0xc1, 0x08, 0x00, 0x2b, 0xe1, 0x03, 0x18 } };
 
 const static TCHAR szAdapterRegKeyPathTemplate[] = 
TEXT("SYSTEM\\CurrentControlSet\\Control\\Network\\%") TEXT(PRIsLPOLESTR) 
TEXT("\\%") TEXT(PRIsLPOLESTR) TEXT("\\Connection");
 #define ADAPTER_REGKEY_PATH_MAX 
(_countof(TEXT("SYSTEM\\CurrentControlSet\\Control\\Network\\")) - 1 + 38 + 
_countof(TEXT("\\")) - 1 + 38 + _countof(TEXT("\\Connection")))
 
+/**
+ * Dynamically load a library and find a function in it
+ *
+ * @param libname Name of the library to load
+ * @param funcnameName of the function to find
+ * @param m   Pointer to a module. On return this is set to the
+ *the handle to the loaded library. The caller must
+ *free it by calling FreeLibrary() if not NULL.
+ *
+ * @returnPointer to the function
+ *NULL on error -- use GetLastError() to find the error 
code.
+ *
+ **/
+static void *
+find_function(const WCHAR *libname, const char *funcname, HMODULE *m)
+{
+WCHAR libpath[MAX_PATH];
+void *fptr = NULL;
+
+/* Make sure the dll is loaded from the system32 folder */
+if (!GetSystemDirectoryW(libpath, _countof(libpath)))
+{
+   return NULL;
+}
+
+size_t len = _countof(libpath) - wcslen(libpath) - 1;
+if (len < wcslen(libname) + 1)
+{
+   SetLastError(ERROR_INSUFFICIENT_BUFFER);
+   return NULL;
+}
+wcsncat(libpath, L"\\", len);
+wcsncat(libpath, libname, len-1);
+
+*m = LoadLibraryW(libpath);
+if (*m == NULL)
+{
+   return NULL;
+}
+fptr = GetProcAddress(*m, funcname);
+if (!fptr)
+{
+   FreeLibrary(*m);
+   *m = NULL;
+   return NULL;
+}
+return fptr;
+}
 
 /**
  * Returns length of string of strings
@@ -678,6 +729,7 @@ tap_create_adapter(
 _Out_ LPGUID pguidAdapter)
 {
 DWORD dwResult;
+HMODULE libnewdev = NULL;
 
 if (szHwId == NULL
 || pbRebootRequired == NULL
@@ -746,129 +798,7 @@ tap_create_adapter(
 goto cleanup_hDevInfoList;
 }
 
-/* Search for the driver. */
-if (!SetupDiBuildDriverInfoList(
-hDevInfoList,
-_data,
-SPDIT_CLASSDRIVER))
-{
-dwResult = GetLastError();
-msg(M_NONFATAL, "%s: SetupDiBuildDriverInfoList failed", __FUNCTION__);
-goto cleanup_hDevInfoList;
-}
-DWORDLONG dwlDriverVersion = 0;
-DWORD drvinfo_detail_data_size = sizeof(SP_DRVINFO_DETAIL_DATA) + 0x100;
-SP_DRVINFO_DETAIL_DATA *drvinfo_detail_data = (SP_DRVINFO_DETAIL_DATA 
*)malloc(drvinfo_detail_data_size);
-if (drvinfo_detail_data == NULL)
-{
-msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__, 
drvinfo_detail_data_size);
-dwResult = ERROR_OUTOFMEMORY; goto cleanup_DriverInfoList;
-}
-
-for (DWORD dwIndex = 0;; dwIndex++)
-{
-/* Get a driver from the list. */
-SP_DRVINFO_DATA drvinfo_data = { .cbSize = sizeof(SP_DRVINFO_DATA) };
-

Re: [Openvpn-devel] On tap-windows6 adapter installation by tapctl.exe

2020-09-03 Thread Selva Nair
Hi Lev,

Thanks for confirming. What you tested is exactly what I have in mind.

I suppose you tested it using MSVC. I recall when I worked on creating tap
adapters on the fly (patch abandoned for lack of time) some functions in
newdev.dll did not resolve with mingw and I always had to load them on
runtime. I'll check those hard corners again and submit a patch soonish
(hopefully today).

Selva

On Thu, Sep 3, 2020 at 8:11 AM Lev Stipakov  wrote:

> Hi,
>
> >
> > As per setupapi.dev.log, it appears that step 4 (d) is failing with some
> access error to the driver store unless elevated to SYSTEM (see Trac 1321).
> This leaves the adapter not fully configured. Hard to say exactly what
> fails as none of the function calls return any error.
>
> I can confirm that. This call
>
> /* Install the device. */
> if (!SetupDiCallClassInstaller(
> DIF_INSTALLDEVICE,
> hDevInfoList,
> _data))
> {
> dwResult = GetLastError();
> msg(M_NONFATAL | M_ERRNO, "%s:
> SetupDiCallClassInstaller(DIF_INSTALLDEVICE) failed", __FUNCTION__);
> goto cleanup_remove_device;
> }
>
> produces this error in setupapi.dev.log
>
> >>>  [Configure Driver Package -
> c:\windows\system32\driverstore\filerepository\oemvista.inf_amd64_8a00bc07868b5df3\oemvista.inf]
> >>>  Section start 2020/09/03 12:08:49.525
>   cmd: "C:\Users\lev\Projects\openvpn\x64-Output\Debug\tapctl.exe"
> create
>  sto: Source Filter  = tap0901.ndi
>  sto: Target Filter  = ROOT\NET\0004
>  inf: Class GUID = {4d36e972-e325-11ce-bfc1-08002be10318}
>  inf: Class Options  = Configurable
> !!!  idb: Failed to open driver package object
> 'oemvista.inf_amd64_8a00bc07868b5df3'. Error = 0x0005
> <<<  Section end 2020/09/03 12:08:49.558
> <<<  [Exit status: FAILURE(0x0005)]
>
>
> >>>  [Restart Device - ROOT\NET\0004]
> >>>  Section start 2020/09/03 12:08:49.565
>   cmd: "C:\Users\lev\Projects\openvpn\x64-Output\Debug\tapctl.exe"
> create
>  dvi: Device Status: 0x01802501 [0x0e - 0x]
> !dvi: Device restart was skipped (NEEDREBOOT/NEEDRESTART).
> <<<  Section end 2020/09/03 12:08:49.588
> <<<  [Exit status: SUCCESS]
>
> > I propose to replace 4(d) with a call to DiInstallDevice() instead. My
> tests show this completes without error.
>
> I can confirm that too. This patch
>
> diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c
> index 7cb3dedc..8d071320 100644
> --- a/src/tapctl/tap.c
> +++ b/src/tapctl/tap.c
> @@ -29,6 +29,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -38,6 +39,7 @@
>  #pragma comment(lib, "advapi32.lib")
>  #pragma comment(lib, "ole32.lib")
>  #pragma comment(lib, "setupapi.lib")
> +#pragma comment(lib, "newdev.lib")
>  #endif
>
>  const static GUID GUID_DEVCLASS_NET = { 0x4d36e972L, 0xe325, 0x11ce,
> { 0xbf, 0xc1, 0x08, 0x00, 0x2b, 0xe1, 0x03, 0x18 } };
> @@ -900,19 +902,13 @@ tap_create_adapter(
>  }
>
>  /* Install the device. */
> -if (!SetupDiCallClassInstaller(
> -DIF_INSTALLDEVICE,
> -hDevInfoList,
> -_data))
> +if (!DiInstallDevice(NULL, hDevInfoList, _data, NULL, 0,
> pbRebootRequired))
>  {
>  dwResult = GetLastError();
> -msg(M_NONFATAL | M_ERRNO, "%s:
> SetupDiCallClassInstaller(DIF_INSTALLDEVICE) failed", __FUNCTION__);
> +msg(M_NONFATAL | M_ERRNO, "%s: DiInstallDevice failed",
> __FUNCTION__);
>  goto cleanup_remove_device;
>  }
>
> -/* Check if a system reboot is required. (Ignore errors) */
> -check_reboot(hDevInfoList, _data, pbRebootRequired);
> -
>  /* Get network adapter ID from registry. Retry for max 30sec. */
>  dwResult = get_net_adapter_guid(hDevInfoList, _data, 30,
> pguidAdapter);
>
> works for both tapctl and MSI installer - created adapters are usable
> by non-admin.
>
> >
> > This also has an advantage that we could call it with driver_info = NULL
> which will force the system to use the latest matching driver. That would
> also eliminate step 3 which is right now very inefficient, though not
> required to fix the problem at hand.
>
> Yeah, I also works for me without
>
> for (DWORD dwIndex = 0;; dwIndex++)
> {
>
> loop.
>
> >
> > If this sounds sane, I'll submit a patch.
>
> Please do!
>
> --
> -Lev
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] On tap-windows6 adapter installation by tapctl.exe

2020-09-02 Thread Selva Nair
Hi,

tldr: a fix for Trac 1321

Currently tapctl.exe does the following to create an adapter and install
the driver on it.

1. Create a device info structure
2. Set the hardware id on it
3. Search the driver store for the latest matching driver
4. Select the driver, set it in the device info and then call
SetupDiCallClassInstaller() with
  (a) DIF_REGISTERDEVICE
  (b) DIF_REGISTER_COINSTALLERS
  (c) DIF_INSTALL_INTERFACES
  (d) DIF_INSTALLDEVICE

As per setupapi.dev.log, it appears that step 4 (d) is failing with some
access error to the driver store unless elevated to SYSTEM (see Trac 1321).
This leaves the adapter not fully configured. Hard to say exactly what
fails as none of the function calls return any error.

I propose to replace 4(d) with a call to DiInstallDevice() instead. My
tests show this completes without error.

This also has an advantage that we could call it with driver_info = NULL
which will force the system to use the latest matching driver. That would
also eliminate step 3 which is right now very inefficient, though not
required to fix the problem at hand.

If this sounds sane, I'll submit a patch.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] openvpnmsica: make adapter renaming non-fatal

2020-09-02 Thread Selva Nair
Hi

On Wed, Sep 2, 2020 at 9:54 AM Lev Stipakov  wrote:

> Hi,
>
> >>  if (dwResult != ERROR_SUCCESS)
> >>  {
> >> -tap_delete_adapter(NULL, ,
> );
> >> +/* failed renaming is not a fatal error, continue
> */
> >> +dwResult = ERROR_SUCCESS;
> >
> >
> > This looks strange. If we are going to rewrite dwResult, why set it and
> check it at all, just call
> > tap_set_adapter() without storing the return value and add the comment.
>
> I think this is good for documentation purposes - that we are aware
> that function may fail, but in this case it is not fatal and we continue.
>

That's achieved also by just adding a comment that we don't check the
return value as failure is not fatal. Anyway, this works too.


>
> > Or, better, print a warning message saying the rename failed.
>
> Warning is printed inside tap_delete_adapter().
>

tap_delete_adapter() is not called here. I was suggesting that if we do
check the return value, let us also print a  warning that renaming failed.
That can't be  done anywhere except just before converting the error
to SUCCESS.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] openvpnmsica: make adapter renaming non-fatal

2020-09-02 Thread Selva Nair
Hi

On Wed, Sep 2, 2020 at 9:39 AM Lev Stipakov  wrote:

> From: Lev Stipakov 
>
> For some users renaming adapter mysteriously fails
> (https://github.com/OpenVPN/openvpn-build/issues/187),
>
> Since renaming is just a a "nice to have", make it not fatail.
>
> Signed-off-by: Lev Stipakov 
> ---
>  src/openvpnmsica/openvpnmsica.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpnmsica/openvpnmsica.c
> b/src/openvpnmsica/openvpnmsica.c
> index 31e90bd2..d5b3f09e 100644
> --- a/src/openvpnmsica/openvpnmsica.c
> +++ b/src/openvpnmsica/openvpnmsica.c
> @@ -1100,7 +1100,8 @@ ProcessDeferredAction(_In_ MSIHANDLE hInstall)
>  dwResult = tap_set_adapter_name(, szName);
>  if (dwResult != ERROR_SUCCESS)
>  {
> -tap_delete_adapter(NULL, ,
> );
> +/* failed renaming is not a fatal error, continue */
> +dwResult = ERROR_SUCCESS;
>

This looks strange. If we are going to rewrite dwResult, why set it and
check it at all, just call
tap_set_adapter() without storing the return value and add the comment.

Or, better, print a warning message saying the rename failed.

Selva


>  }
>  }
>  }
> --
> 2.17.1
>
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] openvpnmsica: remove adapter renaming

2020-09-02 Thread Selva Nair
Hi,

I would suggest to keep this renaming but make it not fatal. A
descriptive name is nice to have and we could even make the name
configurable at some point in future.

Selva

On Wed, Sep 2, 2020 at 8:40 AM Lev Stipakov  wrote:

> From: Lev Stipakov 
>
> Renaming doesn't work on some machines (
> https://github.com/OpenVPN/openvpn-build/issues/187)
> and we couldn't find the cause. Also others do not seem to do it, so let's
> get rid of it.
>
> Signed-off-by: Lev Stipakov 
> ---
>  src/openvpnmsica/openvpnmsica.c | 9 -
>  1 file changed, 9 deletions(-)
>
> diff --git a/src/openvpnmsica/openvpnmsica.c
> b/src/openvpnmsica/openvpnmsica.c
> index 31e90bd2..10d4a1cc 100644
> --- a/src/openvpnmsica/openvpnmsica.c
> +++ b/src/openvpnmsica/openvpnmsica.c
> @@ -1094,15 +1094,6 @@ ProcessDeferredAction(_In_ MSIHANDLE hInstall)
>
>  GUID guidAdapter;
>  dwResult = tap_create_adapter(NULL, NULL, szHardwareId,
> , );
> -if (dwResult == ERROR_SUCCESS)
> -{
> -/* Set adapter name. */
> -dwResult = tap_set_adapter_name(, szName);
> -if (dwResult != ERROR_SUCCESS)
> -{
> -tap_delete_adapter(NULL, ,
> );
> -}
> -}
>  }
>  else if (wcsncmp(szArg[i], L"deleteN=", 8) == 0)
>  {
> --
> 2.17.1
>
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] Help testing OpenVPN 2.5-beta2 driver installation?

2020-08-31 Thread Selva Nair
Hi

>
> > (2) At the end of install the GUI is launched as admin, not user.
>
> I couldn't reproduce that on my Windows 10 laptop:
>

I too can't reproduce it any longer. So please ignore that comment.

I was installing from the command line (easier to generate logs that way)
and probably used an elevated command prompt which will obviously lead to
that behaviour.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] Help testing OpenVPN 2.5-beta2 driver installation?

2020-08-29 Thread Selva Nair
Hi

On Fri, Aug 28, 2020 at 9:10 AM Samuli Seppänen  wrote:

> Hi,
>
> It would be great if somebody would find time to test the following
> installer:
>
>
> https://build.openvpn.net/downloads/releases/OpenVPN-2.5-beta2-I601-amd64.msi
>
> In particular I'd like to know if anyone else has problems installing
> Wintun or Tap-windows6. My exact issue is described here:
>
> https://github.com/OpenVPN/openvpn-build/issues/187
>
> At the moment two people have successfully ran that installer and one
> (me) have failed.
>

For me the installer (with beta1 installed) without errors and tap driver
does get updated, but some issues are noticed (Windows 10 home 64 bit):

(1) Only openvpn-gui.exe gets updated. openvpn.exe and openvpnserv.exe stay
at the old beta1 (binaries dated Aug 14th, version 2.5_beta1 etc..).

This may be because the file version of openvpn.exe is not changed
(2.5.0.0) -- we may have to add a build number ( the 4th last digit) and
increment it each time. That would make the final 2.5.0 release something
like 2.5.0.xx for the file versions though the product version can be
2.5.0, hopefully.

LZO dll gets updated, but other dlls (crypto and pkcs11) are not  (based on
date). The latter may be okay if their versions have not changed.

(2) At the end of install the GUI is launched as admin, not user.

Although not prompted for reboot, I did reboot and checked again that the
binaries are still the old one's. Fresh install of beta2 brings in all
binaries dated Aug 27.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 1/2] Send auth-fail messages to clients on renegotiation failures via auth-token or user-pass expiry

2020-08-24 Thread Selva Nair
On Mon, Aug 24, 2020 at 3:49 AM Eric Thorpe  wrote:

> Hi Selva,
>
> my suggestion would be to make
> this conditional on MANAGEMNET_DEF_AUTH so that we can
> then get it from  session->opt->mda_context just as we do it when
> auth is done via the management. In practice, that would cover
> most builds where this is really useful.
>
> Unfortunately this doesn't help. The mda_context flags are only set when
> --management-client-auth is in use, meaning this patch would not cover
> plugin or script authentication, which are the more commonly used, and this
> patch set specifically addresses plugin authentication.
>

Are you sure of that?

In multi_connection_estableished, we have

#ifdef MANAGEMENT_DEF_AUTH
if (management)
{
management_connection_established(management,
  >context.c2.mda_context, mi->
context.c2.es);
}
#endif

management_connection_established will set DAF_CONENCTION_ESATBLISHED in
mdac->flags and that's what's used to determine REAUTH.

I do not see why this requires --management-client-auth or any management
related options to be in use. Sure, management support and deferred auth
support have to be enabled but restricting the usefulness of your patch to
those cases is not really a limitation.

What am I missing?

Selva

---
> Eric Thorpe
> SparkLabs 
> Developerhttps://www.sparklabs.comhttps://twitter.com/sparklabssupp...@sparklabs.com
>
> On 23/08/2020 12:13 pm, Selva Nair wrote:
>
> Hi,
>
> On Thu, Aug 13, 2020 at 4:37 AM Eric Thorpe  wrote:
>
>> Hi Arne,
>>
>> The issue is your state is not accessible from where that boolean needs
>> to be used unless I am missing something? Please advise if I'm mistaken
>> or of another route.
>>
>
> I agree with Arne that duplicating a state machine variable is
> not a good approach. But we have to somehow get the REAUTH (reneg)
> info in here.
>
> This has stalled for too long, so my suggestion would be to make
> this conditional on MANAGEMNET_DEF_AUTH so that we can
> then get it from  session->opt->mda_context just as we do it when
> auth is done via the management. In practice, that would cover
> most builds where this is really useful.
>
> In fact, I think we should always enable MANAGEMENT_DEF_AUTH
> when management is enabled. That also gets rid of a lot of IFDEFs and
> allow the use of useful bits like CID more widely in the code. I see
> no compelling reason for such fine-grained build options.
>
> A marginal increase in code size is of little consequence all but
> embedded devices which can continue to cope without this
> as they do now.
>
> Selva
>
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 1/2] Send auth-fail messages to clients on renegotiation failures via auth-token or user-pass expiry

2020-08-22 Thread Selva Nair
Hi,

On Thu, Aug 13, 2020 at 4:37 AM Eric Thorpe  wrote:

> Hi Arne,
>
> The issue is your state is not accessible from where that boolean needs
> to be used unless I am missing something? Please advise if I'm mistaken
> or of another route.
>

I agree with Arne that duplicating a state machine variable is
not a good approach. But we have to somehow get the REAUTH (reneg)
info in here.

This has stalled for too long, so my suggestion would be to make
this conditional on MANAGEMNET_DEF_AUTH so that we can
then get it from  session->opt->mda_context just as we do it when
auth is done via the management. In practice, that would cover
most builds where this is really useful.

In fact, I think we should always enable MANAGEMENT_DEF_AUTH
when management is enabled. That also gets rid of a lot of IFDEFs and
allow the use of useful bits like CID more widely in the code. I see
no compelling reason for such fine-grained build options.

A marginal increase in code size is of little consequence all but
embedded devices which can continue to cope without this
as they do now.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] tun.c: enable using wintun driver under SYSTEM

2020-08-21 Thread Selva Nair
Hi,

On Wed, Aug 19, 2020 at 3:08 AM Lev Stipakov  wrote:

> From: Lev Stipakov 
>
> Commit 6d19775a468 has removed SYSTEM elevation hack,
> but introduced regression - inability to use wintun without interactive
> service.
>
> Proceed with ring buffers registration even if iservice is unavailable and
> display
> relevant error message.
>
> Trac #1318
>
> Signed-off-by: Lev Stipakov 
> ---
>  src/openvpn/tun.c | 30 +-
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 30454454..62557364 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -6158,12 +6158,32 @@ wintun_register_ring_buffer(struct tuntap *tt,
> const char *device_guid)
>  }
>  else
>  {
> -msg(M_FATAL, "ERROR:  Wintun requires SYSTEM privileges and
> therefore "
> - "should be used with interactive service. If you
> want to "
> - "use openvpn from command line, you need to do
> SYSTEM "
> - "elevation yourself (for example with psexec).");
> -}
> +if (!register_ring_buffers(tt->hand,
> +   tt->wintun_send_ring,
> +   tt->wintun_receive_ring,
> +   tt->rw_handle.read,
> +   tt->rw_handle.write))
> +{
> +switch (GetLastError())
> +{
> +case ERROR_ACCESS_DENIED:
> +msg(M_FATAL, "ERROR:  Wintun requires SYSTEM
> privileges and therefore "
> + "should be used with interactive
> service. If you want to "
> + "use openvpn from command line, you need
> to do SYSTEM "
> + "elevation yourself (for example with
> psexec).");
> +break;
> +
> +case ERROR_ALREADY_INITIALIZED:
> +msg(M_NONFATAL, "Adapter %s is already in use",
> device_guid);
> +break;
>
> +default:
> +msg(M_NONFATAL | M_ERRNO, "Failed to register ring
> buffers");
> +}
> +ret = false;
> +}
> +
> +}
>  return ret;
>  }
>

Looks good and running as SYSTEM works now as expected. Tested on 64 bit
Windows 10.

Acked-by: selva.n...@gmail.com
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] 2.5-beta-1 Wintun requires SYSTEM privileges

2020-08-18 Thread Selva Nair
Hi

On Tue, Aug 18, 2020 at 3:42 PM Gert Doering  wrote:

> Hi,
>
> On Tue, Aug 18, 2020 at 03:29:19PM -0400, Selva Nair wrote:
> > > If you already have SYSTEM, accessing wintun from openvpn directly will
> > > also work and should bring quite a bit of speed improvement.
> >
> > I was wrong to assume that this just works. Looking at it again, the
> current
> > implementation of wintun support does not seem to cover this. Meaning the
> > only working approach is to use the interactive service.
>
> Indeed, you are right.  Somewhere on the track we lost the ability
> to do wintun "from OpenVPN" if we *have* SYSTEM.
>
> This is the code in tun.c
>
> if (tt->options.msg_channel)
> {
> ret = service_register_ring_buffers(tt);
> }
> else
> {
> msg(M_FATAL, "ERROR:  Wintun requires SYSTEM privileges and
> therefore "
>  "should be used with interactive service. If you want
> to "
>  "use openvpn from command line, you need to do SYSTEM
> "
>  "elevation yourself (for example with psexec).");
> }
>
>
> ... so while I'm happy that we got rid of impersonating SYSTEM, the current
> code does not even try anymore, just assumes "if there is no message
> channel,
> we have not enough privileges".
>
> OTOH the message is severely misleading now, since it suggests "having
> the right privileges will make this work".
>
>
> This needs fixing - either we *try*, and if we fail, print the message
> about insufficient privileges, or we change the message to actually
> print something like "Wintun support is only possible if the interactive
> service is used.  Do not run from the CLI.  Use the GUI in non-admin
> mode.".
>

We have all the necessary code to do "register ring buffers" that the
service uses, so just calling it and printing that message on failure
should fix it.

Looks like something lost by Lev during a rebase or conflict resolution.

Trac #1318

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] 2.5-beta-1 Wintun requires SYSTEM privileges

2020-08-18 Thread Selva Nair
Hi,

On Tue, Aug 18, 2020 at 3:21 PM Gert Doering  wrote:

> Hi,
>
> On Tue, Aug 18, 2020 at 12:09:11PM -0700, Marvin Adeff wrote:
> > I???m sorry for the confusing response.
> >
> > Our systems do M2M monitoring and need to run OpenVPN even without a
> user logged in.  In previous versions we created a script run as a service
> (as SYSTEM) that started OpenVPN (using certificates for authentication).
> It also monitored tunnel status and restarted OpenVPN if necessary.  We
> never used the GUI.
> >
> > So we are watching how v2.5 develops to know how we will need to
> implement the new version.  We are also very interested in seeing what
> speed improvements wintun will offer.
> >
> > I hope that is clearer.
>
> Thanks for the clarification.
>
> In that regard, 2.5 will bring no surprises - if you already have SYSTEM
> privileges, and do not want/need a GUI, you can "just run OpenVPN" as
> you did before.
>
> You can do this with your script, or with the "openvpnsrv2" service,
> which basically runs openvpn on all config it finds in its config
> directory at system startup.  Not sure if these instances get restarted
> at exit (last time I looked at this was before 2.4.0 release...).
>
> If you already have SYSTEM, accessing wintun from openvpn directly will
> also work and should bring quite a bit of speed improvement.
>

I was wrong to assume that this just works. Looking at it again, the current
implementation of wintun support does not seem to cover this. Meaning the
only working approach is to use the interactive service.

If developing a new service, I would suggest to have
the service talk to the interactive service for starting openvpn. It will
return you the PID of openvpn.exe which can be monitored. An advantage
of this approach is that your service and openvpn.exe can run with limited
privileges like LOCAL SERVICE or a dedicated openvpn service user.
That said, I don't know anyone who has tested such a usage though it
should work in theory.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] 2.5-beta-1 Wintun requires SYSTEM privileges

2020-08-18 Thread Selva Nair
>
>
>
> An additional check in openvpn.exe whether it's started as SYSTEM could be
> useful as well, but less critical, IMO.
>
> Yes Please!  We run 2500+ systems that run it this way as SYSTEM.
>

In most such cases (not using the GUI) one could use the automatic service
which runs as SYSTEM. For those running interactively from the command line
(for occasional use),  the logs should already show what's wrong, isn't it?
Anyway, I was not aware of a wide-spread use-case that doesn't rely on one
of the services.

Even with an early error-out it one will have to check the logs (or use the
GUI) to see what went wrong.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] 2.5-beta-1 Wintun requires SYSTEM privileges

2020-08-18 Thread Selva Nair
Hi

On Tue, Aug 18, 2020 at 2:33 AM Gert Doering  wrote:

> Hi,
>
> On Tue, Aug 18, 2020 at 08:23:35AM +0200, Gert Doering wrote:
> > This can also happen if you run the GUI with admin privs (because then
> > it will not use the iservice *but* openvpn needs *more* privs than
> > "just administrator", and wintun can not be used at all).
>
> Continueing this thought: I think we might want to abort earlier in
> the OpenVPN startup in this case, that is, "wintun and no iservice pipe".
>

+1 to that. Should be easy to add this check in the GUI.

I think we can also relax the "do not connect to iservice if admin"
restriction
as that was added to protect against some Windows Vista mis-behaviour.

An additional check in openvpn.exe whether it's started as SYSTEM could be
useful as well, but less critical, IMO.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2] Improve the documentation for --dhcp-option

2020-08-16 Thread selva . nair
From: Selva Nair 

- Stress that these are handled internally only on some platforms
- Correct the statement about wintun 
- Document DOMAIN-SEARCH

Signed-off-by: Selva Nair 
---
v2: Rebase to master and reword to match the new rst version
Add doc for DOMAIN-SEARCH

 doc/man-sections/vpn-network-options.rst | 23 +--
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/doc/man-sections/vpn-network-options.rst 
b/doc/man-sections/vpn-network-options.rst
index 7100c1ae..825dd1ca 100644
--- a/doc/man-sections/vpn-network-options.rst
+++ b/doc/man-sections/vpn-network-options.rst
@@ -93,12 +93,18 @@ routing.
   or :code:`tap`.
 
 --dhcp-option args
-  Set additional network settings via DHCP.  On Windows, this is parsed by
-  the ``tap-windows6`` or ``wintun`` driver.  On other platforms these
-  options can be picked up by an ``--up`` script or plug-in if it has been
-  pushed by the OpenVPN server.  The option will then be saved in the
-  client's environment before the ``--up`` script is called, under the name
-  :code:`foreign_option_{n}`.
+  Set additional network parameters on supported platforms. May be specified
+  on the client or pushed from the server. On Windows these options are
+  handled by the ``tap-windows6`` driver by default or directly by OpenVPN
+  if dhcp is disabled or the ``wintun`` driver is in use. The
+  ``OpenVPN for Android`` client also handles them internally.
+
+  On all other platforms these options are only saved in the client's
+  environment under the name :code:`foreign_options_{n}` before the
+  ``--up`` script is called. A plugin or an ``--up`` script must be used to
+  pick up and interpret these as required. Many Linux distributions include
+  such scripts and some third-party user interfaces such as tunnelblick also
+  come with scripts that process these options.
 
   Valid syntax:
   ::
@@ -108,6 +114,11 @@ routing.
   :code:`DOMAIN` ``name``
 Set Connection-specific DNS Suffix to :code:`name`.
 
+  :code:`DOMAIN-SEARCH` ``name``
+Add :code:`name` to the domain search list.
+Repeat this option to add more entries. Up to
+10 domains are supported.
+
   :code:`DNS` ``address``
 Set primary domain name server IPv4 or IPv6 address.
 Repeat this option to set secondary DNS server addresses.
-- 
2.17.1



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Allow management to kill client instances by CN wildcard

2020-08-14 Thread Selva Nair
Hi

On Fri, Aug 14, 2020 at 3:06 PM Vladislav Grishenko
 wrote:
>
> Hi,
>
> Yes, killing a client with cn ending in * will also lead to killing all the
> clients whose cn starts with that prefix.
> Use other char would no-intuitive (ex. +).
> What about optional "prefix" mode word for explicit mode (can be also
> enhanced one day with suffix/regexp/etc).
>
> kill cn [mode]: Kill the client instance(s) having common name cn.

That sounds good to me -- avoids the use of any special character.

Also, updating the "help" command of management interface was missed
in the previous version of the patch.

Selva

>
> --
> Best Regards, Vladislav Grishenko
>
> -Original Message-
> From: Selva Nair 
> Sent: Friday, August 14, 2020 11:22 PM
> To: openvpn-devel 
> Subject: Re: [Openvpn-devel] [PATCH v2] Allow management to kill client
> instances by CN wildcard
>
> Hi
>
> On Fri, Aug 14, 2020 at 1:36 PM Arne Schwabe  wrote:
> >
> > Am 14.08.20 um 19:12 schrieb Vladislav Grishenko:
> > > In case of some permanent part of common name (ex. domain) and/or
> > > long complex common name consisting of multiple x509 fields, it's
> > > handly to kill client instances via management interface with just
> > > prefix of common name, not by exact match only.
> > >
> > > Patch allows to use asterisk as wildcard placeholder in the last
> > > trailing symbol of kill command parameter.
> > > Single asterisk - empty prefix would be too greedy and can be too
> > > harmful, therefore not allowed. Wildcards in the middle of parameter
> > > string are not supported to keep the the things simple at the moment.
> > >
> > > v2: fine tune comments
> > >
> >
> > Thanks for v2,
> >
> > Acked-By; Arne Schwabe 
>
> '*' is an allowed character in x509 common name unless we explicitly forbid
> it. So killing a client with name ending in * would get tricky if not
> impossible without side effects.
>
> Selva
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Allow management to kill client instances by CN wildcard

2020-08-14 Thread Selva Nair
Hi

On Fri, Aug 14, 2020 at 1:36 PM Arne Schwabe  wrote:
>
> Am 14.08.20 um 19:12 schrieb Vladislav Grishenko:
> > In case of some permanent part of common name (ex. domain) and/or
> > long complex common name consisting of multiple x509 fields, it's
> > handly to kill client instances via management interface with just
> > prefix of common name, not by exact match only.
> >
> > Patch allows to use asterisk as wildcard placeholder in the last
> > trailing symbol of kill command parameter.
> > Single asterisk - empty prefix would be too greedy and can be too
> > harmful, therefore not allowed. Wildcards in the middle of
> > parameter string are not supported to keep the the things simple at the 
> > moment.
> >
> > v2: fine tune comments
> >
>
> Thanks for v2,
>
> Acked-By; Arne Schwabe 

'*' is an allowed character in x509 common name unless we explicitly
forbid it. So killing a client with name ending in * would get tricky
if not impossible without side effects.

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Improve error msg when all TAP adapters are in use "or disabled"

2020-08-06 Thread Selva Nair
Hi,

This looks good but can we do better? We don't check the error
(GetLastError()) after the CreateFile() failure -- can we determine
whether the error was due to permissions, busy file (in use) or
disabled device and print out a more specific error message? I'm not
sure what errors are triggered by CreateFile, so just wondering..

Selva

On Thu, Aug 6, 2020 at 3:02 PM Richard Bonhomme  wrote:
>
> Ref: https://github.com/OpenVPN/openvpn-gui/issues/356
>
> Signed-off-by: Richard Bonhomme 
> ---
>  src/openvpn/tun.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index cc7b65cf..44ca8450 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -6436,7 +6436,7 @@ tun_open_device(struct tuntap *tt, const char 
> *dev_node, const char **device_gui
>
>  if (!*device_guid)
>  {
> -msg(M_FATAL, "All %s adapters on this system are currently 
> in use.", print_windows_driver(tt->windows_driver));
> +msg(M_FATAL, "All %s adapters on this system are currently 
> in use or disabled.", print_windows_driver(tt->windows_driver));
>  }
>
>  if (tt->windows_driver != windows_driver)
> --
> 2.17.1
>
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] Regarding deprecation of --route-nopull

2020-07-23 Thread Selva Nair
Hi

On Thu, Jul 23, 2020 at 4:50 PM Arne Schwabe  wrote:
>
> Am 23.07.2020 um 20:14 schrieb André via Openvpn-devel:
> > Hi,
> >
> > Regarding,
> >  
> > https://community.openvpn.net/openvpn/wiki/DeprecatedOptions#Option:--route-nopull
> >  "Openvpn devs would like to know if you use this option".
> >
> > Many pfSense users use this option to policy route.
>
> I would also vote for keeping this option. Yes you can emulate the
> option by using a number of pull-filter lines but that feels like not a
> good user experience.

That's because route-nopull is not what it appears to be. A single
line of "pull-fliter ignore route" will do exactly what one would
think "route-nopull" should do, but the latter does more. It's poorly
named and now we have better ways to control pulled options, so I have
no love for it.

> Also route-pull works in both OpenVPN 2.x and 3.x
> clients while pull-filter is currently 2.x only.

Actually pull-filter cannot be compared with route-nopull as the
former is customizable. The real question is whether there is any
compelling reason to use it other than lack of alternatives in 2.3 and
older. I didn't know 3.x does not support pull-filter. Why? It's easy
to code (at least I know that for sure) so that can't be the reason.

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [Openvpn-users] Join PC with OpenVpn to Active Directory

2020-07-19 Thread Selva Nair
Hi,

If your VPN establishes a route to the domain controller(s) and the
domain name resolves from the client, you can join the domain just as
you would do while directly connected to the LAN. For example, if the
domain name is example.local, "nslookup example.local" should return
the IP addresses of domain controllers, and those IPs should be
reachable from the client.

In the most common scenario where the domain controllers are on the
server-side LAN, this requires the VPN to set up a route to the
server-side LAN, and push a dns server that resolves the domain name.
Both of these are described in OpenVPN howto. See
https://community.openvpn.net/openvpn/wiki/HOWTO#IncludingmultiplemachinesontheserversidewhenusingaroutedVPNdevtun
and
https://community.openvpn.net/openvpn/wiki/HOWTO#PushingDHCPoptionstoclients


Selva

On Sun, Jul 19, 2020 at 1:07 PM Fermin Francisco via Openvpn-users
 wrote:
>
> Good afternoon!
>
> How Can I join a PC with openVPN to the Active Directory, does exists a 
> manual, Video, something like that??
>
>
>
> José Fermín Francisco Ferreras Registered User #579535 (LinuxCounter.net)
>
>
> ___
> Openvpn-users mailing list
> openvpn-us...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-users


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v3] Add deferred authentication support to plugin-auth-pam

2020-07-15 Thread Selva Nair
Hi,

Thanks for v3. All good except Changes.rst has diverged, so the patch
doesn't apply as is. Can be fixed at merge time.

The code is unchanged from the last version and the added text in
README is clear and detailed. A minor grammar thingy:

"all forwarding for all other client" -- > "all forwarding for all
other clients"

Acked-by: Selva Nair 

On Wed, Jul 15, 2020 at 5:02 AM Gert Doering  wrote:
>
> If OpenVPN signals deferred authentication support (by setting
> the internal environment variables "auth_control_file" and
> "deferred_auth_pam"), do not wait for PAM stack to finish.  Instead,
> the privileged PAM process returns RESPONSE_DEFER via the control
> socket, which gets turned into OPENVPN_PLUGIN_FUNC_DEFERRED towards
> openvpn.
>
> The PAM process will then fork() and handle all the PAM auth in
> the new process, signalling success/failure back by means of the
> auth_control_file (forking twice, to simplify wait() handling).
>
> With the extra fork(), multiple deferred authentications can run at
> the same time - otherwise the first one would block the next auth
> call (because the child would not be ready again to read from the
> control socket).
>
> Lightly tested on Linux.
>
> Signed-off-by: Gert Doering 
>
> --
> v2:
>   - only do deferred auth if "deferred_auth_pam" is set (env)
>   - put deferred auth logic into do_deferred_pam_auth()
>   - line-wrap lines where needed
>   - close "background end" of socketpair in deferred auth process
>   - remove leftover /* plugin_log() */ lines from initial testing
>   - tested over a few hundred "15s delayed" authentication cycles
>
> v3:
>   - uncrustify new code
>   - do not abort background process if do_deferred_pam_auth() fails
> (this can only happen if fork() fails, which is assumed to be
> temporary, or if something is wrong with the socketpair which we
> should notice on the next read()) --> change do_deferred_pam_auth()
> to "void"
>   - add documentation to README.auth-pam and Changes.rst
> ---
>  Changes.rst  |   3 +
>  src/plugins/auth-pam/README.auth-pam |  35 
>  src/plugins/auth-pam/auth-pam.c  | 122 ++-
>  3 files changed, 157 insertions(+), 3 deletions(-)
>
> diff --git a/Changes.rst b/Changes.rst
> index 00dd6ed8..8d9d74c1 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -13,6 +13,9 @@ ChaCha20-Poly1305 cipher support
>  Added support for using the ChaCha20-Poly1305 cipher in the OpenVPN data
>  channel.
>
> +Asynchronous (deferred) authentication support for auth-pam plugin.
> +See src/plugins/auth-pam/README.auth-pam for details.
> +
>
>  Overview of changes in 2.4
>  ==
> diff --git a/src/plugins/auth-pam/README.auth-pam 
> b/src/plugins/auth-pam/README.auth-pam
> index 4d3d4ecc..e604d5a8 100644
> --- a/src/plugins/auth-pam/README.auth-pam
> +++ b/src/plugins/auth-pam/README.auth-pam
> @@ -73,6 +73,41 @@ underlying PAM module.  This is a useful debugging tool to 
> figure
>  out which queries a given PAM module is making, so that you can
>  craft the appropriate plugin directive to answer it.
>
> +Since running OpenVPN with verb 7 is quite verbose, alternatively
> +you can put
> +
> +   verb 3
> +   setenv verb 9
> +
> +in the openvpn config which will only increase logging for this plugin.
> +
> +
> +ASYNCHRONOUS OPERATION
> +
> +Sometimes PAM modules take very long to complete (for example, a LDAP
> +or Radius query might timeout trying to connect an unreachable external
> +server).  Normal plugin auth operation will block the whole OpenVPN
> +process in this time, that is, all forwarding for all other client stops.
> +
> +The auth-pam plugin can operate asynchronously ("deferred authentication")
> +to remedy this situation.  To enable this, put
> +
> +  setenv deferred_auth_pam 1
> +
> +in your openvpn server config.  If set, this will make the "PAM background
> +process" fork() and do its job detached from OpenVPN.  When finished, a
> +status file is written, which OpenVPN will then pick up and read the
> +success/failure result from it.
> +
> +While the plugin is working in the background, OpenVPN will continue to
> +service other clients normally.
> +
> +Asynchronous operation is recommended for all PAM queries that could
> +"take time" (LDAP, Radius, NIS, ...).  If only local files are queried
> +(passwd, pam_userdb, ...), synchronous operation has slightly lower
> +overhead, so this is still the default mode of operation.
> +
> +
>  CAVEATS
>
>  This module will only work on *n

Re: [Openvpn-devel] [PATCH v2] Add deferred authentication support to plugin-auth-pam

2020-07-14 Thread Selva Nair
vice, const struct user_pass *up)
>  return ret;
>  }
>
> +/*
> + * deferred auth handler
> + *   - fork() (twice, to avoid the need for async wait / SIGCHLD handling)
> + *   - query PAM stack via pam_auth()
> + *   - send response back to OpenVPN via "ac_file_name"
> + *
> + * parent process returns "0" for "fork() and wait() succeeded",
> + *"-1" for "something went wrong, abort program"
> + */
> +
> +static int
> +do_deferred_pam_auth(int fd, const char *ac_file_name,
> + const char *service, const struct user_pass *up)
> +{
> +if (send_control(fd, RESPONSE_DEFER) == -1)
> +{
> +plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: write error on 
> response socket [4]");
> +return -1;
> +}
> +
> +/* double forking so we do not need to wait() for async auth kids */
> +pid_t p1 = fork();
> +
> +if ( p1 < 0 )
> +{
> +plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: fork(1) 
> failed");
> +return -1;
> +}
> +if ( p1 != 0 ) /* parent */

Same here..

> +{
> +waitpid(p1, NULL, 0);
> +return 0;  /* parent's job succeeded */
> +}
> +
> +/* child */
> +close(fd);  /* socketpair no longer needed */
> +
> +pid_t p2 = fork();
> +if ( p2 < 0 )

and here

> +{
> +plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: fork(2) 
> failed");
> +exit(1);
> +}
> +
> +if ( p2 != 0 )  /* new parent: exit right away */

ditto..

> +{
> +exit(0);
> +}
> +
> +/* grandchild */
> +plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: deferred auth for '%s', 
> pid=%d",
> +   up->username, (int) getpid() );

extra space before the closing )

> +
> +/* the rest is very simple: do PAM, write status byte to file, done */
> +int ac_fd = open( ac_file_name, O_WRONLY );

and around here

> +if ( ac_fd < 0 )

and here

> +{
> +plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "cannot open '%s' for 
> writing",
> +   ac_file_name );

Space before the closing )

> +exit(1);
> +}
> +int pam_success = pam_auth(service, up);
> +
> +if ( write( ac_fd, pam_success? "1": "0", 1 ) != 1 )

and here

> +{
> +plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "cannot write to '%s'",
> +   ac_file_name );

and before the last )

> +}
> +close(ac_fd);
> +plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: %s: deferred auth: PAM %s",
> +   up->username, pam_success? "succeeded": "rejected" );

Same here before the closing ")"

> +exit(0);
> +}
> +
>  /*
>   * Background process -- runs with privilege.
>   */
> @@ -789,6 +891,7 @@ static void
>  pam_server(int fd, const char *service, int verb, const struct 
> name_value_list *name_value_list)
>  {
>  struct user_pass up;
> +char ac_file_name[PATH_MAX];
>  int command;
>  #ifdef USE_PAM_DLOPEN
>  static const char pam_so[] = "libpam.so";
> @@ -847,7 +950,8 @@ pam_server(int fd, const char *service, int verb, const 
> struct name_value_list *
>  case COMMAND_VERIFY:
>  if (recv_string(fd, up.username, sizeof(up.username)) == -1
>  || recv_string(fd, up.password, sizeof(up.password)) == 
> -1
> -|| recv_string(fd, up.common_name, 
> sizeof(up.common_name)) == -1)
> +|| recv_string(fd, up.common_name, 
> sizeof(up.common_name)) == -1
> +|| recv_string(fd, ac_file_name, sizeof(ac_file_name)) 
> == -1)
>  {
>  plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: 
> read error on command channel: code=%d, exiting",
>  command);
> @@ -867,6 +971,22 @@ pam_server(int fd, const char *service, int verb, const 
> struct name_value_list *
>  /* If password is of the form SCRV1:base64:base64 split it 
> up */
>  split_scrv1_password();
>
> +/* client wants deferred auth
> + */
> +if ( strlen(ac_file_name) > 0 )
> +{
> +if (do_deferred_pam_auth(fd, ac_file_name,
> + service, ) < 0)
&g

Re: [Openvpn-devel] [Openvpn-users] Multiple DNS search suffixes on Windows

2020-06-23 Thread Selva Nair
Hi,

On Tue, Jun 23, 2020 at 3:22 AM Jan Just Keijser  wrote:
>
> Hi,
>
> On 21/06/20 17:14, Selva Nair wrote:
> > On Sun, Jun 21, 2020 at 7:14 AM Gert Doering  wrote:
> >>
> >> going through OpenVPN threads that went stale - I think this is
> >> actually a nice addition (read: other people have already asked
> >> me if this can be done).
> >>
> >> On Thu, Mar 05, 2020 at 01:53:12PM +0100, Jan Just Keijser wrote:
> >>> So, for what it's worth, I've dusted off the patch again and rebased it
> >>> to the current openvpn master tree. See attached. Note that I did only
> >>> rudimentary testing, as I don't use Windows 10 a lot and I was testing
> >>> using a mingw cross-compile only. In wireshark I *do* see that the
> >>> correct DHCP offer is sent to the tap-win adapter.
> >>>
> >>> Also note that I implemented multiple search domains by separating them
> >>> using semi-colons, e.g.
> >>>
> >>>  --dhcp-option SEARCH example.com;example.org;example.nl;example.de
> >>>
> >>> etc as that was easier to implement
> >> The patch looks okay-ish on quick reading.
> > Feature wise yes, but some improvements needed..
> >
> > I do not like that option format
> >
> > "example.com; example.org" with quotes will parse right and fail later
> > on because of the space, for example. And, we support multiple
> > statements of dhcp options like DNS, so this is counter-intuitive.
> > It's only a little more work to support a more forgiving format.
> actually, the patch will be a bit more invasive if we add that; parsing
> DNS/WINS etc is already there and it parses an option into an IP/IPv6
> address. changing the dns-search option to an array of strings will
> require more work. I did look into that initially but decided not to do
> it as the patch would be lengthier.
>
> Note that "multiple domain" option is also not support, really. The
> domain is stored in the tuntap_options struct as
>
>   90 const char *domain;  /* DOMAIN (15) */

Windows is probably the only platform where these options are "natively"
supported through DHCP, and indeed DOMAIN (option 15) has to  be a
single domain. Android also seem to treat them internally. However, we
export these as foreign_option in all other platforms, and do export anything
that looks like "dhcp-option name value" with repetitions allowed. That said, I
don't mind breaking scripts that use and misuse these...

Anyway my point about multiple values was about DNS and WINS where
it's allowed, and we parse them one entry at a time. I believe
this option (search list) also should use the same format instead of
inventing yet another format.

Yes, it may need a bit "more work" to support one domain at a time, but that's
no excuse not to use a sensible format. Concatenation of strings read one at a
time with a delimiter added doesn't look that hard either.

>
> so it's "last option wins"  ; the fact that some platforms abuse this
> option to actually set multiple search domains is a different matter.
>
> >>> Also note that I did not fully implement the RFC3397 encoding of the
> >>> search list, as that requires one to merge domain names that occur more
> >>> than once - that would have made the code far more complicated.
> >> Indeed.  I haven't looked at what other DHCP implementations do, but
> >> "correct" encoding definitely sounds like quite a bit of extra code just
> >> to save a few bytes on the wire - might come handy if you have many
> >> subdomains of a long internal DNS domain, though, but this can be
> >> added "if needed".
> > Same as my thoughts, the encoding part could be kept simple as in here
> > and possibly improved later. But option format is hard to change or
> > deprecate.
> >
> >>
> >> More interesting is the question "which option to use" - it should
> >> be synchronized between openvpn platform handlers.  So if systemd-networkd
> >> uses "SEARCH-DOMAIN" it would make sense to use that for windows
> >> as well.
> > I think we need both --- the current one retained as the connection
> > specific suffix which would be just one entry and then this search
> > list. As we allow multiple entries for DOMAIN right now, a user
> > friendly approach would be to continue doing so but internally treat
> > all but the first as a part of --dhcp-option DOMAIN-SEARCH. We could
> > also deprecate the use of multiple entries in the dhcp-option DOMAIN
> > keyword.
> >
> See above: like I said, multiple DOMAIN entries do not seem to be supported.
>
> So what option do we want?
>
> --dhcp-option SEARCH
> --dhcp-option DOMAIN-SEARCH
> --dhcp-option SEARCH-DOMAIN

RFC 3397 calls it "Domain Search" so it has to be DOMAIN-SEARCH, in my
view.  Platform scripts accepting other forms in foreign_option is up
to them. We don't have to officially support that.


Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] systemd: Change the default cipher to AES-256-GCM for server configs

2020-06-22 Thread Selva Nair
On Mon, Jun 22, 2020 at 7:31 AM David Sommerseth  wrote:
>
> This change makes the server use AES-256-GCM instead of BF-CBC as the
> default cipher for the VPN tunnel when starting OpenVPN via systemd
> and the openvpn-server@.service unit file.
>
> To avoid breaking existing running configurations defaulting to BF-CBC,
> the Negotiable Crypto Parameters (NCP) list contains the BF-CBC in
> addition to AES-CBC.  This makes it possible to migrate existing older
> client configurations one-by-one to use at least AES-CBC unless the
> client is updated to v2.4 or newer (which defaults to upgrade to
> AES-GCM automatically)
>
> This has been tested in Fedora 27 (released November 2017) with no
> reported issues.  By making this default for all Linux distributions
> with systemd shipping with the unit files we provide, we gradually
> expand setups using this possibility.  As we gather experience from
> this change, we can further move these changes into the defaults of
> the OpenVPN binary itself with time.
>
> Signed-off-by: David Sommerseth 
> ---
>  Changes.rst   | 15 +++
>  distro/systemd/openvpn-ser...@.service.in |  2 +-
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/Changes.rst b/Changes.rst
> index 00dd6ed8..e76d3c73 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -14,6 +14,21 @@ ChaCha20-Poly1305 cipher support
>  channel.
>
>
> +User-visible Changes
> +
> +New default cipher for systemd based Linux distributions
> +For Linux distributions with systemd which packages the systemd unit 
> files
> +from the OpenVPN project, the default cipher is now changed to 
> AES-256-GCM,
> +with BF-CBC as a fallback through the NCP feature.  This change has been
> +tested successfully since the Fedora 27 release (released November 2017).
> +
> +*WARNING*   This MAY break configurations where the client uses
> +``--disable-occ`` feature where the ``--cipher`` has
> +not been explicitly configured on both client and
> +server side.  It is recommended to remove the 
> ``--disable-occ``
> +option *or* explicitly add ``--cipher AES-256-GCM`` on the
> +client side if ``--disable-occ`` is strictly needed.
> +
>  Overview of changes in 2.4
>  ==
>
> diff --git a/distro/systemd/openvpn-ser...@.service.in 
> b/distro/systemd/openvpn-ser...@.service.in
> index d1cc72cb..f3545ff5 100644
> --- a/distro/systemd/openvpn-ser...@.service.in
> +++ b/distro/systemd/openvpn-ser...@.service.in
> @@ -10,7 +10,7 @@ 
> Documentation=https://community.openvpn.net/openvpn/wiki/HOWTO
>  Type=notify
>  PrivateTmp=true
>  WorkingDirectory=/etc/openvpn/server
> -ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log 
> --status-version 2 --suppress-timestamps --config %i.conf
> +ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log 
> --status-version 2 --suppress-timestamps --cipher AES-256-GCM --ncp-ciphers 
> AES-256-GCM:AES-128-GCM:AES-256-CBC:AES-128-CBC:BF-CBC --config %i.conf

This is why I keep my openvpn servers out of systemd's view -- it
keeps deciding what's good for us. I want to run my configs as is.

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Convert plugin/auth-pam.c from stderr logging to plugin_log().

2020-06-21 Thread Selva Nair
Hi,

This was long overdue after patches after patches sprinkling fprintf() all
over the place.. mea culpa too..

Acked-by: Selva Nair 

On Sat, Jun 20, 2020 at 11:18 AM Gert Doering  wrote:
>
> More recent OpenVPN APIs pass a function pointer for a logging function
> (plugin_log()) to plugins.  Using this will make the plugin logs appear
> wherever openvpn logs to - file, syslog, stderr.
>
> This patch converts plugin/auth-pam.c "fairly mechanically" to use this
> new API.  Real errors are logged with PLOG_ERR or PLOG_ERR|PLOG_ERRNO,
> while debug info is logged with PLOG_NOTE (subject to the already-existing
> debug level handling inside plugin/auth-pam, via "setenv verb ").
>
> Signed-off-by: Gert Doering 
> ---
>  src/plugins/auth-pam/auth-pam.c | 62 +++--
>  1 file changed, 35 insertions(+), 27 deletions(-)
>
> diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
> index 88b53204..9a11876d 100644
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -64,9 +64,13 @@
>  #define RESPONSE_VERIFY_FAILED13
>
>  /* Pointers to functions exported from openvpn */
> +static plugin_log_t plugin_log = NULL;
>  static plugin_secure_memzero_t plugin_secure_memzero = NULL;
>  static plugin_base64_decode_t plugin_base64_decode = NULL;
>
> +/* module name for plugin_log() */
> +static char * MODULE = "AUTH-PAM";
> +
>  /*
>   * Plugin state, used by foreground
>   */
> @@ -211,7 +215,7 @@ daemonize(const char *envp[])
>  }
>  if (daemon(0, 0) < 0)
>  {
> -fprintf(stderr, "AUTH-PAM: daemonization failed\n");
> +plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "daemonization failed");
>  }
>  else if (fd >= 3)
>  {
> @@ -297,7 +301,7 @@ split_scrv1_password(struct user_pass *up)
>  char *tmp = strdup(up->password);
>  if (!tmp)
>  {
> -fprintf(stderr, "AUTH-PAM: out of memory parsing static challenge 
> password\n");
> +plugin_log(PLOG_ERR, MODULE, "out of memory parsing static challenge 
> password");
>  goto out;
>  }
>
> @@ -319,7 +323,7 @@ split_scrv1_password(struct user_pass *up)
>  up->response[n] = '\0';
>  if (DEBUG(up->verb))
>  {
> -fprintf(stderr, "AUTH-PAM: BACKGROUND: parsed static 
> challenge password\n");
> +plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: parsed static 
> challenge password");
>  }
>  goto out;
>  }
> @@ -330,7 +334,7 @@ split_scrv1_password(struct user_pass *up)
>  plugin_secure_memzero(up->response, sizeof(up->response));
>  strcpy(up->password, tmp); /* tmp is guaranteed to fit in up->password */
>
> -fprintf(stderr, "AUTH-PAM: base64 decode error while parsing static 
> challenge password\n");
> +plugin_log(PLOG_ERR, MODULE, "base64 decode error while parsing static 
> challenge password");
>
>  out:
>  if (tmp)
> @@ -379,6 +383,7 @@ openvpn_plugin_open_v3(const int v3structver,
>  ret->type_mask = 
> OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY);
>
>  /* Save global pointers to functions exported from openvpn */
> +plugin_log = args->callbacks->plugin_log;
>  plugin_secure_memzero = args->callbacks->plugin_secure_memzero;
>  plugin_base64_decode = args->callbacks->plugin_base64_decode;
>
> @@ -388,7 +393,7 @@ openvpn_plugin_open_v3(const int v3structver,
>   */
>  if (string_array_len(argv) < base_parms)
>  {
> -fprintf(stderr, "AUTH-PAM: need PAM service parameter\n");
> +plugin_log(PLOG_ERR, MODULE, "need PAM service parameter");
>  goto error;
>  }
>
> @@ -404,7 +409,7 @@ openvpn_plugin_open_v3(const int v3structver,
>
>  if ((nv_len & 1) == 1 || (nv_len / 2) > N_NAME_VALUE)
>  {
> -fprintf(stderr, "AUTH-PAM: bad name/value list length\n");
> +plugin_log(PLOG_ERR, MODULE, "bad name/value list length");
>  goto error;
>  }
>
> @@ -434,7 +439,7 @@ openvpn_plugin_open_v3(const int v3structver,
>   */
>  if (socketpair(PF_UNIX, SOCK_DGRAM, 0, fd) == -1)
>  {
> -fprintf(stderr, "AUTH-PAM: socketpair call failed\n");
> +plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "socketpair call failed");
>  goto error;
>  }
>
> @@ -460,7 +465,7 @@ openvpn_plugin_ope

Re: [Openvpn-devel] [PATCH] Add deferred authentication support to plugin-auth-pam

2020-06-21 Thread Selva Nair
Hi,

On Sat, Jun 20, 2020 at 12:23 PM Gert Doering  wrote:
>
> If OpenVPN signals deferred authentication support (by setting the
> internal environment variable "auth_control_file"), do not wait
> for PAM stack to finish.  Instead, the privileged PAM process
> returns RESPONSE_DEFER via the control socket, which gets turned
> into OPENVPN_PLUGIN_FUNC_DEFERRED towards openvpn.
>
> The PAM process will then fork() and handle all the PAM auth in the
> new process, signalling success/failure back by means of the
> auth_control_file (forking twice, to simplify wait() handling).
>
> With the extra fork(), multiple deferred authentications can run at
> the same time - otherwise the first one would block the next auth
> call (because the child would not be ready again to read from the
> control socket).

I have no doubt this will work, but can we avoid the double fork? Instead,
we could set a handler for SIGCHLD and reap all child pids there using
a non blocking waitpid(-1, NULL, WNOHANG). Will have to also check for
EINTR and retry in recv_control() and possibly recv_string().

Fork is probably not expensive, but two of them per auth event look
excessive and not very elegant, somehow.

>
> Lightly tested on Linux.
>
> Signed-off-by: Gert Doering 
> ---
>  src/plugins/auth-pam/auth-pam.c | 88 -
>  1 file changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
> index 9a11876d..777957fa 100644
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -62,6 +62,7 @@
>  #define RESPONSE_INIT_FAILED  11
>  #define RESPONSE_VERIFY_SUCCEEDED 12
>  #define RESPONSE_VERIFY_FAILED13
> +#define RESPONSE_DEFER14
>
>  /* Pointers to functions exported from openvpn */
>  static plugin_log_t plugin_log = NULL;
> @@ -524,12 +525,21 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, 
> const int type, const cha
>  const char *password = get_env("password", envp);
>  const char *common_name = get_env("common_name", envp) ? 
> get_env("common_name", envp) : "";
>
> +/* get auth_control_file filename from envp string array*/
> +const char *auth_control_file = get_env("auth_control_file", envp);
> +if ( auth_control_file == NULL )
> +{
> +auth_control_file = "";
> +}
> +plugin_log(PLOG_NOTE, MODULE, "deferred auth '%s'", 
> auth_control_file);
> +
>  if (username && strlen(username) > 0 && password)
>  {
>  if (send_control(context->foreground_fd, COMMAND_VERIFY) == -1
>  || send_string(context->foreground_fd, username) == -1
>  || send_string(context->foreground_fd, password) == -1
> -|| send_string(context->foreground_fd, common_name) == -1)
> +|| send_string(context->foreground_fd, common_name) == -1
> +|| send_string(context->foreground_fd, auth_control_file) == 
> -1)
>  {
>  plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "Error sending auth 
> info to background process");
>  }
> @@ -540,6 +550,11 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, 
> const int type, const cha
>  {
>  return OPENVPN_PLUGIN_FUNC_SUCCESS;
>  }
> +if (status == RESPONSE_DEFER)
> +{
> +plugin_log(PLOG_NOTE, MODULE, "deferred authentication 
> active");
> +return OPENVPN_PLUGIN_FUNC_DEFERRED;
> +}
>  if (status == -1)
>  {
>  plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "Error receiving 
> auth confirmation from background process");
> @@ -789,6 +804,7 @@ static void
>  pam_server(int fd, const char *service, int verb, const struct 
> name_value_list *name_value_list)
>  {
>  struct user_pass up;
> +char ac_file_name[PATH_MAX];
>  int command;
>  #ifdef USE_PAM_DLOPEN
>  static const char pam_so[] = "libpam.so";
> @@ -847,7 +863,8 @@ pam_server(int fd, const char *service, int verb, const 
> struct name_value_list *
>  case COMMAND_VERIFY:
>  if (recv_string(fd, up.username, sizeof(up.username)) == -1
>  || recv_string(fd, up.password, sizeof(up.password)) == 
> -1
> -|| recv_string(fd, up.common_name, 
> sizeof(up.common_name)) == -1)
> +|| recv_string(fd, up.common_name, 
> sizeof(up.common_name)) == -1
> +|| recv_string(fd, ac_file_name, sizeof(ac_file_name)) 
> == -1)
>  {
>  plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: 
> read error on command channel: code=%d, exiting",
>  command);
> @@ -867,6 +884,73 @@ pam_server(int fd, const char *service, int verb, const 
> struct name_value_list *
>   

Re: [Openvpn-devel] [Openvpn-users] Multiple DNS search suffixes on Windows

2020-06-21 Thread Selva Nair
Hi,

On Sun, Jun 21, 2020 at 7:14 AM Gert Doering  wrote:
>
> Hi,
>
> going through OpenVPN threads that went stale - I think this is
> actually a nice addition (read: other people have already asked
> me if this can be done).
>
> On Thu, Mar 05, 2020 at 01:53:12PM +0100, Jan Just Keijser wrote:
> > So, for what it's worth, I've dusted off the patch again and rebased it
> > to the current openvpn master tree. See attached. Note that I did only
> > rudimentary testing, as I don't use Windows 10 a lot and I was testing
> > using a mingw cross-compile only. In wireshark I *do* see that the
> > correct DHCP offer is sent to the tap-win adapter.
> >
> > Also note that I implemented multiple search domains by separating them
> > using semi-colons, e.g.
> >
> > --dhcp-option SEARCH example.com;example.org;example.nl;example.de
> >
> > etc as that was easier to implement
>
> The patch looks okay-ish on quick reading.

Feature wise yes, but some improvements needed..

I do not like that option format

"example.com; example.org" with quotes will parse right and fail later
on because of the space, for example. And, we support multiple
statements of dhcp options like DNS, so this is counter-intuitive.
It's only a little more work to support a more forgiving format.

>
> > Also note that I did not fully implement the RFC3397 encoding of the
> > search list, as that requires one to merge domain names that occur more
> > than once - that would have made the code far more complicated.
>
> Indeed.  I haven't looked at what other DHCP implementations do, but
> "correct" encoding definitely sounds like quite a bit of extra code just
> to save a few bytes on the wire - might come handy if you have many
> subdomains of a long internal DNS domain, though, but this can be
> added "if needed".

Same as my thoughts, the encoding part could be kept simple as in here
and possibly improved later. But option format is hard to change or
deprecate.

>
>
> More interesting is the question "which option to use" - it should
> be synchronized between openvpn platform handlers.  So if systemd-networkd
> uses "SEARCH-DOMAIN" it would make sense to use that for windows
> as well.

I think we need both --- the current one retained as the connection
specific suffix which would be just one entry and then this search
list. As we allow multiple entries for DOMAIN right now, a user
friendly approach would be to continue doing so but internally treat
all but the first as a part of --dhcp-option DOMAIN-SEARCH. We could
also deprecate the use of multiple entries in the dhcp-option DOMAIN
keyword.

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] async plugin-auth-pam

2020-06-12 Thread Selva Nair
On Tue, Jun 9, 2020 at 1:55 PM Gert Doering  wrote:

> Hi,
>
> I ran into a problem at a customer installation recently, where
> plugin-auth-pam was blocking for some extended time (~30 seconds?)
> due to pam_radius not receiving answers due to problems in the backend.
>
> Now, maybe I should use radiusplugin in the first place, but since
> the pam_radius setup on this machine is shared between sshd and OpenVPN,
> I actually *like* using plugin-auth-pam -> pam_radius ("test one service,
> know that radius very likely works for both").
>
> That said, I'm considering modifying the plugin-auth-pam plugin to
> add async authentication - which is supposedly not so hard
> ("sample-plugins/defer/simple.c").
>
> Has one of you already done this, and just forgot to send in patches? :-)
>
> Any particular caveats?
>

I do not have a patch, but had briefly considered this while looking into
handling "dynamic challenge" through the plugin.

Just deferring an authentication is easy but the server will still stall if
another client comes in during that deferred period as pam auth is handled
by a single process. The plugin forks leaving a child running as root, and
talks to it via a scoket. The latter does the pam authentication. As pam
runs within that process, it will still be waiting for the authentication
to complete in a blocking call to pam_authenticate(). I felt it will take
considerable effort to make it handle pam auth asynchronously.

Things may be easier if modules that take long to verify the credentials,
return something like PAM_INCOMPLETE, so that it can be called back later.
But that is beyond our control, and I'm not familiar with async support
within PAM or in PAM modules.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2] Allow repeated cycles through remotes when management-query-remote is in use

2020-05-15 Thread selva . nair
From: Selva Nair 

(i) Let the management-client predictably cycle through remote entries. This
is done by not aborting after two cycles. The client can abort or restart
the connection  using  signals (USR/HUP/TERM) as necessary.

In the current behaviour, the daemon can unexpectedly exit when the last remote
is skipped. When management-query-remote is not in use, the behaviour is
unchanged.

(ii) Do not count skipping a remote as an unsuccessful connection attempt.
As the latter count is used for backoff it should count only failed attempts.

Signed-off-by: Selva Nair 
---
 - rebased to master, no changes

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

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 70cd493..1bb39ee 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -471,14 +471,6 @@ next_connection_entry(struct context *c)
 c->c1.link_socket_addr.remote_list;
 }
 
-/*
- * Increase the number of connection attempts
- * If this is connect-retry-max * size(l)
- * OpenVPN will quit
- */
-
-c->options.unsuccessful_attempts++;
-
 if (++l->current >= l->len)
 {
 
@@ -504,6 +496,9 @@ next_connection_entry(struct context *c)
 {
 /* allow management interface to override connection entry details 
*/
 ce_defined = ce_management_query_remote(c);
+
+/* ignore cycles when management-query-remote is in use */
+n_cycles = 0;
 if (IS_SIG(c))
 {
 break;
@@ -520,6 +515,7 @@ next_connection_entry(struct context *c)
 #endif
 } while (!ce_defined);
 
+c->options.unsuccessful_attempts++;
 /* Check if this connection attempt would bring us over the limit */
 if (c->options.connect_retry_max > 0
 && c->options.unsuccessful_attempts > (l->len  * 
c->options.connect_retry_max))
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 2/2] Allow repeated cycles through remotes when management-query-remote is in use

2020-05-13 Thread Selva Nair
Hi,

On Wed, May 13, 2020 at 12:36 PM Gert Doering  wrote:
>
> Hi,
>
> On Sun, Jun 09, 2019 at 03:33:55PM -0400, Selva Nair wrote:
> > Ref: https://patchwork.openvpn.net/project/openvpn2/list/?series=201
> >
> > These patches were meant to help implement choosing the remote through
> > the GUI. I may not find time for that but the patches by themselves
> > are still relevant.
> >
> > If there is some interest I'll rebase to master.
>
> I'm working my way through the patch queue these days, and now I'm
> at this one :-)
>
> Can you elaborate a bit how this would work, and how much work on the
> GUI side would be needed?  (And, yes, a rebased patch :) ).

>From what I can recall...

Two points to note:

(i) With multiple remotes, openvpn exits if no successful connection
could be made after two cycles through all remotes (undocumented?) .
(ii) When --management-query-remote is used, the core presents one
remote at a time and the user has to make a choice to skip, accept or
replace without knowing which remotes are available.

Now, for a user-friendly implementation of selecting the remote from a
GUI dialog, the plan is to silently cycle through all remotes, make a
list and then allow the user to make a selection from the list. This
will be aided by having a safe way to cycle through all remotes
multiple times without the core exiting --- arguably, one cycle is
enough to make a list and the list building is complete when the
second cycle starts. But it would be much easier to do this without
having to worry about the core exiting unexpectedly. The GUI knows how
to restart or terminate the core exit if need be.

The behaviour is unchanged if management-query-remote is not in use.

The patch also changes the way failed connections are counted: skipped
remotes should not be counted as failed as that count is used in the
back-off logic.

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH applied] Re: Parse static challenge response in auth-pam plugin

2020-04-23 Thread Selva Nair
Hi,

On Tue, Aug 7, 2018 at 3:01 PM Gert Doering  wrote:
>
> Your patch has been applied to the master branch.
>
> (I'm a bit undecided about release/2.4 - this is in "new feature!" land,
> and all the challenge stuff is "master" territory.  OTOH, it's not openvpn
> main code, and the code is sane enough - so if folks think it should be
> in release/2.4, tell me)
>
> commit 7369d01bf360bcfa02f26c05b86dde5496d120f6 (master)

I failed to respond to this that time.

Although this is a new feature, I too think its safe and useful to
include this in 2.4.
If we do, we'll need this one (commit
7369d01bf360bcfa02f26c05b86dde5496d120f6) and the followup one
7a8109023f4c345fe12f23421c5fa7e88e1ea85b

Both should cherry-pick without conflicts.

See also Trac #1275 https://community.openvpn.net/openvpn/ticket/1275

Thanks,

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH applied] Re: Skip expired certificates in Windows certificate store

2020-04-15 Thread Selva Nair
Hi,

> is this one and aa6affe6df811db11577847366a569def0a3e314 also material
> for release/2.4?  So "feature" or "bug" category?

Yes it would be good to get this one and aa6affe into 2.4. This one
will cherry-pick with a minor conflict in cryptoapicert.c, easily
resolved. aa6affe should cherry-pick with no issues.

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password query the management interface (if available).

2020-04-03 Thread selva . nair
From: Selva Nair 

When only username is found in the file, redirect the auth-user-pass
query to the management interface if management-query-passwords is
enabled.  Otherwise the user is prompted on console, if available,
as before.

This changes the behaviour for those who run from the command line,
with --management-query-passwords, but still expect the prompt
on the console.

Note that the management interface will prompt for both username and
password ignoring the username read from the file. As most GUIs can
save the the username, this is a one-time inconvenience.

Currently, the password is queried on the console (or systemd)
in such cases. This is not sensible when console is not available
(windows GUI, tunnelblick etc.) or when the log is redirected
to a file on Windows (for some reason prompt goes to the log file).

Trac # 757

Signed-off-by: Selva Nair 
---
This may be cherry-picked from 57578310992d1fbe8eff97049087c5308089acb5
in master without conflicts.

 src/openvpn/misc.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 2b0d10c..9c5e96e 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -1030,6 +1030,22 @@ get_user_pass_cr(struct user_pass *up,
 {
 strncpy(up->password, password_buf, USER_PASS_LEN);
 }
+/* The auth-file does not have the password: get both username
+ * and password from the management interface if possible.
+ * Otherwise set to read password from console.
+ */
+#if defined(ENABLE_MANAGEMENT)
+else if (management
+ && (flags & GET_USER_PASS_MANAGEMENT)
+ && management_query_user_pass_enabled(management))
+{
+msg(D_LOW, "No password found in %s authfile '%s'. Querying 
the management interface", prefix, auth_file);
+if (!auth_user_pass_mgmt(up, prefix, flags, auth_challenge))
+{
+return false;
+}
+}
+#endif
 else
 {
 password_from_stdin = 1;
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH for-2.4 1/2] Move querying username/password from management interface to a function

2020-04-03 Thread selva . nair
From: Selva Nair 

This helps the next patch. No functionality changes, only
refactoring.

Same as commit 461e566fb274d6f7647dc3aa81c02e4fbf362a23 in master
except for additional ifdef ENABLE_CLIENT_CR

Signed-off-by: Selva Nair 
---
 src/openvpn/misc.c | 61 ++
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index f44c65f..2b0d10c 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -880,6 +880,43 @@ absolute_pathname(const char *pathname)
 }
 }
 
+#ifdef ENABLE_MANAGEMENT
+
+/* Get username/password from the management interface */
+static bool
+auth_user_pass_mgmt(struct user_pass *up, const char *prefix, const unsigned 
int flags,
+const char *auth_challenge)
+{
+const char *sc = NULL;
+
+if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
+{
+management_auth_failure(management, prefix, "previous auth credentials 
failed");
+}
+
+#ifdef ENABLE_CLIENT_CR
+if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
+{
+sc = auth_challenge;
+}
+#endif
+
+if (!management_query_user_pass(management, up, prefix, flags, sc))
+{
+if ((flags & GET_USER_PASS_NOFATAL) != 0)
+{
+return false;
+}
+else
+{
+msg(M_FATAL, "ERROR: could not read %s username/password/ok/string 
from management interface", prefix);
+}
+}
+return true;
+}
+
+#endif /* ifdef ENABLE_MANAGEMENT */
+
 /*
  * Get and store a username/password
  */
@@ -913,30 +950,10 @@ get_user_pass_cr(struct user_pass *up,
 && (!from_authfile && (flags & GET_USER_PASS_MANAGEMENT))
 && management_query_user_pass_enabled(management))
 {
-const char *sc = NULL;
 response_from_stdin = false;
-
-if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
-{
-management_auth_failure(management, prefix, "previous auth 
credentials failed");
-}
-
-#ifdef ENABLE_CLIENT_CR
-if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
+if (!auth_user_pass_mgmt(up, prefix, flags, auth_challenge))
 {
-sc = auth_challenge;
-}
-#endif
-if (!management_query_user_pass(management, up, prefix, flags, sc))
-{
-if ((flags & GET_USER_PASS_NOFATAL) != 0)
-{
-return false;
-}
-else
-{
-msg(M_FATAL, "ERROR: could not read %s 
username/password/ok/string from management interface", prefix);
-}
+return false;
 }
 }
 else
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2 2/2] When auth-user-pass file has no password, query the management

2020-04-02 Thread Selva Nair
Hi,

On Thu, Apr 2, 2020 at 12:56 PM Jonathan K. Bullard 
wrote:

> Hi,
>
> On Mon, Mar 30, 2020 at 2:06 PM  wrote:
> >
> > From: Selva Nair 
> >
> > When only username is found in the file, redirect the auth-user-pass
> > query to the management if management-query-passwords is enabled.
> > Otherwise the user is prompted on console, if available, as before.
> >
> > This changes the behaviour for those who run from the command line,
> > with --management-query-passwords, but still expect the prompt
> > on the console.
> >
> > Note that the management will prompt for both username and password
> > ignoring the username read from the file. As most GUIs can save the
> > the username, this is a one-time inconvenience.
> >
> > Currently, the password is queried on the console (or systemd)
> > in such cases. This is not sensible when console is not available
> > (windows GUI, tunnelblick etc.) or when the log is redirected
> > to a file on Windows (for some reason prompt goes to the log file).
> >
> > Trac # 757
>
> snip..


>
> Works for Tunnelblick, thanks!
>

Good to know.


>
> One minor point: in all four places, plus in the email subject, "the
> management" should be changed to "the management interface".
>
>
agreed.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] [PATCH v5] Insert client connection data into PAM environment

2020-03-30 Thread Selva Nair
Hi,

On Mon, Mar 30, 2020 at 8:59 AM Paolo Cerrito  wrote:

> 1) so remote was set to the maxlenght of ipv6 address defined into
> arpa/inet.h  + 1 for string terminator
>
> 2) I refactored the call to get_env to take first ipv6 address, then
>only if it is NULL, i make a call for ipv4
> ---
>  src/plugins/auth-pam/auth-pam.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/plugins/auth-pam/auth-pam.c
> b/src/plugins/auth-pam/auth-pam.c
> index ae0d495a..cd91a33c 100644
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -48,7 +48,7 @@
>  #include 
>  #include 
>  #include "utils.h"
> -
> +#include 

 #include 
>
>  #define DEBUG(verb) ((verb) >= 4)
> @@ -115,7 +115,7 @@ struct user_pass {
>  char password[128];
>  char common_name[128];
>  char response[128];
> -char remote[46]; //46 as ipv6 form n:n:n:n:n:n:d.d.d.d and +
> terminator \0
> +char remote[INET6_ADDRSTRLEN+1]; //INET6_ADDRSTRLEN  is the lenght of
> ipv6 + terminator \0
>

INET6_ADDRSTRLEN = 46 already includes space for nul termination

More importantly, you have to provide a single updated patch
preferably with version indicated in the subject and sent out with
--in-reply-to  referring to the previous version.

Submitting incremental pieces of fixup commits doesn't help. And please
wait for review before making changes unless correcting a critical error.

Thanks,

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2 1/2] Move querying username/password from management to a function

2020-03-30 Thread selva . nair
From: Selva Nair 

This helps the next patch. No functionality changes, only
refactoring.

Signed-off-by: Selva Nair 
---
No changes from v1

 src/openvpn/misc.c | 54 ++
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 1931149..0d5ac30 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -116,6 +116,38 @@ hostname_randomize(const char *hostname, struct gc_arena 
*gc)
 #undef n_rnd_bytes
 }
 
+#ifdef ENABLE_MANAGEMENT
+/* Get username/password from the management interface */
+static bool
+auth_user_pass_mgmt(struct user_pass *up, const char *prefix, const unsigned 
int flags,
+const char *auth_challenge)
+{
+const char *sc = NULL;
+
+if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
+{
+management_auth_failure(management, prefix, "previous auth credentials 
failed");
+}
+
+if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
+{
+sc = auth_challenge;
+}
+if (!management_query_user_pass(management, up, prefix, flags, sc))
+{
+if ((flags & GET_USER_PASS_NOFATAL) != 0)
+{
+return false;
+}
+else
+{
+msg(M_FATAL, "ERROR: could not read %s username/password/ok/string 
from management interface", prefix);
+}
+}
+return true;
+}
+#endif
+
 /*
  * Get and store a username/password
  */
@@ -149,28 +181,10 @@ get_user_pass_cr(struct user_pass *up,
 && (!from_authfile && (flags & GET_USER_PASS_MANAGEMENT))
 && management_query_user_pass_enabled(management))
 {
-const char *sc = NULL;
 response_from_stdin = false;
-
-if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
+if (!auth_user_pass_mgmt(up, prefix, flags, auth_challenge))
 {
-management_auth_failure(management, prefix, "previous auth 
credentials failed");
-}
-
-if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
-{
-sc = auth_challenge;
-}
-if (!management_query_user_pass(management, up, prefix, flags, sc))
-{
-if ((flags & GET_USER_PASS_NOFATAL) != 0)
-{
-return false;
-}
-else
-{
-msg(M_FATAL, "ERROR: could not read %s 
username/password/ok/string from management interface", prefix);
-}
+return false;
 }
 }
 else
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2 2/2] When auth-user-pass file has no password, query the management

2020-03-30 Thread selva . nair
From: Selva Nair 

When only username is found in the file, redirect the auth-user-pass
query to the management if management-query-passwords is enabled.
Otherwise the user is prompted on console, if available, as before.

This changes the behaviour for those who run from the command line,
with --management-query-passwords, but still expect the prompt
on the console.

Note that the management will prompt for both username and password
ignoring the username read from the file. As most GUIs can save the
the username, this is a one-time inconvenience.

Currently, the password is queried on the console (or systemd)
in such cases. This is not sensible when console is not available
(windows GUI, tunnelblick etc.) or when the log is redirected
to a file on Windows (for some reason prompt goes to the log file).

Trac # 757

Signed-off-by: Selva Nair 
---

v2: Following discussions with Jonathan and Gert, removed the dependence
on stdout redirection and applied to all platforms.

 src/openvpn/misc.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 0d5ac30..546cd71 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -261,6 +261,22 @@ get_user_pass_cr(struct user_pass *up,
 {
 strncpy(up->password, password_buf, USER_PASS_LEN);
 }
+/* The auth-file does not have the password: get both username
+ * and password from the management if possible.
+ * Otherwise set to read password from console.
+ */
+#if defined(ENABLE_MANAGEMENT)
+else if (management
+ && (flags & GET_USER_PASS_MANAGEMENT)
+ && management_query_user_pass_enabled(management))
+{
+msg(D_LOW, "No password found in %s authfile '%s'. Querying 
the management", prefix, auth_file);
+if (!auth_user_pass_mgmt(up, prefix, flags, auth_challenge))
+{
+return false;
+}
+}
+#endif
 else
 {
 password_from_stdin = 1;
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password, query the management

2020-03-30 Thread Selva Nair
Hi,

On Mon, Mar 30, 2020 at 12:11 PM Jonathan K. Bullard 
wrote:

> Hi,
>
> On Mon, Mar 30, 2020 at 11:12 AM Selva Nair  wrote:
> > Jonathan K. Bullard  wrote:
> > >
> > > If the OS X command line user was using --management-query-passwords
> > > (as Tunnelblick does), they wouldn't see the password prompt on
> > > /dev/tty, would they?
> >
> > In case of auth-file missing password, they would see it on /dev/tty
> > on linux, and I would guess on OSX as well, but I've not checked.
>
> The password prompt appears on /dev/tty on OS X only if --daemon is not
> used.
>
> If --daemon and --management-query-passwords are used but --askpass is
> not (whether or not --auth-nocache is also used), which is typical for
> a Tunnelblick configuration on OS X, the following appears in the log:
>
>  neither stdin nor stderr are a tty device and you have neither a
>   controlling tty nor systemd - can't ask for 'Enter Auth
> Password:'.
>   If you used --daemon, you need to use --askpass to make
>   passphrase-protected keys work, and you can not use
>   --auth-nocache.
>  Exiting due to fatal error
>
> if --daemon, --management-query-passwords, and --askpass are all used
> (whether or not --auth-nocache is used), you get:
>
>  Need password(s) from management interface, waiting...
>
> If Windows GUI uses --daemon, that could be an additional requirement
> that would work for Tunnelblick and OS X, which would mean one less
> incompatibility between Windows and OS X.
>

--daemon is a unix/linux option (not supported on Windows) and after
deamonizing there is no controlling tty leading to the behaviour you mention
above. I think that's documented.


> Or it could test for Windows || (OS X && --daemon).
>

Personally I would prefer to enable this code for all platforms although
its a minor regression.

That is, if management-query-passwords is enabled and auth file is
missing password, query the management, not on console irrespective
of other options and OS. If that's acceptable, I'll submit a v2.

Selva



> Best regards,
>
> Jon Bullard
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password, query the management

2020-03-30 Thread Selva Nair
Hi,

On Mon, Mar 30, 2020 at 2:07 AM Gert Doering  wrote:
>
> Hi,
>
> On Sun, Mar 29, 2020 at 07:58:15PM -0400, Selva Nair wrote:
> > Yes, that's right. However, that logic wont be proper on OS-X, would it?
> > Command line users who use --log can still see password
> > prompt on /dev/tty. We'll be breaking that behaviour.
> >
> > I considered checking for env vars like IV_UI_VER set by the UI
> > client, but that's not readily accessible from auth_user_pass_cr()
> > call. Alternatives like checking whether /dev/tty can be opened and/or
> > systemd is available didn't appeal to me. If at all, that would have
> > to be a separate patch.
>
> Not sure if the case "there is an active management client, and
> --management-query-passwords is set, but we *could* ask on /dev/tty"
> is really worth considering.

I agree, but the problem is that we currently do that when auth-file has
username but no password.

Current behaviour:
   if auth-file is set always read from it
   else query management if management-query-passwords is set etc.
   else ask on /dev/tty or windows console
with a quirk that if auth-file has username but no password, ask on /dev/tty
or console, not the management ever..

(ignoring systemd which just replaces the query on console).

I'm all for changing the latter behaviour to query the management if
possible,
/dev/tty otherwise. But that's a regression and some may complain.

Jonathan K. Bullard  wrote:
>
> If the OS X command line user was using --management-query-passwords
> (as Tunnelblick does), they wouldn't see the password prompt on
> /dev/tty, would they?

In case of auth-file missing password, they would see it on /dev/tty
on linux, and I would guess on OSX as well, but I've not checked.


Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password, query the management

2020-03-29 Thread Selva Nair
Hi,

On Sun, Mar 29, 2020 at 7:13 PM Jonathan K. Bullard  wrote:
>
> Hi,
>
> On Sun, Mar 29, 2020 at 4:34 PM  wrote:
> >
> > From: Selva Nair 
> >
> > If only username is found in the file, redirect the auth-user-pass
> > query to the management on Windows if (i) management-query-passwords
> > is enabled and (ii) stdout is redirected to a log file. These
> > restrictions avoid regressive behaviour: those running from the
> > command line will continue to get the prompt on the console
> > and if both username and password are in the file those will
> > continue to get used.
> >
> > Note that the management will prompt for both username and password
> > ignoring the username read from the file. As the GUI saves the
> > username, this is a one-time inconvenience.
> >
> > Currently, the password is queried on the console (or systemd)
> > in such cases. This is not sensible on windows if log file is
> > redirected (prompt goes to the log file), or the console
> > is not available as happens when the GUI is in use.
>
> Why only Windows? I'd like this for macOS, too!

I did not know what other platforms were affected and, in particular,
how to handle them.

We can't make this the default as some have systemd. Also on
unix-like OSes, we can and we do prompt on the controlling tty
even if stdout and stderr are redirected to files or /dev/null.

>
> On a Mac using Tunnelblick (which uses the management interface with
> management-query-passwords enabled), if the auth-user-pass file
> contains only the password (and a LF), then the following occurs:
>
>  neither stdin nor stderr are a tty device and you have neither a
> controlling tty nor systemd - can't ask for 'Enter Auth Password:'.
> If you used --daemon, you need to use --askpass to make
> passphrase-protected keys work, and you can not use --auth-nocache.
>  Exiting due to fatal error

In those cases it looks obviously wrong to use auth-file with username
only, and I would consider that a user error. The purpose of
my patch was to handle only some naive usages where the user
expects the console prompt to get automatically directed to the GUI.
Indeed, that does happen (from user's POV) for all cases except user-pass
with only username in a file.

But I agree, we should do something like this for other GUIs such as
tunnelblick too.

>
> Note: Tunnelblick uses the "--log" option to redirect output to a
> file. I am assuming that's what is meant by "stdout is redirected to a
> log file".

Yes, that's right. However, that logic wont be proper on OS-X, would it?
Command line users who use --log can still see password
prompt on /dev/tty. We'll be breaking that behaviour.

I considered checking for env vars like IV_UI_VER set by the UI
client, but that's not readily accessible from auth_user_pass_cr()
call. Alternatives like checking whether /dev/tty can be opened and/or
systemd is available didn't appeal to me. If at all, that would have
to be a separate patch.

Any suggestions?

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Document some limitations of --auth-user-pass

2020-03-29 Thread Selva Nair
Hi,

On Tue, Mar 17, 2020 at 6:25 AM Gert Doering  wrote:
>
> Hi,
>
> On Tue, Mar 17, 2020 at 11:06:53AM +0100, David Sommerseth wrote:
> > On 16/03/2020 14:48, Selva Nair wrote:
> > [...snip...]
> > >> I would just rephrase it to say:
> > >>
> > >>   OpenVPN GUI v11 and newer uses its own internal username/password 
> > >> storage
> > >>   independent of the --auth-user-pass file provided.  The file argument 
> > >> is
> > >>   ignored on such installations.
> > >
> > > I wish it behaved  like that. Unfortunately the file argument is not
> > > ignored in such cases. If the file has only username, openvpn.exe
> > > reads it from the file and then fails to prompt for password as there
> > > is no console available.
> >
> > Ouch ... that is a pointless misbehavior.  Lets try to fix that.
>
> Have you recovered from your latest adventures in "password query code
> in OpenVPN" already? :-)
>
> Not sure if the management commands permit the "we have a username but
> no password" flow today... Arne, Selva?
>
> But yes, this needs to be either a clear error, or "work correctly"
>
> > > I propose to change this behaviour to: if --management-query-passwords
> > > is set (which the GUI does), ignore the file given in auth-user-pass
> > > and prompt both username and password from management. I think its
> > > only logical for a later option (in this case the one set by the GUI)
> > > to override a previous one. Anyway we do already ignore it if the file
> > > is "stdin".
> >
> > Agreed!
>
> No, as this will break working configs *if* both username + password
> are in the file (did we ever merge the "inline auth-user-pass" patch?).

See the patch in mail for what looks like an acceptable solution to me.


Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password, query the management

2020-03-29 Thread selva . nair
From: Selva Nair 

If only username is found in the file, redirect the auth-user-pass
query to the management on Windows if (i) management-query-passwords
is enabled and (ii) stdout is redirected to a log file. These
restrictions avoid regressive behaviour: those running from the
command line will continue to get the prompt on the console
and if both username and password are in the file those will
continue to get used.

Note that the management will prompt for both username and password
ignoring the username read from the file. As the GUI saves the
username, this is a one-time inconvenience.

Currently, the password is queried on the console (or systemd)
in such cases. This is not sensible on windows if log file is
redirected (prompt goes to the log file), or the console
is not available as happens when the GUI is in use.

Trac # 757

Signed-off-by: Selva Nair 
---
 src/openvpn/error.c |  9 +
 src/openvpn/error.h |  3 +++
 src/openvpn/misc.c  | 17 +
 3 files changed, 29 insertions(+)

diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index ad4f0ef..8ce6873 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -190,6 +190,15 @@ errors_to_stderr(void)
 }
 
 /*
+ * Return true if stdout is redirected to log file
+ */
+bool
+is_stdout_redirected(void)
+{
+return std_redir;
+}
+
+/*
  * Return a file to print messages to before syslog is opened.
  */
 FILE *
diff --git a/src/openvpn/error.h b/src/openvpn/error.h
index eaedf17..5078f6a 100644
--- a/src/openvpn/error.h
+++ b/src/openvpn/error.h
@@ -398,6 +398,9 @@ nonfatal(const unsigned int err)
 return err & M_FATAL ? (err ^ M_FATAL) | M_NONFATAL : err;
 }
 
+/** Return true if stdout is redirected to log file */
+bool is_stdout_redirected(void);
+
 #include "errlevel.h"
 
 #endif /* ifndef ERROR_H */
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 0d5ac30..02afd98 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -261,6 +261,23 @@ get_user_pass_cr(struct user_pass *up,
 {
 strncpy(up->password, password_buf, USER_PASS_LEN);
 }
+/* The auth-file does not have the password: if we are on Windows
+ * and stdout has been redirected to log file, try to get both 
username
+ * and password from the management.
+ * Otherwise set to read password from console.
+ */
+#if defined(ENABLE_MANAGEMENT) && defined(_WIN32)
+else if (is_stdout_redirected()
+ && management
+ && (flags & GET_USER_PASS_MANAGEMENT)
+ && management_query_user_pass_enabled(management))
+{
+if (!auth_user_pass_mgmt(up, prefix, flags, auth_challenge))
+{
+return false;
+}
+}
+#endif
 else
 {
 password_from_stdin = 1;
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH 1/2] Move querying username/password from management to a function

2020-03-29 Thread selva . nair
From: Selva Nair 

This helps the next patch. No functionality changes, only
refactoring.

Signed-off-by: Selva Nair 
---
 src/openvpn/misc.c | 54 ++
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 1931149..0d5ac30 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -116,6 +116,38 @@ hostname_randomize(const char *hostname, struct gc_arena 
*gc)
 #undef n_rnd_bytes
 }
 
+#ifdef ENABLE_MANAGEMENT
+/* Get username/password from the management interface */
+static bool
+auth_user_pass_mgmt(struct user_pass *up, const char *prefix, const unsigned 
int flags,
+const char *auth_challenge)
+{
+const char *sc = NULL;
+
+if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
+{
+management_auth_failure(management, prefix, "previous auth credentials 
failed");
+}
+
+if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
+{
+sc = auth_challenge;
+}
+if (!management_query_user_pass(management, up, prefix, flags, sc))
+{
+if ((flags & GET_USER_PASS_NOFATAL) != 0)
+{
+return false;
+}
+else
+{
+msg(M_FATAL, "ERROR: could not read %s username/password/ok/string 
from management interface", prefix);
+}
+}
+return true;
+}
+#endif
+
 /*
  * Get and store a username/password
  */
@@ -149,28 +181,10 @@ get_user_pass_cr(struct user_pass *up,
 && (!from_authfile && (flags & GET_USER_PASS_MANAGEMENT))
 && management_query_user_pass_enabled(management))
 {
-const char *sc = NULL;
 response_from_stdin = false;
-
-if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
+if (!auth_user_pass_mgmt(up, prefix, flags, auth_challenge))
 {
-management_auth_failure(management, prefix, "previous auth 
credentials failed");
-}
-
-if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
-{
-sc = auth_challenge;
-}
-if (!management_query_user_pass(management, up, prefix, flags, sc))
-{
-if ((flags & GET_USER_PASS_NOFATAL) != 0)
-{
-return false;
-}
-else
-{
-msg(M_FATAL, "ERROR: could not read %s 
username/password/ok/string from management interface", prefix);
-}
+return false;
 }
 }
 else
-- 
2.1.4



___
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 (26th March 2020)

2020-03-26 Thread Selva Nair
Hi,

Quoting from the 26th March meeting summary

> Noted that the combination of a username-only --auth-user-pass and
> --management-query-passwords does not work. Dazo will take a stab at
> fixing the actual problem. There is already a
> GET_USER_PASS_PASSWORD_ONLY flag which just needs to be processed
> correctly when the management interface is in action.

That's not very useful as GET_USER_PASS_PASSWORD_ONLY is currently
meant to prompt for the private key password, token password etc. The GUI will
treat any 'Auth' request to mean both username and password. In other
words, a management client only sees the prompt and there is no defined
prompt string for auth-user-pass password only.

Also, asking for a password without at least displaying the username is
confusing and there is currently no way of indicating the username in
such a request.

I considered several options for fixing this but all involve some
regression that may not be acceptable. An option is to step back when
only username is found in the file and ask for both username and password
from the management with the usual Auth request. Do this only if
--management-query-passwords is present.

But even that is a regression as currently, in such cases, the console
will be queried. There could be some users out there with those
options in the config, but not using the GUI or any management client,
and rely on prompting for password via the console.



> An attempt to document the limitation plus related discussion is here:
>
> 
>
> Further discussion of the issue is available here:
>
> 
>

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Document some limitations of --auth-user-pass

2020-03-16 Thread Selva Nair
Hi,

On Mon, Mar 16, 2020 at 8:39 AM David Sommerseth
 wrote:
>
> On 13/03/2020 14:01, sam...@openvpn.net wrote:
> > From: Samuli Seppänen 
> >
> > URL: https://community.openvpn.net/openvpn/ticket/757
> > Signed-off-by: Samuli Seppänen 
> > ---
> >  doc/openvpn.8 | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/doc/openvpn.8 b/doc/openvpn.8
> > index 864f94e8..9e54890e 100644
> > --- a/doc/openvpn.8
> > +++ b/doc/openvpn.8
> > @@ -4127,6 +4127,12 @@ The server configuration must specify an
> >  .B \-\-auth\-user\-pass\-verify
> >  script to verify the username/password provided by
> >  the client.
> > +
> > +Note that OpenVPN GUI on Windows does not prompt for the
> > +password if the file contains only the username. However,
> > +OpenVPN versions from 2.4 up bundle OpenVPN GUI version 11
> > +which is able to cache usernames and passwords internally.
> > +
>
> Could we rephrase this, to not live in the past.  This will go into master and
> probably also release/2.4.  I also doubt anyone using man pages on 2.3 would
> even read this.  If there are Windows users on 2.3, there are no excuse not to
> upgrade - unless it's an enterprise deployment, where end users most likely
> would not even care (they should anyway complain to their IT department
> regardless, for using outdated security software).
>
> I would just rephrase it to say:
>
>   OpenVPN GUI v11 and newer uses its own internal username/password storage
>   independent of the --auth-user-pass file provided.  The file argument is
>   ignored on such installations.

I wish it behaved  like that. Unfortunately the file argument is not
ignored in such cases. If the file has only username, openvpn.exe
reads it from the file and then fails to prompt for password as there
is no console available.

I propose to change this behaviour to: if --management-query-passwords
is set (which the GUI does), ignore the file given in auth-user-pass
and prompt both username and password from management. I think its
only logical for a later option (in this case the one set by the GUI)
to override a previous one. Anyway we do already ignore it if the file
is "stdin".

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] interactive.c: remove unused function

2020-02-29 Thread Selva Nair
Hi,

On Sat, Feb 29, 2020 at 7:36 AM Lev Stipakov  wrote:
>
> From: Lev Stipakov 
>
> Function ReturnOpenvpnOutput was used to read
> openvpn process output and write it to openvpn-gui.
>
> Commit 852f1e4 has directed stdout/stderr streams of openvpn
> process to NUL, after which ReturnOpenvpnOutput() has become unused.
>
> Signed-off-by: Lev Stipakov 
> ---
>  src/openvpnserv/interactive.c | 25 -
>  1 file changed, 25 deletions(-)
>
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 8da49be6..1f13163e 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -361,31 +361,6 @@ ReturnLastError(HANDLE pipe, LPCWSTR func)
>  ReturnError(pipe, GetLastError(), func, 1, _event);
>  }
>
> -
> -static VOID
> -ReturnOpenvpnOutput(HANDLE pipe, HANDLE ovpn_output, DWORD count, LPHANDLE 
> events)
> -{
> -WCHAR *wide_output = NULL;
> -CHAR output[512];
> -DWORD size;
> -
> -ReadFile(ovpn_output, output, sizeof(output), , NULL);
> -if (size == 0)
> -{
> -return;
> -}
> -
> -wide_output = malloc((size) * sizeof(WCHAR));
> -if (wide_output)
> -{
> -MultiByteToWideChar(CP_UTF8, 0, output, size, wide_output, size);
> -wide_output[size - 1] = 0;
> -}
> -
> -ReturnError(pipe, ERROR_OPENVPN_STARTUP, wide_output, count, events);
> -free(wide_output);
> -}
> -
>  /*
>   * Validate options against a white list. Also check the config_file is
>   * inside the config_dir. The white list is defined in validate.c

Makes sense to get rid of this unused static function.
Compile tested.

Acked-by: 


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2] Persist management-query-remote and proxy prompts

2020-02-20 Thread selva . nair
From: Selva Nair 

Currently this prompt is only output once, not re-written to the
management interface when the management client connects. It is thus
not seen by a client that connects after the prompt is output or one that
disconnects and reconnects. This leads to a deadlock: the daemon waiting
for the "remote" command from the client, the latter not aware of it.

Resolve by adding the ">REMOTE" and ">PROXY" prompt to
man.persist.special_state_msg as done for other persisted prompts such
as ">PASSWORD"

Signed-off-by: Selva Nair 
---
v2: bump and rebase to master

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

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 1cfffbb..b4781a2 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -271,6 +271,7 @@ ce_management_query_proxy(struct context *c)
 buf_printf(, ">PROXY:%u,%s,%s", (l ? l->current : 0) + 1,
(proto_is_udp(ce->proto) ? "UDP" : "TCP"), 
np(ce->remote));
 management_notify_generic(management, BSTR());
+management->persist.special_state_msg = BSTR();
 }
 ce->flags |= CE_MAN_QUERY_PROXY;
 while (ce->flags & CE_MAN_QUERY_PROXY)
@@ -282,6 +283,7 @@ ce_management_query_proxy(struct context *c)
 break;
 }
 }
+management->persist.special_state_msg = NULL;
 gc_free();
 }
 
@@ -351,6 +353,7 @@ ce_management_query_remote(struct context *c)
 buf_printf(, ">REMOTE:%s,%s,%s", np(ce->remote), ce->remote_port,
proto2ascii(ce->proto, ce->af, false));
 management_notify_generic(management, BSTR());
+management->persist.special_state_msg = BSTR();
 
 ce->flags &= ~(CE_MAN_QUERY_REMOTE_MASK << CE_MAN_QUERY_REMOTE_SHIFT);
 ce->flags |= (CE_MAN_QUERY_REMOTE_QUERY << CE_MAN_QUERY_REMOTE_SHIFT);
@@ -364,6 +367,7 @@ ce_management_query_remote(struct context *c)
 break;
 }
 }
+management->persist.special_state_msg = NULL;
 }
 gc_free();
 
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH applied] Fix possible access of uninitialized pipe handles

2020-02-20 Thread Selva Nair
Hi

On Thu, Feb 20, 2020 at 1:20 PM David Sommerseth  wrote:
>
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA512
>
> Your patch has been applied to the master branch
>
> commit 32723d29b2775d63d3fe329d017e7a08e0cdcb72
> Author: Selva Nair
> Date:   Wed Feb 19 20:56:43 2020 -0500

I think this and next one could also go into 2.4.

Here are the commits, in case
.
32723d29b2775d63d3fe329d017e7a08e0cdcb72
e1f7d7885752ac3a0279ecc7e31ccee2af40fbe4

Thanks,

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Fix possible access of uninitialized pipe handles

2020-02-20 Thread Selva Nair
Hi

On Thu, Feb 20, 2020 at 4:24 AM Lev Stipakov  wrote:
>
> Strangely, I do not see this warning (unlike another one about error
> in common.c)
> with GCC 7.3 despite adding -O1 and -Wmaybe-uninitialized.

I saw it on the travis build. With gcc 7.3, for some reason, -O1
doesn't show it but -O2 or higher does. Some older versions of gcc
seem to show it only with require -O3 or higher!

But the potential for attempting to close wrong handles looks real.

Thanks,

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] Fix possible access of uninitialized pipe handles

2020-02-19 Thread selva . nair
From: Selva Nair 

Compile time warning for openvpnserv.exe
interactive.c: In function ‘RunOpenvpn’:
interactive.c:160:27: warning: ‘svc_pipe’ may be used uninitialized in
this function [-Wmaybe-uninitialized]

When RunOpenvpn exits early due to errors, uninitialized svc_pipe and
ovpn_pipe vars could get passed to CloseHandleEx(). Fix by initializing
to NULL.

Signed-off-by: Selva Nair 
---
 src/openvpnserv/interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 710f9c7..8da49be 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1468,7 +1468,7 @@ static DWORD WINAPI
 RunOpenvpn(LPVOID p)
 {
 HANDLE pipe = p;
-HANDLE ovpn_pipe, svc_pipe;
+HANDLE ovpn_pipe = NULL, svc_pipe = NULL;
 PTOKEN_USER svc_user = NULL, ovpn_user = NULL;
 HANDLE svc_token = NULL, imp_token = NULL, pri_token = NULL;
 HANDLE stdin_read = NULL, stdin_write = NULL;
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] Fix possibly uninitialized return value in GetOpenvpnSettings()

2020-02-19 Thread selva . nair
From: Selva Nair 

Compile time warning for openvpnserv.exe
common.c:90:11: warning: ‘error’ may be used uninitialized in this
function [-Wmaybe-uninitialized];

Uninitialized value gets returned if install-path is not found
in the registry. Fix by setting it to the return value of
GetRegString().

Signed-off-by: Selva Nair 
---
Behaviour change: previously, on not finding the install_path
in registry, the service logged an error and often continued
as the uninitialized value of error could be zero. With
this change it will consistently exit with error.

 src/openvpnserv/common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index fd51776..958643d 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -118,8 +118,10 @@ GetOpenvpnSettings(settings_t *s)
 }
 
 /* The default value of REG_KEY is the install path */
-if (GetRegString(key, NULL, install_path, sizeof(install_path), NULL) != 
ERROR_SUCCESS)
+status = GetRegString(key, NULL, install_path, sizeof(install_path), NULL);
+if (status != ERROR_SUCCESS)
 {
+error = status;
 goto out;
 }
 
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH 2.4 v3] Swap the order of checks for validating interactive service user

2020-02-18 Thread selva . nair
From: Selva Nair 

Check the config file location and command line options first
and membership in OpenVPNAdministrators group after that as
the latter could be a slow process for active directory users.

When connection to domain controllers is poor or unavailable, checking
the group membership is slow and causes timeouts in the GUI (Trac
1051). However, in cases where the config is in the global directory,
no group membership check should be required. The re-ordering here
avoids the redundant check in such cases.

In addition to this, its also proposed to improve the timeout handling
in the GUI, but this change is still useful as it should completely
eliminate the timeout issue for many users.

v3: Do not send error message to the client pipe from ValidateOptions().
Instead save the error and send it on only if user authorization also
fails. The error buffer size is increased to 512 wide chars as these
messages could get long in some cases and may get truncated otherwise.

Also see: https://github.com/OpenVPN/openvpn-gui/issues/332

Signed-off-by: Selva Nair 
---
 cherry-picked from commit c6cc66a13568dd1078bfbeb763998c1b9e2a2999
 with one change:
 - openvpn_swprintf() -> swprintf() as the latter is not readily accessible
   here in 2.4

 src/openvpnserv/interactive.c | 39 ++-
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index d7c9eea..a2b3b20 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -360,14 +360,13 @@ ReturnOpenvpnOutput(HANDLE pipe, HANDLE ovpn_output, 
DWORD count, LPHANDLE event
 /*
  * Validate options against a white list. Also check the config_file is
  * inside the config_dir. The white list is defined in validate.c
- * Returns true on success
+ * Returns true on success, false on error with reason set in errmsg.
  */
 static BOOL
-ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
+ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options, WCHAR 
*errmsg, DWORD capacity)
 {
 WCHAR **argv;
 int argc;
-WCHAR buf[256];
 BOOL ret = FALSE;
 int i;
 const WCHAR *msg1 = L"You have specified a config file location (%s 
relative to %s)"
@@ -382,8 +381,10 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const 
WCHAR *options)
 
 if (!argv)
 {
-ReturnLastError(pipe, L"CommandLineToArgvW");
-ReturnError(pipe, ERROR_STARTUP_DATA, L"Cannot validate options", 1, 
_event);
+swprintf(errmsg, capacity,
+L"Cannot validate options: CommandLineToArgvW failed with 
error = 0x%08x",
+GetLastError());
+errmsg[capacity-1] = L'\0';
 goto out;
 }
 
@@ -403,10 +404,9 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const 
WCHAR *options)
 
 if (!CheckOption(workdir, 2, argv_tmp, ))
 {
-swprintf(buf, _countof(buf), msg1, argv[0], workdir,
+swprintf(errmsg, capacity, msg1, argv[0], workdir,
  settings.ovpn_admin_group);
-buf[_countof(buf) - 1] = L'\0';
-ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, _event);
+errmsg[capacity-1] = L'\0';
 }
 goto out;
 }
@@ -422,18 +422,15 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const 
WCHAR *options)
 {
 if (wcscmp(L"--config", argv[i]) == 0 && argc-i > 1)
 {
-swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
+swprintf(errmsg, capacity, msg1, argv[i+1], workdir,
  settings.ovpn_admin_group);
-buf[_countof(buf) - 1] = L'\0';
-ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, _event);
 }
 else
 {
-swprintf(buf, _countof(buf), msg2, argv[i],
+swprintf(errmsg, capacity, msg2, argv[i],
  settings.ovpn_admin_group);
-buf[_countof(buf) - 1] = L'\0';
-ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, _event);
 }
+errmsg[capacity-1] = L'\0';
 goto out;
 }
 }
@@ -1367,6 +1364,7 @@ RunOpenvpn(LPVOID p)
 WCHAR *cmdline = NULL;
 size_t cmdline_size;
 undo_lists_t undo_lists;
+WCHAR errmsg[512] = L"";
 
 SECURITY_ATTRIBUTES inheritable = {
 .nLength = sizeof(inheritable),
@@ -1459,10 +1457,17 @@ RunOpenvpn(LPVOID p)
 goto out;
 }
 
-/* Check user is authorized or options are white-listed */
-if (!IsAuthorizedUser(ovpn_user->User.Sid, imp_token, 
settings.ovpn_admin_group)
-&& !ValidateOptions(pipe, sud.directory, sud.options))
+/*
+ * Only authorized users are allowed to use any command line options or
+ * have the config file in locat

Re: [Openvpn-devel] [PATCH] cryptoapi.c: fix run-time check failure in msvc debugger

2020-02-13 Thread Selva Nair
Hi,

On Thu, Feb 13, 2020 at 4:57 AM Lev Stipakov  wrote:
>
> From: Lev Stipakov 
>
> When using certificate without RSA_PKCS1_PSS_PADDING padding,
> "saltlen" is passed unitialized to priv_enc_CNG(), which causes
>
>  > Run-Time Check Failure #3 - The variable 'saltlen' is being used without 
> being initialized.
>
> in VS debugger.
>
> Initialize saltlen (and other variable for the sake of consistence) to zero

"consistency"

> to avoid above failure.
>
> Signed-off-by: Lev Stipakov 
> ---
>  src/openvpn/cryptoapi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
> index 1bf74fcd..30eba7b2 100644
> --- a/src/openvpn/cryptoapi.c
> +++ b/src/openvpn/cryptoapi.c
> @@ -882,9 +882,9 @@ pkey_rsa_sign(EVP_PKEY_CTX *ctx, unsigned char *sig, 
> size_t *siglen,
>  EVP_MD *md = NULL;
>  const wchar_t *alg = NULL;
>
> -int padding;
> -int hashlen;
> -int saltlen;
> +int padding = 0;
> +int hashlen = 0;
> +int saltlen = 0;
>
>  pkey = EVP_PKEY_CTX_get0_pkey(ctx);
>  if (pkey)

Yeah, technically it may be "undefined behaviour" to pass an
uninitialized var to a function even when its not used there.

Acked-by: Selva Nair 




Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v4 2/2] Allow unicode search string in --cryptoapicert option

2020-02-12 Thread selva . nair
From: Selva Nair 

Currently when the certificate is specified as "SUBJ:foo", the
string foo is assumed to be ascii. Change that and interpret
it as utf-8, convert to a wide string, and flag it as unicode
in CertFindCertifcateInStore().

Signed-off-by: Selva Nair 
---
v4: matched to v4 of 1/2 

 src/openvpn/cryptoapi.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index b9f1328..1bf74fc 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -51,6 +51,7 @@
 
 #include "buffer.h"
 #include "openssl_compat.h"
+#include "win32.h"
 
 /* MinGW w32api 3.17 is still incomplete when it comes to CryptoAPI while
  * MinGW32-w64 defines all macros used. This is a hack around that problem.
@@ -746,12 +747,13 @@ find_certificate_in_store(const char *cert_prop, 
HCERTSTORE cert_store)
 const void *find_param;
 unsigned char hash[255];
 CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
+struct gc_arena gc = gc_new();
 
 if (!strncmp(cert_prop, "SUBJ:", 5))
 {
 /* skip the tag */
-find_param = cert_prop + 5;
-find_type = CERT_FIND_SUBJECT_STR_A;
+find_param = wide_string(cert_prop + 5, );
+find_type = CERT_FIND_SUBJECT_STR_W;
 }
 else if (!strncmp(cert_prop, "THUMB:", 6))
 {
@@ -779,7 +781,7 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE 
cert_store)
 if (!*++p)  /* unexpected end of string */
 {
 msg(M_WARN, "WARNING: cryptoapicert: error parsing 
.", cert_prop);
-return NULL;
+goto out;
 }
 if (*p >= '0' && *p <= '9')
 {
@@ -803,7 +805,7 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE 
cert_store)
 }
 else {
 msg(M_WARN, "WARNING: cryptoapicert: unsupported certificate 
specification <%s>", cert_prop);
-return NULL;
+goto out;
 }
 
 while(true)
@@ -824,6 +826,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE 
cert_store)
 validity < 0 ? "not yet valid" : "that has expired");
 }
 
+out:
+gc_free();
 return rv;
 }
 
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v4 1/2] Skip expired certificates in Windows certificate store

2020-02-12 Thread selva . nair
From: Selva Nair 

Have the cryptoapicert option find the first matching certificate
in store that is valid at the present time. Currently the first
found item, even if expired, is returned.

This makes it possible to update certifiates in store without having
to delete old ones. As a side effect, if only expired certificates are
found, the connection fails.

Also remove some unnecessary casts.

Tested on Windows 10.
Trac #966

v4: Handle the case when an unknown certificate specification is passed
to find_certificate_in_store().

Note: Warnings printed from find_certificate_in_store() could show up
multiple times as its called for each certificate store. This could
be improved in a future patch.

Signed-off-by: Selva Nair 
---
 src/openvpn/cryptoapi.c | 46 ++
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 2f2eee7..b9f1328 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -739,27 +739,30 @@ find_certificate_in_store(const char *cert_prop, 
HCERTSTORE cert_store)
  * SUBJ:
  * THUMB:, e.g.
  * THUMB:f6 49 24 41 01 b4 fb 44 0c ce f4 36 ae d0 c4 c9 df 7a b6 28
+ * The first matching certificate that has not expired is returned.
  */
 const CERT_CONTEXT *rv = NULL;
+DWORD find_type;
+const void *find_param;
+unsigned char hash[255];
+CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
 
 if (!strncmp(cert_prop, "SUBJ:", 5))
 {
 /* skip the tag */
-cert_prop += 5;
-rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | 
PKCS_7_ASN_ENCODING,
-0, CERT_FIND_SUBJECT_STR_A, cert_prop, 
NULL);
-
+find_param = cert_prop + 5;
+find_type = CERT_FIND_SUBJECT_STR_A;
 }
 else if (!strncmp(cert_prop, "THUMB:", 6))
 {
-unsigned char hash[255];
-char *p;
+const char *p;
 int i, x = 0;
-CRYPT_HASH_BLOB blob;
+find_type = CERT_FIND_HASH;
+find_param = 
 
 /* skip the tag */
 cert_prop += 6;
-for (p = (char *) cert_prop, i = 0; *p && i < sizeof(hash); i++)
+for (p = cert_prop, i = 0; *p && i < sizeof(hash); i++)
 {
 if (*p >= '0' && *p <= '9')
 {
@@ -775,7 +778,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE 
cert_store)
 }
 if (!*++p)  /* unexpected end of string */
 {
-break;
+msg(M_WARN, "WARNING: cryptoapicert: error parsing 
.", cert_prop);
+return NULL;
 }
 if (*p >= '0' && *p <= '9')
 {
@@ -796,10 +800,28 @@ find_certificate_in_store(const char *cert_prop, 
HCERTSTORE cert_store)
 }
 }
 blob.cbData = i;
-blob.pbData = (unsigned char *) 
-rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | 
PKCS_7_ASN_ENCODING,
-0, CERT_FIND_HASH, , NULL);
+}
+else {
+msg(M_WARN, "WARNING: cryptoapicert: unsupported certificate 
specification <%s>", cert_prop);
+return NULL;
+}
 
+while(true)
+{
+int validity = 1;
+/* this frees previous rv, if not NULL */
+rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | 
PKCS_7_ASN_ENCODING,
+0, find_type, find_param, rv);
+if (rv)
+{
+validity = CertVerifyTimeValidity(NULL, rv->pCertInfo);
+}
+if (!rv || validity == 0)
+{
+break;
+}
+msg(M_WARN, "WARNING: cryptoapicert: ignoring certificate in store 
%s.",
+validity < 0 ? "not yet valid" : "that has expired");
 }
 
 return rv;
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 1/2 v3] Skip expired certificates in Windows certificate store

2020-02-11 Thread Selva Nair
Hi,

Thanks for reviewing this.

On Tue, Feb 11, 2020 at 4:52 AM Lev Stipakov  wrote:
>
> Hi,
>
>> +DWORD find_type;
>> +const void *find_param;
>>
>>
>>
>>  if (!strncmp(cert_prop, "SUBJ:", 5))
>>  {
>>
>> +find_param = cert_prop + 5;
>> +find_type = CERT_FIND_SUBJECT_STR_A;
>>  }
>>  else if (!strncmp(cert_prop, "THUMB:", 6))
>>  {
>> +find_type = CERT_FIND_HASH;
>> +find_param = 
>> +}
>> +while(true)
>> +{
>>  rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | 
>> PKCS_7_ASN_ENCODING,
>> +0, find_type, find_param, rv);
>
>
> This explodes if cert_prop doesn't start with either "SUBJ:" or "THUMB:" 
> since we pass
> uninitialized arguments.

That's a terrible oversight. My bad. v4 is coming.

>
> This problem didn't exist before, since we looked for certificate inside 
> above "if" blocks where
> both variables are initialized.
>
> Another thing:
>
>> +unsigned char hash[255];
>> +CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
>>
>>  else if (!strncmp(cert_prop, "THUMB:", 6))
>>  {
>> -unsigned char hash[255];
>> -CRYPT_HASH_BLOB blob;
>
>
> Why did you move "hash" and "blob" to the outer scope? I think those
> variables should stay where they have been, since they're not used outside of 
> "if".

The actual certificate search is now done outside (in the while loop)
and it refers
to find_param which could be 

Alternatively one could do the search separately for SUBJ and THUMB as
in the original,
but with the new logic of iterating through the store, a combined
approach looked
"cleaner".

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH 1/2 v3] Skip expired certificates in Windows certificate store

2020-02-10 Thread selva . nair
From: Selva Nair 

Have the cryptoapicert option find the first matching certificate
in store that is valid at the present time. Currently the first
found item, even if expired, is returned.

This makes it possible to update certifiates in store without having
to delete old ones. As a side effect, if only expired certificates are
found, the connection fails.

Also remove some unnecessary casts.

Tested on Windows 10.
Trac #966

Signed-off-by: Selva Nair 
---
v3: nudging again with a rebase to master

 src/openvpn/cryptoapi.c | 41 +
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 2f2eee7..3b70c33 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -739,27 +739,30 @@ find_certificate_in_store(const char *cert_prop, 
HCERTSTORE cert_store)
  * SUBJ:
  * THUMB:, e.g.
  * THUMB:f6 49 24 41 01 b4 fb 44 0c ce f4 36 ae d0 c4 c9 df 7a b6 28
+ * The first matching certificate that has not expired is returned.
  */
 const CERT_CONTEXT *rv = NULL;
+DWORD find_type;
+const void *find_param;
+unsigned char hash[255];
+CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
 
 if (!strncmp(cert_prop, "SUBJ:", 5))
 {
 /* skip the tag */
-cert_prop += 5;
-rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | 
PKCS_7_ASN_ENCODING,
-0, CERT_FIND_SUBJECT_STR_A, cert_prop, 
NULL);
-
+find_param = cert_prop + 5;
+find_type = CERT_FIND_SUBJECT_STR_A;
 }
 else if (!strncmp(cert_prop, "THUMB:", 6))
 {
-unsigned char hash[255];
-char *p;
+const char *p;
 int i, x = 0;
-CRYPT_HASH_BLOB blob;
+find_type = CERT_FIND_HASH;
+find_param = 
 
 /* skip the tag */
 cert_prop += 6;
-for (p = (char *) cert_prop, i = 0; *p && i < sizeof(hash); i++)
+for (p = cert_prop, i = 0; *p && i < sizeof(hash); i++)
 {
 if (*p >= '0' && *p <= '9')
 {
@@ -775,7 +778,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE 
cert_store)
 }
 if (!*++p)  /* unexpected end of string */
 {
-break;
+msg(M_WARN, "WARNING: cryptoapicert: error parsing 
.", cert_prop);
+return NULL;
 }
 if (*p >= '0' && *p <= '9')
 {
@@ -796,10 +800,23 @@ find_certificate_in_store(const char *cert_prop, 
HCERTSTORE cert_store)
 }
 }
 blob.cbData = i;
-blob.pbData = (unsigned char *) 
+}
+while(true)
+{
+int validity = 1;
+/* this frees previous rv, if not NULL */
 rv = CertFindCertificateInStore(cert_store, X509_ASN_ENCODING | 
PKCS_7_ASN_ENCODING,
-0, CERT_FIND_HASH, , NULL);
-
+0, find_type, find_param, rv);
+if (rv)
+{
+validity = CertVerifyTimeValidity(NULL, rv->pCertInfo);
+}
+if (!rv || validity == 0)
+{
+break;
+}
+msg(M_WARN, "WARNING: cryptoapicert: ignoring certificate in store 
%s.",
+validity < 0 ? "not yet valid" : "that has expired");
 }
 
 return rv;
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH 2/2 v3] Allow unicode search string in --cryptoapicert option

2020-02-10 Thread selva . nair
From: Selva Nair 

Currently when the certificate is specified as "SUBJ:foo", the
string foo is assumed to be ascii. Change that and interpret
it as utf-8, convert to a wide string, and flag it as unicode
in CertFindCertifcateInStore().

Signed-off-by: Selva Nair 
---
v3: nudging again, with a rebase to master

 src/openvpn/cryptoapi.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 3b70c33..acae96f 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -51,6 +51,7 @@
 
 #include "buffer.h"
 #include "openssl_compat.h"
+#include "win32.h"
 
 /* MinGW w32api 3.17 is still incomplete when it comes to CryptoAPI while
  * MinGW32-w64 defines all macros used. This is a hack around that problem.
@@ -746,12 +747,13 @@ find_certificate_in_store(const char *cert_prop, 
HCERTSTORE cert_store)
 const void *find_param;
 unsigned char hash[255];
 CRYPT_HASH_BLOB blob = {.cbData = 0, .pbData = hash};
+struct gc_arena gc = gc_new();
 
 if (!strncmp(cert_prop, "SUBJ:", 5))
 {
 /* skip the tag */
-find_param = cert_prop + 5;
-find_type = CERT_FIND_SUBJECT_STR_A;
+find_param = wide_string(cert_prop + 5, );
+find_type = CERT_FIND_SUBJECT_STR_W;
 }
 else if (!strncmp(cert_prop, "THUMB:", 6))
 {
@@ -779,7 +781,7 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE 
cert_store)
 if (!*++p)  /* unexpected end of string */
 {
 msg(M_WARN, "WARNING: cryptoapicert: error parsing 
.", cert_prop);
-return NULL;
+goto out;
 }
 if (*p >= '0' && *p <= '9')
 {
@@ -819,6 +821,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE 
cert_store)
 validity < 0 ? "not yet valid" : "that has expired");
 }
 
+out:
+gc_free();
 return rv;
 }
 
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v3] Swap the order of checks for validating interactive service user

2020-02-09 Thread selva . nair
From: Selva Nair 

Check the config file location and command line options first
and membership in OpenVPNAdministrators group after that as
the latter could be a slow process for active directory users.

When connection to domain controllers is poor or unavailable, checking
the group membership is slow and causes timeouts in the GUI (Trac
1051). However, in cases where the config is in the global directory,
no group membership check should be required. The re-ordering here
avoids the redundant check in such cases.

In addition to this, its also proposed to improve the timeout handling
in the GUI, but this change is still useful as it should completely
eliminate the timeout issue for many users.

v3: Do not send error message to the client pipe from ValidateOptions().
Instead save the error and send it on only if user authorization also
fails. The error buffer size is increased to 512 wide chars as these
messages could get long in some cases and may get truncated otherwise.

Also see: https://github.com/OpenVPN/openvpn-gui/issues/332

Signed-off-by: Selva Nair 
---
v2: Add missing closing parenthesis and improve the comment
above the edited chunk.

 src/openvpnserv/interactive.c | 33 +++--
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 6e72a14..710f9c7 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -389,14 +389,13 @@ ReturnOpenvpnOutput(HANDLE pipe, HANDLE ovpn_output, 
DWORD count, LPHANDLE event
 /*
  * Validate options against a white list. Also check the config_file is
  * inside the config_dir. The white list is defined in validate.c
- * Returns true on success
+ * Returns true on success, false on error with reason set in errmsg.
  */
 static BOOL
-ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
+ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options, WCHAR 
*errmsg, DWORD capacity)
 {
 WCHAR **argv;
 int argc;
-WCHAR buf[256];
 BOOL ret = FALSE;
 int i;
 const WCHAR *msg1 = L"You have specified a config file location (%s 
relative to %s)"
@@ -411,8 +410,9 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const 
WCHAR *options)
 
 if (!argv)
 {
-ReturnLastError(pipe, L"CommandLineToArgvW");
-ReturnError(pipe, ERROR_STARTUP_DATA, L"Cannot validate options", 1, 
_event);
+openvpn_swprintf(errmsg, capacity,
+L"Cannot validate options: CommandLineToArgvW failed 
with error = 0x%08x",
+GetLastError());
 goto out;
 }
 
@@ -432,9 +432,8 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const 
WCHAR *options)
 
 if (!CheckOption(workdir, 2, argv_tmp, ))
 {
-openvpn_swprintf(buf, _countof(buf), msg1, argv[0], workdir,
+openvpn_swprintf(errmsg, capacity, msg1, argv[0], workdir,
  settings.ovpn_admin_group);
-ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, _event);
 }
 goto out;
 }
@@ -450,15 +449,13 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const 
WCHAR *options)
 {
 if (wcscmp(L"--config", argv[i]) == 0 && argc-i > 1)
 {
-openvpn_swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
+openvpn_swprintf(errmsg, capacity, msg1, argv[i+1], workdir,
  settings.ovpn_admin_group);
-ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, _event);
 }
 else
 {
-openvpn_swprintf(buf, _countof(buf), msg2, argv[i],
+openvpn_swprintf(errmsg, capacity, msg2, argv[i],
  settings.ovpn_admin_group);
-ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, _event);
 }
 goto out;
 }
@@ -1487,6 +1484,7 @@ RunOpenvpn(LPVOID p)
 size_t cmdline_size;
 undo_lists_t undo_lists;
 ring_buffer_handles_t ring_buffer_handles;
+WCHAR errmsg[512] = L"";
 
 SECURITY_ATTRIBUTES inheritable = {
 .nLength = sizeof(inheritable),
@@ -1580,10 +1578,17 @@ RunOpenvpn(LPVOID p)
 goto out;
 }
 
-/* Check user is authorized or options are white-listed */
-if (!IsAuthorizedUser(ovpn_user->User.Sid, imp_token, 
settings.ovpn_admin_group)
-&& !ValidateOptions(pipe, sud.directory, sud.options))
+/*
+ * Only authorized users are allowed to use any command line options or
+ * have the config file in locations other than the global config 
directory.
+ *
+ * Check options are white-listed and config is in the global directory
+ * OR user is authorized to run any config.
+ */
+if (!ValidateOptions(pipe, sud.directory, sud.options, errms

Re: [Openvpn-devel] [PATCH 1/2] Skip DNS address validation

2020-02-05 Thread Selva Nair
Hi,

On Wed, Feb 5, 2020 at 10:28 AM Lev Stipakov  wrote:
>
> Hi,
>
> Built and tested with msvc, works as expected - "validate=no" is added to 
> netsh command line.
>
> There is a similar commit in Simon's repo (not yet sent to ml) : 
> https://github.com/rozmansi/openvpn/commit/6b746cb0bf72a75e9963cc1a037c18cfb856702a
>
> I haven't noticed any slowness on my machine, but since fix has been 
> implemented separately
> by two persons and there is similar code for ipv6, I am ok with that.

>
> Acked-by: Lev Stipakov 

We explicitly added validate=no for IPv6 in
commit 786e06ade9f5dfad8ac360499187fa8e536d15cb
for the same reason as in this patch. The ipv4 DNS code belongs to an
era when this
option was not available.

ACK from me too.


Selva

>
> Acked-by: Lev Stipakov 
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] Fwd: [PATCH 2/2] Fix linking issues on MinGW

2020-02-05 Thread Selva Nair
-- Forwarded message -
From: Selva Nair 
Date: Wed, Feb 5, 2020 at 10:16 AM
Subject: Re: [Openvpn-devel] [PATCH 2/2] Fix linking issues on MinGW
To: Domagoj Pensa 
Cc: Gert Doering 


Hi,

On Wed, Feb 5, 2020 at 8:31 AM Domagoj Pensa  wrote:
>
> Hi!
>
> On Wed, Feb 05, 2020 at 02:05:11PM +0100, Gert Doering wrote:
> > Hi,
> >
> > On Wed, Feb 05, 2020 at 01:46:15PM +0100, Domagoj Pensa wrote:
> > > MinGW linking fails for several files due to a missing "static"
> > > declaration in functions tuntap_is_wintun() and tuntap_ring_empty().
> >
> >


Are you building without optimization? Please try using the latest
openvpn-build (or just add -O2 to complier options).

I suspect this is caused by the fact that gcc does not inline when
optimization is not on, but also not generating exported code in that
case. Inline implementation in gcc has so many subtleties, so not sure
whether this is a compiler bug or "feature".

Adding a static appears to be not a bad solution as gcc will emit
external code for static inline functions only if necessary.

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v2] Swap the order of checks for validating interactive service user

2020-02-03 Thread Selva Nair
Hi,

On Mon, Feb 3, 2020 at 3:49 AM Lev Stipakov  wrote:
>
> I am sorry, I have to retract my ACK.
>
> When ValidateOptions is called first and config is non located in global 
> directory (Program Files),
> service replies to gui via pipe with error message:
>
> 0x2001
> You have specified a config file location (client.xxx.ovpn relative to 
> C:\Users\lev\OpenVPN\config\client.xxx) that requires admin approval. This 
> error may be avoided by adding your account to the "OpenVPN Administrators" 
> group
> (null)
>

Nothing is as simple as it looks, is it..

After the first round of stupidity, I had run-tested version 2,  but
still did not catch this.

> This message is incorrect, since account is in admin group, we just haven't 
> checked that yet. When following check verifies that user indeed belongs to 
> the admin group, service runs openvpn and connection got established. By some 
> reasons, above error message is not read on the gui side. However, by adding 
> small delay to service code or a breakpoint, message reaches gui, which 
> displays an error.
>

Its a timing bug in the GUI. The I/O completion routine for reading
from the service is initialized from the thread that handles the
connection. But this thread is created in a suspended state and
resumed only after sending the start request to the service from the
main thread. That could be the reason why this message is missed by
the GUI. This needs to be fixed as it could potentially affect normal
operation of the GUI.

> While under normal circumstances it works, I elieve it just works by accident 
> (error message is not read on gui side without delay on service side). We 
> need to move sending error message out of ValidateOptions.

This error message was originally added so that the GUI can notify the
user with a helpful message. But the GUI now checks config location
and user's group memberships and offers to spawn an elevated process
to add the user to ovpn_admin_group before calling the service.  But
still useful to return this message in case some other client decides
to use the service. I'll move it out of ValidateOptions and add code
to return it only when appropriate.


Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2] Swap the order of checks for validating interactive service user

2020-01-31 Thread selva . nair
From: Selva Nair 

Check the config file location and command line options first
and membership in OpenVPNAdministrators group after that as
the latter could be a slow process for active directory users.

When connection to domain controllers is poor or unavailable, checking
the group membership is slow and causes timeouts in the GUI (Trac
1051). However, in cases where the config is in the global directory,
no group membership check should be required. The re-ordering here
avoids the redundant check in such cases.

In addition to this, its also proposed to improve the timeout handling
in the GUI, but this change is still useful as it should completely
eliminate the timeout issue for many users.

Also see: https://github.com/OpenVPN/openvpn-gui/issues/332

Signed-off-by: Selva Nair 
---

v2: Add missing closing parenthesis and improve the comment
above the edited chunk.

 src/openvpnserv/interactive.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 6e72a14..ff5b08b 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1580,9 +1580,15 @@ RunOpenvpn(LPVOID p)
 goto out;
 }
 
-/* Check user is authorized or options are white-listed */
-if (!IsAuthorizedUser(ovpn_user->User.Sid, imp_token, 
settings.ovpn_admin_group)
-&& !ValidateOptions(pipe, sud.directory, sud.options))
+/*
+ * Only authorized users are allowed to use any command line options or
+ * have the config file in locations other than the global config 
directory.
+ *
+ * Check options are white-listed and config is in the global directory
+ * OR user is authorized to run any config.
+ */
+if (!ValidateOptions(pipe, sud.directory, sud.options)
+&& !IsAuthorizedUser(ovpn_user->User.Sid, imp_token, 
settings.ovpn_admin_group))
 {
 goto out;
 }
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH] Swap the order of checks for validating interactive service user

2020-01-31 Thread Selva Nair
Hi,

On Fri, Jan 31, 2020 at 5:29 AM Lev Stipakov  wrote:
>
> Hi,
>
>> +if (!ValidateOptions(pipe, sud.directory, sud.options)
>> +&& !IsAuthorizedUser(ovpn_user->User.Sid, imp_token, 
>> settings.ovpn_admin_group)
>>  {
>
>
> Closing parenthesis is missing:

That is embarrassing.. I went through the motions of compile testing it without
updating the source tree in openvpnbuild. Yikes..

>
> >C:\Users\lev\Projects\openvpn\src\openvpnserv\interactive.c(1586,5): error 
> >C2143: syntax error: missing ')' before '{'
>
> Also it is probably just me, but took me a while to figure out what that code 
> is doing and why.
>
> Could you slightly improve the comment above lines you touch
> ("Check user is authorized or options are white-listed") and mention 
> something like
> "Non-authorized users are allowed to use only white-listed options and
> have config only in global openvpn config directory"?

Will do.

>
> When I started to debug it, I realized that I have to be authorized user when 
> config
> is in my current user's directory (C:\Users\lev\OpenVPN\config) and not in 
> "global" config
> directory ("C:\Program Files\OpenVPN\config"). Is this by design?

Yes, that is by design. As iservice allows the users to do some actions that
normally require admin privilege, a limited user is only allowed to use some
whitelisted options or a config installed by an admin in the global config
directory. They are not allowed to run arbitrary configs
that they can edit. Unless an admin explicitly gives them permission to do
so --- checked by membership in "OpenVPNAdministrators" group. Users
who have admin privilege (even if not enabled in the token by UAC) are
considered authorized.

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] Swap the order of checks for validating interactive service user

2020-01-30 Thread selva . nair
From: Selva Nair 

Check the config file location and command line options first
and membership in OpenVPNAdministrators group after that as
the latter could be a slow process for active directory users.

When connection to domain controllers is poor or unavailable, checking
the group membership is slow and causes timeouts in the GUI (Trac
1051). However, in cases where the config is in the global directory,
no group membership check should be required. The re-ordering here
avoids the redundant check in such cases.

In addition to this, its also proposed to improve the timeout handling
in the GUI, but this change is still useful as it should completely
eliminate the timeout issue for many users.

Also see: https://github.com/OpenVPN/openvpn-gui/issues/332

Signed-off-by: Selva Nair 
---
 src/openvpnserv/interactive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 6e72a14..dafd5c6 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1581,8 +1581,8 @@ RunOpenvpn(LPVOID p)
 }
 
 /* Check user is authorized or options are white-listed */
-if (!IsAuthorizedUser(ovpn_user->User.Sid, imp_token, 
settings.ovpn_admin_group)
-&& !ValidateOptions(pipe, sud.directory, sud.options))
+if (!ValidateOptions(pipe, sud.directory, sud.options)
+&& !IsAuthorizedUser(ovpn_user->User.Sid, imp_token, 
settings.ovpn_admin_group)
 {
 goto out;
 }
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v3 7/7] wintun: clear adapter settings on tun close

2019-12-17 Thread Selva Nair
Hi,

Probably this is the only one in the series without an ACK.

v2 was reviewed by Simon and suggested changes are in here.
This looks good to me.

On Tue, Nov 12, 2019 at 9:44 AM Lev Stipakov  wrote:
>
> From: Lev Stipakov 
>
> With tap-windows6 we clear adapter settings with DHCP,
> but since wintun doesn't do DHCP we do it with netsh.
>
> Signed-off-by: Lev Stipakov 
> ---
>
>  v3:
>   - simplify convoluted "if" statement
>
>  v2:
>   - rebase on top of master
>
>  src/openvpn/tun.c | 92 +--
>  1 file changed, 57 insertions(+), 35 deletions(-)
>
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 8b16cd6a..a9036a6e 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -6388,6 +6388,50 @@ tun_show_debug(struct tuntap *tt)
>  }
>  }
>
> +static void
> +netsh_delete_address_dns(const struct tuntap *tt, bool ipv6, struct gc_arena 
> *gc)
> +{
> +const char* ifconfig_ip_local;

A minor nit: char *foo is our recommended style, I think.

> +struct argv argv = argv_new();
> +
> +/* "store=active" is needed in Windows 8(.1) to delete the
> + * address we added (pointed out by Cedric Tabary).
> + */
> +
> + /* netsh interface ipvX delete address \"%s\" %s */
> +if (ipv6)
> +{
> +ifconfig_ip_local = print_in6_addr(tt->local_ipv6, 0, gc);
> +}
> +else
> +{
> +ifconfig_ip_local = print_in_addr_t(tt->local, 0, gc);
> +}
> +argv_printf(,
> +"%s%sc interface %s delete address %s %s store=active",
> +get_win_sys_path(),
> +NETSH_PATH_SUFFIX,
> +ipv6 ? "ipv6" : "ipv4",
> +tt->actual_name,
> +ifconfig_ip_local);
> +
> +netsh_command(, 1, M_WARN);
> +
> +/* delete ipvX dns servers if any were set */
> +int len = ipv6 ? tt->options.dns6_len : tt->options.dns_len;
> +if (len > 0)
> +{
> +argv_printf(,
> +"%s%sc interface %s delete dns %s all",
> +get_win_sys_path(),
> +NETSH_PATH_SUFFIX,
> +ipv6 ? "ipv6" : "ipv4",
> +tt->actual_name);
> +netsh_command(, 1, M_WARN);
> +}
> +argv_reset();
> +}
> +
>  void
>  close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
>  {
> @@ -6410,46 +6454,24 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
>  }
>  else
>  {
> -const char *ifconfig_ipv6_local;
> -struct argv argv = argv_new();
> -
> -/* "store=active" is needed in Windows 8(.1) to delete the
> - * address we added (pointed out by Cedric Tabary).
> - */
> -
> -/* netsh interface ipv6 delete address \"%s\" %s */
> -ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0,  );
> -argv_printf(,
> -"%s%sc interface ipv6 delete address %s %s 
> store=active",
> -get_win_sys_path(),
> -NETSH_PATH_SUFFIX,
> -tt->actual_name,
> -ifconfig_ipv6_local);
> -
> -netsh_command(, 1, M_WARN);
> -
> -/* delete ipv6 dns servers if any were set */
> -if (tt->options.dns6_len > 0)
> -{
> -argv_printf(,
> -"%s%sc interface ipv6 delete dns %s all",
> -get_win_sys_path(),
> -NETSH_PATH_SUFFIX,
> -tt->actual_name);
> -netsh_command(, 1, M_WARN);
> -}
> -argv_reset();
> +netsh_delete_address_dns(tt, true, );
>  }
>  }
>  #if 1
> -if (tt->wintun && tt->options.msg_channel)
> +if (tt->wintun)
>  {
> -do_route_ipv4_service_tun(false, tt);
> -do_address_service(false, AF_INET, tt);
> -do_dns_service(false, AF_INET, tt);
> +if (tt->options.msg_channel)
> +{
> +do_route_ipv4_service_tun(false, tt);
> +do_address_service(false, AF_INET, tt);
> +do_dns_service(false, AF_INET, tt);
> +}
> +else
> +{
> +netsh_delete_address_dns(tt, false, );
> +}
>  }
> -else
> -if (tt->ipapi_context_defined)
> +else if (tt->ipapi_context_defined)
>  {
>  DWORD status;
>  if ((status = DeleteIPAddress(tt->ipapi_context)) != NO_ERROR)

Acked by: selva.n...@gmail.com

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O

2019-12-17 Thread Selva Nair
Hi

On Tue, Dec 17, 2019 at 6:09 AM Simon Rozman  wrote:
>
> I have been playing with Lev's patches for the past few days. Tested them, 
> debugged them, did some fixes. There are things to be desired like 
> netsh=>ipcfg, remove or #ifdef the SYSTEM token hack... But those are design 
> choices we should pursue in the future. I believe patches are mature enough 
> to ack them. They should be merged into master to provide wider testing and 
> easier development progress.

I agree. And, if we wont release official binaries with the system
hack, the patch looks good to me too.

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O

2019-12-17 Thread Selva Nair
Hi Simon,

A quick reply:

> > IMO, the right approach on Windows is to run a bare minimal code as a
> > service to get SYSTEM rights and the rest with limited privileges.
>
> Selva, those are two different use-cases. And none is "right" or "wrong". 
> OpenVPN can or should have both. :)
>
> 1. I need to run VPN tunnel as a persistent service - something that comes up 
> with computer (Group Policy Client service waits for about 30 seconds on boot 
> to get network access to AD server). And stays on all the time - any user 
> signed in or not. I connect computers with VPN.

I too use OpenVPN like this, so I do understand the use case. And, the
point was that the exe can be started through interactive service even
in this case. That would allow running openvpn.exe at boot from a
service with low privileges that delegates all privileged actions to
iservice. Years ago when iservice was introduced we did briefly
discuss this (with Heiko) but left it as a future enhancement which of
course no one had time for.

Unless I'm missing some scenario where this wont work. Anyway, this is
beyond the scope of the current patch.

Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O

2019-12-16 Thread Selva Nair
Hi,

On Mon, Dec 16, 2019 at 5:18 PM Simon Rozman  wrote:
>
> Hi,
>
> TLDR:
> (i) stealing SYSTEM access from winlogon.exe is not a good thing to do
>
>
>
> This doesn't happen for the majority of use cases - only when iservice is not 
> used. We also
> elevate only for the single DeviceIOControl call.
>
>
> I understand. But stealing access token from winlogon.exe is a hack and not
> something I would expect to see in a trustworthy executable. Diagnostic
> and forensic tools may be justified in doing such things.
>
>
> Wintun has a hardcoded check to allow ring registration from the SYSTEM user 
> only.
>
> To be honest: I am using a Windows 10 VPC in test mode with a modified Wintun 
> driver installed that also allows ring registration for the members of 
> Administrators group. This allows me to quickly test ideas while 
> reviewing/upgrading Lev's work. I can run Visual Studio to compile 
> openvpn.exe on my computer as an unprivileged user. Then have an elevated 
> Remote Debugger running on the VPC and Visual Studio to remotely run 
> openvpn.exe with debugger attached with a single F5 click.
>
> Having to use OpenVPN GUI and iservice, or running openvpn.exe as service 
> would require a lot more clicks to replace the binary, attach debugger etc.
>
> As far as I am concerned, this elevation hack may be removed from the OpenVPN 
> source code in the final release. However, mind that this would prevent 
> people from running openvpn.exe+Wintun from the command line.
>
> +bool
>
> +impersonate_as_system()
> +{
>
>
> This is implemented by stealing the access token from
> winlogon.exe. I don't think such tricks belong to OpenVPN.
> It may also trip some anti-virus software.
>
> That said, probably there are no "legitimate" ways of getting
> LOCAL SYSTEM rights on Windows without running a service.
>
> Why does wintun require SYSTEM for using it? If there is a
> good reason for that, we should not let every admin
> user bypass it.
>
>
>
> I'll defer it to Simon.
>
>
> Unfortunately, I don't do security decisions in Wintun.
>
> Wintun was originally designed for WireGuard. WireGuard is architectured to 
> run all its tunnels as Windows services running as the SYSTEM user. Wintun's 
> security is as tight as possible so the WireGuard can barely use it. I know a 
> guy who is tempted to introduce a userspace binary code signature check to 
> the Wintun. :)
>
> Given the relative ease to get SYSTEM token just by being an elevated process 
> — mind there's also a hack to get from non-elevated to elevated completely 
> bypassing the UAC prompt as long as you are a member of Administrators — this 
> SYSTEM restriction really doesn't provide considerable additional security 
> compared to being a member of Administrators group.

That's the point. It probably provides no extra security because we
are forced to take the blame for using hacks to cross security
boundaries. And, bypassing UAC is quite different from acquiring
rights you were not supposed to have. Does openvpn wants to be doing
that?

>
> Those who really need to test OpenVPN with wintun from
> command prompt can use diagnostic tools available to get
> a cmd prompt as system (e.g., psexec). That also  makes
> it explicit that SYSTEM privilege is required.
>
> In the longer run, we could provide a script to launch
> openvpn.exe using the interactive service. Modifying the
> automatic service to use interactive service for launching
> looks easy to do as well. Then, all privileged operations could
> be removed from openvpn core.
>
>
>
> I think it is good not to break user experience and allow run openvpn as
> an administrator without iservice using wintun at the expense on elevation
> to system for single API call.
>
>
> I have already said what I think of it. As an admin I wouldn't like to see
> users running processes that elevate to SYSTEM like this.
>
>
> Selva, Windows is full of such hacks internally. :( This is no excuse for us 
> doing the same of course. Just saying Windows is far from ideal world.

In this particular case this has nothing to do with Windows -- its
just a stupid decision by wintun that requires the use of undocumented
hacks to elevate to SYSTEM.

>
> I am running OpenVPN on Windows using NSSM wrapper for years. I had a brief 
> discussion on the Hackathon with Samuli about integrating SCM support 
> directly into openvpn.exe (imagine --daemon for Windows):
>
> sc create OpenVPN$MyTunnel binpath= "C:\Program Files\OpenVPN\bin\openvpn" 
> --daemon --config "C:\Program Files\OpenVPN\config\MyTunnel.ovpn" --log 
> "C:\Program Files\OpenVPN\log\MyTunnel.log" start= auto depend= dhcp
> sc start OpenVPN$MyTunnel
>
> This would install openvpn.exe as a Windows service and run it as the SYSTEM 
> user — no need for iservice, no need for SYSTEM token hack. Other than me, 
> perhaps it could cover at least some of the users now running openvpn.exe 
> directly.

This is not the direction I want to see us moving towards. Instead I
want to 

Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O

2019-12-16 Thread Selva Nair
Hi

On Mon, Dec 16, 2019 at 4:31 PM Lev Stipakov  wrote:
>>
>> I have already said what I think of it. As an admin I wouldn't like to see
>> users running processes that elevate to SYSTEM like this.
>
>
> Would it be too much if
>
>  - openvpn.exe process detects that it is not started by interactive service
>  - interactive service process is running
>  - wintun is used as --windows-driver
>

I think this is possible and in fact I had something like this in mind
as well I wrote we could provide an openvpn launcher script.

And I would like to do this even if wintun is in use as ability to run
openvpn.exe as a regular would be a plus for everyone.

> then openvpn behaves as a command-line version of openvpn-gui - connects to 
> service, which
> runs openvpn.exe, and then prints output, received via management interface, 
> to console?

It may be better to redirect the output to the stdout or file as usual
so that no
user visible changes and easier to implement too. Just keep the process id
returned from the service and when the user kills the front-end, terminate the
background process started by the service too.

This should also allow to run the automatic service as LocalService or a
special service user as many services do.

Selva

>
> --
> -Lev


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


  1   2   3   4   5   6   7   8   9   10   >