openvpn-devel Mailing List for OpenVPN

Attention is currently required from: flichtenheld, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/1336?usp=email to look at the new patch set (#2). Change subject: iservice: make sure directories have trailing \ ...................................................................... iservice: make sure directories have trailing \ At least in the case of the config dir this matters, since the value is used to validate input data for the interactive service. A missing \ at the end would allow a string compare to succeed, if the last element of the path to compare starts with the same substring. The trailing slash ensures that the last element of a path must match completely. Change-Id: If28e66fcc3493821f78fd14d432b22b996918e8f Signed-off-by: Heiko Hund <he...@is...> --- M src/openvpnserv/common.c 1 file changed, 33 insertions(+), 6 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/36/1336/2 diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c index d25d9c0..4202fb0 100644 --- a/src/openvpnserv/common.c +++ b/src/openvpnserv/common.c @@ -54,6 +54,33 @@ } +/** + * Make sure that a dir path ends with a backslash. + * If it doesn't, a \ is added to the end of the path, if there's room in the buffer. + * + * @param dir pointer to the wide dir path string buffer + * @param size maximum number of wide chars the dir path buffer + * @return BOOL to indicate success or failure + */ +static BOOL +ensure_trailing_backslash(PWSTR dir, size_t size) +{ + size_t len = wcslen(dir); + + if (dir[len - 1] != '\\') + { + if (len + 1 > size) + { + return FALSE; + } + dir[len] = '\\'; + dir[len + 1] = '\0'; + } + + return TRUE; +} + + DWORD GetOpenvpnSettings(settings_t *s) { @@ -90,16 +117,16 @@ goto out; } - swprintf(default_value, _countof(default_value), L"%ls\\config", install_path); + swprintf(default_value, _countof(default_value), L"%ls\\config\\", install_path); error = GetRegString(key, L"config_dir", s->config_dir, sizeof(s->config_dir), default_value); - if (error != ERROR_SUCCESS) + if (error != ERROR_SUCCESS || !ensure_trailing_backslash(s->config_dir, _countof(s->config_dir))) { goto out; } - swprintf(default_value, _countof(default_value), L"%ls\\bin", install_path); + swprintf(default_value, _countof(default_value), L"%ls\\bin\\", install_path); error = GetRegString(key, L"bin_dir", s->bin_dir, sizeof(s->bin_dir), default_value); - if (error != ERROR_SUCCESS) + if (error != ERROR_SUCCESS || !ensure_trailing_backslash(s->bin_dir, _countof(s->bin_dir))) { goto out; } @@ -110,9 +137,9 @@ goto out; } - swprintf(default_value, _countof(default_value), L"%ls\\log", install_path); + swprintf(default_value, _countof(default_value), L"%ls\\log\\", install_path); error = GetRegString(key, L"log_dir", s->log_dir, sizeof(s->log_dir), default_value); - if (error != ERROR_SUCCESS) + if (error != ERROR_SUCCESS || !ensure_trailing_backslash(s->log_dir, _countof(s->log_dir))) { goto out; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1336?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: If28e66fcc3493821f78fd14d432b22b996918e8f Gerrit-Change-Number: 1336 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk <he...@op...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> 

Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/1337?usp=email to review the following change. Change subject: iservice: use interface index with netsh ...................................................................... iservice: use interface index with netsh We use the interface index with netsh everywhere else, so convert the remaining invocations of netsh to index use as well. Change-Id: I5cf45cfe0567da8fb5d47118a432a35b358f3809 Signed-off-by: Heiko Hund <he...@is...> --- M src/openvpnserv/interactive.c 1 file changed, 50 insertions(+), 57 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/37/1337/1 diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index c12d34f..b35005c 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -1035,16 +1035,16 @@ } /** - * Run the command: netsh interface $proto $action dns $if_name $addr [validate=no] + * Run the command: netsh interface $proto $action dns $if_index $addr [validate=no] * @param action "delete" or "add" * @param proto "ipv6" or "ip" - * @param if_name "name_of_interface" + * @param if_index index of the interface to modify DNS for * @param addr IPv4 (for proto = ip) or IPv6 address as a string * * If addr is null and action = "delete" all addresses are deleted. */ static DWORD -netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_name, const wchar_t *addr) +netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, int if_index, const wchar_t *addr) { DWORD err = 0; int timeout = 30000; /* in msec */ @@ -1069,10 +1069,10 @@ /* cmd template: * netsh interface $proto $action dns $if_name $addr [validate=no] */ - const wchar_t *fmt = L"netsh interface %ls %ls dns \"%ls\" %ls"; + const wchar_t *fmt = L"netsh interface %ls %ls dns %d %ls"; /* max cmdline length in wchars -- include room for worst case and some */ - size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1; + size_t ncmdline = wcslen(fmt) + 11 /*if_index*/ + wcslen(addr) + 32 + 1; cmdline = malloc(ncmdline*sizeof(wchar_t)); if (!cmdline) { @@ -1080,7 +1080,7 @@ goto out; } - openvpn_swprintf(cmdline, ncmdline, fmt, proto, action, if_name, addr); + openvpn_swprintf(cmdline, ncmdline, fmt, proto, action, if_index, addr); if (IsWindows7OrGreater()) { @@ -1094,16 +1094,16 @@ } /** - * Run the command: netsh interface ip $action wins $if_name [static] $addr + * Run the command: netsh interface ip $action wins $if_index [static] $addr * @param action "delete", "add" or "set" - * @param if_name "name_of_interface" + * @param if_index index of the interface to modify WINS for * @param addr IPv4 address as a string * * If addr is null and action = "delete" all addresses are deleted. * if action = "set" then "static" is added before $addr */ static DWORD -netsh_wins_cmd(const wchar_t *action, const wchar_t *if_name, const wchar_t *addr) +netsh_wins_cmd(const wchar_t *action, int if_index, const wchar_t *addr) { DWORD err = 0; int timeout = 30000; /* in msec */ @@ -1129,11 +1129,11 @@ /* cmd template: * netsh interface ip $action wins $if_name $static $addr */ - const wchar_t *fmt = L"netsh interface ip %ls wins \"%ls\" %ls %ls"; + const wchar_t *fmt = L"netsh interface ip %ls wins %d %ls %ls"; /* max cmdline length in wchars -- include room for worst case and some */ - size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(action) + wcslen(addr) - +wcslen(addr_static) + 32 + 1; + size_t ncmdline = wcslen(fmt) + 11 /*if_index*/ + wcslen(action) + wcslen(addr) + + wcslen(addr_static) + 32 + 1; cmdline = malloc(ncmdline * sizeof(wchar_t)); if (!cmdline) { @@ -1141,7 +1141,7 @@ goto out; } - openvpn_swprintf(cmdline, ncmdline, fmt, action, if_name, addr_static, addr); + openvpn_swprintf(cmdline, ncmdline, fmt, action, if_index, addr_static, addr); err = ExecCommand(argv0, cmdline, timeout); @@ -1184,18 +1184,18 @@ /* Delete all IPv4 or IPv6 dns servers for an interface */ static DWORD -DeleteDNS(short family, wchar_t *if_name) +DeleteDNS(short family, int if_index) { wchar_t *proto = (family == AF_INET6) ? L"ipv6" : L"ip"; - return netsh_dns_cmd(L"delete", proto, if_name, NULL); + return netsh_dns_cmd(L"delete", proto, if_index, NULL); } /* Add an IPv4 or IPv6 dns server to an interface */ static DWORD -AddDNS(short family, wchar_t *if_name, wchar_t *addr) +AddDNS(short family, int if_index, wchar_t *addr) { wchar_t *proto = (family == AF_INET6) ? L"ipv6" : L"ip"; - return netsh_dns_cmd(L"add", proto, if_name, addr); + return netsh_dns_cmd(L"add", proto, if_index, addr); } static BOOL @@ -1295,12 +1295,12 @@ */ if (addr_len > 0 || msg->header.type == msg_del_dns_cfg) { - err = DeleteDNS(msg->family, wide_name); + err = DeleteDNS(msg->family, msg->iface.index); if (err) { goto out; } - free(RemoveListItem(&(*lists)[undo_type], CmpWString, wide_name)); + free(RemoveListItem(&(*lists)[undo_type], CmpAny, NULL)); } if (msg->header.type == msg_del_dns_cfg) @@ -1323,7 +1323,7 @@ { RtlIpv4AddressToStringW(&msg->addr[i].ipv4, addr); } - err = AddDNS(msg->family, wide_name, addr); + err = AddDNS(msg->family, msg->iface.index, addr); if (i == 0 && err) { goto out; @@ -1337,11 +1337,15 @@ if (msg->addr_len > 0) { - wchar_t *tmp_name = _wcsdup(wide_name); - if (!tmp_name || AddListItem(&(*lists)[undo_type], tmp_name)) + int *tmp_index = malloc(sizeof(msg->iface.index)); + if (tmp_index) { - free(tmp_name); - DeleteDNS(msg->family, wide_name); + *tmp_index = msg->iface.index; + } + if (!tmp_index || AddListItem(&(*lists)[undo_type], tmp_index)) + { + free(tmp_index); + DeleteDNS(msg->family, msg->iface.index); err = ERROR_OUTOFMEMORY; goto out; } @@ -1360,7 +1364,7 @@ static DWORD HandleWINSConfigMessage(const wins_cfg_message_t *msg, undo_lists_t *lists) { - DWORD err = 0; + DWORD err = NO_ERROR; wchar_t addr[16]; /* large enough to hold string representation of an ipv4 */ int addr_len = msg->addr_len; @@ -1370,37 +1374,25 @@ addr_len = _countof(msg->addr); } - if (!msg->iface.name[0]) /* interface name is required */ + if (!msg->iface.index) /* interface index is required */ { return ERROR_MESSAGE_DATA; } - /* use a non-const reference with limited scope to enforce null-termination of strings from client */ - { - wins_cfg_message_t *msgptr = (wins_cfg_message_t *)msg; - msgptr->iface.name[_countof(msg->iface.name) - 1] = '\0'; - } - - wchar_t *wide_name = utf8to16(msg->iface.name); /* utf8 to wide-char */ - if (!wide_name) - { - return ERROR_OUTOFMEMORY; - } - /* We delete all current addresses before adding any * OR if the message type is del_wins_cfg */ if (addr_len > 0 || msg->header.type == msg_del_wins_cfg) { - err = netsh_wins_cmd(L"delete", wide_name, NULL); + err = netsh_wins_cmd(L"delete", msg->iface.index, NULL); if (err) { goto out; } - free(RemoveListItem(&(*lists)[undo_wins], CmpWString, wide_name)); + free(RemoveListItem(&(*lists)[undo_wins], CmpAny, NULL)); } - if (msg->header.type == msg_del_wins_cfg) + if (addr_len == 0 || msg->header.type == msg_del_wins_cfg) { goto out; /* job done */ } @@ -1408,7 +1400,7 @@ for (int i = 0; i < addr_len; ++i) { RtlIpv4AddressToStringW(&msg->addr[i].ipv4, addr); - err = netsh_wins_cmd(i == 0 ? L"set" : L"add", wide_name, addr); + err = netsh_wins_cmd(i == 0 ? L"set" : L"add", msg->iface.index, addr); if (i == 0 && err) { goto out; @@ -1418,22 +1410,23 @@ */ } - err = 0; - - if (addr_len > 0) + int *if_index = malloc(sizeof(msg->iface.index)); + if (if_index) { - wchar_t *tmp_name = _wcsdup(wide_name); - if (!tmp_name || AddListItem(&(*lists)[undo_wins], tmp_name)) - { - free(tmp_name); - netsh_wins_cmd(L"delete", wide_name, NULL); - err = ERROR_OUTOFMEMORY; - goto out; - } + *if_index = msg->iface.index; } + if (!if_index || AddListItem(&(*lists)[undo_wins], if_index)) + { + free(if_index); + netsh_wins_cmd(L"delete", msg->iface.index, NULL); + err = ERROR_OUTOFMEMORY; + goto out; + } + + err = 0; + out: - free(wide_name); return err; } @@ -1734,15 +1727,15 @@ break; case undo_dns4: - DeleteDNS(AF_INET, item->data); + DeleteDNS(AF_INET, *(int *)item->data); break; case undo_dns6: - DeleteDNS(AF_INET6, item->data); + DeleteDNS(AF_INET6, *(int *)item->data); break; case undo_wins: - netsh_wins_cmd(L"delete", item->data, NULL); + netsh_wins_cmd(L"delete", *(int *)item->data, NULL); break; case undo_domain: -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1337?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: openvpn Gerrit-Branch: release/2.6 Gerrit-Change-Id: I5cf45cfe0567da8fb5d47118a432a35b358f3809 Gerrit-Change-Number: 1337 Gerrit-PatchSet: 1 Gerrit-Owner: d12fk <he...@op...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> 

Attention is currently required from: cron2, flichtenheld, plaisthos, selvanair. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better ...................................................................... Patch Set 2: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/394d39d7_9ef2361a?usp=email : PS2, Line 80: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > Okay, I think optionally adding a \ when settings_t. […] #1336 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: selvanair <sel...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: selvanair <sel...@gm...> Gerrit-Comment-Date: Fri, 31 Oct 2025 19:36:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk <he...@op...> Comment-In-Reply-To: selvanair <sel...@gm...> 

Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/1336?usp=email to review the following change. Change subject: iservice: make sure directories have trailing \ ...................................................................... iservice: make sure directories have trailing \ At least in the case of the config dir this matters, since the value is used to validate input data for the interactive service. A missing \ at the end would allow a string compare to succeed, if the last element of the path to compare starts with the same substring. The trailing slash ensures that the last element of a path must match completely. Change-Id: If28e66fcc3493821f78fd14d432b22b996918e8f Signed-off-by: Heiko Hund <he...@is...> --- M src/openvpnserv/common.c 1 file changed, 33 insertions(+), 6 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/36/1336/1 diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c index d25d9c0..969b8ac 100644 --- a/src/openvpnserv/common.c +++ b/src/openvpnserv/common.c @@ -54,6 +54,33 @@ } +/** + * Make sure that a dir path ends with a backslash. + * If it doesn't, a \ is added to the end of the path, if there's room in the buffer. + * + * @param dir pointer to the wide dir path string buffer + * @param size maximum number of wide chars the dir path buffer + * @return BOOL to indicate success or failure + */ +static BOOL +ensure_trailing_backslash(WSTR dir, size_t size) +{ + size_t len = wcslen(dir); + + if (dir[len - 1] != '\\') + { + if (len + 1 > size) + { + return FALSE; + } + dir[len] = '\\'; + dir[len + 1] = '\0'; + } + + return TRUE; +} + + DWORD GetOpenvpnSettings(settings_t *s) { @@ -90,16 +117,16 @@ goto out; } - swprintf(default_value, _countof(default_value), L"%ls\\config", install_path); + swprintf(default_value, _countof(default_value), L"%ls\\config\\", install_path); error = GetRegString(key, L"config_dir", s->config_dir, sizeof(s->config_dir), default_value); - if (error != ERROR_SUCCESS) + if (error != ERROR_SUCCESS || !ensure_trailing_backslash(s->config_dir, _countof(s->config_dir))) { goto out; } - swprintf(default_value, _countof(default_value), L"%ls\\bin", install_path); + swprintf(default_value, _countof(default_value), L"%ls\\bin\\", install_path); error = GetRegString(key, L"bin_dir", s->bin_dir, sizeof(s->bin_dir), default_value); - if (error != ERROR_SUCCESS) + if (error != ERROR_SUCCESS || !ensure_trailing_backslash(s->bin_dir, _countof(s->bin_dir))) { goto out; } @@ -110,9 +137,9 @@ goto out; } - swprintf(default_value, _countof(default_value), L"%ls\\log", install_path); + swprintf(default_value, _countof(default_value), L"%ls\\log\\", install_path); error = GetRegString(key, L"log_dir", s->log_dir, sizeof(s->log_dir), default_value); - if (error != ERROR_SUCCESS) + if (error != ERROR_SUCCESS || !ensure_trailing_backslash(s->log_dir, _countof(s->log_dir))) { goto out; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1336?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: If28e66fcc3493821f78fd14d432b22b996918e8f Gerrit-Change-Number: 1336 Gerrit-PatchSet: 1 Gerrit-Owner: d12fk <he...@op...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> 

Attention is currently required from: cron2, flichtenheld, plaisthos, selvanair. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better ...................................................................... Patch Set 2: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/af7d5233_8513dfb0?usp=email : PS2, Line 80: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > https://github. […] Okay, I think optionally adding a \ when settings_t.config_dir is set makes more sense, as it would apply to any other (future) use as well then. Will add a commit dealing with that. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: selvanair <sel...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: selvanair <sel...@gm...> Gerrit-Comment-Date: Fri, 31 Oct 2025 18:41:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk <he...@op...> Comment-In-Reply-To: selvanair <sel...@gm...> 

Attention is currently required from: cron2, d12fk, flichtenheld, plaisthos. selvanair has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better ...................................................................... Patch Set 2: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/3e51da9a_275be521?usp=email : PS2, Line 80: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > Which github issue # is this? https://github.com/OpenVPN/openvpn-private-issues/issues/22 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: selvanair <sel...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: d12fk <he...@op...> Gerrit-Comment-Date: Fri, 31 Oct 2025 18:27:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: d12fk <he...@op...> Comment-In-Reply-To: selvanair <sel...@gm...> 

Attention is currently required from: cron2, flichtenheld, plaisthos, selvanair. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better ...................................................................... Patch Set 2: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/fe993544_9469c998?usp=email : PS2, Line 80: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; > hmm. This misses the purpose of the previous patch -- i.e. […] Which github issue # is this? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: selvanair <sel...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: selvanair <sel...@gm...> Gerrit-Comment-Date: Fri, 31 Oct 2025 18:19:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: selvanair <sel...@gm...> 

Attention is currently required from: cron2, d12fk, flichtenheld, plaisthos. selvanair has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better ...................................................................... Patch Set 2: (1 comment) File src/openvpnserv/validate.c: http://gerrit.openvpn.net/c/openvpn/+/1307/comment/65edbdf0_59c409fa?usp=email : PS2, Line 80: return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; hmm. This misses the purpose of the previous patch -- i.e., to ensure that config_dir has a trailing slash which was the source of the zeropath report. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: selvanair <sel...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: d12fk <he...@op...> Gerrit-Comment-Date: Fri, 31 Oct 2025 18:11:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 

Attention is currently required from: d12fk, flichtenheld, plaisthos. Hello cron2, flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email to look at the new patch set (#2). Change subject: iservice: validate config path better ...................................................................... iservice: validate config path better Instead of just rejecting any path that contains ".." use some WIN32 API functions to combine, canonicalize and then check if the resulting path is located under the config directory. Makes the code prettier and more correct. Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Signed-off-by: Heiko Hund <he...@is...> --- M src/openvpnserv/validate.c 1 file changed, 9 insertions(+), 22 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/07/1307/2 diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c index 2187fb5..80ccb67 100644 --- a/src/openvpnserv/validate.c +++ b/src/openvpnserv/validate.c @@ -24,8 +24,8 @@ #include <lmaccess.h> #include <shlwapi.h> -#include <lm.h> #include <pathcch.h> +#include <lm.h> #ifndef HAVE_PATHCCH_ENSURE_TRAILING_SLASH #define PATHCCH_ENSURE_TRAILING_SLASH 0x20 @@ -58,39 +58,26 @@ static PTOKEN_GROUPS GetTokenGroups(const HANDLE token); /* - * Check workdir\fname is inside config_dir - * The logic here is simple: we may reject some valid paths if ..\ is in any of the strings + * Check that config path is inside config_dir + * The logic here is simple: if the path isn't prefixed with config_dir it's rejected */ static BOOL CheckConfigPath(const WCHAR *workdir, const WCHAR *fname, const settings_t *s) { - WCHAR tmp[MAX_PATH]; - const WCHAR *config_file = NULL; - WCHAR config_dir[MAX_PATH]; + HRESULT res; + WCHAR config_path[MAX_PATH]; - /* convert fname to full path */ + /* convert fname to full canonical path */ if (PathIsRelativeW(fname)) { - swprintf(tmp, _countof(tmp), L"%ls\\%ls", workdir, fname); - config_file = tmp; + res = PathCchCombine(config_path, sizeof(config_path), workdir, fname); } else { - config_file = fname; + res = PathCchCanonicalize(config_path, sizeof(config_path), fname); } - /* canonicalize config_dir and add trailing slash before comparison */ - HRESULT res = PathCchCanonicalizeEx(config_dir, _countof(config_dir), s->config_dir, - PATHCCH_ENSURE_TRAILING_SLASH); - - if (res == S_OK - && wcsncmp(config_dir, config_file, wcslen(config_dir)) == 0 - && wcsstr(config_file + wcslen(config_dir), L"..") == NULL) - { - return TRUE; - } - - return FALSE; + return res == S_OK && wcsncmp(config_path, s->config_dir, wcslen(s->config_dir)) == 0; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 2 Gerrit-Owner: d12fk <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: d12fk <he...@op...> 

Attention is currently required from: cron2, flichtenheld, plaisthos. d12fk has posted comments on this change by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email ) Change subject: iservice: validate config path better ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > This conflicted with Selva working on the same function, and I saw the other one first :-) - so we h […] Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1307?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0e94068f467f2899daf133b032a785d2d7fc05e4 Gerrit-Change-Number: 1307 Gerrit-PatchSet: 1 Gerrit-Owner: d12fk <he...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Fri, 31 Oct 2025 17:37:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> 

Attention is currently required from: flichtenheld, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/1335?usp=email to look at the new patch set (#2). Change subject: tapctl: overhaul adapter management helpers and improve CLI UX ...................................................................... tapctl: overhaul adapter management helpers and improve CLI UX - default to creating ovpn-dco adapters, refresh help text, validate CLI input, and print both GUID and name after creation; - refactor create/list/delete into dedicated helpers, add bulk deletion via delete --hwid, and centralize GUID/name resolution; - harden auto-naming to recognise both ovpn-dco and tap0901 HWIDs, honour reserved names, and reuse friendly names when deleting by GUID. Change-Id: I4d822bf7ae5f037e2414778e8fa74eca5e289e65 Signed-off-by: Lev Stipakov <le...@op...> --- M src/tapctl/main.c 1 file changed, 601 insertions(+), 243 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/35/1335/2 diff --git a/src/tapctl/main.c b/src/tapctl/main.c index 031e262..4a1aff5 100644 --- a/src/tapctl/main.c +++ b/src/tapctl/main.c @@ -1,5 +1,5 @@ /* - * tapctl -- Utility to manipulate TUN/TAP adapters on Windows + * tapctl -- Utility to manipulate virtual network adapters on Windows * https://community.openvpn.net/openvpn/wiki/Tapctl * * Copyright (C) 2002-2025 OpenVPN Inc <sa...@op...> @@ -28,6 +28,7 @@ #include <objbase.h> #include <setupapi.h> #include <stdio.h> +#include <stdlib.h> #include <wchar.h> #ifdef _MSC_VER @@ -50,8 +51,8 @@ L"\n" L"Commands:\n" L"\n" - L"create Create a new TUN/TAP adapter\n" - L"list List TUN/TAP adapters\n" + L"create Create a new network adapter\n" + L"list List network adapters\n" L"delete Delete specified network adapter\n" L"help Display this text\n" L"\n" @@ -61,7 +62,7 @@ static const WCHAR usage_message_create[] = L"%ls\n" L"\n" - L"Creates a new TUN/TAP adapter\n" + L"Creates a new network adapter\n" L"\n" L"Usage:\n" L"\n" @@ -69,24 +70,24 @@ L"\n" L"Options:\n" L"\n" - L"--name <name> Set TUN/TAP adapter name. Should the adapter with given name \n" + L"--name <name> Set network adapter name. Should the adapter with given name \n" L" already exist, an error is returned. If this option is not \n" L" specified, a default adapter name is chosen by Windows. \n" L" Note: This name can also be specified as OpenVPN's --dev-node \n" L" option. \n" - L"--hwid <hwid> Adapter hardware ID. Default value is root\\tap0901, which \n" - L" describes tap-windows6 driver. To work with ovpn-dco driver, \n" - L" driver, specify 'ovpn-dco'. \n" + L"--hwid <hwid> Adapter hardware ID. Default value is ovpn-dco, which creates \n" + L" a Data Channel Offload adapter. To create a tap-windows6 \n" + L" adapter, specify 'root\\tap0901'. \n" L"\n" L"Output:\n" L"\n" - L"This command prints newly created TUN/TAP adapter's GUID to stdout. \n" + L"This command prints newly created network adapter's GUID to stdout. \n" ; static const WCHAR usage_message_list[] = L"%ls\n" L"\n" - L"Lists TUN/TAP adapters\n" + L"Lists network adapters\n" L"\n" L"Usage:\n" L"\n" @@ -94,22 +95,28 @@ L"\n" L"Options:\n" L"\n" - L"--hwid <hwid> Adapter hardware ID. By default, root\\tap0901, tap0901 and \n" - L" ovpn-dco adapters are listed. Use this switch to limit the list.\n" + L"--hwid <hwid> Adapter hardware ID. By default, ovpn-dco, root\\tap0901 and \n" + L" tap0901 adapters are listed. Use this switch to limit the list.\n" L"\n" L"Output:\n" L"\n" - L"This command prints all TUN/TAP adapters to stdout. \n" + L"This command prints all network adapters to stdout. \n" ; static const WCHAR usage_message_delete[] = L"%ls\n" L"\n" - L"Deletes the specified network adapter\n" + L"Deletes network adapters\n" L"\n" L"Usage:\n" L"\n" L"tapctl delete <adapter GUID | adapter name>\n" + L"tapctl delete --hwid <hardware ID>\n" + L"\n" + L"Options:\n" + L"\n" + L"--hwid <hwid> Delete all adapters that use the specified hardware ID. Cannot\n" + L" be combined with adapter GUID or name.\n" ; /* clang-format on */ @@ -123,84 +130,633 @@ fwprintf(stderr, usage_message, title_string); } +static int tapctl_cmd_create(int argc, LPCWSTR argv[], BOOL *pbRebootRequired); +static int tapctl_cmd_list(int argc, LPCWSTR argv[]); +static int tapctl_cmd_delete(int argc, LPCWSTR argv[], BOOL *pbRebootRequired); + /** - * Checks if adapter with given name doesn't already exist + * Locate an adapter node by its friendly name within the enumerated list. + * + * @param name Friendly name to search for. Comparison is case-insensitive. + * @param adapter_list Head of the adapter list returned by tap_list_adapters(). + * + * @return Pointer to the matching node, or NULL when not found. */ -static BOOL -is_adapter_name_available(LPCWSTR name, struct tap_adapter_node *adapter_list, BOOL log) +static struct tap_adapter_node * +find_adapter_by_name(LPCWSTR name, struct tap_adapter_node *adapter_list) { for (struct tap_adapter_node *a = adapter_list; a; a = a->pNext) { - if (wcsicmp(name, a->szName) == 0) + if (_wcsicmp(name, a->szName) == 0) { - if (log) - { - LPOLESTR adapter_id = NULL; - StringFromIID((REFIID)&a->guid, &adapter_id); - fwprintf(stderr, - L"Adapter \"%ls\" already exists (GUID %" - L"ls).\n", - a->szName, adapter_id); - CoTaskMemFree(adapter_id); - } - - return FALSE; + return a; } } - return TRUE; + return NULL; } /** - * Returns unique adapter name based on hwid or NULL if name cannot be generated. - * Caller is responsible for freeing it. + * Check whether the registry still reserves a given network-connection name. + * + * Windows keeps friendly names under + * \\HKLM\\SYSTEM\\CurrentControlSet\\Control\\Network\\{NETCLASS}\\{GUID}\\Connection\\Name, + * even after an adapter is removed. netsh refuses to rename to any reserved name. + * + * @param name Friendly name to test. + * + * @return TRUE if the name exists in the registry, FALSE otherwise. + */ +static BOOL +registry_name_exists(LPCWSTR name) +{ + static const WCHAR class_key[] = + L"SYSTEM\\CurrentControlSet\\Control\\Network\\{4d36e972-e325-11ce-bfc1-08002be10318}"; + + HKEY hClassKey = NULL; + if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, class_key, 0, KEY_READ, &hClassKey) != ERROR_SUCCESS) + { + return FALSE; + } + + BOOL found = FALSE; + + for (DWORD index = 0;; ++index) + { + WCHAR adapter_id[64]; + DWORD adapter_id_len = _countof(adapter_id); + LONG result = RegEnumKeyEx(hClassKey, index, adapter_id, &adapter_id_len, NULL, NULL, NULL, + NULL); + if (result == ERROR_NO_MORE_ITEMS) + { + break; + } + else if (result != ERROR_SUCCESS) + { + continue; + } + + WCHAR connection_key[512]; + swprintf_s(connection_key, _countof(connection_key), L"%ls\\%ls\\Connection", class_key, + adapter_id); + + DWORD value_size = 0; + LONG query = RegGetValueW(HKEY_LOCAL_MACHINE, connection_key, L"Name", + RRF_RT_REG_SZ | RRF_NOEXPAND, NULL, NULL, &value_size); + if (query != ERROR_SUCCESS || value_size < sizeof(WCHAR)) + { + continue; + } + + LPWSTR value = (LPWSTR)malloc(value_size); + if (!value) + { + continue; + } + + query = RegGetValueW(HKEY_LOCAL_MACHINE, connection_key, L"Name", + RRF_RT_REG_SZ | RRF_NOEXPAND, NULL, value, &value_size); + if (query == ERROR_SUCCESS && _wcsicmp(name, value) == 0) + { + found = TRUE; + free(value); + break; + } + + free(value); + + if (found) + { + break; + } + } + + RegCloseKey(hClassKey); + return found; +} + +/** + * Determine whether a friendly name is currently in use by an adapter or reserved + * in the registry. + * + * @param name Friendly name to test. + * @param adapter_list Head of the adapter list returned by tap_list_adapters(). + * + * @return TRUE when the name is taken/reserved, FALSE when available. + */ +static BOOL +tap_name_in_use(LPCWSTR name, struct tap_adapter_node *adapter_list) +{ + if (name == NULL) + { + return FALSE; + } + + if (find_adapter_by_name(name, adapter_list)) + { + return TRUE; + } + + return registry_name_exists(name); +} + +/** + * Resolve the adapter name we should apply: + * - For user-specified names, ensure they are unique both in the adapter list and + * in the registry. On conflict, an explanatory message is printed and NULL is returned. + * - For automatic naming, derive the base string from HWID and append the first available + * suffix recognised by Windows. + * + * @param requested_name Name provided via CLI or configuration (may be NULL/empty). + * @param hwid Hardware identifier of the adapter being created. + * @param adapter_list Existing adapters enumerated via tap_list_adapters(). + * + * @return Newly allocated wide string containing the final name, or NULL on failure. */ static LPWSTR -get_unique_adapter_name(LPCWSTR hwid, struct tap_adapter_node *adapter_list) +tap_resolve_adapter_name(LPCWSTR requested_name, LPCWSTR hwid, + struct tap_adapter_node *adapter_list) { + if (requested_name && requested_name[0]) + { + struct tap_adapter_node *conflict = find_adapter_by_name(requested_name, adapter_list); + if (conflict) + { + LPOLESTR adapter_id = NULL; + StringFromIID((REFIID)&conflict->guid, &adapter_id); + fwprintf(stderr, + L"Adapter \"%ls\" already exists (GUID %" + L"ls).\n", + conflict->szName, adapter_id); + CoTaskMemFree(adapter_id); + return NULL; + } + + if (registry_name_exists(requested_name)) + { + fwprintf(stderr, L"Adapter name \"%ls\" is already in use.\n", requested_name); + return NULL; + } + + return wcsdup(requested_name); + } + if (hwid == NULL) { return NULL; } - LPCWSTR base_name; - if (wcsicmp(hwid, L"ovpn-dco") == 0) + LPCWSTR base_name = NULL; + if (_wcsicmp(hwid, L"ovpn-dco") == 0) { base_name = L"OpenVPN Data Channel Offload"; } - else if (wcsicmp(hwid, L"root\\" _L(TAP_WIN_COMPONENT_ID)) == 0) + else if (_wcsicmp(hwid, L"root\\" _L(TAP_WIN_COMPONENT_ID)) == 0 + || _wcsicmp(hwid, _L(TAP_WIN_COMPONENT_ID)) == 0) { base_name = L"OpenVPN TAP-Windows6"; } else { + fwprintf(stderr, + L"Cannot auto-generate adapter name for hardware ID \"%ls\".\n", hwid); return NULL; } - if (is_adapter_name_available(base_name, adapter_list, FALSE)) + if (!tap_name_in_use(base_name, adapter_list)) { return wcsdup(base_name); } size_t name_len = wcslen(base_name) + 10; - LPWSTR name = malloc(name_len * sizeof(WCHAR)); + LPWSTR name = (LPWSTR)malloc(name_len * sizeof(WCHAR)); if (name == NULL) { return NULL; } - for (int i = 1; i < 100; ++i) + + /* Windows never assigns the "#1" suffix, so skip it to avoid netsh failures. */ + for (int i = 2; i < 100; ++i) { swprintf_s(name, name_len, L"%ls #%d", base_name, i); - if (is_adapter_name_available(name, adapter_list, FALSE)) + if (!tap_name_in_use(name, adapter_list)) { return name; } } + free(name); + fwprintf(stderr, L"Unable to find available adapter name based on \"%ls\".\n", base_name); return NULL; } +static int +tapctl_cmd_create(int argc, LPCWSTR argv[], BOOL *pbRebootRequired) +{ + LPCWSTR szName = NULL; + LPCWSTR defaultHwId = L"ovpn-dco"; + LPCWSTR szHwId = defaultHwId; + LPWSTR adapter_name = NULL; + struct tap_adapter_node *pAdapterList = NULL; + GUID guidAdapter; + LPOLESTR szAdapterId = NULL; + DWORD dwResult; + int iResult = 1; + BOOL adapter_created = FALSE; + + for (int i = 2; i < argc; i++) + { + if (wcsicmp(argv[i], L"--name") == 0) + { + if (++i >= argc) + { + fwprintf(stderr, L"--name option requires a value. Ignored.\n"); + break; + } + szName = argv[i]; + if (szName[0] == L'\0') + { + fwprintf(stderr, L"--name option cannot be empty. Ignored.\n"); + szName = NULL; + } + } + else if (wcsicmp(argv[i], L"--hwid") == 0) + { + if (++i >= argc) + { + fwprintf(stderr, + L"--hwid option requires a value. Using default \"%ls\".\n", + defaultHwId); + break; + } + szHwId = argv[i]; + if (szHwId[0] == L'\0') + { + fwprintf(stderr, + L"--hwid option cannot be empty. Using default \"%ls\".\n", + defaultHwId); + szHwId = defaultHwId; + } + } + else + { + fwprintf(stderr, + L"Unknown option \"%ls" + L"\". Please, use \"tapctl help create\" to list supported options. Ignored.\n", + argv[i]); + } + } + + dwResult = tap_create_adapter(NULL, L"Virtual Ethernet", szHwId, pbRebootRequired, + &guidAdapter); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, L"Creating network adapter failed (error 0x%x).\n", dwResult); + goto cleanup; + } + adapter_created = TRUE; + + dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, L"Enumerating adapters failed (error 0x%x).\n", dwResult); + goto cleanup; + } + + adapter_name = tap_resolve_adapter_name(szName, szHwId, pAdapterList); + if (adapter_name == NULL) + { + goto cleanup; + } + + dwResult = tap_set_adapter_name(&guidAdapter, adapter_name, FALSE); + if (dwResult != ERROR_SUCCESS) + { + StringFromIID((REFIID)&guidAdapter, &szAdapterId); + fwprintf(stderr, + L"Renaming network adapter %ls to \"%ls\" failed (error 0x%x).\n", szAdapterId, + adapter_name, dwResult); + CoTaskMemFree(szAdapterId); + goto cleanup; + } + + iResult = 0; + StringFromIID((REFIID)&guidAdapter, &szAdapterId); + fwprintf(stdout, L"%ls\t%ls\n", szAdapterId, + (adapter_name && adapter_name[0]) ? adapter_name : L"(unnamed)"); + CoTaskMemFree(szAdapterId); + +cleanup: + if (pAdapterList) + { + tap_free_adapter_list(pAdapterList); + } + free(adapter_name); + + if (adapter_created && iResult != 0) + { + tap_delete_adapter(NULL, &guidAdapter, pbRebootRequired); + } + + return iResult; +} + +static int +tapctl_cmd_list(int argc, LPCWSTR argv[]) +{ + WCHAR szzHwId[0x100] = L"ovpn-dco\0" + L"root\\" _L(TAP_WIN_COMPONENT_ID) L"\0" _L(TAP_WIN_COMPONENT_ID) L"\0"; + struct tap_adapter_node *pAdapterList = NULL; + DWORD dwResult; + int iResult = 0; + + for (int i = 2; i < argc; i++) + { + if (wcsicmp(argv[i], L"--hwid") == 0) + { + if (++i >= argc) + { + fwprintf(stderr, + L"--hwid option requires a value. Please, use \"tapctl help list\" to " + L"list supported options.\n"); + return 1; + } + memset(szzHwId, 0, sizeof(szzHwId)); + memcpy_s(szzHwId, + sizeof(szzHwId) - 2 * sizeof(WCHAR) /*requires double zero termination*/, + argv[i], wcslen(argv[i]) * sizeof(WCHAR)); + } + else + { + fwprintf(stderr, + L"Unknown option \"%ls" + L"\". Please, use \"tapctl help list\" to list supported options. Ignored.\n", + argv[i]); + } + } + + dwResult = tap_list_adapters(NULL, szzHwId, &pAdapterList); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, L"Enumerating network adapters failed (error 0x%x).\n", dwResult); + return 1; + } + + for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext) + { + LPOLESTR szAdapterId = NULL; + StringFromIID((REFIID)&pAdapter->guid, &szAdapterId); + fwprintf(stdout, L"%ls\t%ls\n", szAdapterId, pAdapter->szName); + CoTaskMemFree(szAdapterId); + } + + tap_free_adapter_list(pAdapterList); + return iResult; +} + +/** + * Delete every adapter matching the provided hardware ID. + * + * @param hwid Hardware ID string (must be non-NULL and non-empty). + * @param pbRebootRequired Pointer to reboot flag propagated from caller. + * + * @return 0 when every adapter was deleted successfully, 1 otherwise. + */ +static int +tapctl_delete_by_hwid(LPCWSTR hwid, BOOL *pbRebootRequired) +{ + if (hwid == NULL || hwid[0] == L'\0') + { + fwprintf(stderr, L"--hwid option cannot be empty.\n"); + return 1; + } + + size_t hw_len = wcslen(hwid); + size_t buf_len = hw_len + 2; + LPWSTR szzHwId = (LPWSTR)calloc(buf_len, sizeof(WCHAR)); + if (szzHwId == NULL) + { + fwprintf(stderr, L"Out of memory.\n"); + return 1; + } + wmemcpy(szzHwId, hwid, hw_len); + + struct tap_adapter_node *pAdapterList = NULL; + DWORD dwResult = tap_list_adapters(NULL, szzHwId, &pAdapterList); + free(szzHwId); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, L"Enumerating network adapters failed (error 0x%x).\n", dwResult); + return 1; + } + + BOOL deleted_any = FALSE; + BOOL delete_error = FALSE; + + for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext) + { + LPOLESTR szAdapterId = NULL; + StringFromIID((REFIID)&pAdapter->guid, &szAdapterId); + + dwResult = tap_delete_adapter(NULL, &pAdapter->guid, pbRebootRequired); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, + L"Deleting adapter \"%ls\" (GUID %ls) failed (error 0x%x).\n", + pAdapter->szName, szAdapterId, dwResult); + delete_error = TRUE; + } + else + { + fwprintf(stdout, L"%ls\t%ls\n", szAdapterId, pAdapter->szName); + deleted_any = TRUE; + } + + CoTaskMemFree(szAdapterId); + } + + tap_free_adapter_list(pAdapterList); + + if (!deleted_any && !delete_error) + { + fwprintf(stderr, L"No adapters with hardware ID \"%ls\" found.\n", hwid); + } + + return delete_error ? 1 : 0; +} + +/** + * Resolve an adapter identifier supplied on the command line. + * + * @param target Adapter GUID or friendly name provided by the user. + * @param pguidAdapter Receives the resolved adapter GUID on success. + * @param pszMatchedName Optional pointer that receives a duplicated adapter name when + * the lookup was done by name. Caller must free() it. + * + * @return 0 on success, non-zero otherwise (an error message is printed on failure). + */ +static int +tapctl_resolve_delete_target(LPCWSTR target, GUID *pguidAdapter, LPWSTR *pszMatchedName) +{ + if (SUCCEEDED(IIDFromString(target, (LPIID)pguidAdapter))) + { + if (pszMatchedName) + { + *pszMatchedName = NULL; + struct tap_adapter_node *pAdapterList = NULL; + if (tap_list_adapters(NULL, NULL, &pAdapterList) == ERROR_SUCCESS) + { + for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; + pAdapter = pAdapter->pNext) + { + if (IsEqualGUID(&pAdapter->guid, pguidAdapter)) + { + LPWSTR duplicated = wcsdup(pAdapter->szName); + if (duplicated == NULL) + { + fwprintf(stderr, L"Out of memory.\n"); + tap_free_adapter_list(pAdapterList); + return 1; + } + *pszMatchedName = duplicated; + break; + } + } + tap_free_adapter_list(pAdapterList); + } + } + return 0; + } + + struct tap_adapter_node *pAdapterList = NULL; + DWORD dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, L"Enumerating network adapters failed (error 0x%x).\n", dwResult); + return 1; + } + + struct tap_adapter_node *match = NULL; + for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext) + { + if (wcsicmp(target, pAdapter->szName) == 0) + { + match = pAdapter; + break; + } + } + + if (match == NULL) + { + fwprintf(stderr, L"\"%ls\" adapter not found.\n", target); + tap_free_adapter_list(pAdapterList); + return 1; + } + + LPWSTR duplicated_name = pszMatchedName ? wcsdup(match->szName) : NULL; + if (pszMatchedName && duplicated_name == NULL) + { + fwprintf(stderr, L"Out of memory.\n"); + tap_free_adapter_list(pAdapterList); + return 1; + } + + memcpy(pguidAdapter, &match->guid, sizeof(GUID)); + tap_free_adapter_list(pAdapterList); + + if (pszMatchedName) + { + *pszMatchedName = duplicated_name; + } + return 0; +} + +static int +tapctl_cmd_delete(int argc, LPCWSTR argv[], BOOL *pbRebootRequired) +{ + LPCWSTR szDeleteTarget = NULL; + LPCWSTR szDeleteHwId = NULL; + + for (int i = 2; i < argc; i++) + { + if (wcsicmp(argv[i], L"--hwid") == 0) + { + if (++i >= argc) + { + fwprintf(stderr, + L"--hwid option requires a value. Please, use \"tapctl help delete\" for " + L"usage info.\n"); + return 1; + } + if (argv[i][0] == L'\0') + { + fwprintf(stderr, + L"--hwid option cannot be empty. Please, use \"tapctl help delete\" for " + L"usage info.\n"); + return 1; + } + szDeleteHwId = argv[i]; + } + else if (szDeleteTarget == NULL) + { + szDeleteTarget = argv[i]; + } + else + { + fwprintf(stderr, + L"Unknown option \"%ls" + L"\". Please, use \"tapctl help delete\" for usage info. Ignored.\n", + argv[i]); + } + } + + if (szDeleteHwId) + { + if (szDeleteTarget) + { + fwprintf(stderr, L"--hwid option cannot be combined with adapter GUID or name.\n"); + return 1; + } + return tapctl_delete_by_hwid(szDeleteHwId, pbRebootRequired); + } + + if (szDeleteTarget == NULL) + { + fwprintf( + stderr, + L"Missing adapter GUID or name. Please, use \"tapctl help delete\" for usage info.\n"); + return 1; + } + + GUID guidAdapter; + LPWSTR matched_name = NULL; + if (tapctl_resolve_delete_target(szDeleteTarget, &guidAdapter, &matched_name) != 0) + { + return 1; + } + + DWORD dwResult = tap_delete_adapter(NULL, &guidAdapter, pbRebootRequired); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, + L"Deleting adapter \"%ls" + L"\" failed (error 0x%x).\n", + szDeleteTarget, dwResult); + free(matched_name); + return 1; + } + + LPOLESTR szAdapterId = NULL; + StringFromIID((REFIID)&guidAdapter, &szAdapterId); + fwprintf(stdout, L"%ls\t%ls\n", szAdapterId, + (matched_name && matched_name[0]) ? matched_name : szDeleteTarget); + CoTaskMemFree(szAdapterId); + free(matched_name); + + return 0; +} + /** * Program entry point */ @@ -248,215 +804,17 @@ } else if (wcsicmp(argv[1], L"create") == 0) { - LPCWSTR szName = NULL; - LPCWSTR szHwId = L"root\\" _L(TAP_WIN_COMPONENT_ID); - - /* Parse options. */ - for (int i = 2; i < argc; i++) - { - if (wcsicmp(argv[i], L"--name") == 0) - { - szName = argv[++i]; - } - else if (wcsicmp(argv[i], L"--hwid") == 0) - { - szHwId = argv[++i]; - } - else - { - fwprintf( - stderr, - L"Unknown option \"%ls" - L"\". Please, use \"tapctl help create\" to list supported options. Ignored.\n", - argv[i]); - } - } - - /* Create TUN/TAP adapter. */ - GUID guidAdapter; - LPOLESTR szAdapterId = NULL; - DWORD dwResult = - tap_create_adapter(NULL, L"Virtual Ethernet", szHwId, &bRebootRequired, &guidAdapter); - if (dwResult != ERROR_SUCCESS) - { - fwprintf(stderr, L"Creating TUN/TAP adapter failed (error 0x%x).\n", dwResult); - iResult = 1; - goto quit; - } - - /* Get existing network adapters. */ - struct tap_adapter_node *pAdapterList = NULL; - dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); - if (dwResult != ERROR_SUCCESS) - { - fwprintf(stderr, L"Enumerating adapters failed (error 0x%x).\n", dwResult); - iResult = 1; - goto create_delete_adapter; - } - - LPWSTR adapter_name = - szName ? wcsdup(szName) : get_unique_adapter_name(szHwId, pAdapterList); - if (adapter_name) - { - /* Check for duplicates when name was specified, - * otherwise get_adapter_default_name() takes care of it */ - if (szName && !is_adapter_name_available(adapter_name, pAdapterList, TRUE)) - { - iResult = 1; - goto create_cleanup_pAdapterList; - } - - /* Rename the adapter. */ - dwResult = tap_set_adapter_name(&guidAdapter, adapter_name, FALSE); - if (dwResult != ERROR_SUCCESS) - { - StringFromIID((REFIID)&guidAdapter, &szAdapterId); - fwprintf(stderr, - L"Renaming TUN/TAP adapter %ls" - L" to \"%ls\" failed (error 0x%x).\n", - szAdapterId, adapter_name, dwResult); - CoTaskMemFree(szAdapterId); - iResult = 1; - goto quit; - } - } - - iResult = 0; - -create_cleanup_pAdapterList: - free(adapter_name); - - tap_free_adapter_list(pAdapterList); - if (iResult) - { - goto create_delete_adapter; - } - - /* Output adapter GUID. */ - StringFromIID((REFIID)&guidAdapter, &szAdapterId); - fwprintf(stdout, L"%ls\n", szAdapterId); - CoTaskMemFree(szAdapterId); - - iResult = 0; - goto quit; - -create_delete_adapter: - tap_delete_adapter(NULL, &guidAdapter, &bRebootRequired); - iResult = 1; + iResult = tapctl_cmd_create(argc, argv, &bRebootRequired); goto quit; } else if (wcsicmp(argv[1], L"list") == 0) { - WCHAR szzHwId[0x100] = - L"root\\" _L(TAP_WIN_COMPONENT_ID) L"\0" _L(TAP_WIN_COMPONENT_ID) L"\0" - L"ovpn-dco\0"; - - /* Parse options. */ - for (int i = 2; i < argc; i++) - { - if (wcsicmp(argv[i], L"--hwid") == 0) - { - memset(szzHwId, 0, sizeof(szzHwId)); - ++i; - memcpy_s(szzHwId, - sizeof(szzHwId) - 2 * sizeof(WCHAR) /*requires double zero termination*/, - argv[i], wcslen(argv[i]) * sizeof(WCHAR)); - } - else - { - fwprintf( - stderr, - L"Unknown option \"%ls" - L"\". Please, use \"tapctl help list\" to list supported options. Ignored.\n", - argv[i]); - } - } - - /* Output list of adapters with given hardware ID. */ - struct tap_adapter_node *pAdapterList = NULL; - DWORD dwResult = tap_list_adapters(NULL, szzHwId, &pAdapterList); - if (dwResult != ERROR_SUCCESS) - { - fwprintf(stderr, L"Enumerating TUN/TAP adapters failed (error 0x%x).\n", dwResult); - iResult = 1; - goto quit; - } - - for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext) - { - LPOLESTR szAdapterId = NULL; - StringFromIID((REFIID)&pAdapter->guid, &szAdapterId); - fwprintf(stdout, - L"%ls\t%" - L"ls\n", - szAdapterId, pAdapter->szName); - CoTaskMemFree(szAdapterId); - } - - iResult = 0; - tap_free_adapter_list(pAdapterList); + iResult = tapctl_cmd_list(argc, argv); + goto quit; } else if (wcsicmp(argv[1], L"delete") == 0) { - if (argc < 3) - { - fwprintf( - stderr, - L"Missing adapter GUID or name. Please, use \"tapctl help delete\" for usage info.\n"); - return 1; - } - - GUID guidAdapter; - if (FAILED(IIDFromString(argv[2], (LPIID)&guidAdapter))) - { - /* The argument failed to covert to GUID. Treat it as the adapter name. */ - struct tap_adapter_node *pAdapterList = NULL; - DWORD dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); - if (dwResult != ERROR_SUCCESS) - { - fwprintf(stderr, L"Enumerating TUN/TAP adapters failed (error 0x%x).\n", dwResult); - iResult = 1; - goto quit; - } - - for (struct tap_adapter_node *pAdapter = pAdapterList;; pAdapter = pAdapter->pNext) - { - if (pAdapter == NULL) - { - fwprintf(stderr, L"\"%ls\" adapter not found.\n", argv[2]); - iResult = 1; - goto delete_cleanup_pAdapterList; - } - else if (wcsicmp(argv[2], pAdapter->szName) == 0) - { - memcpy(&guidAdapter, &pAdapter->guid, sizeof(GUID)); - break; - } - } - - iResult = 0; - -delete_cleanup_pAdapterList: - tap_free_adapter_list(pAdapterList); - if (iResult) - { - goto quit; - } - } - - /* Delete the network adapter. */ - DWORD dwResult = tap_delete_adapter(NULL, &guidAdapter, &bRebootRequired); - if (dwResult != ERROR_SUCCESS) - { - fwprintf(stderr, - L"Deleting adapter \"%ls" - L"\" failed (error 0x%x).\n", - argv[2], dwResult); - iResult = 1; - goto quit; - } - - iResult = 0; + iResult = tapctl_cmd_delete(argc, argv, &bRebootRequired); goto quit; } else -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1335?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I4d822bf7ae5f037e2414778e8fa74eca5e289e65 Gerrit-Change-Number: 1335 Gerrit-PatchSet: 2 Gerrit-Owner: stipa <lst...@gm...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> 

Cc: the list ---------- Forwarded message --------- From: Selva Nair <sel...@gm...> Date: Thu, Oct 30, 2025 at 6:06 PM Subject: Re: [Openvpn-devel] [M] Change in openvpn[master]: openvpnserv: validate openvpn-gui process path To: Lev Stipakov <lst...@gm...> On Thu, Oct 30, 2025, 17:16 Lev Stipakov <lst...@gm...> wrote: > Indeed, this is to allow only programs under openvpn/bin talk to > interactive service. > > Are there any valid use cases where any other processes are supposed > to talk to the service? I can think of a few: - use a powershell script to start openvpn for testing or otherwise - run openvpn from task scheduler - use a third party GUI with the service (like eduVPN) In fact we already use it to start openvpn through the automatic service: though that will continue to work with this patch. Anyway, iircc, it was a deliberate decision to leave interactive service as a generic way for running openvpn without admin access, not just as a helper for OpenVPN-GUI. With this change, anyone wanting to write a new UI leveraging the service will have to fork it, or install their program under OpenVPN\bin. In any case what exactly is achieved by locking down the service this way? It's necessary for the service to start a valid openvpn.exe and we do check that. It's also necessary for the GUI to be sure that it's talking to a legitimate openvpn.exe -- we did enforce that the other day. But I fail to see a strong reason to restrict service clients in this manner. What am I missing? Selva 

Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/1335?usp=email to review the following change. Change subject: tapctl: overhaul adapter management helpers and improve CLI UX ...................................................................... tapctl: overhaul adapter management helpers and improve CLI UX - default to creating ovpn-dco adapters, refresh help text, validate CLI input, and print both GUID and name after creation; - refactor create/list/delete into dedicated helpers, add bulk deletion via delete --hwid, and centralize GUID/name resolution; - harden auto-naming to recognise both ovpn-dco and tap0901 HWIDs, honour reserved names, and reuse friendly names when deleting by GUID. Change-Id: I4d822bf7ae5f037e2414778e8fa74eca5e289e65 Signed-off-by: Lev Stipakov <le...@op...> --- M src/tapctl/main.c 1 file changed, 601 insertions(+), 243 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/35/1335/1 diff --git a/src/tapctl/main.c b/src/tapctl/main.c index 031e262..f72d261 100644 --- a/src/tapctl/main.c +++ b/src/tapctl/main.c @@ -1,5 +1,5 @@ /* - * tapctl -- Utility to manipulate TUN/TAP adapters on Windows + * tapctl -- Utility to manipulate virtual network adapters on Windows * https://community.openvpn.net/openvpn/wiki/Tapctl * * Copyright (C) 2002-2025 OpenVPN Inc <sa...@op...> @@ -28,6 +28,7 @@ #include <objbase.h> #include <setupapi.h> #include <stdio.h> +#include <stdlib.h> #include <wchar.h> #ifdef _MSC_VER @@ -50,8 +51,8 @@ L"\n" L"Commands:\n" L"\n" - L"create Create a new TUN/TAP adapter\n" - L"list List TUN/TAP adapters\n" + L"create Create a new network adapter\n" + L"list List network adapters\n" L"delete Delete specified network adapter\n" L"help Display this text\n" L"\n" @@ -61,7 +62,7 @@ static const WCHAR usage_message_create[] = L"%ls\n" L"\n" - L"Creates a new TUN/TAP adapter\n" + L"Creates a new network adapter\n" L"\n" L"Usage:\n" L"\n" @@ -69,24 +70,24 @@ L"\n" L"Options:\n" L"\n" - L"--name <name> Set TUN/TAP adapter name. Should the adapter with given name \n" + L"--name <name> Set network adapter name. Should the adapter with given name \n" L" already exist, an error is returned. If this option is not \n" L" specified, a default adapter name is chosen by Windows. \n" L" Note: This name can also be specified as OpenVPN's --dev-node \n" L" option. \n" - L"--hwid <hwid> Adapter hardware ID. Default value is root\\tap0901, which \n" - L" describes tap-windows6 driver. To work with ovpn-dco driver, \n" - L" driver, specify 'ovpn-dco'. \n" + L"--hwid <hwid> Adapter hardware ID. Default value is ovpn-dco, which creates \n" + L" a Data Channel Offload adapter. To create a tap-windows6 \n" + L" adapter, specify 'root\\tap0901'. \n" L"\n" L"Output:\n" L"\n" - L"This command prints newly created TUN/TAP adapter's GUID to stdout. \n" + L"This command prints newly created network adapter's GUID to stdout. \n" ; static const WCHAR usage_message_list[] = L"%ls\n" L"\n" - L"Lists TUN/TAP adapters\n" + L"Lists network adapters\n" L"\n" L"Usage:\n" L"\n" @@ -94,22 +95,28 @@ L"\n" L"Options:\n" L"\n" - L"--hwid <hwid> Adapter hardware ID. By default, root\\tap0901, tap0901 and \n" - L" ovpn-dco adapters are listed. Use this switch to limit the list.\n" + L"--hwid <hwid> Adapter hardware ID. By default, ovpn-dco, root\\tap0901 and \n" + L" tap0901 adapters are listed. Use this switch to limit the list.\n" L"\n" L"Output:\n" L"\n" - L"This command prints all TUN/TAP adapters to stdout. \n" + L"This command prints all network adapters to stdout. \n" ; static const WCHAR usage_message_delete[] = L"%ls\n" L"\n" - L"Deletes the specified network adapter\n" + L"Deletes network adapters\n" L"\n" L"Usage:\n" L"\n" L"tapctl delete <adapter GUID | adapter name>\n" + L"tapctl delete --hwid <hardware ID>\n" + L"\n" + L"Options:\n" + L"\n" + L"--hwid <hwid> Delete all adapters that use the specified hardware ID. Cannot\n" + L" be combined with adapter GUID or name.\n" ; /* clang-format on */ @@ -123,84 +130,633 @@ fwprintf(stderr, usage_message, title_string); } +static int tapctl_cmd_create(int argc, LPCWSTR argv[], BOOL *pbRebootRequired); +static int tapctl_cmd_list(int argc, LPCWSTR argv[]); +static int tapctl_cmd_delete(int argc, LPCWSTR argv[], BOOL *pbRebootRequired); + /** - * Checks if adapter with given name doesn't already exist + * Locate an adapter node by its friendly name within the enumerated list. + * + * @param name Friendly name to search for. Comparison is case-insensitive. + * @param adapter_list Head of the adapter list returned by tap_list_adapters(). + * + * @return Pointer to the matching node, or NULL when not found. */ -static BOOL -is_adapter_name_available(LPCWSTR name, struct tap_adapter_node *adapter_list, BOOL log) +static struct tap_adapter_node * +find_adapter_by_name(LPCWSTR name, struct tap_adapter_node *adapter_list) { for (struct tap_adapter_node *a = adapter_list; a; a = a->pNext) { - if (wcsicmp(name, a->szName) == 0) + if (_wcsicmp(name, a->szName) == 0) { - if (log) - { - LPOLESTR adapter_id = NULL; - StringFromIID((REFIID)&a->guid, &adapter_id); - fwprintf(stderr, - L"Adapter \"%ls\" already exists (GUID %" - L"ls).\n", - a->szName, adapter_id); - CoTaskMemFree(adapter_id); - } - - return FALSE; + return a; } } - return TRUE; + return NULL; } /** - * Returns unique adapter name based on hwid or NULL if name cannot be generated. - * Caller is responsible for freeing it. + * Check whether the registry still reserves a given network-connection name. + * + * Windows keeps friendly names under + * HKLM\SYSTEM\CurrentControlSet\Control\Network\{NETCLASS}\{GUID}\Connection\Name, + * even after an adapter is removed. netsh refuses to rename to any reserved name. + * + * @param name Friendly name to test. + * + * @return TRUE if the name exists in the registry, FALSE otherwise. + */ +static BOOL +registry_name_exists(LPCWSTR name) +{ + static const WCHAR class_key[] = + L"SYSTEM\\CurrentControlSet\\Control\\Network\\{4d36e972-e325-11ce-bfc1-08002be10318}"; + + HKEY hClassKey = NULL; + if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, class_key, 0, KEY_READ, &hClassKey) != ERROR_SUCCESS) + { + return FALSE; + } + + BOOL found = FALSE; + + for (DWORD index = 0;; ++index) + { + WCHAR adapter_id[64]; + DWORD adapter_id_len = _countof(adapter_id); + LONG result = RegEnumKeyEx(hClassKey, index, adapter_id, &adapter_id_len, NULL, NULL, NULL, + NULL); + if (result == ERROR_NO_MORE_ITEMS) + { + break; + } + else if (result != ERROR_SUCCESS) + { + continue; + } + + WCHAR connection_key[512]; + swprintf_s(connection_key, _countof(connection_key), L"%ls\\%ls\\Connection", class_key, + adapter_id); + + DWORD value_size = 0; + LONG query = RegGetValueW(HKEY_LOCAL_MACHINE, connection_key, L"Name", + RRF_RT_REG_SZ | RRF_NOEXPAND, NULL, NULL, &value_size); + if (query != ERROR_SUCCESS || value_size < sizeof(WCHAR)) + { + continue; + } + + LPWSTR value = (LPWSTR)malloc(value_size); + if (!value) + { + continue; + } + + query = RegGetValueW(HKEY_LOCAL_MACHINE, connection_key, L"Name", + RRF_RT_REG_SZ | RRF_NOEXPAND, NULL, value, &value_size); + if (query == ERROR_SUCCESS && _wcsicmp(name, value) == 0) + { + found = TRUE; + free(value); + break; + } + + free(value); + + if (found) + { + break; + } + } + + RegCloseKey(hClassKey); + return found; +} + +/** + * Determine whether a friendly name is currently in use by an adapter or reserved + * in the registry. + * + * @param name Friendly name to test. + * @param adapter_list Head of the adapter list returned by tap_list_adapters(). + * + * @return TRUE when the name is taken/reserved, FALSE when available. + */ +static BOOL +tap_name_in_use(LPCWSTR name, struct tap_adapter_node *adapter_list) +{ + if (name == NULL) + { + return FALSE; + } + + if (find_adapter_by_name(name, adapter_list)) + { + return TRUE; + } + + return registry_name_exists(name); +} + +/** + * Resolve the adapter name we should apply: + * - For user-specified names, ensure they are unique both in the adapter list and + * in the registry. On conflict, an explanatory message is printed and NULL is returned. + * - For automatic naming, derive the base string from HWID and append the first available + * suffix recognised by Windows. + * + * @param requested_name Name provided via CLI or configuration (may be NULL/empty). + * @param hwid Hardware identifier of the adapter being created. + * @param adapter_list Existing adapters enumerated via tap_list_adapters(). + * + * @return Newly allocated wide string containing the final name, or NULL on failure. */ static LPWSTR -get_unique_adapter_name(LPCWSTR hwid, struct tap_adapter_node *adapter_list) +tap_resolve_adapter_name(LPCWSTR requested_name, LPCWSTR hwid, + struct tap_adapter_node *adapter_list) { + if (requested_name && requested_name[0]) + { + struct tap_adapter_node *conflict = find_adapter_by_name(requested_name, adapter_list); + if (conflict) + { + LPOLESTR adapter_id = NULL; + StringFromIID((REFIID)&conflict->guid, &adapter_id); + fwprintf(stderr, + L"Adapter \"%ls\" already exists (GUID %" + L"ls).\n", + conflict->szName, adapter_id); + CoTaskMemFree(adapter_id); + return NULL; + } + + if (registry_name_exists(requested_name)) + { + fwprintf(stderr, L"Adapter name \"%ls\" is already in use.\n", requested_name); + return NULL; + } + + return wcsdup(requested_name); + } + if (hwid == NULL) { return NULL; } - LPCWSTR base_name; - if (wcsicmp(hwid, L"ovpn-dco") == 0) + LPCWSTR base_name = NULL; + if (_wcsicmp(hwid, L"ovpn-dco") == 0) { base_name = L"OpenVPN Data Channel Offload"; } - else if (wcsicmp(hwid, L"root\\" _L(TAP_WIN_COMPONENT_ID)) == 0) + else if (_wcsicmp(hwid, L"root\\" _L(TAP_WIN_COMPONENT_ID)) == 0 + || _wcsicmp(hwid, _L(TAP_WIN_COMPONENT_ID)) == 0) { base_name = L"OpenVPN TAP-Windows6"; } else { + fwprintf(stderr, + L"Cannot auto-generate adapter name for hardware ID \"%ls\".\n", hwid); return NULL; } - if (is_adapter_name_available(base_name, adapter_list, FALSE)) + if (!tap_name_in_use(base_name, adapter_list)) { return wcsdup(base_name); } size_t name_len = wcslen(base_name) + 10; - LPWSTR name = malloc(name_len * sizeof(WCHAR)); + LPWSTR name = (LPWSTR)malloc(name_len * sizeof(WCHAR)); if (name == NULL) { return NULL; } - for (int i = 1; i < 100; ++i) + + /* Windows never assigns the "#1" suffix, so skip it to avoid netsh failures. */ + for (int i = 2; i < 100; ++i) { swprintf_s(name, name_len, L"%ls #%d", base_name, i); - if (is_adapter_name_available(name, adapter_list, FALSE)) + if (!tap_name_in_use(name, adapter_list)) { return name; } } + free(name); + fwprintf(stderr, L"Unable to find available adapter name based on \"%ls\".\n", base_name); return NULL; } +static int +tapctl_cmd_create(int argc, LPCWSTR argv[], BOOL *pbRebootRequired) +{ + LPCWSTR szName = NULL; + LPCWSTR defaultHwId = L"ovpn-dco"; + LPCWSTR szHwId = defaultHwId; + LPWSTR adapter_name = NULL; + struct tap_adapter_node *pAdapterList = NULL; + GUID guidAdapter; + LPOLESTR szAdapterId = NULL; + DWORD dwResult; + int iResult = 1; + BOOL adapter_created = FALSE; + + for (int i = 2; i < argc; i++) + { + if (wcsicmp(argv[i], L"--name") == 0) + { + if (++i >= argc) + { + fwprintf(stderr, L"--name option requires a value. Ignored.\n"); + break; + } + szName = argv[i]; + if (szName[0] == L'\0') + { + fwprintf(stderr, L"--name option cannot be empty. Ignored.\n"); + szName = NULL; + } + } + else if (wcsicmp(argv[i], L"--hwid") == 0) + { + if (++i >= argc) + { + fwprintf(stderr, + L"--hwid option requires a value. Using default \"%ls\".\n", + defaultHwId); + break; + } + szHwId = argv[i]; + if (szHwId[0] == L'\0') + { + fwprintf(stderr, + L"--hwid option cannot be empty. Using default \"%ls\".\n", + defaultHwId); + szHwId = defaultHwId; + } + } + else + { + fwprintf(stderr, + L"Unknown option \"%ls" + L"\". Please, use \"tapctl help create\" to list supported options. Ignored.\n", + argv[i]); + } + } + + dwResult = tap_create_adapter(NULL, L"Virtual Ethernet", szHwId, pbRebootRequired, + &guidAdapter); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, L"Creating network adapter failed (error 0x%x).\n", dwResult); + goto cleanup; + } + adapter_created = TRUE; + + dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, L"Enumerating adapters failed (error 0x%x).\n", dwResult); + goto cleanup; + } + + adapter_name = tap_resolve_adapter_name(szName, szHwId, pAdapterList); + if (adapter_name == NULL) + { + goto cleanup; + } + + dwResult = tap_set_adapter_name(&guidAdapter, adapter_name, FALSE); + if (dwResult != ERROR_SUCCESS) + { + StringFromIID((REFIID)&guidAdapter, &szAdapterId); + fwprintf(stderr, + L"Renaming network adapter %ls to \"%ls\" failed (error 0x%x).\n", szAdapterId, + adapter_name, dwResult); + CoTaskMemFree(szAdapterId); + goto cleanup; + } + + iResult = 0; + StringFromIID((REFIID)&guidAdapter, &szAdapterId); + fwprintf(stdout, L"%ls\t%ls\n", szAdapterId, + (adapter_name && adapter_name[0]) ? adapter_name : L"(unnamed)"); + CoTaskMemFree(szAdapterId); + +cleanup: + if (pAdapterList) + { + tap_free_adapter_list(pAdapterList); + } + free(adapter_name); + + if (adapter_created && iResult != 0) + { + tap_delete_adapter(NULL, &guidAdapter, pbRebootRequired); + } + + return iResult; +} + +static int +tapctl_cmd_list(int argc, LPCWSTR argv[]) +{ + WCHAR szzHwId[0x100] = L"ovpn-dco\0" + L"root\\" _L(TAP_WIN_COMPONENT_ID) L"\0" _L(TAP_WIN_COMPONENT_ID) L"\0"; + struct tap_adapter_node *pAdapterList = NULL; + DWORD dwResult; + int iResult = 0; + + for (int i = 2; i < argc; i++) + { + if (wcsicmp(argv[i], L"--hwid") == 0) + { + if (++i >= argc) + { + fwprintf(stderr, + L"--hwid option requires a value. Please, use \"tapctl help list\" to " + L"list supported options.\n"); + return 1; + } + memset(szzHwId, 0, sizeof(szzHwId)); + memcpy_s(szzHwId, + sizeof(szzHwId) - 2 * sizeof(WCHAR) /*requires double zero termination*/, + argv[i], wcslen(argv[i]) * sizeof(WCHAR)); + } + else + { + fwprintf(stderr, + L"Unknown option \"%ls" + L"\". Please, use \"tapctl help list\" to list supported options. Ignored.\n", + argv[i]); + } + } + + dwResult = tap_list_adapters(NULL, szzHwId, &pAdapterList); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, L"Enumerating network adapters failed (error 0x%x).\n", dwResult); + return 1; + } + + for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext) + { + LPOLESTR szAdapterId = NULL; + StringFromIID((REFIID)&pAdapter->guid, &szAdapterId); + fwprintf(stdout, L"%ls\t%ls\n", szAdapterId, pAdapter->szName); + CoTaskMemFree(szAdapterId); + } + + tap_free_adapter_list(pAdapterList); + return iResult; +} + +/** + * Delete every adapter matching the provided hardware ID. + * + * @param hwid Hardware ID string (must be non-NULL and non-empty). + * @param pbRebootRequired Pointer to reboot flag propagated from caller. + * + * @return 0 when every adapter was deleted successfully, 1 otherwise. + */ +static int +tapctl_delete_by_hwid(LPCWSTR hwid, BOOL *pbRebootRequired) +{ + if (hwid == NULL || hwid[0] == L'\0') + { + fwprintf(stderr, L"--hwid option cannot be empty.\n"); + return 1; + } + + size_t hw_len = wcslen(hwid); + size_t buf_len = hw_len + 2; + LPWSTR szzHwId = (LPWSTR)calloc(buf_len, sizeof(WCHAR)); + if (szzHwId == NULL) + { + fwprintf(stderr, L"Out of memory.\n"); + return 1; + } + wmemcpy(szzHwId, hwid, hw_len); + + struct tap_adapter_node *pAdapterList = NULL; + DWORD dwResult = tap_list_adapters(NULL, szzHwId, &pAdapterList); + free(szzHwId); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, L"Enumerating network adapters failed (error 0x%x).\n", dwResult); + return 1; + } + + BOOL deleted_any = FALSE; + BOOL delete_error = FALSE; + + for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext) + { + LPOLESTR szAdapterId = NULL; + StringFromIID((REFIID)&pAdapter->guid, &szAdapterId); + + dwResult = tap_delete_adapter(NULL, &pAdapter->guid, pbRebootRequired); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, + L"Deleting adapter \"%ls\" (GUID %ls) failed (error 0x%x).\n", + pAdapter->szName, szAdapterId, dwResult); + delete_error = TRUE; + } + else + { + fwprintf(stdout, L"%ls\t%ls\n", szAdapterId, pAdapter->szName); + deleted_any = TRUE; + } + + CoTaskMemFree(szAdapterId); + } + + tap_free_adapter_list(pAdapterList); + + if (!deleted_any && !delete_error) + { + fwprintf(stderr, L"No adapters with hardware ID \"%ls\" found.\n", hwid); + } + + return delete_error ? 1 : 0; +} + +/** + * Resolve an adapter identifier supplied on the command line. + * + * @param target Adapter GUID or friendly name provided by the user. + * @param pguidAdapter Receives the resolved adapter GUID on success. + * @param pszMatchedName Optional pointer that receives a duplicated adapter name when + * the lookup was done by name. Caller must free() it. + * + * @return 0 on success, non-zero otherwise (an error message is printed on failure). + */ +static int +tapctl_resolve_delete_target(LPCWSTR target, GUID *pguidAdapter, LPWSTR *pszMatchedName) +{ + if (SUCCEEDED(IIDFromString(target, (LPIID)pguidAdapter))) + { + if (pszMatchedName) + { + *pszMatchedName = NULL; + struct tap_adapter_node *pAdapterList = NULL; + if (tap_list_adapters(NULL, NULL, &pAdapterList) == ERROR_SUCCESS) + { + for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; + pAdapter = pAdapter->pNext) + { + if (IsEqualGUID(&pAdapter->guid, pguidAdapter)) + { + LPWSTR duplicated = wcsdup(pAdapter->szName); + if (duplicated == NULL) + { + fwprintf(stderr, L"Out of memory.\n"); + tap_free_adapter_list(pAdapterList); + return 1; + } + *pszMatchedName = duplicated; + break; + } + } + tap_free_adapter_list(pAdapterList); + } + } + return 0; + } + + struct tap_adapter_node *pAdapterList = NULL; + DWORD dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, L"Enumerating network adapters failed (error 0x%x).\n", dwResult); + return 1; + } + + struct tap_adapter_node *match = NULL; + for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext) + { + if (wcsicmp(target, pAdapter->szName) == 0) + { + match = pAdapter; + break; + } + } + + if (match == NULL) + { + fwprintf(stderr, L"\"%ls\" adapter not found.\n", target); + tap_free_adapter_list(pAdapterList); + return 1; + } + + LPWSTR duplicated_name = pszMatchedName ? wcsdup(match->szName) : NULL; + if (pszMatchedName && duplicated_name == NULL) + { + fwprintf(stderr, L"Out of memory.\n"); + tap_free_adapter_list(pAdapterList); + return 1; + } + + memcpy(pguidAdapter, &match->guid, sizeof(GUID)); + tap_free_adapter_list(pAdapterList); + + if (pszMatchedName) + { + *pszMatchedName = duplicated_name; + } + return 0; +} + +static int +tapctl_cmd_delete(int argc, LPCWSTR argv[], BOOL *pbRebootRequired) +{ + LPCWSTR szDeleteTarget = NULL; + LPCWSTR szDeleteHwId = NULL; + + for (int i = 2; i < argc; i++) + { + if (wcsicmp(argv[i], L"--hwid") == 0) + { + if (++i >= argc) + { + fwprintf(stderr, + L"--hwid option requires a value. Please, use \"tapctl help delete\" for " + L"usage info.\n"); + return 1; + } + if (argv[i][0] == L'\0') + { + fwprintf(stderr, + L"--hwid option cannot be empty. Please, use \"tapctl help delete\" for " + L"usage info.\n"); + return 1; + } + szDeleteHwId = argv[i]; + } + else if (szDeleteTarget == NULL) + { + szDeleteTarget = argv[i]; + } + else + { + fwprintf(stderr, + L"Unknown option \"%ls" + L"\". Please, use \"tapctl help delete\" for usage info. Ignored.\n", + argv[i]); + } + } + + if (szDeleteHwId) + { + if (szDeleteTarget) + { + fwprintf(stderr, L"--hwid option cannot be combined with adapter GUID or name.\n"); + return 1; + } + return tapctl_delete_by_hwid(szDeleteHwId, pbRebootRequired); + } + + if (szDeleteTarget == NULL) + { + fwprintf( + stderr, + L"Missing adapter GUID or name. Please, use \"tapctl help delete\" for usage info.\n"); + return 1; + } + + GUID guidAdapter; + LPWSTR matched_name = NULL; + if (tapctl_resolve_delete_target(szDeleteTarget, &guidAdapter, &matched_name) != 0) + { + return 1; + } + + DWORD dwResult = tap_delete_adapter(NULL, &guidAdapter, pbRebootRequired); + if (dwResult != ERROR_SUCCESS) + { + fwprintf(stderr, + L"Deleting adapter \"%ls" + L"\" failed (error 0x%x).\n", + szDeleteTarget, dwResult); + free(matched_name); + return 1; + } + + LPOLESTR szAdapterId = NULL; + StringFromIID((REFIID)&guidAdapter, &szAdapterId); + fwprintf(stdout, L"%ls\t%ls\n", szAdapterId, + (matched_name && matched_name[0]) ? matched_name : szDeleteTarget); + CoTaskMemFree(szAdapterId); + free(matched_name); + + return 0; +} + /** * Program entry point */ @@ -248,215 +804,17 @@ } else if (wcsicmp(argv[1], L"create") == 0) { - LPCWSTR szName = NULL; - LPCWSTR szHwId = L"root\\" _L(TAP_WIN_COMPONENT_ID); - - /* Parse options. */ - for (int i = 2; i < argc; i++) - { - if (wcsicmp(argv[i], L"--name") == 0) - { - szName = argv[++i]; - } - else if (wcsicmp(argv[i], L"--hwid") == 0) - { - szHwId = argv[++i]; - } - else - { - fwprintf( - stderr, - L"Unknown option \"%ls" - L"\". Please, use \"tapctl help create\" to list supported options. Ignored.\n", - argv[i]); - } - } - - /* Create TUN/TAP adapter. */ - GUID guidAdapter; - LPOLESTR szAdapterId = NULL; - DWORD dwResult = - tap_create_adapter(NULL, L"Virtual Ethernet", szHwId, &bRebootRequired, &guidAdapter); - if (dwResult != ERROR_SUCCESS) - { - fwprintf(stderr, L"Creating TUN/TAP adapter failed (error 0x%x).\n", dwResult); - iResult = 1; - goto quit; - } - - /* Get existing network adapters. */ - struct tap_adapter_node *pAdapterList = NULL; - dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); - if (dwResult != ERROR_SUCCESS) - { - fwprintf(stderr, L"Enumerating adapters failed (error 0x%x).\n", dwResult); - iResult = 1; - goto create_delete_adapter; - } - - LPWSTR adapter_name = - szName ? wcsdup(szName) : get_unique_adapter_name(szHwId, pAdapterList); - if (adapter_name) - { - /* Check for duplicates when name was specified, - * otherwise get_adapter_default_name() takes care of it */ - if (szName && !is_adapter_name_available(adapter_name, pAdapterList, TRUE)) - { - iResult = 1; - goto create_cleanup_pAdapterList; - } - - /* Rename the adapter. */ - dwResult = tap_set_adapter_name(&guidAdapter, adapter_name, FALSE); - if (dwResult != ERROR_SUCCESS) - { - StringFromIID((REFIID)&guidAdapter, &szAdapterId); - fwprintf(stderr, - L"Renaming TUN/TAP adapter %ls" - L" to \"%ls\" failed (error 0x%x).\n", - szAdapterId, adapter_name, dwResult); - CoTaskMemFree(szAdapterId); - iResult = 1; - goto quit; - } - } - - iResult = 0; - -create_cleanup_pAdapterList: - free(adapter_name); - - tap_free_adapter_list(pAdapterList); - if (iResult) - { - goto create_delete_adapter; - } - - /* Output adapter GUID. */ - StringFromIID((REFIID)&guidAdapter, &szAdapterId); - fwprintf(stdout, L"%ls\n", szAdapterId); - CoTaskMemFree(szAdapterId); - - iResult = 0; - goto quit; - -create_delete_adapter: - tap_delete_adapter(NULL, &guidAdapter, &bRebootRequired); - iResult = 1; + iResult = tapctl_cmd_create(argc, argv, &bRebootRequired); goto quit; } else if (wcsicmp(argv[1], L"list") == 0) { - WCHAR szzHwId[0x100] = - L"root\\" _L(TAP_WIN_COMPONENT_ID) L"\0" _L(TAP_WIN_COMPONENT_ID) L"\0" - L"ovpn-dco\0"; - - /* Parse options. */ - for (int i = 2; i < argc; i++) - { - if (wcsicmp(argv[i], L"--hwid") == 0) - { - memset(szzHwId, 0, sizeof(szzHwId)); - ++i; - memcpy_s(szzHwId, - sizeof(szzHwId) - 2 * sizeof(WCHAR) /*requires double zero termination*/, - argv[i], wcslen(argv[i]) * sizeof(WCHAR)); - } - else - { - fwprintf( - stderr, - L"Unknown option \"%ls" - L"\". Please, use \"tapctl help list\" to list supported options. Ignored.\n", - argv[i]); - } - } - - /* Output list of adapters with given hardware ID. */ - struct tap_adapter_node *pAdapterList = NULL; - DWORD dwResult = tap_list_adapters(NULL, szzHwId, &pAdapterList); - if (dwResult != ERROR_SUCCESS) - { - fwprintf(stderr, L"Enumerating TUN/TAP adapters failed (error 0x%x).\n", dwResult); - iResult = 1; - goto quit; - } - - for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext) - { - LPOLESTR szAdapterId = NULL; - StringFromIID((REFIID)&pAdapter->guid, &szAdapterId); - fwprintf(stdout, - L"%ls\t%" - L"ls\n", - szAdapterId, pAdapter->szName); - CoTaskMemFree(szAdapterId); - } - - iResult = 0; - tap_free_adapter_list(pAdapterList); + iResult = tapctl_cmd_list(argc, argv); + goto quit; } else if (wcsicmp(argv[1], L"delete") == 0) { - if (argc < 3) - { - fwprintf( - stderr, - L"Missing adapter GUID or name. Please, use \"tapctl help delete\" for usage info.\n"); - return 1; - } - - GUID guidAdapter; - if (FAILED(IIDFromString(argv[2], (LPIID)&guidAdapter))) - { - /* The argument failed to covert to GUID. Treat it as the adapter name. */ - struct tap_adapter_node *pAdapterList = NULL; - DWORD dwResult = tap_list_adapters(NULL, NULL, &pAdapterList); - if (dwResult != ERROR_SUCCESS) - { - fwprintf(stderr, L"Enumerating TUN/TAP adapters failed (error 0x%x).\n", dwResult); - iResult = 1; - goto quit; - } - - for (struct tap_adapter_node *pAdapter = pAdapterList;; pAdapter = pAdapter->pNext) - { - if (pAdapter == NULL) - { - fwprintf(stderr, L"\"%ls\" adapter not found.\n", argv[2]); - iResult = 1; - goto delete_cleanup_pAdapterList; - } - else if (wcsicmp(argv[2], pAdapter->szName) == 0) - { - memcpy(&guidAdapter, &pAdapter->guid, sizeof(GUID)); - break; - } - } - - iResult = 0; - -delete_cleanup_pAdapterList: - tap_free_adapter_list(pAdapterList); - if (iResult) - { - goto quit; - } - } - - /* Delete the network adapter. */ - DWORD dwResult = tap_delete_adapter(NULL, &guidAdapter, &bRebootRequired); - if (dwResult != ERROR_SUCCESS) - { - fwprintf(stderr, - L"Deleting adapter \"%ls" - L"\" failed (error 0x%x).\n", - argv[2], dwResult); - iResult = 1; - goto quit; - } - - iResult = 0; + iResult = tapctl_cmd_delete(argc, argv, &bRebootRequired); goto quit; } else -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1335?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I4d822bf7ae5f037e2414778e8fa74eca5e289e65 Gerrit-Change-Number: 1335 Gerrit-PatchSet: 1 Gerrit-Owner: stipa <lst...@gm...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> 

