Opened 10 years ago
Closed 8 years ago
#276 closed Bug / Defect (fixed)
Segmentation fault if signal received during address resolution
Reported by: | MacGyver | Owned by: | plaisthos |
---|---|---|---|
Priority: | minor | Milestone: | release 2.3.7 |
Component: | Networking | Version: | OpenVPN 2.3.0 (Community Ed) |
Severity: | Not set (select this one, unless your'e a OpenVPN developer) | Keywords: | segfault dns sigint |
Cc: | plaisthos |
Description
If an interrupt signal is sent to the openvpn process during name resolution there is a chance that the process will crash with a segmentation fault instead of exiting cleanly.
Affected source files: socket.c, observed in 2.3.0 and in master. Lines 1264-1289 and lines 260-272 at the moment of writing.
Cause: If a signal (SIGINT) is received during name resolution in openvpn_getaddrinfo(...) called in resolve_remote(...), the address resolution fails and *ai is filled with bogus data. However, openvpn_getaddrinfo still returns 0 on exit, prompting a copy of ai->ai_addr - signal checking is done *after* this potential copy, not before. Since the pointer is bogus data, odds are that it's outside of memory bounds and therefore will cause a segmentation fault before openvpn has a chance to handle the SIGINT.
Attachments (1)
Change History (9)
comment:1 Changed 9 years ago by
Keywords: | resolution removed |
---|---|
Priority: | major → minor |
comment:3 Changed 9 years ago by
Owner: | set to plaisthos |
---|---|
Status: | new → assigned |
Plaisthos' dual-stack patches fix this issue. They will be merged into Git master shortly.
comment:5 Changed 8 years ago by
Cc: | plaisthos added |
---|---|
Milestone: | → release 2.3.8 |
comment:6 Changed 8 years ago by
I *think* it might actually be these beauty here, in openvpn_getaddrinfo()
if (*signal_received == SIGUSR1) /* ignore SIGUSR1 */
{
msg (level, "RESOLVE: Ignored SIGUSR1 signal received duri
ng DNS resolution attempt");
*signal_received = 0;
}
else
{
if (0 == status) {
ASSERT(res);
freeaddrinfo(*res);
res = NULL;
}
goto done;
... which, if a signal is received while in getaddrinfo() *but* getaddrinfo() succeeds anyway (to reproduce: put a non-reachable nameserver first in resolv.conf, a working one second, switchover takes a few seconds, kill -INT in between), it will actually destroy the returned data AND fail to null the pointer, so the caller gets pointed to memory that might or might not still have valid data.
On my FreeBSDs, I cannot reproduce the crash, but if I change the code to read "*res = NULL", it will nicely crash in resolve_remote(), trying to access ai->ai_addr - as it should be, if the data has been freeaddrinfo()ed...
So, enough talking to myself, I *think* a proper fix might look like this (for release/2.3, with some fuzz due to extra debugging, but should apply to 2.3.6 as well - there has been a code change for 2.3.4, not actually fixing the problem but hiding it better - 47a963681a6ed):
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -219,10 +219,13 @@ msg(M_INFO, "if(*signal_received)");
}
else
{
+ /* turn success into failure (interrupted syscall) */
if (0 == status) {
ASSERT(res);
freeaddrinfo(*res);
- res = NULL;
+ *res = NULL;
+ status = EAI_SYSTEM;
+ errno = EINTR;
}
goto done;
}
(not only fixing the spurious bug, but also introducing generall well-behaviour here - if we freeadrinfo(*res) we should no longer claim "success!")
Plaisthos, what do you think?
MacGyver?: can you reproduce the actual crash in your environment, and if yes, could you give this patch a try?
(As a side note: this code is also in git master, but as the surrounding code is different, it might be harder to trigger now - but should be fixed anyway)
Changed 8 years ago by
Attachment: | 0001-On-signal-reception-return-EAI_SYSTEM-from-openvpn_g.patch added |
---|
patch as described
comment:7 Changed 8 years ago by
Milestone: | release 2.3.8 → release 2.3.7 |
---|
comment:8 Changed 8 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Patch has been applied to the master and release/2.3 branch.
commit 5f6c01ea6172ed1d8ed04e31f9f6c3f8e4696109 (master)
commit 38c9f980d4f7cb7061f9db3ed8645ab3404e533d (release/2.3)
Author: Gert Doering
Date: Sun May 31 22:41:58 2015 +0200
On signal reception, return EAI_SYSTEM from openvpn_getaddrinfo().
Signed-off-by: Gert Doering <gert@…>
Acked-by: Arne Schwabe <arne@…>
Message-Id: <1433104918-9523-1-git-send-email-gert@…>
URL: http://article.gmane.org/gmane.network.openvpn.devel/9764
I'm confident that this is the right thing, and Plaisthos ACKed it :-) - so I'm closing this ticket. MacGyver?: if you can still reproduce the crashes, I have overlooked something (obviously), so please reopen.
I can see that this could be quite annoying if name resolution is slow or does not work at all, and you'd need to kill the OpenVPN daemon. Does the segmentation fault happen for every type of signal (see man openvpn) or just SIGINT/SIGTERM?