Opened 8 years ago

Closed 5 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)

0001-On-signal-reception-return-EAI_SYSTEM-from-openvpn_g.patch (1.5 KB) - added by Gert Döring 5 years ago.
patch as described

Download all attachments as: .zip

Change History (9)

comment:1 Changed 7 years ago by Samuli Seppänen

Keywords: resolution removed
Priority: majorminor

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?

comment:2 Changed 7 years ago by Samuli Seppänen

This seems somewhat related to #311.

comment:3 Changed 7 years ago by Samuli Seppänen

Owner: set to plaisthos
Status: newassigned

Plaisthos' dual-stack patches fix this issue. They will be merged into Git master shortly.

comment:4 Changed 5 years ago by Samuli Seppänen

Fixed in Git "master". The 2.3 branch still has this issue.

comment:5 Changed 5 years ago by Gert Döring

Cc: plaisthos added
Milestone: release 2.3.8

comment:6 Changed 5 years ago by Gert Döring

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 5 years ago by Gert Döring

patch as described

comment:7 Changed 5 years ago by Gert Döring

Milestone: release 2.3.8release 2.3.7

comment:8 Changed 5 years ago by Gert Döring

Resolution: fixed
Status: assignedclosed

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.

Note: See TracTickets for help on using tickets.