Re: [Openvpn-devel] Summary of the community meeting (26th March 2020)
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: > > <https://patchwork.openvpn.net/patch/1040/> > > Further discussion of the issue is available here: > > <https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg12835.html> > 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
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
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
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
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
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
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()
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
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
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
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
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
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
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
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
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
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
-- 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
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
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
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
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
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
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
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
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
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi
On Mon, Dec 16, 2019 at 3:01 PM Lev Stipakov wrote:
>
> Hi,
>
> Thanks for looking into this. See my comments below.
>
>> 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.
>
> Below you mentioned psexec as a one of workarounds, but I think
> those will make user experience worse.
I was not suggesting it as an approach for "users" -- only for testing
or for those
who insist running OpenVPN from command line.
> Also consider the case when OpenVPN GUI is running as Administrator.
That would be a wrong way of using the GUI in 2.4 + releases. We need not and
should not (IMO) support the GUI running as admin. That said, now that vista is
no longer supported we can enable the use of iservice with GUI running as admin.
>
>> (ii) with a small change we can support multiple tunnels and provide better
>> diagnostics when there are no free wintun adapters -- but this could also
>> be a follow up patch.
>
>
> This is already implemented by Simon on top of my patches - see
> https://github.com/rozmansi/openvpn/commits/wintun
>
>> > -/* for wintun kernel doesn't send DHCP requests, so use ipapi to set
>> > IP address and netmask */
>>
>> > +/* for wintun kernel doesn't send DHCP requests, so use netsh to set
>> > IP address and netmask */
>> > if (options->wintun)
>> > {
>> > -options->tuntap_options.ip_win32_type = IPW32_SET_IPAPI;
>> > +options->tuntap_options.ip_win32_type = IPW32_SET_NETSH;
>>
>> This shouldn't be required. I would prefer to not use netsh
>> for tasks where API calls are available. We know IPAPI works
>> as we use it in the service.
>
>
> I remember having some issues with IPAPI which got resolved when I switched
> to netsh.
>
> However, I did some testing and I cannot reproduce IPAPI specific issues
> anymore. Too bad I didn't investigate
> it further back then. The problem I see now can also be reproduced also with
> netsh:
>
> - connect with tap adapter
> - kill openvpn process
> - connect with wintun adapter
>
> IPAPI / netsh fails to set IP address, since it is already assigned to
> another adapter.
>
> If this is OK, I would prefer to submit a separate patch which switches back
> to netsh. tun.c code
> has changed in follow-up patches (mostly here
> https://patchwork.openvpn.net/patch/918/), those patches
> are based on an assumption that wintun uses netsh.
>
>>
>> > +msg(M_FATAL, "ERROR: Failed to register ring buffers:
>> > %lu", GetLastError());
>>
>> The correct error here is, very likely, the adapter is
>> already in use. To trigger that, why not do register_ring_buffers
>> soon after opening the device and on failure move on to the
>> next device (if any).
>>
>> That will also allow running multiple tunnels with no further changes
>> provided the user manually creates wintum adapters.
>
>
> As mentioned before, this is already implemented by Simon:
> https://github.com/rozmansi/openvpn/commit/5bc09580e8771d8422feae4504fe2a74b7412fd1
>
>>
>> > +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.
>
>>
>>
>> I would suggest to not elevate to system as the code will still
>> work in two important cases
>> (i) started by automatic service
>> (ii) started using interactive service (e.g., through the GUI)
>>
>> 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 co
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
ights 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 would suggest to not elevate to system as the code will still
work in two important cases
(i) started by automatic service
(ii) started using interactive service (e.g., through the GUI)
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.
> +HANDLE thread_token, process_snapshot, winlogon_process, winlogon_token,
> duplicated_token;
> +PROCESSENTRY32 entry;
> +BOOL ret;
> +DWORD pid = 0;
> +TOKEN_PRIVILEGES privileges;
> +
> +CLEAR(entry);
> +CLEAR(privileges);
> +
> +entry.dwSize = sizeof(PROCESSENTRY32);
> +
> +privileges.PrivilegeCount = 1;
> +privileges.Privileges->Attributes = SE_PRIVILEGE_ENABLED;
> +
> +if (!LookupPrivilegeValue(NULL, SE_DEBUG_NAME,
> [0].Luid))
> +{
> +return false;
In case we keep this function, please log such
errors. Here and several such cases below.
> +}
> +
> +if (!ImpersonateSelf(SecurityImpersonation))
> +{
> +return false;
> +}
> +
> +if (!OpenThreadToken(GetCurrentThread(), TOKEN_ADJUST_PRIVILEGES, FALSE,
> _token))
> +{
> +RevertToSelf();
> +return false;
> +}
> +if (!AdjustTokenPrivileges(thread_token, FALSE, ,
> sizeof(privileges), NULL, NULL))
> +{
> +CloseHandle(thread_token);
> +RevertToSelf();
> +return false;
> +}
> +CloseHandle(thread_token);
> +
> +process_snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
> +if (process_snapshot == INVALID_HANDLE_VALUE)
> +{
> +RevertToSelf();
> +return false;
> +}
> +for (ret = Process32First(process_snapshot, ); ret; ret =
> Process32Next(process_snapshot, ))
> +{
> +if (!_stricmp(entry.szExeFile, "winlogon.exe"))
> +{
> +pid = entry.th32ProcessID;
> +break;
> +}
> +}
> +CloseHandle(process_snapshot);
> +if (!pid)
> +{
> +RevertToSelf();
> +return false;
> +}
> +
> +winlogon_process = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);
> +if (!winlogon_process)
> +{
> +RevertToSelf();
> +return false;
> +}
> +
> +if (!OpenProcessToken(winlogon_process, TOKEN_IMPERSONATE |
> TOKEN_DUPLICATE, _token))
> +{
> +CloseHandle(winlogon_process);
> +RevertToSelf();
> +return false;
> +}
> +CloseHandle(winlogon_process);
> +
> +if (!DuplicateToken(winlogon_token, SecurityImpersonation,
> _token))
> +{
> +CloseHandle(winlogon_token);
> +RevertToSelf();
> +return false;
> +}
> +CloseHandle(winlogon_token);
> +
> +if (!SetThreadToken(NULL, duplicated_token))
> +{
> +CloseHandle(duplicated_token);
> +RevertToSelf();
> +return false;
> +}
> +CloseHandle(duplicated_token);
> +
> +return true;
> +}
> +
> +bool
> +register_ring_buffers(HANDLE device,
> + struct tun_ring* send_ring,
> + struct tun_ring* receive_ring,
> + HANDLE send_tail_moved,
> + HANDLE receive_tail_moved)
> +{
> +struct tun_register_rings rr;
> +BOOL res;
> +DWORD bytes_returned;
> +
> +ZeroMemory(, sizeof(rr));
> +
> +rr.send.ring = send_ring;
> +rr.send.ring_size = sizeof(send_ring->data);
> +rr.send.tail_moved = send_tail_moved;
> +
> +rr.receive.ring = receive_ring;
> +rr.receive.ring_size = sizeof(receive_ring->data);
> +rr.receive.tail_moved = receive_tail_moved;
> +
> +res = DeviceIoControl(device, TUN_IOCTL_REGISTER_RINGS, , sizeof(rr),
> + NULL, 0, _returned, NULL);
> +
> +return res == TRUE;
> +}
> +
> #endif /* ifdef _WIN32 */
> diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
> index 4814bbc5..f6d45bdc 100644
> --- a/src/openvpn/win32.h
> +++ b/src/openvpn/win32.h
> @@ -25,6 +25,8 @@
> #ifndef OPENVPN_WIN32_H
> #define OPENVPN_WIN32_H
>
> +#include
> +
> #include "mtu.h"
> #include "openvpn-msg.h"
> #include "argv.h"
> @@ -323,5 +325,54 @@ bool send_msg_iservice(HANDLE pipe, const void *data,
> size_t size,
> int
> openvpn_execve(const struct argv *a, const struct env_set *es, const
> unsigned int flags);
>
> +/*
> + * Values below are taken from Wireguard Windows client
> + *
> https://github.com/WireGuard/wireguard-go/blob/master/tun/wintun/ring_windows.go#L14
> + */
> +#define WINTUN_RING_CAPACITY 0x80
> +#define WINTUN_RING_TRAILING_BYTES 0x1
> +#define WINTUN_RING_FRAMING_SIZE 12
> +#define WINTUN_MAX_PACKET_SIZE 0x
> +#define WINTUN_PACKET_ALIGN 4
> +
> +struct tun_ring
> +{
> +volatile ULONG head;
> +volatile ULONG tail;
> +volatile LONG alertable;
> +UCHAR data[WINTUN_RING_CAPACITY + WINTUN_RING_TRAILING_BYTES +
> WINTUN_RING_FRAMING_SIZE];
> +};
> +
> +struct tun_register_rings
> +{
> +struct
> +{
> +ULONG ring_size;
> +struct tun_ring *ring;
> +HANDLE tail_moved;
> +} send, receive;
> +};
> +
> +struct TUN_PACKET_HEADER
> +{
> +uint32_t size;
> +};
> +
> +struct TUN_PACKET
> +{
> +uint32_t size;
> +UCHAR data[WINTUN_MAX_PACKET_SIZE];
> +};
> +
> +#define TUN_IOCTL_REGISTER_RINGS CTL_CODE(51820U, 0x970U, METHOD_BUFFERED,
> FILE_READ_DATA | FILE_WRITE_DATA)
> +
> +bool impersonate_as_system();
> +
> +bool register_ring_buffers(HANDLE device,
> + struct tun_ring *send_ring,
> + struct tun_ring *receive_ring,
> + HANDLE send_tail_moved,
> + HANDLE receive_tail_moved);
> +
> #endif /* ifndef OPENVPN_WIN32_H */
> #endif /* ifdef _WIN32 */
> --
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] fix clang warning about missing braces
Hi
On Thu, Nov 28, 2019 at 10:23 AM Steffan Karger <
steffan.kar...@foxcrypto.com> wrote:
> On 28-11-2019 09:06, Lev Stipakov wrote:
> > A struct with subobjects should be initialized
> > with double braces.
>
> This is not true. {0} is a valid initializer for structs in C. Both
> clang and gcc used to have a bug where they incorrectly warned about
> this. GCC fixed this a while ago[0]. I thought clang had fixed it too
> recently, but apparently not?
>
Pretty much same as my thoughts.
I think the correct fix here is to remove -Werror from travis build.
I have tried and failed to lobby for this earlier, but one more try
can't hurt, I suppose :)
That said, it seems clang has fixed this some time after clang-7.
I don't get this warning anymore after upgrading to clang-9.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi
On Mon, Nov 25, 2019 at 4:03 AM Lev Stipakov wrote:
> Hi,
>
> (cc:ed to -devel)
>
>
>> I would vote for B and not the combination.
>>
>> With wintun there is no backwards compatibility requirements, so we could
>> use a cleaner, consistent and simpler approach (i.e B). Do not create any
>> adapter during installation and dynamically create a temporary adapter at
>> connection time.
>>
>
> My main concern with creating tun adapter on demand is that it is far from
> instant:
>
> $ time ./tapctl.exe create --hwid wintun
> {D9F56B7A-3054-4ADC-9457-61030F0B469D}
>
> real0m2,090s
>
> I don't think we want to add it to connection time.
>
No, we don't want that.
I do realize what I wrote earlier was far from ideal. I had forgotten my
travails two years back attempting to add dynamic tap adapter creation to
OpnVPN
As an easy way for supporting multiple tunnels, can we treat success of
open-device + ring-buffer-registration calls together as a successful open
of the adapter, and move on to the next one when that fails? That would be
very similar to what we do for tapwindows right now.
To assist users, when we fail with no free adapters, we could add a feature
to the GUI to add a new one using the RunAsAdmin shell-execute we already
have support for. Will need an improved devcon/tapinstall that does not
reset all adapters when a new one is added, except when unavoidable due to
driver version change.
Simon's suggestion of persistent adapters as required and kept disabled
when not in use sounds interesting. As some providers do supply 100's of
configs, I think we should refrain from creating one adapter per ovpn,
though. They should be using multiple --remote lines in a single config,
but, for that to work well, we need to improve --management-remote option
to provide a friendly UI for remote selection.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v7 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key
Hi, Thanks for the updates. In spite of several nits below, I'm ACKing this. All remarks are typos or grammar, important only for docs and some comments. I suggest to handle these as a minor follow up patch. I'm also ignoring most typos in commit message except a few that could be corrected during merge to add clarity. On Fri, Nov 22, 2019 at 9:34 AM Arne Schwabe wrote: > For TLS 1.0 to 1.2 and OpenSSL 1.1.0 calls us and requires a PKCS1 padded response. As TLS 1.3 mandates RSA-PSS padding support and also > requires an TLS 1.3 implementation to support RSA-PSS for older TLS version, OpenSSL will query us to sign an already RSA-PSS padded string. > > This patch adds an 'unpadded' and 'pkcs1' parameter to the management-external-key option to signal that the client is > able to support pkcs1 as well as unpadded signature requests. > > Since clients that implement the management-external-key interface > are usually rather tightly integrated solutions (OpenVPN Connect in the > past, OpenVPN for Android), it is reasonable to expect that > upgrading the OpenSSL library can be done together with > management interface changes. Therefore we provide no backwards > compatbility for mangement-interface clients not supporting > OpenSSL 1.1.1. > > Using the management api client version instead might seem like the > more logical way but since we only now that version very late, > now --> know it would extra logic and complexity to deal with this asynchronous > would --> would require behaviour. Instead just give an error early if OpenSSL 1.1.1 and > management-external-key without nopadding is detected. > > The interface is prepared for signalling PCKS1 and RSA-PSS support > instead of signalling unpadded support. > > Patch v3: fix overlong lines and few other style patches. Note > two overlong lines concerning mbedtls are not fixed as they > are removed/shortend by the mbed tls patch to avoid conflicts > > Patch v4: Setting minimum TLS version proved to be not enough and > instead of implementing a whole compability layer we require mangement-clients to implement the new feature when they want > to use OpenSSL 1.1.1 > > Add a padding=ALGORITHM argument to pk-sig to indicate the > algorithm. Drop adding PKCS1 ourselves. > Patch v5: Send the right version of the patch > Patch v6: rebase on master > Patch v7: change style and reword documentation. Make thing more consistent > Signed-off-by: Arne Schwabe > --- > doc/management-notes.txt | 21 - > doc/openvpn.8 | 7 +++-- > src/openvpn/manage.c | 20 +--- > src/openvpn/manage.h | 19 +++- > src/openvpn/options.c | 36 +- > src/openvpn/ssl_mbedtls.c | 8 +++-- > src/openvpn/ssl_openssl.c | 64 ++- > 7 files changed, 143 insertions(+), 32 deletions(-) > > diff --git a/doc/management-notes.txt b/doc/management-notes.txt > index 17645c1d..c64e594d 100644 > --- a/doc/management-notes.txt > +++ b/doc/management-notes.txt > @@ -816,6 +816,7 @@ actual private key. When the SSL protocol needs to > perform a sign > operation, the data to be signed will be sent to the management interface > via a notification as follows: > > +>PK_SIGN:[BASE64_DATA],[ALG] (if client announces support for management > version > 2) > >PK_SIGN:[BASE64_DATA] (if client announces support for management > version > 1) > >RSA_SIGN:[BASE64_DATA] (only older clients will be prompted like this) > > @@ -823,7 +824,7 @@ The management interface client should then create an > appropriate signature of > the (decoded) BASE64_DATA using the private key and return the SSL > signature as > follows: > > -pk-sig (or rsa-sig) > +pk-sig (or rsa-sig) > [BASE64_SIG_LINE] > . > . > @@ -833,6 +834,12 @@ END > Base 64 encoded output of RSA_private_encrypt for RSA or ECDSA_sign() > for EC using OpenSSL or mbedtls_pk_sign() using mbed TLS will provide a > correct signature. > +The rsa-sig interfaces expects PKCS1 padded signatures for RSA keys > "interface expects" or" interfaces expect" +(RSA_PKCS1_PADDING). EC signatures are always unpadded. > + > +The padding field is only present when pk-sig is used and > +currently the following values can be requested PCKS1 and NOPADDING for > RSA > +certificates and NOPADDING for EC certificates. > Remove this statement. Its no longer correct after the latest changes. Anyway this info is repeated below with correct keywords. > This capability is intended to allow the use of arbitrary cryptographic > service providers with OpenVPN via the management interface. > @@ -840,6 +847,18 @@ service providers with OpenVPN via the management > interface. > New and updated clients are expected to use the version command to > announce > a version > 1 and handle '>PK_SIGN' prompt and respond with 'pk-sig'. > > +The signature algorithm is indicated in the PK_SIGN request only if the > +management
Re: [Openvpn-devel] [PATCH v7 1/2] Make tls_version_max return the actual maximum version
Hi,
On Fri, Nov 22, 2019 at 9:34 AM Arne Schwabe wrote:
> Before OpenSSL 1.1.1 there could be no mismatch between
> compiled and actual OpenSSL version. With OpenSSL 1.1.1 we need
> runtime detection to detect the actual best TLS version supported.
>
> Allowing this runtime detection also allows removing some of the
> TLS 1.3/OpenSSL 1.1.1 #ifdefs
>
> Without this patch tls-min-version 1.3 or-highest will actually
> downgrade to TLS 1.3
I think that should read "TLS 1.2" not "TLS 1.3"
> in the "compiled with 1.1.0 and linked against
> 1.1.1" scenario.
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/ssl.c | 11 +--
> src/openvpn/ssl_openssl.c | 39 ---
> 2 files changed, 41 insertions(+), 9 deletions(-)
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 4455ebb8..e708fc93 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -4194,12 +4194,11 @@ show_available_tls_ciphers(const char *cipher_list,
> {
> printf("Available TLS Ciphers, listed in order of preference:\n");
>
> -#if (ENABLE_CRYPTO_OPENSSL && OPENSSL_VERSION_NUMBER >= 0x1010100fL)
> -printf("\nFor TLS 1.3 and newer (--tls-ciphersuites):\n\n");
> -show_available_tls_ciphers_list(cipher_list_tls13, tls_cert_profile,
> true);
> -#else
> -(void) cipher_list_tls13; /* Avoid unused warning */
> -#endif
> +if (tls_version_max() >= TLS_VER_1_3)
> +{
> +printf("\nFor TLS 1.3 and newer (--tls-ciphersuites):\n\n");
> +show_available_tls_ciphers_list(cipher_list_tls13,
> tls_cert_profile, true);
> +}
>
> printf("\nFor TLS 1.2 and older (--tls-cipher):\n\n");
> show_available_tls_ciphers_list(cipher_list, tls_cert_profile, false);
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 07916c3c..a080338e 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -215,7 +215,26 @@ int
> tls_version_max(void)
> {
> #if defined(TLS1_3_VERSION)
> +/* If this is defined we can safely assume TLS 1.3 support */
> return TLS_VER_1_3;
> +#elif OPENSSL_VERSION_NUMBER >= 0x1010L
> +/*
> + * If TLS_VER_1_3 is not defined, we were compiled against a version
> that
> + * did not support TLS 1.3.
> + *
> + * However, the library we are *linked* against might be OpenSSL 1.1.1
> + * and therefore supports TLS 1.3. This needs to be checked at runtime
> + * since we can be compiled against 1.1.0 and then the library can be
> + * upgraded to 1.1.1
> + */
> +if (OpenSSL_version_num() >= 0x1010100fL)
> +{
> +return TLS_VER_1_3;
> +}
> +else
> +{
> +return TLS_VER_1_2;
> +}
> #elif defined(TLS1_2_VERSION) || defined(SSL_OP_NO_TLSv1_2)
> return TLS_VER_1_2;
> #elif defined(TLS1_1_VERSION) || defined(SSL_OP_NO_TLSv1_1)
> @@ -241,12 +260,25 @@ openssl_tls_version(int ver)
> {
> return TLS1_2_VERSION;
> }
> -#if defined(TLS1_3_VERSION)
> else if (ver == TLS_VER_1_3)
> {
> +/*
> + * Supporting the library upgraded to TLS1.3 without recompile
> + * is enough to support here with a simple constant that the same
> + * as in the TLS 1.3, so spec it is very unlikely that OpenSSL
> + * will change this constant
>
I have no idea what that comment means, but I get the idea and will pass :)
> + */
> +#ifndef TLS1_3_VERSION
> +/*
> + * We do not want to define TLS_VER_1_3 if not defined
> + * since other parts of the code use the existance of this macro
> + * as proxy for TLS 1.3 support
> + */
> +return 0x0304;
> +#else
> return TLS1_3_VERSION;
> -}
> #endif
> +}
> return 0;
> }
>
> @@ -2015,7 +2047,8 @@ show_available_tls_ciphers_list(const char
> *cipher_list,
> #if defined(TLS1_3_VERSION)
> if (tls13)
> {
> -SSL_CTX_set_min_proto_version(tls_ctx.ctx, TLS1_3_VERSION);
> +SSL_CTX_set_min_proto_version(tls_ctx.ctx,
> + openssl_tls_version(TLS_VER_1_3));
> tls_ctx_restrict_ciphers_tls13(_ctx, cipher_list);
> }
> else
> --
>
Tested using OpenSSL 1.1.0 based build with OpenSSL 1.1.1 at
runtime.
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] [PATCH] Fix ACL_CHECK_ADD_COMPILE_FLAGS to work with clang
Hi, On Tue, Nov 19, 2019 at 9:09 AM David Sommerseth < open...@sf.lists.topphemmelig.net> wrote: > On 14/11/2019 22:58, Selva Nair wrote: > > Hi David > > > > Thanks for the comments > > > > My idea was just to add -Werror right in the line above, and not > extend the > > ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument. > > > > > > I'm fine with that approach as well. Let me know if you want a v2. > > Sorry for the delay responding. I've done some pondering, and I have come > to > the same conclusion as Arne, so this is a reasonable approach to add > -Werror. > > That said, is there any specific reasons why adding the [-Werror] to > essentially all ACL_CHECK_ADD_COMPILE_FLAGS() calls? Our ./configure > script > is the only one implementing and supporting and using this macro, and it is > defined in configure.ac. > > If there are no foreseeable reason for that now, I think it's just fine to > change the AC_COMPILE_IFELSE() call to use CFLAGS="$1 -Werror $old_cflags". > Just add a comment in that area why we add the -Werror explicitly. This > makes > the whole change smaller and clearer. If we need the flexibility later > on, we > can adjust this when that time arrives. > Agreed. v2 follows. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] wintun: add --windows-driver config option
Hi,
On Tue, Nov 19, 2019 at 3:29 AM Lev Stipakov wrote:
> Hello,
>
> ti 19. marrask. 2019 klo 9.37 Gert Doering (g...@greenie.muc.de)
> kirjoitti:
>
> > Looks like this will most likely break any dhcp-related options
>> > in the client config.. Say "dhcp-option DNS xxx".
>
>
> Oops, indeed. When I add those to client config, I got:
>
> > Options error: --dhcp-options requires --ip-win32 dynamic or adaptive
>
>
>> We currently require ip_win32_type to be adaptive or dynamic when such
>> options are specified.
>>
>
> That limitation has been presented since the very first commit in github:
>
> if (options->tuntap_options.dhcp_options
> && 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");
> }
>
> I commented it out and client-side --dhcp-options worked for me. Any ideas
> why we have this code?
>
>
>> > I think we should set this to adaptive here, and then handle it in tun.c
>>
>
> Actually, in follow-up commit I have changed IPAPI to NETSH - if I
> remember correctly
> I could not set up routing using former, although it works when done by
> iserv.
>
> If we don't know why we have that check and we could apply dhcp options
> from client config
> without it, shouldn't we just get rid of it?
>
>
>> >
>> > Although there is no dhcp when wintun is used, we'll still support
>> > dhcp-options such as DNS etc using netsh or service, right?
>>
>
> That's correct. At the moment we use:
>
> - netsh when running openvpn without interactive service
> - iserv, which itself uses both IPAPI (for setting up IP address and
> routing) and netsh (for setting up DNS)
>
The ip-win32 mess does need clenup but at least it would be nice to use the
same API/method in iservice and the core. Can we figure out why IP helper
API is not working for setting IP? Unfortunately there is no API for
setting DNS (not that I know of) so there the use of netsh is unavoidable.
Anyway, wintun + dhcp-option in client config is broken right now. Looks
like it may work when dhcp-option is pushed (not tested) but there are
cases where we do use it directly in the config.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi, On Tue, Nov 19, 2019 at 3:50 AM Lev Stipakov wrote: > Hi, > > Doesn't this mean that if --dev-node is specified, we'll open tapwindows >> adapter >> with that name even if "--window-driver wintun" is specified? The open >> may succeed >> but subsequent wintun-specific processing will fail. >> > > --dev-node is not (yet) supported and open will fail (just re-tested). > I did a quick test and found the open to succeed with a misleading message that Wintun device opened (though it opened the tap device) and then fails on register ring buffers. I haven't applied all patches as yet, so not sure whether this is changed in later patches. If I'm not mistaken wintun device can be opened multiple times, so we'll >> never get the >> "All wintun adapters on this system" error. >> > Well, we get it if wintun driver is not installed :) but yep, open will > succeed. > Instead, open will succeed here and something else may fail later. >> > > Right after opening we register ring buffers, and that will fail (with a > friendly message): > > ERROR: Failed to register ring buffers: 1247 > > We should probably make it even more user friendly and write something > like "make sure wintun adapter is not in use". > Apart from the error message, there is a larger issue especially when we use iservice. In that case, we have to preserve privilege separation and allowing a user to open a device handle in use by another has to be avoided. The service could keep a lock on devices it manages, but I have no idea how we are going to lock out devices opened by other means (say instances started by automatic service, or wireguard or some other third party product). Can we get the wintun folks to support exclusive open (say FILE_SHARE_READ = 0) or expose the internal count of number of open handles? That would also make it easier to fix this and provide better error messages. > Wireguard creates wintun adapter on demand (on connect) and openvpn during > installation and then > picks the first one found on connect. Maybe that logic could be more > clever to not to pick adapter which > is currently used by wireguard. > > I haven't looked yet into running wireguard and openvpn side by side (or > multiple openvpn instances > with wintun), just tested that wg and openvpn could co-exist without > problems on the same machine. > Hmm.. if multiple openvpn instances are not tested this is not ready for review yet, is it? Again, a quick test shows that, with multiple openvpn instances, it does open an already opened device and then fails while registering ring buffers. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi,
On Thu, Nov 7, 2019 at 12:49 PM Lev Stipakov wrote:
> From: Lev Stipakov
>
> To open wintun device, we cannot use "\\.\Global\Wintun"
> path as before. To get device path which we supply to CreateFile,
> we have to use SetupAPI to:
>
> - enumerate network adapters with "wintun" as component id
> - for each adapter save its guid
> - open device information set
> - for each item in set
>- open corresponding registry key to get net_cfg_instance_id
>- get symbolic link name of device interface by instance id
> - path will be symbolic link name of device instance matched with
> adapter's guid
>
> See
> https://github.com/OpenVPN/openvpn3/blob/master/openvpn/tun/win/tunutil.hpp
> and
>
> https://github.com/WireGuard/wireguard-go/blob/master/tun/wintun/wintun_windows.go
> for
> implementation examples.
>
> Signed-off-by: Lev Stipakov
>
This also has been ACKed and merged, but two questions that may need some
attention:
@@ -5541,7 +5668,8 @@ void
> open_tun(const char *dev, const char *dev_type, const char *dev_node,
> struct tuntap *tt)
> {
> struct gc_arena gc = gc_new();
> -char device_path[256];
> +char tuntap_device_path[256];
> +char *path = NULL;
> const char *device_guid = NULL;
> DWORD len;
> bool dhcp_masq = false;
> @@ -5571,6 +5699,8 @@ open_tun(const char *dev, const char *dev_type,
> const char *dev_node, struct tun
> {
> const struct tap_reg *tap_reg = get_tap_reg();
> const struct panel_reg *panel_reg = get_panel_reg();
> +const struct device_instance_id_interface
> *device_instance_id_interface = get_device_instance_id_interface();
> +
> char actual_buffer[256];
>
> at_least_one_tap_win(tap_reg);
> @@ -5586,24 +5716,22 @@ open_tun(const char *dev, const char *dev_type,
> const char *dev_node, struct tun
> }
>
> /* Open Windows TAP-Windows adapter */
> -openvpn_snprintf(device_path, sizeof(device_path), "%s%s%s",
> +openvpn_snprintf(tuntap_device_path,
> sizeof(tuntap_device_path), "%s%s%s",
> USERMODEDEVICEDIR,
> device_guid,
> TAP_WIN_SUFFIX);
>
> -tt->hand = CreateFile(
> -device_path,
> -GENERIC_READ | GENERIC_WRITE,
> -0,/* was: FILE_SHARE_READ */
> -0,
> -OPEN_EXISTING,
> -FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED,
> -0
> -);
> +tt->hand = CreateFile(tuntap_device_path,
> + GENERIC_READ | GENERIC_WRITE,
> + 0,/* was:
> FILE_SHARE_READ */
> + 0,
> + OPEN_EXISTING,
> + FILE_ATTRIBUTE_SYSTEM |
> FILE_FLAG_OVERLAPPED,
> + 0);
>
> if (tt->hand == INVALID_HANDLE_VALUE)
> {
> -msg(M_ERR, "CreateFile failed on TAP device: %s",
> device_path);
> +msg(M_ERR, "CreateFile failed on TAP device: %s",
> tuntap_device_path);
>
Doesn't this mean that if --dev-node is specified, we'll open tapwindows
adapter
with that name even if "--window-driver wintun" is specified? The open may
succeed
but subsequent wintun-specific processing will fail.
}
> }
> else
> @@ -5613,43 +5741,78 @@ open_tun(const char *dev, const char *dev_type,
> const char *dev_node, struct tun
> /* Try opening all TAP devices until we find one available */
> while (true)
> {
> +bool is_picked_device_wintun = false;
> device_guid = get_unspecified_device_guid(device_number,
>actual_buffer,
>
> sizeof(actual_buffer),
>tap_reg,
>panel_reg,
> +
> _picked_device_wintun,
>);
>
> if (!device_guid)
> {
> -msg(M_FATAL, "All TAP-Windows adapters on this system
> are currently in use.");
> + msg(M_FATAL, "All %s adapters on this system are
> currently in use.", tt->wintun ? "wintun" : "TAP - Windows");
>
If I'm not mistaken wintun device can be opened multiple times, so we'll
never get the
"All wintun adapters on this system" error. Instead, open will succeed
here and
something else may fail later. FILE_SHARE_READ = 0 will not save us when
the driver
does not enforce it.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] wintun: add --windows-driver config option
Hi,
I have been late to this wintun party (no time for anything fun, these days)
and this has already been committed, it seems. But some concerns below..
> +/* for wintun kernel doesn't send DHCP requests, so use ipapi to set
> IP address and netmask */
> +if (options->wintun)
> +{
> +options->tuntap_options.ip_win32_type = IPW32_SET_IPAPI;
> +}
>
Looks like this will most likely break any dhcp-related options
in the client config.. Say "dhcp-option DNS xxx". We currently require
ip_win32_type to be adaptive or dynamic when such options are specified.
I think we should set this to adaptive here, and then handle it in tun.c
Although there is no dhcp when wintun is used, we'll still support
dhcp-options such as DNS etc using netsh or service, right?
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix ACL_CHECK_ADD_COMPILE_FLAGS to work with clang
Hi, On Thu, Nov 14, 2019 at 3:16 PM Илья Шипицин wrote: > Thank you for your efforts. > As you are touching this, can you try "dist: bionic" ? It might bring > newer compilers > I don't expect newer compilers on bionic break this patch. But fwiw, I've started a travis build with dist: bionic. For results see https://travis-ci.org/selvanair/openvpn/jobs/612099524 Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix ACL_CHECK_ADD_COMPILE_FLAGS to work with clang
Hi David Thanks for the comments My idea was just to add -Werror right in the line above, and not extend the > ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument. > I'm fine with that approach as well. Let me know if you want a v2. > I think you said it pretty well in your mail: > > > Darn compilers and darn -Werror > > > So your change does improve Clang ... I dunno what we will do with GCC. > It doesn't affect gcc as gcc does error (or ignore -- see below) unsupported options with or without -Werror. So I think we are okay. travis build on my fork confirms that: https://travis-ci.org/selvanair/openvpn/builds/612019849 However, gcc does have quirky behaviour when it comes to options like -Wno-xx-yy. It just accepts them silently[*] with or without -Werror. That works fine for us, but it does generate an error about that unsupported option if any other error is encountered during the build. As other errors are anyway a show-stopper, I think we can live with that. Selva [*] I think their reasoning is that -Wno-xx-yy can be thus used to suppress warnings added to newer versions without breaking builds with older ones. But clang doesn't share that view. ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Fix ACL_CHECK_ADD_COMPILE_FLAGS to work with clang
From: Selva Nair
Some compilers (e.g., clang) only issue a warning for
unsupported options unless additional flags such
as -Werror are used to convert the warning to an error.
Add support for extra flags in ACL_CHECK_ADD_COMPILE_FLAGS.
Note: a similar approach is used in AX_CHECK_COMPILE_FLAG
in autoconf archives.
Signed-off-by: Selva Nair
---
For successful travis build result see
https://travis-ci.org/selvanair/openvpn/builds/612019849
But this + https://patchwork.openvpn.net/patch/908/
alone does not fix the travis build as clang is not
happy with struct initializers: Like this
crypto.c:1860:31: error: suggest braces around initialization of subobject
[-Werror,-Wmissing-braces]
struct key server_key = { 0 };
I think clang wants {{0}} there.
Darn compilers and darn -Werror :)
configure.ac | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/configure.ac b/configure.ac
index 807804e5..e59bd91b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1275,18 +1275,21 @@ if test "${enable_pkcs11}" = "yes"; then
)
fi
+# Usage: ACL_CHECK_ADD_COMPILE_FLAGS(flag, [extra-flags])
+# Some compilers require extra flags to trigger an error on
+# unsupported options. E.g., clang requires -Werror.
AC_DEFUN([ACL_CHECK_ADD_COMPILE_FLAGS], [
old_cflags="$CFLAGS"
-CFLAGS="$1 $CFLAGS"
-AC_MSG_CHECKING([whether the compiler acceppts $1])
-AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])],
+CFLAGS="$2 $1 $CFLAGS"
+AC_MSG_CHECKING([whether the compiler accepts $1])
+AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])]; CFLAGS="$1
$old_cflags",
[AC_MSG_RESULT([no]); CFLAGS="$old_cflags"])]
)
-ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-stringop-truncation])
-ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-function])
-ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-parameter])
-ACL_CHECK_ADD_COMPILE_FLAGS([-Wall])
+ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-stringop-truncation], [-Werror])
+ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-function], [-Werror])
+ACL_CHECK_ADD_COMPILE_FLAGS([-Wno-unused-parameter], [-Werror])
+ACL_CHECK_ADD_COMPILE_FLAGS([-Wall], [-Werror])
if test "${enable_pedantic}" = "yes"; then
enable_strict="yes"
--
2.20.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] travis: compile with -Werror on Linux
Forgot to hit send on this, and probably this is only partially relevant now, but here goes. Hi On Sun, Nov 10, 2019 at 12:03 PM Gert Doering wrote: > Hi, > > On Sun, Nov 10, 2019 at 11:48:16AM -0500, Selva Nair wrote: > > But it seems it may also affect mingw builds on travis. I would resist > the > > temptation to add -Werror in general and to Windows cross-build in > > particular. > > Yeah, we're not intending to. > But the patch does affect cross-compilation for Windows on linux. Unless restricted to a very small cross-section (say gcc x.y + linux) this is going to cause a lot of grief. Note that every check run by configure that succeeds with warnings will become mysterious error at build time. There may be other unexpected failures too. -Werror is never fit for production in my view. There is a reason why warnings are warnings. Just my 2c. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] travis: compile with -Werror on Linux
Hi,
On Sun, Nov 10, 2019 at 8:36 AM Antonio Quartulli wrote:
> Signed-off-by: Antonio Quartulli
> ---
> .travis/build-check.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/.travis/build-check.sh b/.travis/build-check.sh
> index 039a7dcf..250bb454 100755
> --- a/.travis/build-check.sh
> +++ b/.travis/build-check.sh
> @@ -8,6 +8,7 @@ fi
>
> if [ "${TRAVIS_OS_NAME}" = "linux" ]; then
> export LD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH:-}"
> + export CFLAGS="${CFLAGS} -Werror"
> fi
>
If this is only for linux (and travis), and works with the current code
base, probably its fine.
But it seems it may also affect mingw builds on travis. I would resist the
temptation to add -Werror in general and to Windows cross-build in
particular.
Not all warnings can be fixed easily. Often the only option is to hide the
warning (e.g, by adding a cast) which imo is not always a good strategy.
-Werror just forces one's hand to use tricks that hide warnings.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key
* upgraded to 1.1.1 without recompile (>= 1.1.0)
> */
> if (OpenSSL_version_num() >= 0x1010100fL)
> {
> @@ -1125,24 +1127,46 @@ openvpn_extkey_rsa_finish(RSA *rsa)
> return 1;
> }
>
> -/* Pass the input hash in 'dgst' to management and get the signature back.
> +/*
> + * Convert OpenSSL's constant to the strings used in the management
> + * interface query
> + */
> +const char*
> +get_sig_padding_name(const int padding)
> +{
> +switch(padding)
> +{
> +case RSA_PKCS1_PADDING:
> +return "PKCS1";
> +case RSA_NO_PADDING:
> +return "NOPADDING";
> +default:
> +return "UNKNOWN";
> +}
> +}
> +
> +/*
> + * Pass the input hash in 'dgst' to management and get the signature back.
> * On input siglen contains the capacity of the buffer 'sig'.
> * On return signature is in sig.
> + * pkcs1 controls if pkcs1 padding is required
> * Return value is signature length or -1 on error.
> */
> static int
> get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen,
> - unsigned char *sig, unsigned int siglen)
> + unsigned char *sig, unsigned int siglen,
> + int padding)
> {
> char *in_b64 = NULL;
> char *out_b64 = NULL;
> int len = -1;
>
> -/* convert 'dgst' to base64 */
> -if (management
> -&& openvpn_base64_encode(dgst, dgstlen, _b64) > 0)
> +int bencret = openvpn_base64_encode(dgst, dgstlen, _b64);
> +
> +if (management && bencret > 0)
> {
> -out_b64 = management_query_pk_sig(management, in_b64);
> +out_b64 = management_query_pk_sig(management, in_b64,
> + get_sig_padding_name(padding));
> }
> if (out_b64)
> {
> @@ -1156,18 +1180,19 @@ get_sig_from_man(const unsigned char *dgst,
> unsigned int dgstlen,
>
> /* sign arbitrary data */
> static int
> -rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA
> *rsa, int padding)
> +rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA
> *rsa,
> + int padding)
> {
> unsigned int len = RSA_size(rsa);
> int ret = -1;
>
> -if (padding != RSA_PKCS1_PADDING)
> +if (padding != RSA_PKCS1_PADDING && padding != RSA_NO_PADDING)
> {
> RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT,
> RSA_R_UNKNOWN_PADDING_TYPE);
> return -1;
> }
>
> -ret = get_sig_from_man(from, flen, to, len);
> +ret = get_sig_from_man(from, flen, to, len, padding);
>
> return (ret == len) ? ret : -1;
> }
> @@ -1263,7 +1288,13 @@ ecdsa_sign(int type, const unsigned char *dgst, int
> dgstlen, unsigned char *sig,
> unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r,
> EC_KEY *ec)
> {
> int capacity = ECDSA_size(ec);
> -int len = get_sig_from_man(dgst, dgstlen, sig, capacity);
> +/*
> + * ECDSA does not seem to have proper constants for paddings since
> + * there are only signatures without padding at the moment, reuse
> + * RSA_NO_PADDING for now as it will trigger querying for "NOPADDING"
> in the
> + * management interface
> + */
> +int len = get_sig_from_man(dgst, dgstlen, sig, capacity,
> RSA_NO_PADDING);
>
> if (len > 0)
> {
> --
>
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: VLAN: add basic VLAN tagging support
Hi
On Thu, Nov 7, 2019 at 7:43 AM Lev Stipakov wrote:
> Hi,
>
>
>> I'm a bit unhappy with that one, as it changes behaviour for all
>> non-windows
>> builds (including all the openssl build output even if it succeeds).
>>
>
> The only place it changes behavior is this
>
> install:
>- if [ ! -z "${CHOST}" ]; then unset CC; fi
> - - .travis/build-deps.sh > build-deps.log 2>&1 || (cat build-deps.log &&
> exit 1)
> + - .travis/build-deps.sh
>
> I don't see it as an issue to print output when building dependencies. The
> reason why
> it is done is that travis aborts build if there is no output for more than
> 10 minutes.
>
>
>> Besides this, we need to fix this whole MSVC mess - all other platforms
>> are just done with "add new source file to the Makefile.ac" and done
>> (including mingw builds), and then MSVC is broken again, and this will
>> happen again and again.
>
>
>> Is there no reasonable way to build these project files from Makefile.ac?
>>
>
> I see no reasonable way. Selva, Simon - opinions?
>
Personally, I come from the Unix world, work on Windows only out of
necessity, and either avoid MSVC or leave it to others to figure out as far
as possible. So my opinion may not count for much.
That said, short of moving to a more Windows-friendly build system such as
CMake, I see no good options. But the status quo looks good enough to me --
i.e., just do a patch to fix the project files when you notice a missing
entry.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] [PATCH v2] Insert client connection data into PAM environment
On Fri, Oct 25, 2019 at 7:08 AM wrote:
>
> From: Paolo Cerrito
>
> Without this patch, the PAM environment lacks any information about the
> remote client address.
>
> syslog output for auth and authpriv facilities changes
> from:
>Oct 25 11:52:02 openvpndev openvpn: pam_unix(openvpn:auth): authentication
> failure;
>logname=root uid=0 euid=0 tty= ruser= rhost=
>Oct 25 11:52:33 openvpndev openvpn: pam_unix(openvpn:auth): authentication
> failure;
>logname=root uid=0 euid=0 tty= ruser= rhost= user=
> to:
>Oct 25 10:56:11 openvpndev openvpn: pam_unix(openvpn:auth): authentication
> failure;
>logname=root uid=0 euid=0 tty= ruser= rhost=198.51.100.10
>Oct 25 10:57:02 openvpndev openvpn: pam_unix(openvpn:auth): authentication
> failure;
>logname=root uid=0 euid=0 tty= ruser= rhost=198.51.100.10
> user=
>
> Furthermore, the presence of the remote client address in PAM
> environment, enables usage of pam modules like pam_recent
> [https://github.com/az143/pam_recent].
>
> Signed-off-by: Paolo Cerrito
So, apart from the commit message, what are the changes in v2?
I don't see that my comment about ensuring remote read from env is not NULL
addressed, nor any response to a number of other points raised by David.
> ---
> src/plugins/auth-pam/auth-pam.c | 39 -
> 1 file changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
> index 88b53204..f7b39e36 100644
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -115,6 +115,7 @@ struct user_pass {
> char password[128];
> char common_name[128];
> char response[128];
> +char remote[40];
Textual ipv6 address buffer is usually defined to be at least 46 bytes
including NUL (not 40) to handle all cases.
(cf. INET6_ADDRSTRLEN = 46 in )
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] msvc: OpenSSL 1.1.0 support
On Thu, Oct 17, 2019 at 8:11 AM Lev Stipakov wrote: > > Hi François, > > François Kooman kirjoitti 17.10.2019 klo 13.39: > > > "Version 1.1.0 will be supported until 2019-09-11" [1]. > > > > Is there a plan to update to 1.1.1 for the Windows client? > > Indeed, there is probably no reason to not to switch to newer version. > We'll include 1.1.1 into the next release. Use of 1.1.1 on both client ans server side will default to PSS padding for RSA signature (for TLS 1.2 and 1.3) and break --management-external-key. So hold-off on building Windows release with 1.1.1 unless we can get https://patchwork.openvpn.net/patch/587/ finalized by then. Selva ___ 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 (2nd October 2019)
Hi, On Wed, Oct 2, 2019 at 7:47 AM Samuli Seppänen wrote: > Discussed tap-windows6. Mattock produced a test installer which includes > two PRs: > > <https://github.com/OpenVPN/tap-windows6/pull/84> > <https://github.com/OpenVPN/tap-windows6/pull/86> > > An installer for Windows 10 and Server 2016/2019 is here: > > <https://build.openvpn.net/downloads/temp/tap-windows-9.24.2-I601.exe> > It has only been install-tested so far. Though not a test of the two pending PRs included here, JMRexach on openvpn-gui github reports this fixes another case of "wifi switches off when tun comes up" issue: https://github.com/OpenVPN/openvpn-gui/issues/316 Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment
Hi, On Tue, Oct 1, 2019 at 1:02 PM Antonio Quartulli wrote: > Hi Paolo, > > On 01/10/2019 14:06, Paolo Cerrito wrote: > > From: paolo > > On June 27th another patch with the same subject was sent by you to this > mailing list. Is this new patch any different? > > If so, it should bear a "v2" in the subject and the differences should > be explicitly mentioned to ease the review. > > On top of that I commented your original patch with the following: > > > Why do we need this change? > > What benefit does it give us? > > How can it be used? > > > IMHO it would be nice to add these pieces of information to the commit > > message (right now it feels .. "empty" ) > > But it seems that none of this information has been added to the commit > message (and it is still empty). > Could you please take care of that, so to make the review easier for who > is not deep into those lines of code you have changed? > Aha, I missed the previous thread. Looks like this one is the same patch as the previous one. Paolo: please improve on the commit message, address the comments, and submit a v2 with the message-id of this one in --in-reply-to. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Insert client connection data into PAM environment
Hi,
Its useful to set PAM_RHOSTS which will allow use of
pam_access for access control etc. So feature ACK.
I would like to see a more precise commit message header like:
"Insert remote IP address into PAM environment"
On Tue, Oct 1, 2019 at 8:25 AM Paolo Cerrito wrote:
> From: paolo
>
> ---
> src/plugins/auth-pam/auth-pam.c | 19 ---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/src/plugins/auth-pam/auth-pam.c
> b/src/plugins/auth-pam/auth-pam.c
> index 88b53204..9d8dfb95 100644
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -115,6 +115,7 @@ struct user_pass {
> char password[128];
> char common_name[128];
> char response[128];
> +char remote[128];
>
> const struct name_value_list *name_value_list;
> };
> @@ -517,13 +518,15 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t
> handle, const int type, const cha
> const char *username = get_env("username", envp);
> const char *password = get_env("password", envp);
> const char *common_name = get_env("common_name", envp) ?
> get_env("common_name", envp) : "";
> +const char *remote = get_env("untrusted_ip", envp) ?
> get_env("untrusted_ip", envp) : get_env("untrusted_ip6", envp);
>
Ensure that remote can't be NULL, like:
if (!remote)
{
remote = "";
}
Though get_env() is unlikely to return NULL in this case,
safer not to rely on that and risk a NULL dereferencing later.
> 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, remote) == -1)
> {
> fprintf(stderr, "AUTH-PAM: Error sending auth info to
> background process\n");
> }
> @@ -750,8 +753,16 @@ pam_auth(const char *service, const struct user_pass
> *up)
> status = pam_start(service, name_value_list_provided ? NULL :
> up->username, , );
> if (status == PAM_SUCCESS)
> {
> +/* Set PAM_RHOST environment variable */
> +if (*(up->remote))
> +{
> +status = pam_set_item(pamh, PAM_RHOST, up->remote);
> +}
> /* Call PAM to verify username/password */
> -status = pam_authenticate(pamh, 0);
> +if (status == PAM_SUCCESS)
> +{
> +status = pam_authenticate(pamh, 0);
> +}
> if (status == PAM_SUCCESS)
> {
> status = pam_acct_mgmt(pamh, 0);
> @@ -839,7 +850,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, up.remote, sizeof(up.remote)) ==
> -1)
> {
> fprintf(stderr, "AUTH-PAM: BACKGROUND: read error on
> command channel: code=%d, exiting\n",
> command);
> @@ -853,6 +865,7 @@ pam_server(int fd, const char *service, int verb,
> const struct name_value_list *
> up.username, up.password);
> #else
> fprintf(stderr, "AUTH-PAM: BACKGROUND: USER: %s\n",
> up.username);
> +fprintf(stderr, "AUTH-PAM: BACKGROUND: REMOTE: %s\n",
> up.remote);
>
Just for the record: we have to replace all these xprintf()'s by
plugin_log/plugin_vlog callbacks but that's beyond the scope here.
#endif
> }
>
Looks good otherwise.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v5 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key
Forgot copy this to the list -- sending again On Mon, Sep 23, 2019 at 6:19 AM Arne Schwabe wrote: > > Am 20.09.19 um 22:55 schrieb Selva Nair: > > Hi, > > > > Reviving this thread/patch as now users are running into this padding > > issue (trac 1216 <https://community.openvpn.net/openvpn/ticket/1216>). > > > > IIRC, we more-or-less agreed upon adding an argument (nopadding, pss etc..) > > to >PK_SIGN for new clients and erroring out with old clients that > > cannot sign with PSS padding. > > > > Selva > > > Yeah. > > We did not really to a conclusion if we wanted backwards compatibility > or not. Since it seems that OpenSSL 1.1.1 requires the management-client > to understand the new way of signatures anyway, I would say we require > the management client to be able to understand the signature in any case. > > I think the missing bit of piece for the patch is if we want to error > out early if we detect a config that *might* not work (the nopadding > argument or any other argument to the management-external-key) or if we > do not error at this point and fail then when we actually require PSS > signature. I am more for the first version because otherwise you end up > with configurations that work fine until the server is upgraded to > OpenSSL 1.1.1 and then the client stops working without anything being > change (yes I realise that is already the case at the moment) Well, I can live with that --- at least we'll be able to tell the users to update their client to get the signature request, which is not the case now. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 for 2.4] Handle PSS padding in cryptoapicert
Hi, On Sun, Jul 28, 2019 at 4:34 PM wrote: > > From: Selva Nair > > For PSS padding, CNG requires the digest to be signed > and the digest algorithm in use, which are not accessible > via the rsa_sign and rsa_priv_enc callbacks of OpenSSL. > This patch uses the EVP_KEY interface to hook to > evp_pkey_sign callback if OpenSSL version is > 1.1.0. > > Mapping of OpenSSL hash algorithm types to CNG is moved > to a function for code-reuse. > > To test, both the server and client should be built with > OpenSSL 1.1.1 and use TLS version >= 1.2 > > Tested on Windows 7 client against a Linux server. > > Signed-off-by: Selva Nair > --- > v2: rebased to release/2.4 after siglen -> *siglen change As this is required for cryptoapicert + OpenSSL 1.1.1, nudging for a review and have it included in the next release. We have already merged this into git master, but the patch here is slightly different because of context differences and code refactorings in 2.5. Thanks, Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v5 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key
Hi, Reviving this thread/patch as now users are running into this padding issue (trac 1216 <https://community.openvpn.net/openvpn/ticket/1216>). IIRC, we more-or-less agreed upon adding an argument (nopadding, pss etc..) to >PK_SIGN for new clients and erroring out with old clients that cannot sign with PSS padding. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/7] Visual Studio: upgrade project files to VS2019
Hi, On Fri, Sep 20, 2019 at 1:55 PM Lev Stipakov wrote: > > Hi Steffan, > >> Out of curiosity: does the (signed) driver from wintun.net not work? Of so, >> why? > > > It does. It is just not usable for openvpn yet because: > > 1) Wintun is distributed as msm module, which is supposed to be integrated > into MSI installer. > Our MSI installer doesn't support wintun yet. > > 2) One can install wireguard client, which installs wintun, however it still > won't work > for openvpn out of the box. Wireguard creates tun adapter on demand, and > openvpn > expects it to already exist. So if you try to use openvpn with wintun after > installing wireguard > you'll get error "no tun adapters found" (or something). This is not a > problem, since > we have "tapctl" tool which can create/list/delete tun/tap adapters. It does > not work with > wintun, though, since it has hardcoded "tap0901" hardware id. I plan to make > it customizable > so one could create wintun adapter by running something like "tapctl.exe > create --name openvpn-wintun --hwid wintun". > We could consider creating tun adapter on demand, but this increases > connection time by a few seconds. > The last time I tried, I could use the driver that comes with wireguard by "activatin" an adapter from its GUI but not add a tunnel so that the device remains free. Then the pacthed openvpn could open it. How does one install the driver in the zip file you posted -- using devcon with the hardware id of wintun? Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2 for 2.4] Handle PSS padding in cryptoapicert
From: Selva Nair
For PSS padding, CNG requires the digest to be signed
and the digest algorithm in use, which are not accessible
via the rsa_sign and rsa_priv_enc callbacks of OpenSSL.
This patch uses the EVP_KEY interface to hook to
evp_pkey_sign callback if OpenSSL version is > 1.1.0.
Mapping of OpenSSL hash algorithm types to CNG is moved
to a function for code-reuse.
To test, both the server and client should be built with
OpenSSL 1.1.1 and use TLS version >= 1.2
Tested on Windows 7 client against a Linux server.
Signed-off-by: Selva Nair
---
v2: rebased to release/2.4 after siglen -> *siglen change
Essentially, commits 0cab3475a and ce1c1bee in master
combined and adapted for 2.4.
Required on Windows when built with OpenSSL 1.1.1 and
cryptoapicert is in use against a 1.1.1 peer.
src/openvpn/cryptoapi.c | 377 ++--
1 file changed, 329 insertions(+), 48 deletions(-)
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 35a9ebc..7f2c3c0 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -39,6 +39,7 @@
#ifdef ENABLE_CRYPTOAPI
#include
+#include
#include
#include
#include
@@ -101,6 +102,12 @@ static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
{ 0, NULL }
};
+/* Global EVP_PKEY_METHOD used to override the sign operation */
+static EVP_PKEY_METHOD *pmethod;
+static int (*default_pkey_sign_init) (EVP_PKEY_CTX *ctx);
+static int (*default_pkey_sign) (EVP_PKEY_CTX *ctx, unsigned char *sig,
+ size_t *siglen, const unsigned char *tbs,
size_t tbslen);
+
typedef struct _CAPI_DATA {
const CERT_CONTEXT *cert_context;
HCRYPTPROV_OR_NCRYPT_KEY_HANDLE crypt_prov;
@@ -108,6 +115,80 @@ typedef struct _CAPI_DATA {
BOOL free_crypt_prov;
} CAPI_DATA;
+/**
+ * Translate OpenSSL padding type to CNG padding type
+ * Returns 0 for unknown/unsupported padding.
+ */
+static DWORD
+cng_padding_type(int padding)
+{
+DWORD pad = 0;
+
+switch (padding)
+{
+case RSA_NO_PADDING:
+pad = BCRYPT_PAD_NONE;
+break;
+
+case RSA_PKCS1_PADDING:
+pad = BCRYPT_PAD_PKCS1;
+break;
+
+case RSA_PKCS1_PSS_PADDING:
+pad = BCRYPT_PAD_PSS;
+break;
+
+default:
+msg(M_WARN|M_INFO, "cryptoapicert: unknown OpenSSL padding type
%d.",
+padding);
+}
+
+return pad;
+}
+
+/**
+ * Translate OpenSSL hash OID to CNG algorithm name. Returns
+ * "UNKNOWN" for unsupported algorithms and NULL for MD5+SHA1
+ * mixed hash used in TLS 1.1 and earlier.
+ */
+static const wchar_t *
+cng_hash_algo(int md_type)
+{
+const wchar_t *alg = L"UNKNOWN";
+switch (md_type)
+{
+case NID_md5:
+alg = BCRYPT_MD5_ALGORITHM;
+break;
+
+case NID_sha1:
+alg = BCRYPT_SHA1_ALGORITHM;
+break;
+
+case NID_sha256:
+alg = BCRYPT_SHA256_ALGORITHM;
+break;
+
+case NID_sha384:
+alg = BCRYPT_SHA384_ALGORITHM;
+break;
+
+case NID_sha512:
+alg = BCRYPT_SHA512_ALGORITHM;
+break;
+
+case NID_md5_sha1:
+case 0:
+alg = NULL;
+break;
+
+default:
+msg(M_WARN|M_INFO, "cryptoapicert: Unknown hash type NID=0x%x",
md_type);
+break;
+}
+return alg;
+}
+
static char *
ms_error_text(DWORD ms_err)
{
@@ -217,25 +298,44 @@ rsa_pub_dec(int flen, const unsigned char *from, unsigned
char *to, RSA *rsa, in
* Sign the hash in 'from' using NCryptSignHash(). This requires an NCRYPT
* key handle in cd->crypt_prov. On return the signature is in 'to'. Returns
* the length of the signature or 0 on error.
+ * Only RSA is supported and padding should be BCRYPT_PAD_PKCS1 or
+ * BCRYPT_PAD_PSS.
* If the hash_algo is not NULL, PKCS #1 DigestInfo header gets added
- * to 'from', else it is signed as is.
- * For now we support only RSA and the padding is assumed to be PKCS1 v1.5
+ * to |from|, else it is signed as is. Use NULL for MD5 + SHA1 hash used
+ * in TLS 1.1 and earlier.
+ * In case of PSS padding, |saltlen| should specify the size of salt to use.
+ * If |to| is NULL returns the required buffer size.
*/
static int
priv_enc_CNG(const CAPI_DATA *cd, const wchar_t *hash_algo, const unsigned
char *from,
- int flen, unsigned char *to, int tlen, int padding)
+ int flen, unsigned char *to, int tlen, DWORD padding, DWORD
saltlen)
{
NCRYPT_KEY_HANDLE hkey = cd->crypt_prov;
DWORD len = 0;
ASSERT(cd->key_spec == CERT_NCRYPT_KEY_SPEC);
-msg(D_LOW, "Signing hash using CNG: data size = %d", flen);
-
-BCRYPT_PKCS1_PADDING_INFO padinfo = {hash_algo};
DWORD status;
-status = NCryptSignHash(hkey, padding? : NULL, (BYTE*) from, flen,
-
[Openvpn-devel] [PATCH for 2.4] Correct the return value of cryptoapi RSA signature callbacks
From: Selva Nair Fixes the wrong check on siglen instead of *siglen for signing failures. Bug reported by: lilulo Signed-off-by: Selva Nair --- src/openvpn/cryptoapi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index 720fce09..35a9ebc4 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -393,7 +393,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len, } *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), padding); -return (siglen == 0) ? 0 : 1; +return (*siglen == 0) ? 0 : 1; } /* decrypt */ -- 2.20.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Correct the return value of cryptoapi RSA signature callbacks
From: Selva Nair Fixes the wrong check on siglen instead of *siglen for signing failures. Bug reported by: lilulo Signed-off-by: Selva Nair --- 2.4 will need a separate patch src/openvpn/cryptoapi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index 0c11712e..2f2eee77 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -499,7 +499,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len, *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), cng_padding_type(padding), 0); -return (siglen == 0) ? 0 : 1; +return (*siglen == 0) ? 0 : 1; } /* decrypt */ @@ -973,7 +973,7 @@ pkey_rsa_sign(EVP_PKEY_CTX *ctx, unsigned char *sig, size_t *siglen, *siglen = priv_enc_CNG(cd, alg, tbs, (int)tbslen, sig, *siglen, cng_padding_type(padding), (DWORD)saltlen); -return (siglen == 0) ? 0 : 1; +return (*siglen == 0) ? 0 : 1; } #endif /* OPENSSL_VERSION >= 1.1.0 */ -- 2.20.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6] openvpnserv: enable interactive service to open tun
Hi,
Looks good now and works as expected.
On Tue, Jul 23, 2019 at 5:22 AM Lev Stipakov wrote:
>
> From: Lev Stipakov
>
> This patch enables interactive service to open tun device.
> This is mostly needed by Wintun, which could be opened
> only by privileged process.
>
> When interactive service is used, instead of calling
> CreateFile() directly by openvpn process we pass tun device path
> into service process. There we open device, duplicate handle
> and pass it back to openvpn process.
>
> Signed-off-by: Lev Stipakov
> ---
> v6:
> - simplify and strengthen guid check with wcsspn()
> - fix doxygen comment
>
> v5:
> - further strengthen security by passing only device guid from client
> process
> to service, validating guid and constructing device path on service side
>
> v4:
> - strengthen security by adding basic validation to device path
> - reorder fields in msg_open_tun_device_result for backward compatibility
>
> v3:
> - ensure that device path passed by client is null-terminated
> - support for multiple openvpn processes
> - return proper error code when device handle is invalid
>
> v2:
> - introduce send_msg_iservice_ex() instead of changing
> signature of existing send_msg_iservice()
> - use wchar_t strings in interactive service code
>
> include/openvpn-msg.h | 12 +++
> src/openvpn/tun.c | 83
> ++-
> src/openvpn/win32.c | 9 -
> src/openvpn/win32.h | 30 +---
> src/openvpnserv/interactive.c | 78 ++--
> 5 files changed, 188 insertions(+), 24 deletions(-)
...
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 623c3ff..1b4a5e2 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -58,7 +58,6 @@ static settings_t settings;
> static HANDLE rdns_semaphore = NULL;
> #define RDNS_TIMEOUT 600 /* seconds to wait for the semaphore */
>
> -
> openvpn_service_t interactive_service = {
> interactive,
> TEXT(PACKAGE_NAME "ServiceInteractive"),
> @@ -1198,8 +1197,62 @@ HandleEnableDHCPMessage(const enable_dhcp_message_t
> *dhcp)
> return err;
> }
>
> +static DWORD
> +HandleOpenTunDeviceMessage(const open_tun_device_message_t *open_tun, HANDLE
> ovpn_proc, HANDLE *remote_handle)
> +{
> +DWORD err = ERROR_SUCCESS;
> +HANDLE local_handle;
> +LPWSTR wguid = NULL;
> +WCHAR device_path[256] = {0};
> +const WCHAR prefix[] = L".\\Global\\";
> +const WCHAR tap_suffix[] = L".tap";
> +
> +*remote_handle = INVALID_HANDLE_VALUE;
> +
> +wguid = utf8to16(open_tun->device_guid);
> +if (!wguid)
> +{
> +err = ERROR_OUTOFMEMORY;
> +goto out;
> +}
> +
> +/* validate device guid */
> +const size_t guid_len = wcslen(wguid);
> +if (guid_len != 38 || wcsspn(wguid, L"0123456789ABCDEFabcdef-{}") !=
> guid_len)
> + {
> + err = ERROR_MESSAGE_DATA;
> +MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Invalid device guild (%s)"),
> wguid);
guild -> guid (as pointed out by tincanteksup)
> +goto out;
> +}
> +
whitespace in line above.
These could be fixed at commit time.
Acked-by: Selva Nair
Selva
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v5] openvpnserv: enable interactive service to open tun
Hi,
On Thu, Jul 18, 2019 at 7:42 AM Lev Stipakov wrote:
> From: Lev Stipakov
>
> This patch enables interactive service to open tun device.
> This is mostly needed by Wintun, which could be opened
> only by privileged process.
>
> When interactive service is used, instead of calling
> CreateFile() directly by openvpn process we pass tun device path
> into service process. There we open device, duplicate handle
> and pass it back to openvpn process.
>
> Signed-off-by: Lev Stipakov
> ---
> v5:
> - futher strengthen security by passing only device guid from client
> process
>
"further"
> to service, validating guid and contructing device path on service side
>
"constructing"
>
> v4:
> - strengthen security by adding basic validation to device path
> - reorder fields in msg_open_tun_device_result for backward compatibility
>
> v3:
> - ensure that device path passed by client is null-terminated
> - support for multiple openvpn processes
> - return proper error code when device handle is invalid
>
> v2:
> - introduce send_msg_iservice_ex() instead of changing
> signature of existing send_msg_iservice()
> - use wchar_t strings in interactive service code
>
> include/openvpn-msg.h | 12 +++
> src/openvpn/tun.c | 83
> ++-
> src/openvpn/win32.c | 9 -
> src/openvpn/win32.h | 30 +---
> src/openvpnserv/interactive.c | 83
> +--
> 5 files changed, 193 insertions(+), 24 deletions(-)
diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
> index 4814bbc..8ccddc0 100644
> --- a/src/openvpn/win32.h
> +++ b/src/openvpn/win32.h
> @@ -309,14 +309,36 @@ int win32_version_info(void);
> */
> const char *win32_version_string(struct gc_arena *gc, bool add_name);
>
> -/*
> - * Send the |size| bytes in buffer |data| to the interactive service
> |pipe|
> - * and read the result in |ack|. Returns false on communication error.
> - * The string in |context| is used to prefix error messages.
> +
> +/**
> + * Send data to interactive service and receive response of type
> ack_message_t.
> + *
> + * @param pipe The handle of communication pipe
> + * @param data The data to send
> + * @param size The size of data to send
> + * @param response The pointer to ack_message_t structure to where
> response is written
>
That would be "@param ack"
> + * @param context The string used to prefix error messages
> + *
> + * @returns True on success, false on failure.
> */
> bool send_msg_iservice(HANDLE pipe, const void *data, size_t size,
> ack_message_t *ack, const char *context);
>
>
>
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 623c3ff..782c04d 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -58,7 +58,6 @@ static settings_t settings;
> static HANDLE rdns_semaphore = NULL;
> #define RDNS_TIMEOUT 600 /* seconds to wait for the semaphore */
>
> -
> openvpn_service_t interactive_service = {
> interactive,
> TEXT(PACKAGE_NAME "ServiceInteractive"),
> @@ -1198,8 +1197,67 @@ HandleEnableDHCPMessage(const enable_dhcp_message_t
> *dhcp)
> return err;
> }
>
> +static DWORD
> +HandleOpenTunDeviceMessage(const open_tun_device_message_t *open_tun,
> HANDLE ovpn_proc, HANDLE *remote_handle)
> +{
> +DWORD err = ERROR_SUCCESS;
> +HANDLE local_handle;
> +LPWSTR wguid = NULL;
> +WCHAR device_path[256] = {0};
> +const WCHAR prefix[] = L".\\Global\\";
> +const WCHAR tap_suffix[] = L".tap";
> +
> +*remote_handle = INVALID_HANDLE_VALUE;
> +
> +wguid = utf8to16(open_tun->device_guid);
> +if (!wguid)
> +{
> +err = ERROR_OUTOFMEMORY;
> +goto out;
> +}
> +
> +/* validate device guid */
> +const size_t guid_len = wcslen(wguid);
> +for (int i = 0; i < guid_len; ++i)
> +{
> +const WCHAR ch = wguid[i];
> +
> +if (!(iswalnum(ch)) && (ch != L'-') && (ch != L'{') && (ch !=
> L'}'))
> +{
> +err = ERROR_MESSAGE_DATA;
> +MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Invalid device guild
> (%s)"), wguid);
> +goto out;
> + }
> +}
>
I think the check could be made tighter than alpha numerics (only hex
digits need be allowed) and simpler like:
if (guid_len != 38 || wcspn(wguid, L"0123456789ABCDEFabcdef-{}") !=
guid_len)
{
}
I would have preferred to do a GUID parsing to validate it, but may be this
is good enough?
+
> +openvpn_swprintf(device_path, sizeof(device_path), L"%s%s%s", prefix,
> wguid, tap_suffix);
> +
> +/* open tun device */
> +
>
...
Thanks,
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] openvpnserv: enable interactive service to open tun
Hi On Wed, Jul 17, 2019 at 8:20 AM Lev Stipakov wrote: > Hi, > > Sorry for delay - I was on vacation. > > (i) The new message is named message_open_tun, but it allows opening >> any file using the service. This is not secure. > > > I am thinking of possible vector of attack here. > > In our case it is service which launches openvpn process using > path set in registry, opens pipe and passes pipe handle to launched > process. > > To make service run malicious process one needs either to replace > executable or > modify registry. For both actions you need to be administrator, assuming > default security policy. > > Alternatively, can random process hijack the pipe handle from another > process? > Because the pipe handle is targeted for OpenVPN process id and not inheritable that may not be possible. But an attack is very simple: Just write a plugin and use the service to open files from it. Passing the pipe handle to the plugin is trivial. Anyone in OpenVPNAdministrators group can then use a custom config file and get access any file on the system. Membership in that group is not supposed to provide any privileges to limited users beyond setting up routes or manipulating some interface parameters. > >> We need to restrict it to open tun/tap device nodes only. >> > > Without adding too much code, I can think of: > > - check that path starts with .\\Global\\ to make sure we open > device, not file > > and > > - check that path starts with .\\Global\\WINTUN or ends with .tap > > Is this good enough or do you have better ideas? > That'll probably work with some extra sanity checks on the file name. Ideally we should just pass the dev-node (empty if unspecified) and type of device (TAP6 or WINTUN), but that will require a lot of duplication of code in the service, as you noted. One option is to pass the device guid in case of tap or the index in case of wintun and construct the path in the service. That requires very little extra code. Otherwise a thorough sanitization of the path is required as there could be obscure ways of breaking out using "..\" or otherwise, though I'm not sure. Things like \\.\C:\..\D:\ works on Windows so I won't take any chances. Selva PS. Just noticed you've already posted a v4 -- I haven't looked at it yet. ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] openvpnserv: enable interactive service to open tun
Hi,
On Thu, Jun 27, 2019 at 8:08 AM Lev Stipakov wrote:
>
> From: Lev Stipakov
>
> This patch enables interactive service to open tun device.
> This is mostly needed by Wintun, which could be opened
> only by privileged process.
>
> When interactive service is used, instead of calling
> CreateFile() directly by openvpn process we pass tun device path
> into service process. There we open device, duplicate handle
> and pass it back to openvpn process.
>
> Signed-off-by: Lev Stipakov
> ---
> v3:
> - ensure that device path passed by client is null-terminated
> - support for multiple openvpn processes
> - return proper error code when device handle is invalid
This works but there are two general concerns:
(i) The new message is named message_open_tun, but it allows opening
any file using the service. This is not secure. We need to restrict it
to open tun/tap device nodes only.
(ii) Should we allow all users to open tap6 adapters irrespective of
any other access restrictions that may be present? I'm conflicted
about this as, on closer look, access control in tap-windows6 appears
broken.
> @@ -117,4 +119,14 @@ typedef struct {
> interface_t iface;
> } enable_dhcp_message_t;
> +typedef struct {
> +message_header_t header;
> +char device_path[256];
> +} open_tun_device_message_t;
> +
> +typedef struct {
> +message_header_t header;
> +HANDLE handle;
> +int error_number;
> +} open_tun_device_result_message_t;
Defining this struct with error_number followed by handle would be
better (makes its head match in memory with ack_message_t). That makes
it possible to read a normal ack into it and resolve the error number.
Can happen if openvpn.exe is upgraded but service stays at an old
version -- such a service will respond with ack and
error_number=ERROR_MESSAGE_TYPE.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Using AllowNonAdmin in the advanced options of tap adapter
Hi, On Sat, Jun 29, 2019 at 10:35 AM Lev Stipakov wrote: > > Hi, > >> So Lev's patch proposes to do service calls for wintun *and* tap6, and >> do so "always"? Or only if the registry key says so? > > > Yep, patch always opens wintun and tap6 via service no matter what > registry key says. Yes, that's one of my concerns with the patch. Opening tap6 using the service may be a regression in some installations where limited users are not allowed to open the device. It also provides more access rights to the handle as its opened by SYSTEM: the driver sets it as all access for SYSTEM, RWX for everyone else when AllowNonAdmin is on. Not sure whether that matters in practice. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Using AllowNonAdmin in the advanced options of tap adapter
Hi,
On Sat, Jun 29, 2019 at 10:50 AM Lev Stipakov wrote:
>
> That --allow-nonadmin functionality was included into commit 3c7f2f553 from
> yeah 2005. Here is code from tap-win32/tapdrv.c from the same commit:
>
> +#if ENABLE_NONADMIN
> + /* Read AllowNonAdmin setting from registry */
> + {
> + NDIS_STRING key = NDIS_STRING_CONST("AllowNonAdmin");
> + NdisReadConfiguration (, , configHandle,
> +, NdisParameterInteger);
> + if (status == NDIS_STATUS_SUCCESS)
> + {
> + if (parm->ParameterType == NdisParameterInteger)
> + {
> + if (parm->ParameterData.IntegerData)
> + {
> + enable_non_admin = TRUE;
> + }
> + }
> + }
> + }
> +#endif
>
> As you see, the current version posted by Selva is missing
>
> if (parm->ParameterData.IntegerData)
>
> part.
>
> So it seems for me that this feature has been degraded somewhere between 2005
> and 2019.
If the idea was to remove the feature, the whole code between those
#if-#endif could have been removed. The way it stands looks like a bug.
Anyway, our installer always sets the AllowNonAdmin key as true, so we will
not break any setups by "fixing" this.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Using AllowNonAdmin in the advanced options of tap adapter
Hi, On Fri, Jun 28, 2019 at 5:03 PM Gert Doering wrote: > > Hi, > > On Fri, Jun 28, 2019 at 04:51:47PM -0400, Selva Nair wrote: > > Would that mean we can assume that always allowing all users access to > > the tap (say, using the service to open it) would not be a regression? > > Or are there folks who use local builds of the driver and expect > > openvpn.exe to respect that setting? > > The whole permission model for openvpn on Windows has been funky at > best "forever" - open tap device and setup IPv4 address by means of > DHCP could be done "by everyone", while installing routes or setting > up IPv6 then failed for non-Admin accounts... > > I'm not exactly sure what you are proposing, but aligning this with > wintun and using the service to get access to tap6-windows sounds > like a reasonable plan. That's what lev's patch would do, but it wont respect the registry key which is "supposed" to toggle non-admin access to tap-windows6 adapters on and off. > With a registry key to re-enable "full > access as today", for compat reasons. For some reason that registry key appears to be doing nothing in current versions of tap-windows6. I know this is confusing... Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Using AllowNonAdmin in the advanced options of tap adapter
On Fri, Jun 28, 2019 at 4:51 PM Selva Nair wrote: > > Hi, > > While testing a patch, I failed to toggle AllowNonAdmin access to the > tap-adapter. > Looking at the sources it seems we do not respect that setting. > > From adapter.c ~line The quoted code was from tap-windows6/src/adapter.c ~line 428 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] Using AllowNonAdmin in the advanced options of tap adapter
Hi,
While testing a patch, I failed to toggle AllowNonAdmin access to the
tap-adapter.
Looking at the sources it seems we do not respect that setting.
>From adapter.c ~line
#if ENABLE_NONADMIN
NdisReadConfiguration (
,
,
configHandle,
,
NdisParameterInteger
);
if (localStatus == NDIS_STATUS_SUCCESS)
{
if (configParameter->ParameterType == NdisParameterInteger)
{
Adapter->AllowNonAdmin = TRUE;
}
}
#endif
The code does not appear to check the actual value read from registry
which will be in configParameter->ParameterData.IntegerData, but
enables AllowNonAdmin in any case.
Would that mean we can assume that always allowing all users access to
the tap (say, using the service to open it) would not be a regression?
Or are there folks who use local builds of the driver and expect
openvpn.exe to respect that setting?
Thanks,
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] openvpnserv: enable interactive service to open tun
0, 0,
> + OPEN_EXISTING, FILE_ATTRIBUTE_SYSTEM |
> FILE_FLAG_OVERLAPPED, 0);
As wintun allows the device to be opened multiple times, we'll
eventually need some protection against that. Saying this as the real
purpose of this patch is to enable the use of wintun. But this is fine
for tapwindows.
> +
> +if (local_handle == INVALID_HANDLE_VALUE)
> +{
Need err = GetLastError(); here. Otherwise we're returning success as
the error code with an invalid handle.
> +MsgToEventLog(M_SYSERR, TEXT("Error opening tun device (%s)"),
> device_path);
> +goto out;
> +}
> +
> +if (!DuplicateHandle(GetCurrentProcess(), local_handle, ovpn_process,
> remote_handle, 0, FALSE,
> +DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS))
> +{
> +err = GetLastError();
> +MsgToEventLog(M_SYSERR, TEXT("Error duplicating tun device handle"));
> +}
> +
> +out:
> +free(device_path);
> +
> +return err;
> +}
> +
> static VOID
> HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events,
> undo_lists_t *lists)
> {
> @@ -1210,6 +1246,7 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD count,
> LPHANDLE events, undo_lists
> block_dns_message_t block_dns;
> dns_cfg_message_t dns;
> enable_dhcp_message_t dhcp;
> +open_tun_device_message_t open_tun;
> } msg;
> ack_message_t ack = {
> .header = {
> @@ -1277,6 +1314,22 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD count,
> LPHANDLE events, undo_lists
> }
> break;
>
> +case msg_open_tun_device:
> +if (msg.header.size == sizeof(msg.open_tun))
> +{
> +open_tun_device_result_message_t res = {
> +.header = {
> +.type = msg_open_tun_device_result,
> +.size = sizeof(res),
> +.message_id = msg.header.message_id
> +}
> +};
> +res.error_number = HandleOpenTunDeviceMessage(_tun,
> );
> +WritePipeAsync(pipe, , sizeof(res), count, events);
> +return;
> +}
> +break;
> +
> default:
> ack.error_number = ERROR_MESSAGE_TYPE;
> MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Unknown message type %d"),
> msg.header.type);
> @@ -1603,6 +1656,8 @@ RunOpenvpn(LPVOID p)
> free(input);
> }
>
> +ovpn_process = proc_info.hProcess;
See above.
Selva
Selva
> +
> while (TRUE)
> {
> DWORD bytes = PeekNamedPipeAsync(ovpn_pipe, 1, _event);
> --
> 2.7.4
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] openvpnserv: enable interactive service to open tun
Hi,
What I have in mind would also require editing all calls
to send_msg_iservice() which is essentially what Gert is objecting to.
So ignore me -- a separate send_msg_iservice_ex may be the
best option.
Selva
On Tue, Jun 25, 2019 at 5:00 PM Selva Nair wrote:
>
> Hi,
>
> On Tue, Jun 25, 2019 at 4:38 PM Lev Stipakov wrote:
> >
> > Hi,
> >
> >>
> >> The way interactive service structures are coded should not require
> >> this at all, does it? The size and message type are already in the
> >> header, so why do we need to pass it?
> >
> >
> > But we need to know the response size in send_msg_iservice() since
> > we pass it to ReadFile(). So far we assumed that response is always
> > ask_message_t
> > and we could do sizeof(*ack). With new response type this assumption
> > doesn't hold so
> > as Gert suggested I added another version which accepts arbitrary response
> > type and size.
>
> My point is that, this is not in the spirit of the rest of iservice code. See
> HandleMessage in interactive.c where the data is and then interpreted
> using the header type and size.
>
> For what max-size to pass to ReadFile, we know it from the size in the header
> element of the struct.
>
> Selva
>
>
> >
> > bool
> > send_msg_iservice(HANDLE pipe, const void *data, size_t size,
> > ack_message_t *ack, const char *context)
> > {
> > return send_msg_iservice_ex(pipe, data, size, ack, sizeof(*ack),
> > context);
> > }
> >
> > bool
> > send_msg_iservice_ex(HANDLE pipe, const void *data, size_t size,
> > void *response, size_t response_size, const char
> > *context)
> > {
> >
> > Will send V2 tomorrow with this and CreateFileW changes.
> >
> >
> > --
> > -Lev
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] openvpnserv: enable interactive service to open tun
Hi,
On Tue, Jun 25, 2019 at 4:38 PM Lev Stipakov wrote:
>
> Hi,
>
>>
>> The way interactive service structures are coded should not require
>> this at all, does it? The size and message type are already in the
>> header, so why do we need to pass it?
>
>
> But we need to know the response size in send_msg_iservice() since
> we pass it to ReadFile(). So far we assumed that response is always
> ask_message_t
> and we could do sizeof(*ack). With new response type this assumption doesn't
> hold so
> as Gert suggested I added another version which accepts arbitrary response
> type and size.
My point is that, this is not in the spirit of the rest of iservice code. See
HandleMessage in interactive.c where the data is and then interpreted
using the header type and size.
For what max-size to pass to ReadFile, we know it from the size in the header
element of the struct.
Selva
>
> bool
> send_msg_iservice(HANDLE pipe, const void *data, size_t size,
> ack_message_t *ack, const char *context)
> {
> return send_msg_iservice_ex(pipe, data, size, ack, sizeof(*ack), context);
> }
>
> bool
> send_msg_iservice_ex(HANDLE pipe, const void *data, size_t size,
> void *response, size_t response_size, const char
> *context)
> {
>
> Will send V2 tomorrow with this and CreateFileW changes.
>
>
> --
> -Lev
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] openvpnserv: enable interactive service to open tun
Hi On Tue, Jun 25, 2019 at 4:34 PM Gert Doering wrote: > > Hi, > > On Tue, Jun 25, 2019 at 03:57:18PM -0400, Selva Nair wrote: > > The way interactive service structures are coded should not require > > this at all, does it? The size and message type are already in the > > header, so why do we need to pass it? The result here is a new kind of > > ack message with a different size and type and that could be checked > > by accessing the header element. Unless I'm missing something. > > You could, indeed, do this inside send_msg_iservice() by looking > at "what request did we sent? so what should be coming back?" but > I'm not sure I find this safe enough (caller allocates memory, and > maybe we shouldn't rely on "it being big enough" unless explicitly > told). It would work, yes, but leaves me with a bit uneasy feeling. I was thinking of dereferening the response pointer as a union and check the header size which the caller is supposed to have set. So change ack * to void * in send_msg_iservice() as in the patch, and treat it as ack or and extended ack depending on the specified size. Further, when reading from the pipe one should also check the size of data received matches what is expected. It may be also useful to make all reply messages made up of an ack message plus optional additional data -- ie., all start with a header and an error code. Selva > > gert > -- > "If was one thing all people took for granted, was conviction that if you > feed honest figures into a computer, honest figures come out. Never doubted > it myself till I met a computer with a sense of humor." > Robert A. Heinlein, The Moon is a Harsh Mistress > > Gert Doering - Munich, Germany g...@greenie.muc.de ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] openvpnserv: enable interactive service to open tun
Hi
On Tue, Jun 25, 2019 at 3:49 PM Gert Doering wrote:
>
> Hi,
>
> On Tue, Jun 25, 2019 at 10:34:01PM +0300, Lev Stipakov wrote:
> > ack_message_t ack;
> > struct gc_arena gc = gc_new();
> >
> > -if (!send_msg_iservice(pipe, rt, size, , "ROUTE"))
> > +if (!send_msg_iservice(pipe, rt, size, , sizeof(ack), "ROUTE"))
>
> I do not like this. Please find another way to send the request message
> "with length" than to add an extra parameter to every single caller of
> send_msg_iservice().
Gert beat me to it :) Anyway, me too!
The way interactive service structures are coded should not require
this at all, does it? The size and message type are already in the
header, so why do we need to pass it? The result here is a new kind of
ack message with a different size and type and that could be checked
by accessing the header element. Unless I'm missing something.
>
> +HANDLE local_handle = CreateFileA(open_tun->device_path, GENERIC_READ |
> GENERIC_WRITE, 0, 0,
> + OPEN_EXISTING, FILE_ATTRIBUTE_SYSTEM |
> FILE_FLAG_OVERLAPPED, 0);
> +
> +if (local_handle == INVALID_HANDLE_VALUE)
> +{
> +WCHAR *device_path_wchar = NULL;
> +int size = sizeof(open_tun->device_path);
> +err = GetLastError();
> +
> +device_path_wchar = malloc(size * sizeof(WCHAR));
> +if (device_path_wchar)
> +{
> +MultiByteToWideChar(CP_UTF8, 0, open_tun->device_path, size,
> device_path_wchar, size);
> +device_path_wchar[size - 1] = 0;
> +}
> +MsgToEventLog(M_SYSERR, TEXT("Error opening tun device (%s)"),
> device_path_wchar);
> +free(device_path_wchar);
> +return err;
> +}
>
Also this one -- I think we should just use the wide version of
CreateFile -- all strings in OpenVPN.exe are supposed to be in utf8,
so convert to widechar and call CreateFileW.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 0/5] Implement additional two step authentication methods
Hi On Thu, Jun 13, 2019 at 10:42 AM Arne Schwabe wrote: > > These patches mainly implement forwarding passing/forwarding extra > messages between management interface on server and client side. > > These new extra messages can be used to implement a two step > authentication like TOTP (Google Authenticator) or web based > out of band (like SAML). > > Since this requires a tight integration on both client and > server side, it is currently only supported with the management > interface. > > Arne Schwabe (5): > Implement parsing and sending INFO and INFO_PRE control messages > Implement forwarding client CR_RESPONSE messages to management > Implement support for signalling IV_SSO to server > Implement sending response to challenge via CR_RESPONSE > Implement sending SSO challenge to clients I haven't looked at the patches, but a quick question. I haven't come across any 2FA mechanisms that cannot be handled (in principle) by the current static an dynamic CR in OpenVPN. Except that some dynamic CR (e.g, U2F) will require the possibility to transmit larger messages than currently possible -- especially the 256 byte limitation in responses from the management as those are parsed by the config-parser. And possibly the TLS channel buffer size may need to be increased. Once those limits are extended, do we need anything more? You mention google-authenticator OTP but that can perfectly handled as a static challenge as many do right now. The current dynamic response implementation is a bad hack -- fail the auth with challenge embedded in the reason text and then send the response as a "password" during the next round. So is this about making a cleaner implementation? Or am I missing something more subtle? Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Preliminary Wintun support in OpenVPN2
Hi, Copying devel list as there is good info in here.. On Tue, Jun 11, 2019 at 4:24 PM Simon Rozman wrote: > > Hi, > > > > I've taken Jason out of the loop – he's hostile and competitive, and I do not > approve his attitude towards OpenVPN and other nice people trying to use our > technology. Once you get to know him, he is a nice guy under immense time > pressure. > > > > Allowing any unprivileged process to access Wintun data pipe would be a > security vulnerability indeed. Anyone can derive data pipe name from LUID and > then write/read IP packets to Windows networking stack. However, mind that > the SDDL string set via NDIS_DEVICE_OBJECT_ATTRIBUTES is called default SDDL. > I believe, I have read somewhere that there is a way you can set your own > SDDL via registry. Never researched it myself, but there might be a way you > can use stock Wintun binary with some registry tweaks to allow desired ACL > for the data pipe. A quick comment: IMO, we should use the interactive service to open the pipe and pass the handle to OpenVPN.exe. This avoids allowing arbitrary users to access the pipe or requiring users to start OpenVPN as admin which is not a safe practice. As the service only starts a well-defined executable present at a location read from HKLM\... and can pass a duplicated handle targeted for the process, such an approach looks safe to me. And, it should be fairly easy to implement. I'll try to find some time to test OpenVPN + wintun. Thanks for the patch. > Maximum 256 packets limitation no longer applies. This was a leftover from > the time we had 256 preallocated NBLs we copied packets into on write. As we > no longer copy data on write, we no longer have those 256 preallocated NBLs. > The most recent exchange buffer limitations are: > > minimum 16+0xeff0=0xf000 bytes when reading > minimum 16+20 bytes when writing (one Wintun packet header with an empty IPv4 > packet) > maximum 0xf0 bytes when reading or writing > all packets must be <=16+0xeff0=0xf000 bytes (16 bytes for Wintun packet > header with maximum 0xeff0 IPv4/v6 packet). > > Thus, you can pack any number of packets into an exchange buffer, just use > something in the 0xf000-0xf0 range for the exchange buffer size. And each > packet must contain <=0xeff0 sized IP packet. With Wintun 0.2 we are > rejecting entire exchange buffer in case any packet is violating any of above > rules. Or, if it is not IPv4 nor IPv6 etc. See TunGetIrpBuffer() and > TunWriteFromIrp() with majority of exchange buffer checks should you receive > ERROR_INVALID_USER_BUFFER. > > You mean I/O completion ports? Never used or studied it. I understand it > should be done on the userspace side. Mind that you must retain the order of > TCP packets. Should you have more parallel reads/writes racing with each > other, the packets might get reordered. One approach we still have to > implement and test with WireGuard client is to use overlapped I/O and double > buffering. > > > > In case you have any further questions, do not hesitate to ask. I hope we > meet on OpenVPN hackathon in Trento. I have reserved the date on my calendar > and waiting for Antonio to send the accommodation recommendation. > > > > Best regards, > > Simon > > > > From: Lev Stipakov > Date: Tuesday, 11 June 2019 at 19:26 > To: Jason Donenfeld , Simon Rozman , Selva > Nair > Subject: Preliminary Wintun support in OpenVPN2 > > > > Hi guys, > > > > I have implemented preliminary Wintun support in openvpn2: > > > > https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18490.html > > > > It would be great if you could provide some feedback on it. Latest binaries > could be found here: > https://ci.appveyor.com/api/buildjobs/w20knnbut89uoeuw/artifacts/msvc%2Fimage%2Fbin.zip > > > > I would like to ask a few questions about Wintun to Jason / Simon: > > > > 1) At the moment Wintun device can be opened by either privileged or local > system account. This breaks "run as non admin" scenario for OpenVPN community > client - while we do have a component running under system account > (interactive service), it is used to set up IP address, routing etc and not > for opening tun device, which is performed by main openvpn process, which > could well be run under non-privileged account. Would you consider allowing > non-privileged processes open tun device, as it is done by tap-windows6? > Alternatively we were thinking to use the same approach we use in openvpn3 > linux client - to open tun under privileged process and pass handle back to > unprivileged one. Have to look if it is doable on Windows. > > > > 2)
Re: [Openvpn-devel] [PATCH 2/2] Allow repeated cycles through remotes when management-query-remote is in use
Ref: https://patchwork.openvpn.net/project/openvpn2/list/?series=201 Hi, 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. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] New OpenVPN 2.4.7 Windows installers released
Hi, On Wed, Apr 24, 2019 at 6:50 AM Samuli Seppänen wrote: > > Hi, > > New OpenVPN Windows installers have been released. The release > highlights are: > > - Latest openvpn-gui > - Latest openvpnserv2 (OpenVPNService) > - Latest tap-windows6 driver > - ARM64 support > - NDIS 6.30 support > - other enhancements > - fix to local privilege exploit vulnerability > > The installers come in two flavors. Windows 7/8/8.1/Server 2012r2: > > <https://swupdate.openvpn.org/community/releases/openvpn-install-2.4.7-I606-Win7.exe> > > Windows 10 (any version): > > <https://swupdate.openvpn.org/community/releases/openvpn-install-2.4.7-I606-Win10.exe> > > We're unable to release a version for Windows Server 2016 at this point, > so you need to use the old installer: > > <https://swupdate.openvpn.org/community/releases/openvpn-install-2.4.7-I603.exe> > > We're working on getting tap-windows6 pass the HLK test suite on Windows > Server 2016. This will allow us to get a signature from Microsoft and > release an updated tap-windows6 on that platform as well. While waiting > please avoid running OpenVPN on nodes where all users are not trusted. > > For further details see the download page: > > <https://openvpn.net/community-downloads/> I think we were supposed to continue using openvpnserv2 version 1.3.0 until the MSI installer is ready. Version 1.4.0 included here requires some logic in the installer to create the new config_dir location if missing, and move contents from the old location. See also this PR: https://github.com/OpenVPN/openvpn-build/pull/141/commits/9c2774ca3841763ada64986b18d1df7634c59a20 Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] New OpenVPN 2.4.7 installers with tap-windows6 and other componets
Hi, On Sat, Apr 20, 2019 at 5:17 AM Samuli Seppänen wrote: > > Hi all, > > Here are completely untested OpenVPN 2.4.7 installers which I wanted to > get out for testing a.s.a.p.: > > <https://build.openvpn.net/downloads/releases/openvpn-install-2.4.7-I604-Win7.exe> > <https://build.openvpn.net/downloads/releases/openvpn-install-2.4.7-I604-Win10.exe> > > As the name implies, there is a different version for Windows 7 (plus > 8/8.1/Server 2012r2) and for Windows 10 (all versions). This is > necessary due to code signatures in the tap-windows6 driver. > > The above installers bundle the following components: > > - tap-windows6 9.23.2 (many improvements incl. arm64 support) > - openvpn-gui 11 (latest from Git master) > - openvpnserv2 1.4.0.0 (latest from Git master) > > If you want to just test the tap-windows6 installers they're here: > > <https://build.openvpn.net/downloads/releases/tap-windows-9.23.2-I601-Win7.exe> > <https://build.openvpn.net/downloads/releases/tap-windows-9.23.2-I601-Win10.exe> > > The latter installer _might_ work on Windows 10-based ARM64 devices, but > that has not been tested at all. > > I will run my pre-release tests as soon as I can, but probably not > today. Any help in testing these would be most welcome! Sorry for the delayed response: this installer doesn't work for me on Windows 10. Installation of TAP driver fails with a cryptic message "An error occurred" and nothing in setupapi logs. On further tests, one issue is that tapinstall.exe now depends on VCRUNTIME140.dll which is not present in stock Windows. Probably that's what causes the failure. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Issue with smartcard authentication for openvpn
Hi, On Fri, Apr 19, 2019 at 3:15 PM Gert Doering wrote: > Hi, > > On Fri, Apr 19, 2019 at 03:12:49PM +0200, Jan Just Keijser wrote: > > Can you do a pull request for your pkcs11-helper patch on the > > pkcs11helper github page? or shall we simply patch pkcs11-helper > ourselves? > > I agree with Selva that it should go upstream - since this is not about > windows, we do not provide our own pkcs11-helper builds anywhere, so > either we get the distribution maintainers involved (lots...) or > upstream. > > (On *windows* we could just patch whatever library we use for our builds) > > I've submitted a PR upstream: https://github.com/OpenSC/pkcs11-helper/pull/22 Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Issue with smartcard authentication for openvpn
Hi, On Fri, Apr 19, 2019 at 9:13 AM Jan Just Keijser wrote: > Hi Selva,I had not written a patch when I wrote my earlier email, but your > patch is exactly what I had in mind; getting it all to compile and run with > OpenSSL 1.1.1b + OpenVPN 2.4.7 was a bit of a challenge, but I finally > managed... > > and yes, your patch works admirably - I can connect again using TLSv1.3 + > token. If I comment out your patch, do a rebuild of libpkcs11helper.so > then it fails again, proving your patch works. > Good to know. > > Can you do a pull request for your pkcs11-helper patch on the pkcs11helper > github page? or shall we simply patch pkcs11-helper ourselves? > Getting this upstream is better for us, so let's first try a PR. Regards, Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Set the correct mtu on windows based systems
,6 +1441,14 @@ do_ifconfig_ipv4(struct tuntap *tt, const char
> *ifname, int tun_mtu,
>
> break;
> }
> +if (tt->options.msg_channel)
> +{
> +do_set_mtu_service(tt, AF_INET, tun_mtu);
> +}
> +else
> +{
> +windows_set_mtu(tt->adapter_index, AF_INET, tun_mtu);
> +}
> }
>
> #else /* if defined(TARGET_LINUX) */
> @@ -5236,6 +5291,40 @@ out:
> return ret;
> }
>
> +static void
> +windows_set_mtu(const int iface_index, const short family,
> +const int mtu)
> +{
> +DWORD err = 0;
> +struct gc_arena gc = gc_new();
> +MIB_IPINTERFACE_ROW ipiface;
> +InitializeIpInterfaceEntry();
> +const char *family_name = (family == AF_INET6) ? "IPv6" : "IPv4";
> +ipiface.Family = family;
> +ipiface.InterfaceIndex = iface_index;
> +err = GetIpInterfaceEntry();
> +if (err == NO_ERROR)
> +{
> +if (family == AF_INET)
> +{
> +ipiface.SitePrefixLength = 0;
> +}
> +ipiface.NlMtu = mtu;
> +err = SetIpInterfaceEntry();
> +}
> +
> +if (err != NO_ERROR)
> +{
> +msg(M_WARN, "TUN: Setting %s mtu failed: %s [status=%u
> if_index=%d]",
> +family_name, strerror_win32(err, ), err, iface_index);
> +}
> +else
> +{
> +msg(M_INFO, "Successfully set %s mtu on interface %d",
> family_name, iface_index);
> +}
> +}
> +
> +
> /*
> * Return a TAP name for netsh commands.
> */
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 623c3ff7..c24cb22b 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -1198,6 +1198,29 @@ HandleEnableDHCPMessage(const enable_dhcp_message_t
> *dhcp)
> return err;
> }
>
> +static DWORD
> +HandleMTUMessage(const set_mtu_message_t *mtu)
> +{
> +DWORD err = 0;
> +MIB_IPINTERFACE_ROW ipiface;
> +InitializeIpInterfaceEntry();
> +ipiface.Family = mtu->family;
> +ipiface.InterfaceIndex = mtu->iface.index;
> +err = GetIpInterfaceEntry();
> +if (err != NO_ERROR)
> +{
> +return err;
> +}
> +if (mtu->family == AF_INET)
> +{
> +ipiface.SitePrefixLength = 0;
> +}
> +ipiface.NlMtu = mtu->mtu;
> +
> +err = SetIpInterfaceEntry();
> +return err;
> +}
> +
> static VOID
> HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events,
> undo_lists_t *lists)
> {
> @@ -1210,6 +1233,7 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD count,
> LPHANDLE events, undo_lists
> block_dns_message_t block_dns;
> dns_cfg_message_t dns;
> enable_dhcp_message_t dhcp;
> +set_mtu_message_t mtu;
> } msg;
> ack_message_t ack = {
> .header = {
> @@ -1277,6 +1301,13 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD
> count, LPHANDLE events, undo_lists
> }
> break;
>
> +case msg_set_mtu:
> +if (msg.header.size == sizeof(msg.mtu))
> +{
> +ack.error_number = HandleMTUMessage();
> +}
> +break;
> +
> default:
> ack.error_number = ERROR_MESSAGE_TYPE;
> MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Unknown message type
> %d"), msg.header.type);
>
>
Acked-by: Selva Nair
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Issue with smartcard authentication for openvpn
Hi JJK,
On Wed, Apr 17, 2019 at 10:50 AM Jan Just Keijser wrote:
> Hi Selva,
>
> On 10/04/19 19:09, Selva Nair wrote:
>
>
>
> On Wed, Apr 10, 2019 at 12:59 PM Jan Just Keijser
> wrote:
>
>
snipped...
> patching pkcs11-helper does not seem too difficult for this particular
>> case - but how can we test it? I have access to hw tokens but I don't know
>> how to trigger the "raw signature" bit.
>>
>
> If both server and client are built with OpenSSL 1.1.1 and TLS version is
> >= 1.2, PSS padding will get used and trigger this. OpenSSL does PSS
> padding internally and passes the padded data to the rsa_priv_enc calback
> for raw signature.
>
> This is based on my tests for our Windows cryptoapi and
> management-external-key patches for the same -- never tried this using
> pkcs11-helper, but I expect the same behaviour.
>
>
> The good news: I can reproduce this with
> - openvpn 2.4.7
> - openssl 1.1.1b
> - pkcs11helper 1.25.1
> - Safenet etoken
> - client+server CentOS 7
>
> The bad news: I don't have a fix yet.
>
What I has in mind was a very simple patch like the one attached (totally
untested).
But I guess you tried that already and it doesn't work? Is it because the
token
does not support raw signature (not all do) or something else?
Selva
From 547c58ce24e76598b504c9062820dae43fa9cc38 Mon Sep 17 00:00:00 2001
From: Selva Nair
Date: Wed, 17 Apr 2019 11:26:40 -0400
Subject: [PATCH] Support raw RSA signature
Signed-off-by: Selva Nair
---
lib/pkcs11h-openssl.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/pkcs11h-openssl.c b/lib/pkcs11h-openssl.c
index 4ebc211..6a396bb 100644
--- a/lib/pkcs11h-openssl.c
+++ b/lib/pkcs11h-openssl.c
@@ -567,7 +567,13 @@ __pkcs11h_openssl_rsa_enc (
padding
);
- if (padding != RSA_PKCS1_PADDING) {
+ if (padding == RSA_PKCS1_PADDING) {
+ padding = CKM_RSA_PKCS;
+ }
+ else if (padding == RSA_NO_PADDING) {
+ padding = CKM_RSA_X_509;
+ }
+ else {
rv = CKR_MECHANISM_INVALID;
goto cleanup;
}
@@ -587,7 +593,7 @@ __pkcs11h_openssl_rsa_enc (
if (
(rv = pkcs11h_certificate_signAny (
certificate,
- CKM_RSA_PKCS,
+ padding,
from,
flen,
to,
--
2.1.4
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Issue with smartcard authentication for openvpn
Hi, On Wed, Apr 10, 2019 at 6:00 PM David Sommerseth < open...@sf.lists.topphemmelig.net> wrote: > On 10/04/2019 17:58, Selva Nair wrote: > > > > As I replied to the openssl-users list[*], pkcs11-helper only supports > PKCS1 > > signatures, not raw signature needed in this case. > > > > We have to either patch pkcs11-helper or switch to something else. > > It would be wonderful to switch it for something else. Unfortunately, it > does > a lot of gluing between the lower-level operations (similarly available via > p11-kit) and the interfaces implemented in OpenVPN is fairly high-level. > So > this "glue code" which pkcs11-helper is, is not that trivial and last time > I > checked the alternatives were scarce :( > > Is this a Windows only issue? Or is it present on other platforms as well? > If it's Windows only, I think we can get around it by patching it and > ensuring > upstream is aware of this. But if it is more platforms, patching > pkcs11-helper gets nasty quickly. > This has nothing to do with Windows. This is a limitation of pkcs11-helper no matter what OS its used in. In fact on Windows we have an alternative option to use hardware tokens through cryptoapicert, but on other OSes we are dependent on pkcs11-helper. So this has everything non-Windows written on it. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Issue with smartcard authentication for openvpn
On Wed, Apr 10, 2019 at 12:59 PM Jan Just Keijser wrote: > On 10/04/19 17:58, Selva Nair wrote: > > Hi, > > This is more relevant to OpenVPN than OpenSSL, so copying to the > openvpn-devel list. > > On Wed, Apr 10, 2019 at 10:11 AM Francois Gelis > wrote: > >> Hi all, >> >> I have a working openvpn setup with client certificate and private key >> stored on my laptop. Then, I have loaded them into a smartcard (Yubico 5 >> NFC), and modified accordingly the openvpn client config. But running the >> openvpn client now fails with an error that seems to originate inside >> openssl. Here is a verbose openvpn log (only the portion that seems >> relevant for this error, but I have the full log if useful): >> >> Sat Apr 6 15:57:20 2019 us=467260 Incoming Ciphertext -> TLS >> Sat Apr 6 15:57:20 2019 us=467271 SSL state (connect): SSLv3/TLS read >> server hello >> Sat Apr 6 15:57:20 2019 us=467468 VERIFY OK: depth=1, CN=FG-CA >> Sat Apr 6 15:57:20 2019 us=467598 VERIFY KU OK >> Sat Apr 6 15:57:20 2019 us=467609 Validating certificate extended key >> usage >> Sat Apr 6 15:57:20 2019 us=467615 ++ Certificate has EKU (str) TLS Web >> Server Authentication, expects TLS Web Server Authentication >> Sat Apr 6 15:57:20 2019 us=467620 VERIFY EKU OK >> Sat Apr 6 15:57:20 2019 us=467625 VERIFY OK: depth=0, CN=tx2 >> Sat Apr 6 15:57:20 2019 us=467650 SSL state (connect): SSLv3/TLS read >> server certificate >> Sat Apr 6 15:57:20 2019 us=467735 SSL state (connect): SSLv3/TLS read >> server key exchange >> Sat Apr 6 15:57:20 2019 us=467763 SSL state (connect): SSLv3/TLS read >> server certificate request >> Sat Apr 6 15:57:20 2019 us=467771 SSL state (connect): SSLv3/TLS read >> server done >> Sat Apr 6 15:57:20 2019 us=467845 SSL state (connect): SSLv3/TLS write >> client certificate >> Sat Apr 6 15:57:20 2019 us=468012 SSL state (connect): SSLv3/TLS write >> client key exchange >> Sat Apr 6 15:57:20 2019 us=468053 PKCS#11: __pkcs11h_openssl_rsa_enc >> entered - flen=256, from=0x559d078d6e70, to=0x559d078d6bc0, >> rsa=0x559d078b3630, padding=3 >> Sat Apr 6 15:57:20 2019 us=468060 PKCS#11: __pkcs11h_openssl_rsa_enc - >> return rv=112-'CKR_MECHANISM_INVALID' >> Sat Apr 6 15:57:20 2019 us=468070 SSL alert (write): fatal: internal >> error >> Sat Apr 6 15:57:20 2019 us=468085 OpenSSL: error:141F0006:SSL >> routines:tls_construct_cert_verify:EVP lib >> Sat Apr 6 15:57:20 2019 us=468092 TLS_ERROR: BIO read tls_read_plaintext >> error >> Sat Apr 6 15:57:20 2019 us=468097 TLS Error: TLS object -> incoming >> plaintext read error >> Sat Apr 6 15:57:20 2019 us=468101 TLS Error: TLS handshake failed >> >> Somehow, it seems that __pkcs11h_openssl_rsa_enc was called with an >> unexpected padding. Any ideas on what might be the cause of this? >> >> > As I replied to the openssl-users list[*], pkcs11-helper only supports > PKCS1 signatures, not raw signature needed in this case. > > We have to either patch pkcs11-helper or switch to something else. > > > patching pkcs11-helper does not seem too difficult for this particular > case - but how can we test it? I have access to hw tokens but I don't know > how to trigger the "raw signature" bit. > If both server and client are built with OpenSSL 1.1.1 and TLS version is >= 1.2, PSS padding will get used and trigger this. OpenSSL does PSS padding internally and passes the padded data to the rsa_priv_enc calback for raw signature. This is based on my tests for our Windows cryptoapi and management-external-key patches for the same -- never tried this using pkcs11-helper, but I expect the same behaviour. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] Issue with smartcard authentication for openvpn
Hi, This is more relevant to OpenVPN than OpenSSL, so copying to the openvpn-devel list. On Wed, Apr 10, 2019 at 10:11 AM Francois Gelis wrote: > Hi all, > > I have a working openvpn setup with client certificate and private key > stored on my laptop. Then, I have loaded them into a smartcard (Yubico 5 > NFC), and modified accordingly the openvpn client config. But running the > openvpn client now fails with an error that seems to originate inside > openssl. Here is a verbose openvpn log (only the portion that seems > relevant for this error, but I have the full log if useful): > > Sat Apr 6 15:57:20 2019 us=467260 Incoming Ciphertext -> TLS > Sat Apr 6 15:57:20 2019 us=467271 SSL state (connect): SSLv3/TLS read > server hello > Sat Apr 6 15:57:20 2019 us=467468 VERIFY OK: depth=1, CN=FG-CA > Sat Apr 6 15:57:20 2019 us=467598 VERIFY KU OK > Sat Apr 6 15:57:20 2019 us=467609 Validating certificate extended key > usage > Sat Apr 6 15:57:20 2019 us=467615 ++ Certificate has EKU (str) TLS Web > Server Authentication, expects TLS Web Server Authentication > Sat Apr 6 15:57:20 2019 us=467620 VERIFY EKU OK > Sat Apr 6 15:57:20 2019 us=467625 VERIFY OK: depth=0, CN=tx2 > Sat Apr 6 15:57:20 2019 us=467650 SSL state (connect): SSLv3/TLS read > server certificate > Sat Apr 6 15:57:20 2019 us=467735 SSL state (connect): SSLv3/TLS read > server key exchange > Sat Apr 6 15:57:20 2019 us=467763 SSL state (connect): SSLv3/TLS read > server certificate request > Sat Apr 6 15:57:20 2019 us=467771 SSL state (connect): SSLv3/TLS read > server done > Sat Apr 6 15:57:20 2019 us=467845 SSL state (connect): SSLv3/TLS write > client certificate > Sat Apr 6 15:57:20 2019 us=468012 SSL state (connect): SSLv3/TLS write > client key exchange > Sat Apr 6 15:57:20 2019 us=468053 PKCS#11: __pkcs11h_openssl_rsa_enc > entered - flen=256, from=0x559d078d6e70, to=0x559d078d6bc0, > rsa=0x559d078b3630, padding=3 > Sat Apr 6 15:57:20 2019 us=468060 PKCS#11: __pkcs11h_openssl_rsa_enc - > return rv=112-'CKR_MECHANISM_INVALID' > Sat Apr 6 15:57:20 2019 us=468070 SSL alert (write): fatal: internal error > Sat Apr 6 15:57:20 2019 us=468085 OpenSSL: error:141F0006:SSL > routines:tls_construct_cert_verify:EVP lib > Sat Apr 6 15:57:20 2019 us=468092 TLS_ERROR: BIO read tls_read_plaintext > error > Sat Apr 6 15:57:20 2019 us=468097 TLS Error: TLS object -> incoming > plaintext read error > Sat Apr 6 15:57:20 2019 us=468101 TLS Error: TLS handshake failed > > Somehow, it seems that __pkcs11h_openssl_rsa_enc was called with an > unexpected padding. Any ideas on what might be the cause of this? > > As I replied to the openssl-users list[*], pkcs11-helper only supports PKCS1 signatures, not raw signature needed in this case. We have to either patch pkcs11-helper or switch to something else. Selva [*] https://mta.openssl.org/pipermail/openssl-users/2019-April/010266.html ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 4/4] Simplified if statements for better readability
Hi, On Mon, Apr 1, 2019 at 7:22 AM Christopher Schenk < csch...@mail.uni-paderborn.de> wrote: > --- > src/openvpn/tun.c | 58 --- > 1 file changed, 14 insertions(+), 44 deletions(-) > @@ -213,7 +213,7 @@ do_set_mtu_service(const struct tuntap *tt, const short > family, const int mtu) > ack_message_t ack; > struct gc_arena gc = gc_new(); > HANDLE pipe = tt->options.msg_channel; > - > + char *family_name = (family == AF_INET6) ? "IPv6" : "IPv4"; This should be preferably declared as const char *family_name. Also see my response to commit 2 Some general remarks follow: Now we have 4 commits, two for the initial netsh implementation, one replacing that by SetIpInterfaceEntry() and a fixup. We don't want to keep all that irrelevant history, so these have to be squashed into one commit (or two if you wish to separate the interactive service part). And edit the final commit message header and description to reflect the final version. Squashing all into one commit may be easier and that's fine in this case. We also prefer to have patches signed off. Adding -s to "git commit" or doing "git commit -s --amend" will take care of that. Finally, I'm not sure whether we still require patch authors to follow the code formatting style or we automatically run uncrustify during merge. Even if its not mandatory, its a good practice to follow the style in the rest of the code to the extent possible: for example, we use 4 spaces for indent (not 8), no tabs anywhere etc. You can run "uncrustify -c dev-tools/uncrustify.conf" on tun.c, interactive.c and openvpn_msg.h to get most of that rectified. Note that uncrustify may edit unrelated parts of the code -- if it does, do not include such changes in the patch. Thanks Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/4] Use netioapi instead of netsh to set mtu
ce_index);
> + }
> + }
> }
>
> +
> /*
> * Return a TAP name for netsh commands.
> */
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 31e4afa0..6e6a63fe 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -33,6 +33,7 @@
> #include
> #include
> #include
> +#include
>
>
There should be no need to include this header directly. Instead let
the main IP Helper API header (iphlpapi.h) include it correctly.
The latter is already included both in tun.c and interactive.c
> #ifdef HAVE_VERSIONHELPERS_H
> #include
> @@ -1202,42 +1203,13 @@ static DWORD
> HandleMTUMessage(const set_mtu_message_t *mtu)
> {
> DWORD err = 0;
> - DWORD timeout = 5000; /* in milli seconds */
> - wchar_t argv0[MAX_PATH];
> -
> - /* Path of netsh */
> - swprintf(argv0, _countof(argv0), L"%s\\%s", get_win_sys_path(),
> L"netsh.exe");
> - argv0[_countof(argv0) - 1] = L'\0';
> -
> - /* cmd template:
> -* netsh interface $family set subinterface "$if_name" mtu=$mtu
> -*/
> - const wchar_t *fmt;
> - if (mtu->family == AF_INET)
> - {
> - fmt = L"netsh interface ipv4 set subinterface \"%d\" mtu=
> %d";
> - }
> - else if (mtu->family == AF_INET6)
> - {
> - fmt = L"netsh interface ipv6 set subinterface \"%d\" mtu=
> %d";
> - }
> -
> - /* max cmdline length in wchars -- include room for if index:
> -* 20 chars for two 32 bit int in decimal and +1 for NUL
> -*/
> - size_t ncmdline = wcslen(fmt) + 20 + 1;
> - wchar_t *cmdline = malloc(ncmdline * sizeof(wchar_t));
> - if (!cmdline)
> - {
> - err = ERROR_OUTOFMEMORY;
> - return err;
> - }
> -
> - openvpn_sntprintf(cmdline, ncmdline, fmt, mtu->iface.index,
> mtu->mtu);
> -
> - err = ExecCommand(argv0, cmdline, timeout);
> -
> - free(cmdline);
> + MIB_IPINTERFACE_ROW row;
> + InitializeIpInterfaceEntry();
> + row.Family = mtu->family;
> + row.InterfaceIndex = mtu->iface.index;
> + row.NlMtu = mtu->mtu;
> +
> + err = SetIpInterfaceEntry();
>
See the remark above.
> return err;
> }
>
>
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Setting adapter mtu on windows systems
Hi,
On Fri, Mar 29, 2019 at 6:25 AM Christopher Schenk
wrote:
>
> Hi,
>
> On 28/03/2019 16:00, Selva Nair wrote:
> > I would go a step further to say we should not add new features that
> > do not work when started using the interactive service.
> >
> > Secondly, we should avoid the old style use of netsh and instead use the
> > iphelper API as far as possible. It should be possible to
> > set MTU using the SetIfEntry() or SetIpInterfaceEnrty() function -- I
> > haven't
> > checked which one is appropriate here.
>
> I agree on the point that we should add this functionality to the
> interactive service as well. So i modified the patch accordingly.
> I also modified my patch so that it uses the SetIpInterfaceEnrty
> function instead of netsh.
Thanks for the update. Looks okay, but would prefer a version
sent out by git-send-email so that we can apply and test the patch.
Some minor comments below.
>
> ---
> diff --git a/include/openvpn-msg.h b/include/openvpn-msg.h
> index 66177a21..f59c5a21 100644
> --- a/include/openvpn-msg.h
> +++ b/include/openvpn-msg.h
> @@ -39,6 +39,7 @@ typedef enum {
> msg_del_block_dns,
> msg_register_dns,
> msg_enable_dhcp,
> +msg_set_mtu,
> } message_type_t;
>
> typedef struct {
> @@ -117,4 +118,11 @@ typedef struct {
> interface_t iface;
> } enable_dhcp_message_t;
>
> +typedef struct {
> +message_header_t header;
> +interface_t iface;
> +short family;
> +int mtu;
> +} set_mtu_message_t;
> +
> #endif /* ifndef OPENVPN_MSG_H_ */
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 48a8fdf7..9fe8444f 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -69,6 +69,10 @@ static void netsh_ifconfig(const struct
> tuntap_options *to,
> const in_addr_t netmask,
> const unsigned int flags);
>
> +static void windows_set_mtu(const int iface_indey,
iface_indey -> iface_index ?
> +short family,
> + const int mtu);
> +
> static void netsh_set_dns6_servers(const struct in6_addr *addr_list,
> const int addr_len,
> const char *flex_name);
> @@ -201,6 +205,63 @@ out:
> return ret;
> }
>
> +static bool
> +do_set_mtu_service(const struct tuntap *tt, const short family, const
> int mtu)
> +{
> +DWORD len;
> +bool ret = false;
> +ack_message_t ack;
> +struct gc_arena gc = gc_new();
> +HANDLE pipe = tt->options.msg_channel;
> +
> +set_mtu_message_t mtu_msg = {
> +.header = {
> +msg_set_mtu,
> +sizeof(set_mtu_message_t),
> +0
> +},
> +.iface = {.index = tt->adapter_index,.name = tt->actual_name },
> +.mtu = mtu,
> +.family = family
> +};
> +
> +if (!send_msg_iservice(pipe, _msg, sizeof(mtu_msg), ,
> "Set_mtu"))
> +{
> +goto out;
> +}
> +
> +if (family == AF_INET)
> +{
> +if (ack.error_number != NO_ERROR)
> +{
> +msg(M_NONFATAL, "TUN: setting IPv4 mtu using service
> failed: %s [status=%u if_index=%d]",
> +strerror_win32(ack.error_number, ),
> ack.error_number, mtu_msg.iface.index);
> +}
> +else
> +{
> +msg(M_INFO, "IPv4 MTU set to %d on interface %d using
> service", mtu, mtu_msg.iface.index);
> +ret = true;
> +}
> +}
> +else if (family == AF_INET6)
> +{
> +if (ack.error_number != NO_ERROR)
> +{
> +msg(M_NONFATAL, "TUN: setting IPv6 mtu using service
> failed: %s [status=%u if_index=%d]",
> +strerror_win32(ack.error_number, ),
> ack.error_number, mtu_msg.iface.index);
> +}
Though this repetition is fine with me and may be we do
the same in other similar contexts, cant we combine these
two cases into one by defining a string "IPv4" or "IPv6"
based on the value of family?
The same applies to windows_set_mtu() as well.
> +else
> +{
> +msg(M_INFO, "IPv6 MTU set to %d on interface %d using
> service", mtu, mtu_msg.iface.index);
> +ret = true;
> +}
> +}
> +
> +out:
> +gc_free();
> +return ret;
> +}
> +
> #endif /* ifdef _WIN32 */
>
> #ifdef TARGET_SOLARIS
> @@ -984,6 +1045,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char
> *ifname, int tun_mtu,
> {
>
Re: [Openvpn-devel] [PATCH] Setting adapter mtu on windows systems
Hi
On Thu, Mar 28, 2019 at 9:13 AM Arne Schwabe wrote:
> Am 28.03.19 um 13:27 schrieb Christopher Schenk:
> > From: Christopher Schenk
> >
> > ---
> > src/openvpn/tun.c | 39 ++-
> > 1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> > index 48a8fdf7..93d028c8 100644
> > --- a/src/openvpn/tun.c
> > +++ b/src/openvpn/tun.c
> > @@ -69,6 +69,12 @@ static void netsh_ifconfig(const struct
> tuntap_options *to,
> > const in_addr_t netmask,
> > const unsigned int flags);
> >
> > +static void netsh_set_mtu_ipv4(const char *flex_name,
> > + const int mtu);
> > +
> > +static void netsh_set_mtu_ipv6(const char *flex_name,
> > + const int mtu);
> > +
> > static void netsh_set_dns6_servers(const struct in6_addr *addr_list,
> > const int addr_len,
> > const char *flex_name);
> > @@ -1000,6 +1006,7 @@ do_ifconfig_ipv6(struct tuntap *tt, const char
> *ifname, int tun_mtu,
> > netsh_command(, 4, M_FATAL);
> > /* set ipv6 dns servers if any are specified */
> > netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len,
> ifname);
> > +netsh_set_mtu_ipv6(ifname, tun_mtu);
> > }
> >
> > /* explicit route needed */
> > @@ -1391,9 +1398,9 @@ do_ifconfig_ipv4(struct tuntap *tt, const char
> *ifname, int tun_mtu,
> > case IPW32_SET_NETSH:
> > netsh_ifconfig(>options, ifname, tt->local,
> > tt->adapter_netmask,
> NI_IP_NETMASK|NI_OPTIONS);
> > -
> > break;
> > }
> > + netsh_set_mtu_ipv4(ifname, tun_mtu);
> > }
> >
> > #else /* if defined(TARGET_LINUX) */
> > @@ -5236,6 +5243,36 @@ out:
> > return ret;
> > }
> >
> > +static void
> > +netsh_set_mtu_ipv4(const char *flex_name,
> > + const int mtu)
> > +{
> > + struct argv argv = argv_new();
> > + argv_printf(, "%s%sc interface ipv4 set subinterface %s mtu =
> %d store=active",
>
> The spacing here looks a bit odd. I would have expected mtu=%d instead.
> Is this necessary or was there a reason for doing this?
>
> Patch looks okay enough to ACK but:
>
> In general, this patch adds a missing feature (setting MTU) with one
> windows interface only (netsh). And more commonly used interface
> (interactive service)would be different then leading to harder to debug
> problems.
>
I would go a step further to say we should not add new features that
do not work when started using the interactive service.
Secondly, we should avoid the old style use of netsh and instead use the
iphelper API as far as possible. It should be possible to
set MTU using the SetIfEntry() or SetIpInterfaceEnrty() function -- I
haven't
checked which one is appropriate here.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Improve the documentation for --dhcp-option
On Wed, Mar 20, 2019 at 10:52 AM tincanteksup wrote: > > > > On 20/03/2019 13:25, Selva Nair wrote: > > Hi, > > > > On Wed, Mar 20, 2019 at 4:02 AM Antonio Quartulli wrote: > >> > >> Hi, > >> > >> On 18/03/2019 22:30, tincanteksup wrote: > >>> Hi, > >>> > >>> this situation has been hanging around for so long is this brief note > >>> really enough? Considering that the manual has numerous other URLs why > >>> not include this URL here: > >>> https://community.openvpn.net/openvpn/wiki/Pushing-DNS-to-clients > >>> > >>> We already have this: > >>> https://community.openvpn.net/openvpn/wiki/SWEET32 > >>> > >> > >> the problem with URLs is that they become obsolete and we need to > >> re-patch the manual in order to update them (or ensure the URL always > >> work/redirect to something). > > > > > > I have the same opinion about URLs. On top of that the wiki > > description is inadequate and inaccurate. But that's a different > > topic. > > Inadequate: > Compared to NOTHING, the page is at least a step toward documentation. > > Inaccurate: > Please expand .. The wiki states that on Windows using version 2.4, OpenVPN-GUI+interactive service required for processing this option. Not so. Just as in 2.3, it works if run as admin. In fact, several setups run OpenVPN on boot using OpenVPNService, and --dhcp-option does work for them. With 2.4, it also works as limited user if started using the interactive service (for example, using OpenVPN-GUI). In both 2.3 and 2.4, the critical piece is not overriding --ipwin32 method (adaptive or dynamic is required and the latter is the default). Selva On Wed, Mar 20, 2019 at 10:52 AM tincanteksup wrote: > > > On 20/03/2019 13:25, Selva Nair wrote: > > Hi, > > > > On Wed, Mar 20, 2019 at 4:02 AM Antonio Quartulli wrote: > >> > >> Hi, > >> > >> On 18/03/2019 22:30, tincanteksup wrote: > >>> Hi, > >>> > >>> this situation has been hanging around for so long is this brief note > >>> really enough? Considering that the manual has numerous other URLs why > >>> not include this URL here: > >>> https://community.openvpn.net/openvpn/wiki/Pushing-DNS-to-clients > >>> > >>> We already have this: > >>> https://community.openvpn.net/openvpn/wiki/SWEET32 > >>> > >> > >> the problem with URLs is that they become obsolete and we need to > >> re-patch the manual in order to update them (or ensure the URL always > >> work/redirect to something). > > > > > > I have the same opinion about URLs. On top of that the wiki > > description is inadequate and inaccurate. But that's a different > > topic. > > Inadequate: > Compared to NOTHING, the page is at least a step toward documentation. > > Inaccurate: > Please expand .. > > ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Improve the documentation for --dhcp-option
Hi, On Wed, Mar 20, 2019 at 9:45 AM Arne Schwabe wrote: > > Am 20.03.19 um 14:25 schrieb Selva Nair: > > Hi, > > > > On Wed, Mar 20, 2019 at 4:02 AM Antonio Quartulli wrote: > >> > >> Hi, > >> > >> On 18/03/2019 22:30, tincanteksup wrote: > >>> Hi, > >>> > >>> this situation has been hanging around for so long is this brief note > >>> really enough? Considering that the manual has numerous other URLs why > >>> not include this URL here: > >>> https://community.openvpn.net/openvpn/wiki/Pushing-DNS-to-clients > >>> > >>> We already have this: > >>> https://community.openvpn.net/openvpn/wiki/SWEET32 > >>> > >> > >> the problem with URLs is that they become obsolete and we need to > >> re-patch the manual in order to update them (or ensure the URL always > >> work/redirect to something). > > > > > > I have the same opinion about URLs. On top of that the wiki > > description is inadequate and inaccurate. But that's a different > > topic. > > > > The patch was meant to remove the ambiguity in the current > > wording. We could add a line saying many distributions include > > such scripts and front-ends such as network manager, tunnelblick, > > OpenVPN for Android etc. also handle some dhcp-options. (I don't use > > any of them, so guessing here..) > > > > Yes. Most platforms and client interpret this option. Only OpenVPN > itself only interprets it only Windows and Android. It should probably > changed to reflect reality. In case of Android, its only sent to the management, right? As the man page is about OpenVPN core, I would say the current statement that the option is handled only on Windows is correct. But its useful to expand on the usage on other platforms. I can add a line about scripts in OS distributions and front-ends/UI like Android that can transparently handle it. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Improve the documentation for --dhcp-option
Hi, On Wed, Mar 20, 2019 at 4:02 AM Antonio Quartulli wrote: > > Hi, > > On 18/03/2019 22:30, tincanteksup wrote: > > Hi, > > > > this situation has been hanging around for so long is this brief note > > really enough? Considering that the manual has numerous other URLs why > > not include this URL here: > > https://community.openvpn.net/openvpn/wiki/Pushing-DNS-to-clients > > > > We already have this: > > https://community.openvpn.net/openvpn/wiki/SWEET32 > > > > the problem with URLs is that they become obsolete and we need to > re-patch the manual in order to update them (or ensure the URL always > work/redirect to something). I have the same opinion about URLs. On top of that the wiki description is inadequate and inaccurate. But that's a different topic. The patch was meant to remove the ambiguity in the current wording. We could add a line saying many distributions include such scripts and front-ends such as network manager, tunnelblick, OpenVPN for Android etc. also handle some dhcp-options. (I don't use any of them, so guessing here..) > > > However, I was thinking that a WARNING in the log when parsing a > dhcp-option without any script configured (on non-windows platform) may > also be beneficial. This would catch some obvious cases but not when a script is being used for some other purpose. Still, sounds useful. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Improve the documentation for --dhcp-option
From: Selva Nair
Make clear that --dhcp-option is not processed on
non-Windows clients and the user is expected to handle
it using an --up script.
Signed-off-by: Selva Nair
---
doc/openvpn.8 | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index ce44044..993c8bd 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -6026,13 +6026,21 @@ scope IDs. The Scope ID becomes a part of the NetBIOS
name, making the name uniq
.B DISABLE\-NBT \-\-
Disable Netbios\-over\-TCP/IP.
-Note that if
+Note that
+.B \-\-dhcp\-option
+is processed only on Windows clients. If
.B \-\-dhcp\-option
-is pushed via
+is used directly on or pushed via
.B \-\-push
-to a non\-windows client, the option will be saved in the client's
-environment before the up script is called, under
-the name "foreign_option_{n}".
+to a non\-windows client, the option is
+.B only
+saved in the client's
+environment before the
+.B \-\-up
+script is called, under the name "foreign_option_{n}".
+The user has to arrange an
+.B \-\-up
+script to pick up those parameters and act on them as required.
.\"*
.TP
.B \-\-tap\-sleep n
--
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] Better error message when script fails due to script-security setting
From: Selva Nair
- Add a new return value (-2) for openvpn_execve() when external
program execution is not allowed due to a low script-security
setting.
- Add a corresponding error message
Errors and warnings in such cases will now display as
"WARNING: failed running command () :" followed by
"disallowed by script-security setting" on all platforms
instead of the current
"external program did not execute -- returned error code -1"
on Windows and
"external program fork failed" on other platforms.
The error is FATAL for some scripts and that behaviour is unchanged.
This helps the Windows GUI to detect when a connection failure
results from a safer script-security setting enforced by the GUI,
and show a relevant message.
Note: Same as commit 01a3c876d4911 in master except for
script_security() --> script_security and context change:
run_command.[ch] --> misc.[ch]
Signed-off-by: Selva Nair
---
src/openvpn/misc.c | 93 -
src/openvpn/misc.h | 5 +++
src/openvpn/win32.c | 12 ---
3 files changed, 69 insertions(+), 41 deletions(-)
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 581a890..f44c65f 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -99,44 +99,57 @@ save_inetd_socket_descriptor(void)
}
/*
- * Print an error message based on the status code returned by system().
+ * Generate an error message based on the status code returned by
openvpn_execve().
*/
const char *
system_error_message(int stat, struct gc_arena *gc)
{
struct buffer out = alloc_buf_gc(256, gc);
-#ifdef _WIN32
-if (stat == -1)
+
+switch (stat)
{
-buf_printf(, "external program did not execute -- ");
-}
-buf_printf(, "returned error code %d", stat);
+case OPENVPN_EXECVE_NOT_ALLOWED:
+buf_printf(, "disallowed by script-security setting");
+break;
+
+#ifdef _WIN32
+case OPENVPN_EXECVE_ERROR:
+buf_printf(, "external program did not execute -- ");
+/* fall through */
+
+default:
+buf_printf(, "returned error code %d", stat);
+break;
#else /* ifdef _WIN32 */
-if (stat == -1)
-{
-buf_printf(, "external program fork failed");
-}
-else if (!WIFEXITED(stat))
-{
-buf_printf(, "external program did not exit normally");
-}
-else
-{
-const int cmd_ret = WEXITSTATUS(stat);
-if (!cmd_ret)
-{
-buf_printf(, "external program exited normally");
-}
-else if (cmd_ret == 127)
-{
-buf_printf(, "could not execute external program");
-}
-else
-{
-buf_printf(, "external program exited with error status: %d",
cmd_ret);
-}
-}
+
+case OPENVPN_EXECVE_ERROR:
+buf_printf(, "external program fork failed");
+break;
+
+default:
+if (!WIFEXITED(stat))
+{
+buf_printf(, "external program did not exit normally");
+}
+else
+{
+const int cmd_ret = WEXITSTATUS(stat);
+if (!cmd_ret)
+{
+buf_printf(, "external program exited normally");
+}
+else if (cmd_ret == OPENVPN_EXECVE_FAILURE)
+{
+buf_printf(, "could not execute external program");
+}
+else
+{
+buf_printf(, "external program exited with error
status: %d", cmd_ret);
+}
+}
+break;
#endif /* ifdef _WIN32 */
+}
return (const char *)out.data;
}
@@ -186,12 +199,14 @@ openvpn_execve_allowed(const unsigned int flags)
* Run execve() inside a fork(). Designed to replicate the semantics of
system() but
* in a safer way that doesn't require the invocation of a shell or the risks
* assocated with formatting and parsing a command line.
+ * Returns the exit status of child, OPENVPN_EXECVE_NOT_ALLOWED if
openvpn_execve_allowed()
+ * returns false, or OPENVPN_EXECVE_ERROR on other errors.
*/
int
openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned
int flags)
{
struct gc_arena gc = gc_new();
-int ret = -1;
+int ret = OPENVPN_EXECVE_ERROR;
static bool warn_shown = false;
if (a && a->argv[0])
@@ -208,7 +223,7 @@ openvpn_execve(const struct argv *a, const struct env_set
*es, const unsigned in
if (pid == (pid_t)0) /* child side */
{
execve(cmd, argv, envp);
-exit(127);
+exit(OPENVPN_EXECVE_FAILURE);
}
else if (pid &l
Re: [Openvpn-devel] [PATCH applied] Re: Better error message when script fails due to script-security setting
Hi, On Thu, Feb 28, 2019 at 12:25 PM Gert Doering wrote: > Acked-by: Gert Doering > > Thanks. I have not tested the various error conditions, but the code > makes sense to me and it passes compile and t_client tests (which use > --up here), so it's not breaking stuff in fundamental and non-obvious > ways. > > (Since David was mostly happy with the v1 approach except for the > "127" and "switch()", I assume he's totally fine with v2 now :-) ) > > Your patch has been applied to the master branch - it won't apply to > 2.4 as we have no "run_command.*" there yet. Shall we cherrypick > bf97c00f7dba4 and 253f015558 to get there? Or do you want to do a > 2.4 version of the patch? > Would require more cherry-picks than that, or some manual edits, isn't it? I can send a patch for 2.4 -- would you prefer the same as this -- i.e with switch(stat) in misc.c and and preprocessor macros in misc.h or a simpler, return code = -2 and error message as in version 1? Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2] Better error message when script fails due to script-security setting
From: Selva Nair
- Add a new return value (-2) for openvpn_execve() when external
program execution is not allowed due to a low script-security
setting.
- Add a corresponding error message
Errors and warnings in such cases will now display as
"WARNING: failed running command () :" followed by
"disallowed by script-security setting" on all platforms
instead of the current
"external program did not execute -- returned error code -1"
on Windows and
"external program fork failed" on other platforms.
The error is FATAL for some scripts and that behaviour is unchanged.
This helps the Windows GUI to detect when a connection failure
results from a safer script-security setting enforced by the GUI,
and show a relevant message.
v2 changes as suggested by
- define macros for return values of openvpn_execve()
- replace if/else by switch() in system_error_message()
Signed-off-by: Selva Nair
---
src/openvpn/run_command.c | 93 ---
src/openvpn/run_command.h | 4 ++
src/openvpn/win32.c | 12 --
3 files changed, 68 insertions(+), 41 deletions(-)
diff --git a/src/openvpn/run_command.c b/src/openvpn/run_command.c
index 2d75a3e..4c4adf9 100644
--- a/src/openvpn/run_command.c
+++ b/src/openvpn/run_command.c
@@ -54,44 +54,57 @@ script_security_set(int level)
}
/*
- * Print an error message based on the status code returned by system().
+ * Generate an error message based on the status code returned by
openvpn_execve().
*/
static const char *
system_error_message(int stat, struct gc_arena *gc)
{
struct buffer out = alloc_buf_gc(256, gc);
-#ifdef _WIN32
-if (stat == -1)
+
+switch (stat)
{
-buf_printf(, "external program did not execute -- ");
-}
-buf_printf(, "returned error code %d", stat);
+case OPENVPN_EXECVE_NOT_ALLOWED:
+buf_printf(, "disallowed by script-security setting");
+break;
+
+#ifdef _WIN32
+case OPENVPN_EXECVE_ERROR:
+buf_printf(, "external program did not execute -- ");
+/* fall through */
+
+default:
+buf_printf(, "returned error code %d", stat);
+break;
#else /* ifdef _WIN32 */
-if (stat == -1)
-{
-buf_printf(, "external program fork failed");
-}
-else if (!WIFEXITED(stat))
-{
-buf_printf(, "external program did not exit normally");
-}
-else
-{
-const int cmd_ret = WEXITSTATUS(stat);
-if (!cmd_ret)
-{
-buf_printf(, "external program exited normally");
-}
-else if (cmd_ret == 127)
-{
-buf_printf(, "could not execute external program");
-}
-else
-{
-buf_printf(, "external program exited with error status: %d",
cmd_ret);
-}
-}
+
+case OPENVPN_EXECVE_ERROR:
+buf_printf(, "external program fork failed");
+break;
+
+default:
+if (!WIFEXITED(stat))
+{
+buf_printf(, "external program did not exit normally");
+}
+else
+{
+const int cmd_ret = WEXITSTATUS(stat);
+if (!cmd_ret)
+{
+buf_printf(, "external program exited normally");
+}
+else if (cmd_ret == OPENVPN_EXECVE_FAILURE)
+{
+buf_printf(, "could not execute external program");
+}
+else
+{
+buf_printf(, "external program exited with error
status: %d", cmd_ret);
+}
+}
+break;
#endif /* ifdef _WIN32 */
+}
return (const char *)out.data;
}
@@ -114,12 +127,14 @@ openvpn_execve_allowed(const unsigned int flags)
* Run execve() inside a fork(). Designed to replicate the semantics of
system() but
* in a safer way that doesn't require the invocation of a shell or the risks
* associated with formatting and parsing a command line.
+ * Returns the exit status of child, OPENVPN_EXECVE_NOT_ALLOWED if
openvpn_execve_allowed()
+ * returns false, or OPENVPN_EXECVE_ERROR on other errors.
*/
int
openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned
int flags)
{
struct gc_arena gc = gc_new();
-int ret = -1;
+int ret = OPENVPN_EXECVE_ERROR;
static bool warn_shown = false;
if (a && a->argv[0])
@@ -136,7 +151,7 @@ openvpn_execve(const struct argv *a, const struct env_set
*es, const unsigned in
if (pid == (pid_t)0) /* child side */
{
execve(cmd, argv, envp);
-exit(127);
+exit(OPENVPN_EX
Re: [Openvpn-devel] [PATCH] Better error message when script fails due to script-security setting
Hi,
Thanks for the review.
On Tue, Feb 19, 2019 at 12:39 PM David Sommerseth <
open...@sf.lists.topphemmelig.net> wrote:
> On 17/02/2019 02:55, selva.n...@gmail.com wrote:
> > From: Selva Nair
> >
> > - Add a new return value (-2) for openvpn_execve() when external
> > program execution is not allowed due to a low script-security
> > setting.
> >
> > - Add a corresponding error message
> >
> > Errors and warnings in such cases will now display as
> > "WARNING: failed running command () :" followed by
> >
> > "disallowed by script-security setting" on all platforms
> >
> > instead of the current
> >
> > "external program did not execute -- returned error code -1"
> > on Windows and
> > "external program fork failed" on other platforms.
> >
> > The error is FATAL for some scripts and that behaviour is unchanged.
> >
> > This helps the Windows GUI to detect when a connection failure
> > results from a safer script-security setting enforced by the GUI,
> > and show a relevant message.
> >
> > Signed-off-by: Selva Nair
> > ---
> > This is being presented as a better alternative for patch 684.
> > A separate patch may be needed for 2.4 -- will do if this is found
> > acceptable.
>
> Generally speaking, this looks much better and should avoid changing the
> behaviour. It just clarifies the error better. Feature-ACK from me.
>
>
> [...]
> > diff --git a/src/openvpn/run_command.c b/src/openvpn/run_command.c
> > index 2d75a3e..7df7576 100644
> > --- a/src/openvpn/run_command.c
> > +++ b/src/openvpn/run_command.c
> > @@ -65,12 +65,23 @@ system_error_message(int stat, struct gc_arena *gc)
> > {
> > buf_printf(, "external program did not execute -- ");
> > }
> > -buf_printf(, "returned error code %d", stat);
> > +if (stat == -2)
> > +{
> > +buf_printf(, "disallowed by script-security setting");
> > +}
> > +else
> > +{
> > +buf_printf(, "returned error code %d", stat);
> > +}
>
> Perhaps we could swap out the -1 and -2 values with macro constants, for
> code
> clarity? Also, perhaps it would look more structured if using a switch()
> instead of if()/else if().
Because of branching based on if (WIFEXITED(stat)) etc., switch() may be
only
marginally cleaner and cause some deep indentation. But will give it a go.
> Otherwise, the code looks good.
>
> Another interesting are which most likely quite seldom fails, but we
> actually
> have yet another "undocumented" exit code ... This is not this patch's
> fault,
> but should probably be reviewed a bit more carefully.
>
> In run_command.c:149-153:
>
> pid = fork();
> if (pid == (pid_t)0) /* child side */
> {
> execve(cmd, argv, envp);
> exit(127);
> }
>
>
> If execve() fails, the exit code is 127. That would normally be caught by
> the
> waitpid() later on and this exit code would be returned by
> openvpn_execve().
>
> This should be improved in a separate patch though, but is not of high
> urgency. I don't expect most setups having a high execve() failure rate.
> But
> catching it and reporting this error path would be good.
>
In fact its easy to trigger this -- just have a shebang with a non-existent
path like
#!/bin/sh1 and child will exit with 127. In system_error_message() we
translate that as
"could not execute external program"
This "could not" instead of "did not" is a good choice of words, but would
be better
if we somehow marshal the actual errno set by execve (in this case ENOENT)
to the parent. Its lost in the current implementation. But that's beyond
this patch,
as you say.
I'll just add a macro for this 127 as well and leave it at that.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Better error message when script fails due to script-security setting
Hi On Mon, Feb 18, 2019 at 9:24 AM Gert Doering wrote: > Hi, > > On Sat, Feb 16, 2019 at 08:55:41PM -0500, selva.n...@gmail.com wrote: > > From: Selva Nair > > > > - Add a new return value (-2) for openvpn_execve() when external > > program execution is not allowed due to a low script-security > > setting. > > > > - Add a corresponding error message > > I find this a useful way forward (because it will not introduce new FATALs > where we had none before, just make the errors more clear). > > Since David had reservations on the other patch, I want to have explicit > agreement from him before going on - and will poke on IRC to make sure I > get attention. > Just to be explicit: Since David pointed out the "regressive" nature of the earlier patch, I too have reservations about it, and prefer this approach. Nevertheless, as we agreed to tag 2.4.7 *today* to make the release tomorrow > and meet the Debian deadline, I will not include this in 2.4.7. The planned script-security over-ride feature in the GUI is pretty intrusive. Taking time to give it more thought, and preferably get some user feedback, is not a bad thing. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Better error message when script fails due to script-security setting
From: Selva Nair
- Add a new return value (-2) for openvpn_execve() when external
program execution is not allowed due to a low script-security
setting.
- Add a corresponding error message
Errors and warnings in such cases will now display as
"WARNING: failed running command () :" followed by
"disallowed by script-security setting" on all platforms
instead of the current
"external program did not execute -- returned error code -1"
on Windows and
"external program fork failed" on other platforms.
The error is FATAL for some scripts and that behaviour is unchanged.
This helps the Windows GUI to detect when a connection failure
results from a safer script-security setting enforced by the GUI,
and show a relevant message.
Signed-off-by: Selva Nair
---
This is being presented as a better alternative for patch 684.
A separate patch may be needed for 2.4 -- will do if this is found
acceptable.
src/openvpn/run_command.c | 25 +
src/openvpn/win32.c | 10 +++---
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/src/openvpn/run_command.c b/src/openvpn/run_command.c
index 2d75a3e..7df7576 100644
--- a/src/openvpn/run_command.c
+++ b/src/openvpn/run_command.c
@@ -65,12 +65,23 @@ system_error_message(int stat, struct gc_arena *gc)
{
buf_printf(, "external program did not execute -- ");
}
-buf_printf(, "returned error code %d", stat);
+if (stat == -2)
+{
+buf_printf(, "disallowed by script-security setting");
+}
+else
+{
+buf_printf(, "returned error code %d", stat);
+}
#else /* ifdef _WIN32 */
if (stat == -1)
{
buf_printf(, "external program fork failed");
}
+else if (stat == -2)
+{
+buf_printf(, "disallowed by script-security setting");
+}
else if (!WIFEXITED(stat))
{
buf_printf(, "external program did not exit normally");
@@ -114,6 +125,8 @@ openvpn_execve_allowed(const unsigned int flags)
* Run execve() inside a fork(). Designed to replicate the semantics of
system() but
* in a safer way that doesn't require the invocation of a shell or the risks
* associated with formatting and parsing a command line.
+ * Returns the exit status of child, -2 if openvpn_execve_allowed() returned
false,
+ * or -1 on other errors.
*/
int
openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned
int flags)
@@ -150,10 +163,14 @@ openvpn_execve(const struct argv *a, const struct env_set
*es, const unsigned in
}
}
}
-else if (!warn_shown && (script_security() < SSEC_SCRIPTS))
+else
{
-msg(M_WARN, SCRIPT_SECURITY_WARNING);
-warn_shown = true;
+ret = -2;
+if (!warn_shown && (script_security() < SSEC_SCRIPTS))
+{
+msg(M_WARN, SCRIPT_SECURITY_WARNING);
+warn_shown = true;
+}
}
#else /* if defined(ENABLE_FEATURE_EXECVE) */
msg(M_WARN, "openvpn_execve: execve function not available");
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 463ac07..4eb7c40 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -1137,10 +1137,14 @@ openvpn_execve(const struct argv *a, const struct
env_set *es, const unsigned in
free(env);
gc_free();
}
-else if (!exec_warn && (script_security() < SSEC_SCRIPTS))
+else
{
-msg(M_WARN, SCRIPT_SECURITY_WARNING);
-exec_warn = true;
+ret = -2;
+if (!exec_warn && (script_security() < SSEC_SCRIPTS))
+{
+msg(M_WARN, SCRIPT_SECURITY_WARNING);
+exec_warn = true;
+}
}
}
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] Exit early when external scripts are specified with script-security < 2
Hi,
On Sat, Feb 16, 2019 at 8:19 AM David Sommerseth <
open...@sf.lists.topphemmelig.net> wrote:
> On 15/02/2019 21:31, Selva Nair wrote:
> > Hi
> >
> > On Fri, Feb 15, 2019 at 3:26 PM David Sommerseth
> > open...@sf.lists.topphemmelig.net>>
> > wrote:
> >
> > On 11/02/2019 21:46, selva.n...@gmail.com selva.n...@gmail.com> wrote:
> > > From: Selva Nair selva.n...@gmail.com>>
> > >
> > > Currently this raises a warning only. A fatal error is triggered
> > > later with a confusing message that script failed to execute.
> > >
> > > This helps the Windows GUI to show a relevant error message when
> > > script-security is over-ridden as a security measure.
> > >
> > > Signed-off-by: Selva Nair > <mailto:selva.n...@gmail.com>>
> > > ---
> > > src/openvpn/init.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> > > index 3c44967..5863828 100644
> > > --- a/src/openvpn/init.c
> > > +++ b/src/openvpn/init.c
> > > @@ -3206,7 +3206,7 @@ do_option_warnings(struct context *c)
> > > }
> > > else
> > > {
> > > -msg(M_WARN, "NOTE: starting with " PACKAGE_NAME " 2.1,
> > '--script-security 2' or higher is required to call user-defined
> scripts
> > or executables");
> > > +msg(M_FATAL, "ERROR: starting with " PACKAGE_NAME "
> 2.1,
> > '--script-security 2' or higher is required to call user-defined
> scripts
> > or executables");
> >
> > Generally speaking, I am fine with this (so Feature-ACK).
> >
> > What I am struggling with though is that this may break existing
> > configurations for users who do have an invalid configuration file.
> In this
> > case trying to use scripts without --script-security *and* ignoring
> that their
> > scripts does not work. The cynical me says "scr** them, they need
> to fix
> > their configs".
> >
> > I fail to get this.. Users cannot ignore it as currently they do get a
> FATAL
> > error in such cases.
> > I'm only moving the error to happen earlier.
> > Am I missing something?
>
> So another M_FATAL occurs later on? I haven't checked the code yet, but I
> was
> quite sure there were scenarios where scripts failed to run - with with
> some
> other complaints in the log.
>
> If all script hooks results in M_FATAL later on (server and client
> configs),
> then this is a non-issue.
>
That got me worried as I had checked this only with up/down scripts on
client
in which case it causes a FATAL error later in openvpn_run_script.
But you are right, not all scripts cause FATAL error when not executed or
failed..
In that case, a better option may be to get a proper error msg -- instead
of "external program fork failed"
we want "script not executed because of script-security < xxx " or
something like that..
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Exit early when external scripts are specified with script-security < 2
From: Selva Nair
Currently this raises a warning only. A fatal error is triggered
later with a confusing message that script failed to execute.
This helps the Windows GUI to show a relevant error message when
script-security is over-ridden as a security measure.
Signed-off-by: Selva Nair
---
src/openvpn/init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 3c44967..5863828 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -3206,7 +3206,7 @@ do_option_warnings(struct context *c)
}
else
{
-msg(M_WARN, "NOTE: starting with " PACKAGE_NAME " 2.1,
'--script-security 2' or higher is required to call user-defined scripts or
executables");
+msg(M_FATAL, "ERROR: starting with " PACKAGE_NAME " 2.1,
'--script-security 2' or higher is required to call user-defined scripts or
executables");
}
}
}
--
2.1.4
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] OpenVPN 2.4: crypto api patches for OpenSSL 1.1.1? cryptoapicert
Hi On Thu, Jan 31, 2019 at 11:40 AM Gert Doering wrote: > Hi, > > I have changed the Subject: and started a new thread, so that this > isn't lost in the discussion specific to commit ce1c1beef1eb. > > On Thu, Jan 31, 2019 at 11:28:52AM -0500, Selva Nair wrote: > > So now the question -- do we want to support Windows builds with OpenSSL > > 1.1.1 in 2.4? > > Basically, we already do. > > Could you summarize what is current not working right in 2.4 + 1.1.1, > and which patches we need to make it work? > > I admit I have lost track (many many patches related to TLS 1.3... some > bring new features, some bugfixes) and could imagine everyone else being > in the same boat. Maybe except Arne ;-) > OpenSSL 1.1.1 prefers pss padding even for TLS 1.2 so when both client and server are linked against 1.1.1 and cryptoapicert is in use we need the last two commits: 1. commit 0cab3475a83e9bad35b0eeb39b9ca886e6afaf1e Move OpenSSL vs CNG signature digest type mapping to a function 2. commit ce1c1beef1eb9ea776e00861117f72c4a1a6f1f8 Handle PSS padding in cryptoapicert It may take a little more work than cherry-pick + conflict resolve for these as cryptoapi.c in master has diverged from 2.4 due to EC key support in the former. Apart from that, management-external-[cert|key] requires a way to signal pss or "none" padding -- Arne's patch on that is pending review or revision, I forget. But that's not specific to Windows. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH applied] Re: Handle PSS padding in cryptoapicert
Thanks. So now the question -- do we want to support Windows builds with OpenSSL 1.1.1 in 2.4? Selva On Thu, Jan 31, 2019 at 11:22 AM Gert Doering wrote: > Your patch has been applied to the master branch. > > (Test built on ubuntu 16.04 / mingw, not really tested as such) > > commit ce1c1beef1eb9ea776e00861117f72c4a1a6f1f8 > Author: Selva Nair > Date: Wed Jan 30 10:53:20 2019 -0500 > > Handle PSS padding in cryptoapicert > > Signed-off-by: Selva Nair > Acked-by: Arne Schwabe > Message-Id: <1548863600-491-1-git-send-email-selva.n...@gmail.com> > URL: > https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18188.html > Signed-off-by: Gert Doering > > > -- > kind regards, > > Gert Doering > > ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
