Menu
▾
▴
openvpn-devel
|
From: klemens (C. Review) <ge...@op...> - 2025-12-06 14:09:24
|
Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1418?usp=email
to review the following change.
Change subject: Prevent crash on invalid server-ipv6 argument
......................................................................
Prevent crash on invalid server-ipv6 argument
`get_addr_generic()` expects `openvpn_getaddrinfo()` to return a newly
allocated struct, but getaddrinfo(3) failure leaves `*ai = NULL` as-is.
Unlike free(3), freegetaddrinfo(3) requires a valid struct, thus the
following is enough to trigger a NULL pointer dereference in libc:
```
$ openvpn --server-ipv6 ''
2025-12-06 11:59:18 RESOLVE: Cannot resolve host address: :[AF_INET6] (no address associated with name)
Segmentation fault (core dumped)
```
Guard against empty `ai`, i.e. failure, like similar code already does:
```
$ ./openvpn --server-ipv6 ''
2025-12-06 12:05:11 RESOLVE: Cannot resolve host address: :[AF_INET6] (no address associated with name)
Options error: error parsing --server-ipv6 parameter
Use --help for more information.
```
Spotted through a configuration typo "server-ipv6 fd00:/64" with 2.6.17,
reproduced with and tested against 2.7rc3 on OpenBSD/amd64 7.8-current.
Change-Id: I99a6604fdfc682f9609bfe7672aa78285084dcb9
Signed-off-by: Klemens Nanni <kn...@op...>
---
M src/openvpn/socket.c
1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/18/1418/1
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 46bedf4..80c2895 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -189,7 +189,10 @@
*sep = '/';
}
out:
- freeaddrinfo(ai);
+ if (ai)
+ {
+ freeaddrinfo(ai);
+ }
free(var_host);
return ret;
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1418?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: I99a6604fdfc682f9609bfe7672aa78285084dcb9
Gerrit-Change-Number: 1418
Gerrit-PatchSet: 1
Gerrit-Owner: klemens <kn...@op...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-12-06 20:54:40
|
Attention is currently required from: klemens, plaisthos. cron2 has posted comments on this change by klemens. ( http://gerrit.openvpn.net/c/openvpn/+/1418?usp=email ) Change subject: Prevent crash on invalid server-ipv6 argument ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1418?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: I99a6604fdfc682f9609bfe7672aa78285084dcb9 Gerrit-Change-Number: 1418 Gerrit-PatchSet: 1 Gerrit-Owner: klemens <kn...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: klemens <kn...@op...> Gerrit-Comment-Date: Sat, 06 Dec 2025 20:54:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes |
|
From: Gert D. <ge...@gr...> - 2025-12-06 20:55:13
|
From: Klemens Nanni <kn...@op...> `get_addr_generic()` expects `openvpn_getaddrinfo()` to return a newly allocated struct, but getaddrinfo(3) failure leaves `*ai = NULL` as-is. Unlike free(3), freegetaddrinfo(3) requires a valid struct, thus the following is enough to trigger a NULL pointer dereference in libc: ``` $ openvpn --server-ipv6 '' 2025-12-06 11:59:18 RESOLVE: Cannot resolve host address: :[AF_INET6] (no address associated with name) Segmentation fault (core dumped) ``` Guard against empty `ai`, i.e. failure, like similar code already does: ``` $ ./openvpn --server-ipv6 '' 2025-12-06 12:05:11 RESOLVE: Cannot resolve host address: :[AF_INET6] (no address associated with name) Options error: error parsing --server-ipv6 parameter Use --help for more information. ``` Spotted through a configuration typo "server-ipv6 fd00:/64" with 2.6.17, reproduced with and tested against 2.7rc3 on OpenBSD/amd64 7.8-current. Change-Id: I99a6604fdfc682f9609bfe7672aa78285084dcb9 Signed-off-by: Klemens Nanni <kn...@op...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1418 --- 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/+/1418 This mail reflects revision 1 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 46bedf4..80c2895 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -189,7 +189,10 @@ *sep = '/'; } out: - freeaddrinfo(ai); + if (ai) + { + freeaddrinfo(ai); + } free(var_host); return ret; |
|
From: cron2 (C. Review) <ge...@op...> - 2025-12-06 20:57:31
|
Attention is currently required from: klemens, plaisthos. cron2 has posted comments on this change by klemens. ( http://gerrit.openvpn.net/c/openvpn/+/1418?usp=email ) Change subject: Prevent crash on invalid server-ipv6 argument ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: On FreeBSD and Linux, freeaddrinfo(NULL) seems to be safe, but the comment inside FreeBSD's lib sources hint at "this is not clearly defined in the standard" and it seems OpenBSD prefes to make that point very clear. We do actually check getaddrinfo() return everywhere else and only do `freeaddrinfo(ai)` in the success branch, or after testing for `not NULL`, so this is a very reasonable change. (I might add some wording about this to the commit message) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1418?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: I99a6604fdfc682f9609bfe7672aa78285084dcb9 Gerrit-Change-Number: 1418 Gerrit-PatchSet: 1 Gerrit-Owner: klemens <kn...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: klemens <kn...@op...> Gerrit-Comment-Date: Sat, 06 Dec 2025 20:57:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No |
|
From: klemens (C. Review) <ge...@op...> - 2025-12-07 13:31:52
|
Attention is currently required from: cron2, plaisthos. klemens has posted comments on this change by klemens. ( http://gerrit.openvpn.net/c/openvpn/+/1418?usp=email ) Change subject: Prevent crash on invalid server-ipv6 argument ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > (I might add some wording about this to the commit message) Done, thanks. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1418?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: I99a6604fdfc682f9609bfe7672aa78285084dcb9 Gerrit-Change-Number: 1418 Gerrit-PatchSet: 2 Gerrit-Owner: klemens <kn...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Comment-Date: Sun, 07 Dec 2025 13:31:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> |
|
From: cron2 (C. Review) <ge...@op...> - 2025-12-07 21:05:25
|
Attention is currently required from: klemens, plaisthos. cron2 has posted comments on this change by klemens. ( http://gerrit.openvpn.net/c/openvpn/+/1418?usp=email ) Change subject: Prevent crash on invalid server-ipv6 argument ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1418?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: I99a6604fdfc682f9609bfe7672aa78285084dcb9 Gerrit-Change-Number: 1418 Gerrit-PatchSet: 2 Gerrit-Owner: klemens <kn...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: klemens <kn...@op...> Gerrit-Comment-Date: Sun, 07 Dec 2025 21:05:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes |
|
From: Gert D. <ge...@gr...> - 2025-12-07 21:05:44
|
From: Klemens Nanni <kn...@op...>
`get_addr_generic()` expects `openvpn_getaddrinfo()` to return a newly
allocated struct, but getaddrinfo(3) failure leaves `*ai = NULL` as-is.
On OpenBSD, unlike free(3), freegetaddrinfo(3) requires a valid struct,
thus callers must check the argument to avoid NULL-deref or double-free:
```
$ openvpn --server-ipv6 ''
2025-12-06 11:59:18 RESOLVE: Cannot resolve host address: :[AF_INET6] (no address associated with name)
Segmentation fault (core dumped)
```
Guard against empty `ai`, i.e. failure, like similar code already does:
```
$ ./openvpn --server-ipv6 ''
2025-12-06 12:05:11 RESOLVE: Cannot resolve host address: :[AF_INET6] (no address associated with name)
Options error: error parsing --server-ipv6 parameter
Use --help for more information.
```
Spotted through a configuration typo "server-ipv6 fd00:/64" with 2.6.17,
reproduced with and tested against 2.7rc3 on OpenBSD/amd64 7.8-current.
NB: Standards are unclear wrt. freeaddrinfo(3)'s NULL handling;
Linux, FreeBSD and illumos do check it and thus not crash.
Change-Id: I99a6604fdfc682f9609bfe7672aa78285084dcb9
Signed-off-by: Klemens Nanni <kn...@op...>
Acked-by: Gert Doering <ge...@gr...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1418
---
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/+/1418
This mail reflects revision 2 of this Change.
Acked-by according to Gerrit (reflected above):
Gert Doering <ge...@gr...>
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 46bedf4..80c2895 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -189,7 +189,10 @@
*sep = '/';
}
out:
- freeaddrinfo(ai);
+ if (ai)
+ {
+ freeaddrinfo(ai);
+ }
free(var_host);
return ret;
|
|
From: Gert D. <ge...@gr...> - 2025-12-08 11:34:00
|
Thanks for your patch. Repeating what was said in https://github.com/OpenVPN/openvpn/pull/930, for the sake of the archives - on FreeBSD and Linux, this does not crash, because the libraries handle "ai == NULL" gracefully. The FreeBSD implementation mentions that the standard is not clear, so OpenBSD is free to crash on us. This said, we do guard all other paths to freeaddrinfo() (either because the call is only on the "success" branch, or with an explicit check) - so this is a good fix to make our code consistent. Your patch has been applied to the master and release/2.6 branch. commit 0ff66c056f951dcf01cf6ccb3e9b21948e5ca5ad (master) commit 09c35f8421028ce8aed4895a526c2f4c4b4be01b (release/2.6) Author: Klemens Nanni Date: Sun Dec 7 22:05:18 2025 +0100 Prevent crash on invalid server-ipv6 argument Signed-off-by: Klemens Nanni <kn...@op...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1418 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34870.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering |
|
From: cron2 (C. Review) <ge...@op...> - 2025-12-08 11:34:19
|
cron2 has uploaded a new patch set (#3) to the change originally created by klemens. ( http://gerrit.openvpn.net/c/openvpn/+/1418?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: Prevent crash on invalid server-ipv6 argument ...................................................................... Prevent crash on invalid server-ipv6 argument `get_addr_generic()` expects `openvpn_getaddrinfo()` to return a newly allocated struct, but getaddrinfo(3) failure leaves `*ai = NULL` as-is. On OpenBSD, unlike free(3), freegetaddrinfo(3) requires a valid struct, thus callers must check the argument to avoid NULL-deref or double-free: ``` $ openvpn --server-ipv6 '' 2025-12-06 11:59:18 RESOLVE: Cannot resolve host address: :[AF_INET6] (no address associated with name) Segmentation fault (core dumped) ``` Guard against empty `ai`, i.e. failure, like similar code already does: ``` $ ./openvpn --server-ipv6 '' 2025-12-06 12:05:11 RESOLVE: Cannot resolve host address: :[AF_INET6] (no address associated with name) Options error: error parsing --server-ipv6 parameter Use --help for more information. ``` Spotted through a configuration typo "server-ipv6 fd00:/64" with 2.6.17, reproduced with and tested against 2.7rc3 on OpenBSD/amd64 7.8-current. NB: Standards are unclear wrt. freeaddrinfo(3)'s NULL handling; Linux, FreeBSD and illumos do check it and thus not crash. Github: fixes OpenVPN/openvpn#930 Change-Id: I99a6604fdfc682f9609bfe7672aa78285084dcb9 Signed-off-by: Klemens Nanni <kn...@op...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1418 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34870.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/socket.c 1 file changed, 4 insertions(+), 1 deletion(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/18/1418/3 diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 46bedf4..80c2895 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -189,7 +189,10 @@ *sep = '/'; } out: - freeaddrinfo(ai); + if (ai) + { + freeaddrinfo(ai); + } free(var_host); return ret; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1418?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: I99a6604fdfc682f9609bfe7672aa78285084dcb9 Gerrit-Change-Number: 1418 Gerrit-PatchSet: 3 Gerrit-Owner: klemens <kn...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> |
|
From: cron2 (C. Review) <ge...@op...> - 2025-12-08 11:34:21
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1418?usp=email ) Change subject: Prevent crash on invalid server-ipv6 argument ...................................................................... Prevent crash on invalid server-ipv6 argument `get_addr_generic()` expects `openvpn_getaddrinfo()` to return a newly allocated struct, but getaddrinfo(3) failure leaves `*ai = NULL` as-is. On OpenBSD, unlike free(3), freegetaddrinfo(3) requires a valid struct, thus callers must check the argument to avoid NULL-deref or double-free: ``` $ openvpn --server-ipv6 '' 2025-12-06 11:59:18 RESOLVE: Cannot resolve host address: :[AF_INET6] (no address associated with name) Segmentation fault (core dumped) ``` Guard against empty `ai`, i.e. failure, like similar code already does: ``` $ ./openvpn --server-ipv6 '' 2025-12-06 12:05:11 RESOLVE: Cannot resolve host address: :[AF_INET6] (no address associated with name) Options error: error parsing --server-ipv6 parameter Use --help for more information. ``` Spotted through a configuration typo "server-ipv6 fd00:/64" with 2.6.17, reproduced with and tested against 2.7rc3 on OpenBSD/amd64 7.8-current. NB: Standards are unclear wrt. freeaddrinfo(3)'s NULL handling; Linux, FreeBSD and illumos do check it and thus not crash. Github: fixes OpenVPN/openvpn#930 Change-Id: I99a6604fdfc682f9609bfe7672aa78285084dcb9 Signed-off-by: Klemens Nanni <kn...@op...> Acked-by: Gert Doering <ge...@gr...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1418 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg34870.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/socket.c 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 46bedf4..80c2895 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -189,7 +189,10 @@ *sep = '/'; } out: - freeaddrinfo(ai); + if (ai) + { + freeaddrinfo(ai); + } free(var_host); return ret; -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1418?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: I99a6604fdfc682f9609bfe7672aa78285084dcb9 Gerrit-Change-Number: 1418 Gerrit-PatchSet: 3 Gerrit-Owner: klemens <kn...@op...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> |
Thanks for helping keep SourceForge clean.
X