Attention is currently required from: cron2, flichtenheld, plaisthos. stipa has posted comments on this change by stipa. ( http://gerrit.openvpn.net/c/openvpn/+/1332?usp=email ) Change subject: win32: ensure plugin dir has the trailing slash ...................................................................... Patch Set 2: (2 comments) Patchset: PS1: > this breaks autoconf/automake compilation because the new library is not added to src/openvpn/Makefi […] Done Patchset: PS2: Fixed! -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1332?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I5ea90594493ab5cb858f7495f773e080b379e8e8 Gerrit-Change-Number: 1332 Gerrit-PatchSet: 2 Gerrit-Owner: stipa <lst...@gm...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Fri, 31 Oct 2025 11:34:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> 

Attention is currently required from: flichtenheld, plaisthos, stipa. Hello cron2, flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/1332?usp=email to look at the new patch set (#2). Change subject: win32: ensure plugin dir has the trailing slash ...................................................................... win32: ensure plugin dir has the trailing slash This prevents loading plugins from the directories which share initial characters with the trusted path. Reported-by: Joshua Rogers <co...@jo...> Found-by: ZeroPath (https://zeropath.com/) Change-Id: I5ea90594493ab5cb858f7495f773e080b379e8e8 Signed-off-by: Lev Stipakov <le...@op...> --- M CMakeLists.txt M src/openvpn/Makefile.am M src/openvpn/win32.c 3 files changed, 30 insertions(+), 12 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/32/1332/2 diff --git a/CMakeLists.txt b/CMakeLists.txt index c9301e6..6888de3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -245,6 +245,9 @@ check_symbol_exists(chsize io.h HAVE_CHSIZE) check_symbol_exists(getrlimit sys/resource.h HAVE_GETRLIMIT) +# Some old versions of mingw does not have PATHCCH_OPTIONS enums -- add a check +check_symbol_exists(PATHCCH_ENSURE_TRAILING_SLASH pathcch.h HAVE_PATHCCH_ENSURE_TRAILING_SLASH) + # Some OS (e.g. FreeBSD) need some basic headers to allow # including network headers set(NETEXTRA sys/types.h) @@ -338,7 +341,7 @@ if (WIN32) target_link_libraries(${target} PUBLIC ws2_32.lib crypt32.lib fwpuclnt.lib iphlpapi.lib - wininet.lib setupapi.lib rpcrt4.lib wtsapi32.lib ncrypt.lib bcrypt.lib) + wininet.lib setupapi.lib rpcrt4.lib wtsapi32.lib ncrypt.lib bcrypt.lib pathcch.lib) endif () endif () diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index db87dfc..a2b5e92 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -166,5 +166,5 @@	$(OPTIONAL_INOTIFY_LIBS) if WIN32 openvpn_SOURCES += openvpn_win32_resources.rc wfp_block.c wfp_block.h -openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm -lfwpuclnt -lrpcrt4 -lncrypt -lsetupapi -lbcrypt +openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm -lfwpuclnt -lrpcrt4 -lncrypt -lsetupapi -lbcrypt -lpathcch endif diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c index df29dd7..3ed28f6 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -51,6 +51,12 @@ #include "wfp_block.h" +#include <pathcch.h> + +#ifndef HAVE_PATHCCH_ENSURE_TRAILING_SLASH +#define PATHCCH_ENSURE_TRAILING_SLASH 0x20 +#endif + /* * WFP handle */ @@ -1553,12 +1559,12 @@ return false; } - WCHAR plugin_dir[MAX_PATH] = { 0 }; + WCHAR plugin_dir_reg[MAX_PATH] = { 0 }; /* Attempt to retrieve the trusted plugin directory path from the registry, * using installation path as a fallback */ - if (!get_openvpn_reg_value(L"plugin_dir", plugin_dir, _countof(plugin_dir)) - && !get_openvpn_reg_value(NULL, plugin_dir, _countof(plugin_dir))) + if (!get_openvpn_reg_value(L"plugin_dir", plugin_dir_reg, _countof(plugin_dir_reg)) + && !get_openvpn_reg_value(NULL, plugin_dir_reg, _countof(plugin_dir_reg))) { msg(M_WARN, "Installation path could not be determined."); } @@ -1570,26 +1576,35 @@ msg(M_NONFATAL | M_ERRNO, "Failed to get system directory."); } - if ((wcslen(plugin_dir) == 0) && (wcslen(system_dir) == 0)) + if ((wcslen(plugin_dir_reg) == 0) && (wcslen(system_dir) == 0)) { return false; } - WCHAR normalized_plugin_dir[MAX_PATH] = { 0 }; + WCHAR plugin_dir[MAX_PATH] = { 0 }; - /* Normalize the plugin dir */ - if (wcslen(plugin_dir) > 0) + /* normalize and canonicalize the plugin dir and add trailing slash */ + if (wcslen(plugin_dir_reg) > 0) { - if (!GetFullPathNameW(plugin_dir, MAX_PATH, normalized_plugin_dir, NULL)) + WCHAR normalized_plugin_dir[MAX_PATH] = { 0 }; + if (!GetFullPathNameW(plugin_dir_reg, MAX_PATH, normalized_plugin_dir, NULL)) { msg(M_NONFATAL | M_ERRNO, "Failed to normalize plugin dir."); + } + + HRESULT res = PathCchCanonicalizeEx(plugin_dir, _countof(plugin_dir), normalized_plugin_dir, + PATHCCH_ENSURE_TRAILING_SLASH); + if (res != S_OK) + { + /* doc says we cannot rely on GetLastError() */ + msg(M_NONFATAL, "Failed to canonicalize plugin dir."); return false; } } /* Check if the plugin path resides within the plugin/install directory */ - if ((wcslen(normalized_plugin_dir) > 0) - && (wcsnicmp(normalized_plugin_dir, plugin_path, wcslen(normalized_plugin_dir)) == 0)) + if ((wcslen(plugin_dir) > 0) + && (wcsnicmp(plugin_dir, plugin_path, wcslen(plugin_dir)) == 0)) { return true; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1332?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I5ea90594493ab5cb858f7495f773e080b379e8e8 Gerrit-Change-Number: 1332 Gerrit-PatchSet: 2 Gerrit-Owner: stipa <lst...@gm...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> 

cron2 has uploaded a new patch set (#3) to the change originally created by MaxF. ( http://gerrit.openvpn.net/c/openvpn/+/1315?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by plaisthos Change subject: Zeroize tls-crypt-v2 client keys ...................................................................... Zeroize tls-crypt-v2 client keys Joshua Rogers sent in a bug report generated with ZeroPath that the tls-crypt-v2 client key is loaded before running the verify script. If the verify script fails, the key is not zeroized. While investigating this report, I found that free_tls_pre_decrypt_state never zeroizes tls_wrap_tmp.original_wrap_keydata. So also when the check is successful, key data will remain in memory when it is no longer needed. This commit moves the tls-crypt-v2-verify check before loading the key. If it fails, original_wrap_keydata is zeroized. Also, in free_tls_pre_decrypt_state, if a key has been loaded, original_wrap_keydata is zeroized. Reported-By: Joshua Rogers <co...@jo...> Found-by: ZeroPath (https://zeropath.com/) Change-Id: Icfcbf8ee20c1c0016eb98b570f24b9325b157c5c Signed-off-by: Max Fillinger <ma...@ma...> Acked-by: Arne Schwabe <arn...@rf...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1315 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34103.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/ssl_pkt.c M src/openvpn/tls_crypt.c 2 files changed, 7 insertions(+), 5 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/15/1315/3 diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c index 825719c..d7f7ac3 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -280,6 +280,7 @@ if (state->tls_wrap_tmp.cleanup_key_ctx) { free_key_ctx_bi(&state->tls_wrap_tmp.opt.key_ctx_bi); + secure_memzero(&state->tls_wrap_tmp.original_wrap_keydata, sizeof(state->tls_wrap_tmp.original_wrap_keydata)); } } diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c index 51b4eb3..a808de3 100644 --- a/src/openvpn/tls_crypt.c +++ b/src/openvpn/tls_crypt.c @@ -642,6 +642,12 @@ return false; } + if (opt && opt->tls_crypt_v2_verify_script && !tls_crypt_v2_verify_metadata(ctx, opt)) + { + secure_memzero(&ctx->original_wrap_keydata, sizeof(ctx->original_wrap_keydata)); + return false; + } + /* Load the decrypted key */ ctx->mode = TLS_WRAP_CRYPT; ctx->cleanup_key_ctx = true; @@ -652,11 +658,6 @@ /* Remove client key from buffer so tls-crypt code can unwrap message */ ASSERT(buf_inc_len(buf, -(BLEN(&wrapped_client_key)))); - if (opt && opt->tls_crypt_v2_verify_script) - { - return tls_crypt_v2_verify_metadata(ctx, opt); - } - return true; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1315?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Icfcbf8ee20c1c0016eb98b570f24b9325b157c5c Gerrit-Change-Number: 1315 Gerrit-PatchSet: 3 Gerrit-Owner: MaxF <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> 

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1315?usp=email ) Change subject: Zeroize tls-crypt-v2 client keys ...................................................................... Zeroize tls-crypt-v2 client keys Joshua Rogers sent in a bug report generated with ZeroPath that the tls-crypt-v2 client key is loaded before running the verify script. If the verify script fails, the key is not zeroized. While investigating this report, I found that free_tls_pre_decrypt_state never zeroizes tls_wrap_tmp.original_wrap_keydata. So also when the check is successful, key data will remain in memory when it is no longer needed. This commit moves the tls-crypt-v2-verify check before loading the key. If it fails, original_wrap_keydata is zeroized. Also, in free_tls_pre_decrypt_state, if a key has been loaded, original_wrap_keydata is zeroized. Reported-By: Joshua Rogers <co...@jo...> Found-by: ZeroPath (https://zeropath.com/) Change-Id: Icfcbf8ee20c1c0016eb98b570f24b9325b157c5c Signed-off-by: Max Fillinger <ma...@ma...> Acked-by: Arne Schwabe <arn...@rf...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1315 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34103.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/ssl_pkt.c M src/openvpn/tls_crypt.c 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c index 825719c..d7f7ac3 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -280,6 +280,7 @@ if (state->tls_wrap_tmp.cleanup_key_ctx) { free_key_ctx_bi(&state->tls_wrap_tmp.opt.key_ctx_bi); + secure_memzero(&state->tls_wrap_tmp.original_wrap_keydata, sizeof(state->tls_wrap_tmp.original_wrap_keydata)); } } diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c index 51b4eb3..a808de3 100644 --- a/src/openvpn/tls_crypt.c +++ b/src/openvpn/tls_crypt.c @@ -642,6 +642,12 @@ return false; } + if (opt && opt->tls_crypt_v2_verify_script && !tls_crypt_v2_verify_metadata(ctx, opt)) + { + secure_memzero(&ctx->original_wrap_keydata, sizeof(ctx->original_wrap_keydata)); + return false; + } + /* Load the decrypted key */ ctx->mode = TLS_WRAP_CRYPT; ctx->cleanup_key_ctx = true; @@ -652,11 +658,6 @@ /* Remove client key from buffer so tls-crypt code can unwrap message */ ASSERT(buf_inc_len(buf, -(BLEN(&wrapped_client_key)))); - if (opt && opt->tls_crypt_v2_verify_script) - { - return tls_crypt_v2_verify_metadata(ctx, opt); - } - return true; } -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1315?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Icfcbf8ee20c1c0016eb98b570f24b9325b157c5c Gerrit-Change-Number: 1315 Gerrit-PatchSet: 3 Gerrit-Owner: MaxF <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> 

I did not test this, and when reviewing, felt it's above my paygrade - but since Arne is happy, and he really understands that code, perfect :-) BB is happy as well! (I *do* have tested this on the t_server testbed which has tls-crypt[-v2] using instances, and it still works, so confidence level is high ;-)). Your patch has been applied to the master branch. commit 9f71f906ea95331fd9b269502e92c42d1812dd9e Author: Max Fillinger Date: Fri Oct 31 11:08:04 2025 +0100 Zeroize tls-crypt-v2 client keys Signed-off-by: Max Fillinger <ma...@ma...> Acked-by: Arne Schwabe <arn...@rf...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1315 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34103.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

From: Max Fillinger <ma...@ma...> Joshua Rogers sent in a bug report generated with ZeroPath that the tls-crypt-v2 client key is loaded before running the verify script. If the verify script fails, the key is not zeroized. While investigating this report, I found that free_tls_pre_decrypt_state never zeroizes tls_wrap_tmp.original_wrap_keydata. So also when the check is successful, key data will remain in memory when it is no longer needed. This commit moves the tls-crypt-v2-verify check before loading the key. If it fails, original_wrap_keydata is zeroized. Also, in free_tls_pre_decrypt_state, if a key has been loaded, original_wrap_keydata is zeroized. Reported-By: Joshua Rogers <co...@jo...> Found-By: Zeropath Change-Id: Icfcbf8ee20c1c0016eb98b570f24b9325b157c5c Signed-off-by: Max Fillinger <ma...@ma...> Acked-by: Arne Schwabe <arn...@rf...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1315 --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1315 This mail reflects revision 2 of this Change. Acked-by according to Gerrit (reflected above): Arne Schwabe <arn...@rf...> diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c index 825719c..d7f7ac3 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -280,6 +280,7 @@ if (state->tls_wrap_tmp.cleanup_key_ctx) { free_key_ctx_bi(&state->tls_wrap_tmp.opt.key_ctx_bi); + secure_memzero(&state->tls_wrap_tmp.original_wrap_keydata, sizeof(state->tls_wrap_tmp.original_wrap_keydata)); } } diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c index 51b4eb3..a808de3 100644 --- a/src/openvpn/tls_crypt.c +++ b/src/openvpn/tls_crypt.c @@ -642,6 +642,12 @@ return false; } + if (opt && opt->tls_crypt_v2_verify_script && !tls_crypt_v2_verify_metadata(ctx, opt)) + { + secure_memzero(&ctx->original_wrap_keydata, sizeof(ctx->original_wrap_keydata)); + return false; + } + /* Load the decrypted key */ ctx->mode = TLS_WRAP_CRYPT; ctx->cleanup_key_ctx = true; @@ -652,11 +658,6 @@ /* Remove client key from buffer so tls-crypt code can unwrap message */ ASSERT(buf_inc_len(buf, -(BLEN(&wrapped_client_key)))); - if (opt && opt->tls_crypt_v2_verify_script) - { - return tls_crypt_v2_verify_metadata(ctx, opt); - } - return true; } 

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email ) Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements ...................................................................... PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements The number of messages calculated before the call to message_splitter(), used in the memory allocation in the buffer array, could in certain cases be less than one than the actual number of messages, thus causing an override of the sentinel buffer in message_splitter() and therefore an invalid read in send_single_push_update(). The case in question would be, for example, a sequence of three options "A,B,C" with the size of B equal to safe_cap - 1 and the sum of the sizes of A and C less than safe_cap - 2. The buffer array was therefore replaced with a list of buffers to completely avoid calculating the number of messages before it was actually computed. The test case in question has been added to the unit tests. The unit tests have been improved using cmocka macros. Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9 Signed-off-by: Marco Baffo <ma...@ma...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1316 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34073.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/push_util.c M tests/unit_tests/openvpn/test_push_update_msg.c 2 files changed, 137 insertions(+), 57 deletions(-) diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c index 552679a..51c7b5f 100644 --- a/src/openvpn/push_util.c +++ b/src/openvpn/push_util.c @@ -91,7 +91,7 @@ * Return `false` on failure an `true` on success. */ static bool -message_splitter(const char *s, struct buffer *msgs, struct gc_arena *gc, const size_t safe_cap) +message_splitter(const char *s, struct buffer_list *msgs, struct gc_arena *gc, const size_t safe_cap) { if (!s || !*s) { @@ -100,7 +100,6 @@ char *str = gc_strdup(s, gc); size_t i = 0; - int im = 0; while (*str) { @@ -115,37 +114,38 @@ } str[ci] = '\0'; /* copy from i to (ci -1) */ - msgs[im] = forge_msg(str, ",push-continuation 2", gc); + struct buffer tmp = forge_msg(str, ",push-continuation 2", gc); + buffer_list_push(msgs, BSTR(&tmp)); i = ci + 1; } else { - if (im) + if (msgs->head) { - msgs[im] = forge_msg(str, ",push-continuation 1", gc); + struct buffer tmp = forge_msg(str, ",push-continuation 1", gc); + buffer_list_push(msgs, BSTR(&tmp)); } else { - msgs[im] = forge_msg(str, NULL, gc); + struct buffer tmp = forge_msg(str, NULL, gc); + buffer_list_push(msgs, BSTR(&tmp)); } i = strlen(str); } str = &str[i]; - im++; } return true; } /* send the message(s) prepared to one single client */ static bool -send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer *msgs) +send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer_list *msgs) { - if (!msgs[0].data || !*(msgs[0].data)) + if (!msgs->head) { return false; } - int i = -1; unsigned int option_types_found = 0; struct context *c = &mi->context; struct options o; @@ -160,9 +160,10 @@ o.ifconfig_local = canary; o.ifconfig_ipv6_local = canary; - while (msgs[++i].data && *(msgs[i].data)) + struct buffer_entry *e = msgs->head; + while (e) { - if (!send_control_channel_string(c, BSTR(&msgs[i]), D_PUSH)) + if (!send_control_channel_string(c, BSTR(&e->buf), D_PUSH)) { return false; } @@ -182,13 +183,14 @@ * Also we need to make a temporary copy so we can buf_advance() * without modifying original buffer. */ - struct buffer tmp_msg = msgs[i]; + struct buffer tmp_msg = e->buf; buf_string_compare_advance(&tmp_msg, push_update_cmd); unsigned int permission_mask = pull_permission_mask(c); if (process_push_update(c, &o, permission_mask, &option_types_found, &tmp_msg, true) == PUSH_MSG_ERROR) { msg(M_WARN, "Failed to process push update message sent to client ID: %u", c->c2.tls_multi->peer_id); } + e = e->next; } if (option_types_found & OPT_P_UP) @@ -270,12 +272,11 @@ * we want to send exceeds that size we have to split it into smaller messages */ ASSERT(push_bundle_size > extra); const size_t safe_cap = push_bundle_size - extra; - size_t msgs_num = (strlen(msg) / safe_cap) + ((strlen(msg) % safe_cap) != 0); - struct buffer *msgs = gc_malloc((msgs_num + 1) * sizeof(struct buffer), true, &gc); + struct buffer_list *msgs = buffer_list_new(); - msgs[msgs_num].data = NULL; if (!message_splitter(msg, msgs, &gc, safe_cap)) { + buffer_list_free(msgs); gc_free(&gc); return -EINVAL; } @@ -286,6 +287,7 @@ if (!mi) { + buffer_list_free(msgs); gc_free(&gc); return -ENOENT; } @@ -293,6 +295,7 @@ if (!support_push_update(mi)) { msg(M_CLIENT, "PUSH_UPDATE: not sending message to unsupported peer with ID: %u", mi->context.c2.tls_multi->peer_id); + buffer_list_free(msgs); gc_free(&gc); return 0; } @@ -300,11 +303,13 @@ if (!mi->halt && send_single_push_update(m, mi, msgs)) { + buffer_list_free(msgs); gc_free(&gc); return 1; } else { + buffer_list_free(msgs); gc_free(&gc); return 0; } @@ -334,6 +339,7 @@ } hash_iterator_free(&hi); + buffer_list_free(msgs); gc_free(&gc); return count; } diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c index 516e94c..6ef1266 100644 --- a/tests/unit_tests/openvpn/test_push_update_msg.c +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -130,18 +130,11 @@ return true; } #else /* ifndef ENABLE_MANAGEMENT */ -char **res; -int i; bool send_control_channel_string(struct context *c, const char *str, msglvl_t msglevel) { - if (res && res[i] && strcmp(res[i], str)) - { - printf("\n\nexpected: %s\n\n actual: %s\n\n", res[i], str); - return false; - } - i++; + check_expected(str); return true; } @@ -301,44 +294,75 @@ #ifdef ENABLE_MANAGEMENT char *r0[] = { - "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0" + "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0", + NULL }; char *r1[] = { "PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2", "PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway local,push-continuation 2", - "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1" + "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1", + NULL }; char *r3[] = { - "PUSH_UPDATE,,," + "PUSH_UPDATE,,,", + NULL }; char *r4[] = { "PUSH_UPDATE,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2", "PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2", - "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1" + "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1", + NULL }; char *r5[] = { "PUSH_UPDATE,,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2", "PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2", - "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1" + "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1", + NULL }; char *r6[] = { "PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2", "PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8, redirect-gateway 10.10.10.10,,push-continuation 2", - "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1" + "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1", + NULL }; char *r7[] = { "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,push-continuation 2", - "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1" + "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1", + NULL }; char *r8[] = { "PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2", "PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway\n local,push-continuation 2", - "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1" + "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1", + NULL }; char *r9[] = { - "PUSH_UPDATE,," + "PUSH_UPDATE,,", + NULL }; - +char *r11[] = { + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2", + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2", + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2", + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2", + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 1", + NULL +}; +char *r12[] = { + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,push-continuation 2", + "PUSH_UPDATE,abc,push-continuation 1", + NULL +}; +char *r13[] = { + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,", + NULL +}; +char *r14[] = { + "PUSH_UPDATE,a,push-continuation 2", + "PUSH_UPDATE,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,push-continuation 2", + "PUSH_UPDATE,a,push-continuation 1", + NULL +}; const char *msg0 = "redirect-gateway local,route 192.168.1.0 255.255.255.0"; const char *msg1 = "-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf," @@ -365,32 +389,50 @@ "daisy damage damp dance danger daring dash daughter dawn day deal debate debris decade december decide decline" "decorate decrease deer defense define defy degree delay deliver demand demise denial"; +const char *msg11 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a," + "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a," + "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a," + "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a," + "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a"; + +const char *msg12 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,abc"; + +const char *msg13 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,"; + +const char *msg14 = "a,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,a"; + #define PUSH_BUNDLE_SIZE_TEST 184 +#define expect_control_channel_strings(res) \ + do \ + { \ + for (int j = 0; res[j] != NULL; j++) \ + { \ + expect_string(send_control_channel_string, str, res[j]); \ + } \ + } while (0) + static void test_send_push_msg0(void **state) { - i = 0; - res = r0; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r0); assert_int_equal(send_push_update(m, &cid, msg0, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } + static void test_send_push_msg1(void **state) { - i = 0; - res = r1; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r1); assert_int_equal(send_push_update(m, &cid, msg1, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg2(void **state) { - i = 0; - res = NULL; struct multi_context *m = *state; const unsigned long cid = 0; assert_int_equal(send_push_update(m, &cid, msg2, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL); @@ -399,83 +441,110 @@ static void test_send_push_msg3(void **state) { - i = 0; - res = r3; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r3); assert_int_equal(send_push_update(m, &cid, msg3, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg4(void **state) { - i = 0; - res = r4; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r4); assert_int_equal(send_push_update(m, &cid, msg4, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg5(void **state) { - i = 0; - res = r5; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r5); assert_int_equal(send_push_update(m, &cid, msg5, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg6(void **state) { - i = 0; - res = r6; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r6); assert_int_equal(send_push_update(m, &cid, msg6, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg7(void **state) { - i = 0; - res = r7; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r7); assert_int_equal(send_push_update(m, &cid, msg7, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg8(void **state) { - i = 0; - res = r8; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r8); assert_int_equal(send_push_update(m, &cid, msg8, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg9(void **state) { - i = 0; - res = r9; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r9); assert_int_equal(send_push_update(m, &cid, msg9, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg10(void **state) { - i = 0; - res = NULL; struct multi_context *m = *state; const unsigned long cid = 0; assert_int_equal(send_push_update(m, &cid, msg10, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL); } +static void +test_send_push_msg11(void **state) +{ + struct multi_context *m = *state; + const unsigned long cid = 0; + expect_control_channel_strings(r11); + assert_int_equal(send_push_update(m, &cid, msg11, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); +} + +static void +test_send_push_msg12(void **state) +{ + struct multi_context *m = *state; + const unsigned long cid = 0; + expect_control_channel_strings(r12); + assert_int_equal(send_push_update(m, &cid, msg12, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); +} + +static void +test_send_push_msg13(void **state) +{ + struct multi_context *m = *state; + const unsigned long cid = 0; + expect_control_channel_strings(r13); + assert_int_equal(send_push_update(m, &cid, msg13, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); +} + +static void +test_send_push_msg14(void **state) +{ + struct multi_context *m = *state; + const unsigned long cid = 0; + expect_control_channel_strings(r14); + assert_int_equal(send_push_update(m, &cid, msg14, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); +} + #undef PUSH_BUNDLE_SIZE_TEST static int @@ -535,6 +604,7 @@ cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown), cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown), #ifdef ENABLE_MANAGEMENT + cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2), cmocka_unit_test_setup_teardown(test_send_push_msg1, setup2, teardown2), cmocka_unit_test_setup_teardown(test_send_push_msg2, setup2, teardown2), @@ -545,7 +615,11 @@ cmocka_unit_test_setup_teardown(test_send_push_msg7, setup2, teardown2), cmocka_unit_test_setup_teardown(test_send_push_msg8, setup2, teardown2), cmocka_unit_test_setup_teardown(test_send_push_msg9, setup2, teardown2), - cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2) + cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2), + cmocka_unit_test_setup_teardown(test_send_push_msg11, setup2, teardown2), + cmocka_unit_test_setup_teardown(test_send_push_msg12, setup2, teardown2), + cmocka_unit_test_setup_teardown(test_send_push_msg13, setup2, teardown2), + cmocka_unit_test_setup_teardown(test_send_push_msg14, setup2, teardown2) #endif }; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9 Gerrit-Change-Number: 1316 Gerrit-PatchSet: 7 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> 

