| Message ID | 20230710112122.576-1-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] tun.c: enclose DNS domain in single quotes in WMIC call | expand |
Hi, On Mon, Jul 10, 2023 at 7:22 AM Lev Stipakov <lstipakov@gmail.com> wrote: > From: Lev Stipakov <lev@openvpn.net> > > This is needed to support domains with hyphens. > > Not using double quotes here, since our code replaces > them with underbars (see > https://github.com/OpenVPN/openvpn/blob/master/src/openvpn/win32.c#L980). > > Fixes https://github.com/OpenVPN/openvpn/issues/363 > > Change-Id: Iab536922d0731635cef529b5caf542f637b8d491 > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > src/openvpn/tun.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c > index d1fd6def..60974208 100644 > --- a/src/openvpn/tun.c > +++ b/src/openvpn/tun.c > @@ -333,7 +333,7 @@ do_dns_domain_wmic(bool add, const struct tuntap *tt) > } > > struct argv argv = argv_new(); > - argv_printf(&argv, "%s%s nicconfig where (InterfaceIndex=%ld) call > SetDNSDomain %s", > + argv_printf(&argv, "%s%s nicconfig where (InterfaceIndex=%ld) call > SetDNSDomain '%s'", > get_win_sys_path(), WMIC_PATH_SUFFIX, tt->adapter_index, > add ? tt->options.domain : ""); > exec_command("WMIC", &argv, 1, M_WARN); > Quoting is required as wmic interprets characters such as hyphen and /. Double quotes would have been better (as in interactive.c) as there are some cases where characters within single quotes get interpreted special (like 'foo>bar' vs "foo>bar"). That said, for valid domain names, the only expected characters are alpha-numeric, hyphen and period, and single quotes should work. I have only tested this using wmic command line, not the resulting openvpn.exe. Acked-by: Selva Nair <selva.nair@gmail.com> P.S. We probably need to sanitize the user-supplied domain name before passing it to wmic.
I have not tested this beyond "does it still compile?" (it does) - the
whole concept of "how do quotation marks work in windows, and when and
why do we need them?" is a bit fuzzy to me - but I fully trust Selva
and Lev on this.
On Selva's comment "do we need to sanitize this?" - I tried to figure
out how tt->options.domain is populated, and gave up - for "dhcp-option",
we start with o->domain, for "--dns" we start with something else, and
I couldn't find where this moves over to tt->options.domain, and whether
there is a check at this place.
In the "dhcp-option DOMAIN" block, we just do
if ((streq(p[1], "DOMAIN") || streq(p[1], "ADAPTER_DOMAIN_SUFFIX"))
&& p[2] && !p[3])
{
o->domain = p[2];
.. so indeed, depending on how windows exec_command() "does things",
this might or might not be a problem.
Your patch has been applied to the master and release/2.6 branch (bugfix).
commit 4057814a8a783d4fb1475f49f073f6b3a7797677 (master)
commit aceecaef79cd2dae5265e328874ee8263ac79492 (release/2.6)
Author: Lev Stipakov
Date: Mon Jul 10 14:21:22 2023 +0300
tun.c: enclose DNS domain in single quotes in WMIC call
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20230710112122.576-1-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26841.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index d1fd6def..60974208 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -333,7 +333,7 @@ do_dns_domain_wmic(bool add, const struct tuntap *tt) } struct argv argv = argv_new(); - argv_printf(&argv, "%s%s nicconfig where (InterfaceIndex=%ld) call SetDNSDomain %s", + argv_printf(&argv, "%s%s nicconfig where (InterfaceIndex=%ld) call SetDNSDomain '%s'", get_win_sys_path(), WMIC_PATH_SUFFIX, tt->adapter_index, add ? tt->options.domain : ""); exec_command("WMIC", &argv, 1, M_WARN);