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,
> 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.
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 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. gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ 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, &ack, "ROUTE"))
> > +if (!send_msg_iservice(pipe, rt, size, &ack, 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] openvpnserv: enable interactive service to open tun
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, &ack, "ROUTE")) > +if (!send_msg_iservice(pipe, rt, size, &ack, 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(). Possibly introduce a wrapper for the "standard" case which calls a new function send_msg_iservice_ex() that takes a length field for the return data type. And the open wintun / return handle would then use _ex(). gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] openvpnserv: enable interactive service to open tun
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
---
include/openvpn-msg.h | 12
src/openvpn/route.c | 2 +-
src/openvpn/tun.c | 70 +++
src/openvpn/win32.c | 6 ++--
src/openvpn/win32.h | 6 ++--
src/openvpnserv/interactive.c | 59 +++-
6 files changed, 134 insertions(+), 21 deletions(-)
diff --git a/include/openvpn-msg.h b/include/openvpn-msg.h
index 66177a2..273d9a6 100644
--- a/include/openvpn-msg.h
+++ b/include/openvpn-msg.h
@@ -39,6 +39,8 @@ typedef enum {
msg_del_block_dns,
msg_register_dns,
msg_enable_dhcp,
+msg_open_tun_device,
+msg_open_tun_device_result,
} message_type_t;
typedef struct {
@@ -117,4 +119,14 @@ typedef struct {
interface_t iface;
} enable_dhcp_message_t;
+typedef struct {
+message_header_t header;
+char device_path[512];
+} open_tun_device_message_t;
+
+typedef struct {
+message_header_t header;
+HANDLE handle;
+int error_number;
+} open_tun_device_result_message_t;
#endif /* ifndef OPENVPN_MSG_H_ */
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 4cdc4a9..27fa18c 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2992,7 +2992,7 @@ do_route_service(const bool add, const route_message_t
*rt, const size_t size, H
ack_message_t ack;
struct gc_arena gc = gc_new();
-if (!send_msg_iservice(pipe, rt, size, &ack, "ROUTE"))
+if (!send_msg_iservice(pipe, rt, size, &ack, sizeof(ack), "ROUTE"))
{
goto out;
}
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 8f8f7c6..2d7bd0d 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -115,7 +115,7 @@ do_address_service(const bool add, const short family,
const struct tuntap *tt)
addr.prefix_len = tt->netbits_ipv6;
}
-if (!send_msg_iservice(pipe, &addr, sizeof(addr), &ack, "TUN"))
+if (!send_msg_iservice(pipe, &addr, sizeof(addr), &ack, sizeof(ack),
"TUN"))
{
goto out;
}
@@ -181,7 +181,7 @@ do_dns6_service(bool add, const struct tuntap *tt)
msg(D_LOW, "%s IPv6 dns servers on '%s' (if_index = %d) using service",
(add ? "Setting" : "Deleting"), dns.iface.name, dns.iface.index);
-if (!send_msg_iservice(pipe, &dns, sizeof(dns), &ack, "TUN"))
+if (!send_msg_iservice(pipe, &dns, sizeof(dns), &ack, sizeof(ack), "TUN"))
{
goto out;
}
@@ -5227,7 +5227,7 @@ service_enable_dhcp(const struct tuntap *tt)
.iface = { .index = tt->adapter_index, .name = "" }
};
-if (!send_msg_iservice(pipe, &dhcp, sizeof(dhcp), &ack, "Enable_dhcp"))
+if (!send_msg_iservice(pipe, &dhcp, sizeof(dhcp), &ack, sizeof(ack),
"Enable_dhcp"))
{
goto out;
}
@@ -5248,6 +5248,43 @@ out:
return ret;
}
+static HANDLE
+service_open_tun_device(const HANDLE pipe, const char* device_path)
+{
+open_tun_device_result_message_t result_msg;
+struct gc_arena gc = gc_new();
+open_tun_device_message_t open_tun_device = {
+.header = {
+msg_open_tun_device,
+sizeof(open_tun_device_message_t),
+0
+}
+};
+result_msg.handle = INVALID_HANDLE_VALUE;
+
+strncpynt(open_tun_device.device_path, device_path,
sizeof(open_tun_device.device_path));
+
+if (!send_msg_iservice(pipe, &open_tun_device, sizeof(open_tun_device),
+&result_msg, sizeof(result_msg), "Open_tun_device"))
+{
+goto out;
+}
+
+if (result_msg.error_number != NO_ERROR)
+{
+msg(D_TUNTAP_INFO, "TUN: opening tun handle using service failed: %s
[status=%u device_path=%s]",
+strerror_win32(result_msg.error_number, &gc),
result_msg.error_number, device_path);
+}
+else
+{
+msg(M_INFO, "Opened tun device %s using service", device_path);
+}
+
+out:
+gc_free(&gc);
+return result_msg.handle;
+}
+
/*
* Return a TAP name for netsh commands.
*/
@@ -5469,7 +5506,7 @@ register_dns_service(const struct tuntap *tt)
message_header_t rdns = { msg_register_dns, sizeof(message_header_t), 0 };
-if (!send_msg_iservice(msg_channel, &rdns, sizeof(rdns), &ack,
"Register_dns"))
+if (!send_msg_iservice(msg_channel, &rdns, sizeof(rdns), &ack,
sizeof(ack), "Register_dns"))
{
gc_free(&gc);
return;
@@ -5631,15 +5668,22 @@ open_tun(const char *dev, const char *dev_type, const
char *dev_node, struct tun
device_guid,
TAP_WIN_SUFFIX);
-tt->ha