cron2 has uploaded a new patch set (#7) to the change originally created by mrbff. ( http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements ...................................................................... PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements The number of messages calculated before the call to message_splitter(), used in the memory allocation in the buffer array, could in certain cases be less than one than the actual number of messages, thus causing an override of the sentinel buffer in message_splitter() and therefore an invalid read in send_single_push_update(). The case in question would be, for example, a sequence of three options "A,B,C" with the size of B equal to safe_cap - 1 and the sum of the sizes of A and C less than safe_cap - 2. The buffer array was therefore replaced with a list of buffers to completely avoid calculating the number of messages before it was actually computed. The test case in question has been added to the unit tests. The unit tests have been improved using cmocka macros. Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9 Signed-off-by: Marco Baffo <ma...@ma...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1316 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34073.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/push_util.c M tests/unit_tests/openvpn/test_push_update_msg.c 2 files changed, 137 insertions(+), 57 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1316/7 diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c index 552679a..51c7b5f 100644 --- a/src/openvpn/push_util.c +++ b/src/openvpn/push_util.c @@ -91,7 +91,7 @@ * Return `false` on failure an `true` on success. */ static bool -message_splitter(const char *s, struct buffer *msgs, struct gc_arena *gc, const size_t safe_cap) +message_splitter(const char *s, struct buffer_list *msgs, struct gc_arena *gc, const size_t safe_cap) { if (!s || !*s) { @@ -100,7 +100,6 @@ char *str = gc_strdup(s, gc); size_t i = 0; - int im = 0; while (*str) { @@ -115,37 +114,38 @@ } str[ci] = '\0'; /* copy from i to (ci -1) */ - msgs[im] = forge_msg(str, ",push-continuation 2", gc); + struct buffer tmp = forge_msg(str, ",push-continuation 2", gc); + buffer_list_push(msgs, BSTR(&tmp)); i = ci + 1; } else { - if (im) + if (msgs->head) { - msgs[im] = forge_msg(str, ",push-continuation 1", gc); + struct buffer tmp = forge_msg(str, ",push-continuation 1", gc); + buffer_list_push(msgs, BSTR(&tmp)); } else { - msgs[im] = forge_msg(str, NULL, gc); + struct buffer tmp = forge_msg(str, NULL, gc); + buffer_list_push(msgs, BSTR(&tmp)); } i = strlen(str); } str = &str[i]; - im++; } return true; } /* send the message(s) prepared to one single client */ static bool -send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer *msgs) +send_single_push_update(struct multi_context *m, struct multi_instance *mi, struct buffer_list *msgs) { - if (!msgs[0].data || !*(msgs[0].data)) + if (!msgs->head) { return false; } - int i = -1; unsigned int option_types_found = 0; struct context *c = &mi->context; struct options o; @@ -160,9 +160,10 @@ o.ifconfig_local = canary; o.ifconfig_ipv6_local = canary; - while (msgs[++i].data && *(msgs[i].data)) + struct buffer_entry *e = msgs->head; + while (e) { - if (!send_control_channel_string(c, BSTR(&msgs[i]), D_PUSH)) + if (!send_control_channel_string(c, BSTR(&e->buf), D_PUSH)) { return false; } @@ -182,13 +183,14 @@ * Also we need to make a temporary copy so we can buf_advance() * without modifying original buffer. */ - struct buffer tmp_msg = msgs[i]; + struct buffer tmp_msg = e->buf; buf_string_compare_advance(&tmp_msg, push_update_cmd); unsigned int permission_mask = pull_permission_mask(c); if (process_push_update(c, &o, permission_mask, &option_types_found, &tmp_msg, true) == PUSH_MSG_ERROR) { msg(M_WARN, "Failed to process push update message sent to client ID: %u", c->c2.tls_multi->peer_id); } + e = e->next; } if (option_types_found & OPT_P_UP) @@ -270,12 +272,11 @@ * we want to send exceeds that size we have to split it into smaller messages */ ASSERT(push_bundle_size > extra); const size_t safe_cap = push_bundle_size - extra; - size_t msgs_num = (strlen(msg) / safe_cap) + ((strlen(msg) % safe_cap) != 0); - struct buffer *msgs = gc_malloc((msgs_num + 1) * sizeof(struct buffer), true, &gc); + struct buffer_list *msgs = buffer_list_new(); - msgs[msgs_num].data = NULL; if (!message_splitter(msg, msgs, &gc, safe_cap)) { + buffer_list_free(msgs); gc_free(&gc); return -EINVAL; } @@ -286,6 +287,7 @@ if (!mi) { + buffer_list_free(msgs); gc_free(&gc); return -ENOENT; } @@ -293,6 +295,7 @@ if (!support_push_update(mi)) { msg(M_CLIENT, "PUSH_UPDATE: not sending message to unsupported peer with ID: %u", mi->context.c2.tls_multi->peer_id); + buffer_list_free(msgs); gc_free(&gc); return 0; } @@ -300,11 +303,13 @@ if (!mi->halt && send_single_push_update(m, mi, msgs)) { + buffer_list_free(msgs); gc_free(&gc); return 1; } else { + buffer_list_free(msgs); gc_free(&gc); return 0; } @@ -334,6 +339,7 @@ } hash_iterator_free(&hi); + buffer_list_free(msgs); gc_free(&gc); return count; } diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c index 516e94c..6ef1266 100644 --- a/tests/unit_tests/openvpn/test_push_update_msg.c +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -130,18 +130,11 @@ return true; } #else /* ifndef ENABLE_MANAGEMENT */ -char **res; -int i; bool send_control_channel_string(struct context *c, const char *str, msglvl_t msglevel) { - if (res && res[i] && strcmp(res[i], str)) - { - printf("\n\nexpected: %s\n\n actual: %s\n\n", res[i], str); - return false; - } - i++; + check_expected(str); return true; } @@ -301,44 +294,75 @@ #ifdef ENABLE_MANAGEMENT char *r0[] = { - "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0" + "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0", + NULL }; char *r1[] = { "PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2", "PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway local,push-continuation 2", - "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1" + "PUSH_UPDATE,route 192.168.1.0 255.255.255.0,push-continuation 1", + NULL }; char *r3[] = { - "PUSH_UPDATE,,," + "PUSH_UPDATE,,,", + NULL }; char *r4[] = { "PUSH_UPDATE,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2", "PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2", - "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1" + "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1", + NULL }; char *r5[] = { "PUSH_UPDATE,,-dhcp-option, blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2", "PUSH_UPDATE, akakakakakakakakakakakaf,dhcp-option DNS 8.8.8.8, redirect-gateway local,push-continuation 2", - "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1" + "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,push-continuation 1", + NULL }; char *r6[] = { "PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2", "PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8, redirect-gateway 10.10.10.10,,push-continuation 2", - "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1" + "PUSH_UPDATE, route 192.168.1.0 255.255.255.0,,push-continuation 1", + NULL }; char *r7[] = { "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,push-continuation 2", - "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1" + "PUSH_UPDATE,,,,,,,,,,,,,,,,,,,push-continuation 1", + NULL }; char *r8[] = { "PUSH_UPDATE,-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf,push-continuation 2", "PUSH_UPDATE, akakakakakakakakakakakaf, dhcp-option DNS 8.8.8.8,redirect-gateway\n local,push-continuation 2", - "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1" + "PUSH_UPDATE,route 192.168.1.0 255.255.255.0\n\n\n,push-continuation 1", + NULL }; char *r9[] = { - "PUSH_UPDATE,," + "PUSH_UPDATE,,", + NULL }; - +char *r11[] = { + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2", + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2", + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2", + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 2", + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,push-continuation 1", + NULL +}; +char *r12[] = { + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,push-continuation 2", + "PUSH_UPDATE,abc,push-continuation 1", + NULL +}; +char *r13[] = { + "PUSH_UPDATE,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,", + NULL +}; +char *r14[] = { + "PUSH_UPDATE,a,push-continuation 2", + "PUSH_UPDATE,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,push-continuation 2", + "PUSH_UPDATE,a,push-continuation 1", + NULL +}; const char *msg0 = "redirect-gateway local,route 192.168.1.0 255.255.255.0"; const char *msg1 = "-dhcp-option,blablalalalalalalalalalalalalf, lalalalalalalalalalalalalalaf," @@ -365,32 +389,50 @@ "daisy damage damp dance danger daring dash daughter dawn day deal debate debris decade december decide decline" "decorate decrease deer defense define defy degree delay deliver demand demise denial"; +const char *msg11 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a," + "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a," + "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a," + "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a," + "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a"; + +const char *msg12 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,abc"; + +const char *msg13 = "a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,a,,,,,,a,"; + +const char *msg14 = "a,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,a"; + #define PUSH_BUNDLE_SIZE_TEST 184 +#define expect_control_channel_strings(res) \ + do \ + { \ + for (int j = 0; res[j] != NULL; j++) \ + { \ + expect_string(send_control_channel_string, str, res[j]); \ + } \ + } while (0) + static void test_send_push_msg0(void **state) { - i = 0; - res = r0; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r0); assert_int_equal(send_push_update(m, &cid, msg0, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } + static void test_send_push_msg1(void **state) { - i = 0; - res = r1; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r1); assert_int_equal(send_push_update(m, &cid, msg1, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg2(void **state) { - i = 0; - res = NULL; struct multi_context *m = *state; const unsigned long cid = 0; assert_int_equal(send_push_update(m, &cid, msg2, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL); @@ -399,83 +441,110 @@ static void test_send_push_msg3(void **state) { - i = 0; - res = r3; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r3); assert_int_equal(send_push_update(m, &cid, msg3, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg4(void **state) { - i = 0; - res = r4; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r4); assert_int_equal(send_push_update(m, &cid, msg4, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg5(void **state) { - i = 0; - res = r5; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r5); assert_int_equal(send_push_update(m, &cid, msg5, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg6(void **state) { - i = 0; - res = r6; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r6); assert_int_equal(send_push_update(m, &cid, msg6, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg7(void **state) { - i = 0; - res = r7; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r7); assert_int_equal(send_push_update(m, &cid, msg7, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg8(void **state) { - i = 0; - res = r8; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r8); assert_int_equal(send_push_update(m, &cid, msg8, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg9(void **state) { - i = 0; - res = r9; struct multi_context *m = *state; const unsigned long cid = 0; + expect_control_channel_strings(r9); assert_int_equal(send_push_update(m, &cid, msg9, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); } static void test_send_push_msg10(void **state) { - i = 0; - res = NULL; struct multi_context *m = *state; const unsigned long cid = 0; assert_int_equal(send_push_update(m, &cid, msg10, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), -EINVAL); } +static void +test_send_push_msg11(void **state) +{ + struct multi_context *m = *state; + const unsigned long cid = 0; + expect_control_channel_strings(r11); + assert_int_equal(send_push_update(m, &cid, msg11, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); +} + +static void +test_send_push_msg12(void **state) +{ + struct multi_context *m = *state; + const unsigned long cid = 0; + expect_control_channel_strings(r12); + assert_int_equal(send_push_update(m, &cid, msg12, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); +} + +static void +test_send_push_msg13(void **state) +{ + struct multi_context *m = *state; + const unsigned long cid = 0; + expect_control_channel_strings(r13); + assert_int_equal(send_push_update(m, &cid, msg13, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); +} + +static void +test_send_push_msg14(void **state) +{ + struct multi_context *m = *state; + const unsigned long cid = 0; + expect_control_channel_strings(r14); + assert_int_equal(send_push_update(m, &cid, msg14, UPT_BY_CID, PUSH_BUNDLE_SIZE_TEST), 1); +} + #undef PUSH_BUNDLE_SIZE_TEST static int @@ -535,6 +604,7 @@ cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown), cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown), #ifdef ENABLE_MANAGEMENT + cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2), cmocka_unit_test_setup_teardown(test_send_push_msg1, setup2, teardown2), cmocka_unit_test_setup_teardown(test_send_push_msg2, setup2, teardown2), @@ -545,7 +615,11 @@ cmocka_unit_test_setup_teardown(test_send_push_msg7, setup2, teardown2), cmocka_unit_test_setup_teardown(test_send_push_msg8, setup2, teardown2), cmocka_unit_test_setup_teardown(test_send_push_msg9, setup2, teardown2), - cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2) + cmocka_unit_test_setup_teardown(test_send_push_msg10, setup2, teardown2), + cmocka_unit_test_setup_teardown(test_send_push_msg11, setup2, teardown2), + cmocka_unit_test_setup_teardown(test_send_push_msg12, setup2, teardown2), + cmocka_unit_test_setup_teardown(test_send_push_msg13, setup2, teardown2), + cmocka_unit_test_setup_teardown(test_send_push_msg14, setup2, teardown2) #endif }; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1316?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Idba419681fe3ccc4e6e2f6ce7592332dcff62cd9 Gerrit-Change-Number: 1316 Gerrit-PatchSet: 7 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> 

I have stared-at-code for a while (looks reasonable), subjected to the client/server testbed (which does not actually *do* PUSH_UPDATE yet, still), and ran a few manual tests. This all works as it did before - did not try to find new and interesting edge cases. (I did *not* actively try, but still found #889...) We might eventually clean up the repeated { + buffer_list_free(msgs); gc_free(&gc); return 1; } clauses into a common "cleanup:" clause... but this is also not important right now. We should refrain from adding even more cleanups there, though ;-) The extra unit tests are very welcome. Your patch has been applied to the master branch. commit 6b0208e962aadf285ecc7ab47cc973a9018e3f24 Author: Marco Baffo Date: Thu Oct 30 20:52:35 2025 +0100 PUSH_UPDATE server: invalid read bug-fix and unit-tests improvements Signed-off-by: Marco Baffo <ma...@ma...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1316 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34073.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Attention is currently required from: MaxF, cron2, flichtenheld. plaisthos has posted comments on this change by MaxF. ( http://gerrit.openvpn.net/c/openvpn/+/1315?usp=email ) Change subject: Zeroize tls-crypt-v2 client keys ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1315?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Icfcbf8ee20c1c0016eb98b570f24b9325b157c5c Gerrit-Change-Number: 1315 Gerrit-PatchSet: 2 Gerrit-Owner: MaxF <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: MaxF <ma...@ma...> Gerrit-Comment-Date: Fri, 31 Oct 2025 10:05:53 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes 

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1334?usp=email ) Change subject: Add -lpathcch for mingw32 builds using autotools ...................................................................... Add -lpathcch for mingw32 builds using autotools This was missed in commit 05a8ba8 Note: the check for PATHCCH_ENSURE_TRAILING_SLASH in configure.ac may be omitted if we build only using latest mingw32-w64 toolchain. Ubuntu 24.04 is not new enough. Github: closes OpenVPN/openvpn#885 Change-Id: Ifea896e722635a471cc01f930bc1e5d0f2c165be Signed-off-by: Selva Nair <sel...@gm...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1334 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34095.html Signed-off-by: Gert Doering <ge...@gr...> --- M configure.ac M src/openvpnserv/Makefile.am 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 8f3c01d..3117e13 100644 --- a/configure.ac +++ b/configure.ac @@ -1383,6 +1383,17 @@	fi fi +if test "${WIN32}" == "yes"; then +	AC_CHECK_DECLS( +	[PATHCCH_ENSURE_TRAILING_SLASH], +	[AC_DEFINE([HAVE_PATHCCH_ENSURE_TRAILING_SLASH], [1], [PATHCCH_ENSURE_TRAILING_SLASH is defined])], +	, +	[[ +	#include <pathcch.h> +	]] +	) +fi + # When testing a compiler option, we add -Werror to force # an error when the option is unsupported. This is not # required for gcc, but some compilers such as clang need it. diff --git a/src/openvpnserv/Makefile.am b/src/openvpnserv/Makefile.am index a27fbbf..f45d770 100644 --- a/src/openvpnserv/Makefile.am +++ b/src/openvpnserv/Makefile.am @@ -27,7 +27,7 @@	-D_WIN32_WINNT=_WIN32_WINNT_VISTA openvpnserv_LDADD = \	-ladvapi32 -luserenv -liphlpapi -lfwpuclnt -lrpcrt4 \ -	-lshlwapi -lnetapi32 -lws2_32 -lntdll -lole32 +	-lshlwapi -lnetapi32 -lws2_32 -lntdll -lole32 -lpathcch noinst_DATA = \	MSG00409.bin eventmsg.h eventmsg.rc openvpnservmsg.dll BUILT_SOURCES = \ -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1334?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ifea896e722635a471cc01f930bc1e5d0f2c165be Gerrit-Change-Number: 1334 Gerrit-PatchSet: 2 Gerrit-Owner: selvanair <sel...@gm...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> 

cron2 has uploaded a new patch set (#2) to the change originally created by selvanair. ( http://gerrit.openvpn.net/c/openvpn/+/1334?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: Add -lpathcch for mingw32 builds using autotools ...................................................................... Add -lpathcch for mingw32 builds using autotools This was missed in commit 05a8ba8 Note: the check for PATHCCH_ENSURE_TRAILING_SLASH in configure.ac may be omitted if we build only using latest mingw32-w64 toolchain. Ubuntu 24.04 is not new enough. Github: closes OpenVPN/openvpn#885 Change-Id: Ifea896e722635a471cc01f930bc1e5d0f2c165be Signed-off-by: Selva Nair <sel...@gm...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1334 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34095.html Signed-off-by: Gert Doering <ge...@gr...> --- M configure.ac M src/openvpnserv/Makefile.am 2 files changed, 12 insertions(+), 1 deletion(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/34/1334/2 diff --git a/configure.ac b/configure.ac index 8f3c01d..3117e13 100644 --- a/configure.ac +++ b/configure.ac @@ -1383,6 +1383,17 @@	fi fi +if test "${WIN32}" == "yes"; then +	AC_CHECK_DECLS( +	[PATHCCH_ENSURE_TRAILING_SLASH], +	[AC_DEFINE([HAVE_PATHCCH_ENSURE_TRAILING_SLASH], [1], [PATHCCH_ENSURE_TRAILING_SLASH is defined])], +	, +	[[ +	#include <pathcch.h> +	]] +	) +fi + # When testing a compiler option, we add -Werror to force # an error when the option is unsupported. This is not # required for gcc, but some compilers such as clang need it. diff --git a/src/openvpnserv/Makefile.am b/src/openvpnserv/Makefile.am index a27fbbf..f45d770 100644 --- a/src/openvpnserv/Makefile.am +++ b/src/openvpnserv/Makefile.am @@ -27,7 +27,7 @@	-D_WIN32_WINNT=_WIN32_WINNT_VISTA openvpnserv_LDADD = \	-ladvapi32 -luserenv -liphlpapi -lfwpuclnt -lrpcrt4 \ -	-lshlwapi -lnetapi32 -lws2_32 -lntdll -lole32 +	-lshlwapi -lnetapi32 -lws2_32 -lntdll -lole32 -lpathcch noinst_DATA = \	MSG00409.bin eventmsg.h eventmsg.rc openvpnservmsg.dll BUILT_SOURCES = \ -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1334?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ifea896e722635a471cc01f930bc1e5d0f2c165be Gerrit-Change-Number: 1334 Gerrit-PatchSet: 2 Gerrit-Owner: selvanair <sel...@gm...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...>