hardening: add safe FD_SET() wrapper openvpn_fd_set()
PATCH 1/1 hardening: add safe FD_SET() wrapper openvpn_fd_set()
From: Steffan Karger <[email protected]...>
On many platforms (not Windows, for once), FD_SET() can write outside the given fd_set if an fd >= FD_SETSIZE is given. To make sure we don't do that, add an ASSERT() to error out with a clear error message when this does happen.
This patch was inspired by remarks about FD_SET() from Sebastian Krahmer of the SuSE Security Team.
--- src/openvpn/event.c | 8 ++++---- src/openvpn/fdmisc.h | 16 ++++++++++++++++ src/openvpn/proxy.c | 2 +- src/openvpn/socket.c | 4 ++-- src/openvpn/socks.c | 6 +++--- 5 files changed, 26 insertions(+), 10 deletions(-)
src/openvpn/event.c +4 -4
@@ -873,18 +873,18 @@ se_ctl (struct event_set *es, event_t event, unsigned int rwflags, void *arg)if (ses->fast) {if (rwflags &EVENT_READ ) -FD_SET (event, &ses->readfds); +openvpn_fd_set (event, &ses->readfds);if (rwflags &EVENT_WRITE ) -FD_SET (event, &ses->writefds); +openvpn_fd_set (event, &ses->writefds); }else {if (rwflags &EVENT_READ ) -FD_SET (event, &ses->readfds); +openvpn_fd_set (event, &ses->readfds);else FD_CLR (event, &ses->readfds);if (rwflags &EVENT_WRITE ) -FD_SET (event, &ses->writefds); +openvpn_fd_set (event, &ses->writefds);else FD_CLR (event, &ses->writefds); }
src/openvpn/fdmisc.h +16 -0
@@ -22,10 +22,26 @@* 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ +#ifndef FD_MISC_H +#define FD_MISC_H +#include "basic.h" +#include "error.h" +#include "syshead.h" bool set_nonblock_action (int fd);bool set_cloexec_action (int fd);void set_nonblock (int fd);void set_cloexec (int fd); + +static inline void openvpn_fd_set (int fd,fd_set *setp) +{ +#ifndef WIN32 /* The Windows FD_SET() implementation does not overflow */ +ASSERT (fd >=0 && fd <FD_SETSIZE ); +#endif +FD_SET (fd, setp); +} +#undef FD_SET/* prevent direct use of FD_SET() */ + +#endif /* FD_MISC_H */
src/openvpn/proxy.c +1 -1
@@ -92,7 +92,7 @@ recv_line (socket_descriptor_t sd, }FD_ZERO (&reads); -FD_SET (sd, &reads); +openvpn_fd_set (sd, &reads); tv.tv_sec = timeout_sec; tv.tv_usec =0 ;
src/openvpn/socket.c +2 -2
@@ -1003,7 +1003,7 @@ socket_listen_accept (socket_descriptor_t sd,struct timeval tv;FD_ZERO (&reads); -FD_SET (sd, &reads); +openvpn_fd_set (sd, &reads); tv.tv_sec =0 ; tv.tv_usec =0 ;
@@ -1153,7 +1153,7 @@ openvpn_connect (socket_descriptor_t sd,struct timeval tv;FD_ZERO (&writes); -FD_SET (sd, &writes); +openvpn_fd_set (sd, &writes); tv.tv_sec =0 ; tv.tv_usec =0 ;
src/openvpn/socks.c +3 -3
@@ -134,7 +134,7 @@ socks_username_password_auth (struct socks_proxy_info *p,char c;FD_ZERO (&reads); -FD_SET (sd, &reads); +openvpn_fd_set (sd, &reads); tv.tv_sec = timeout_sec; tv.tv_usec =0 ;
@@ -213,7 +213,7 @@ socks_handshake (struct socks_proxy_info *p,char c;FD_ZERO (&reads); -FD_SET (sd, &reads); +openvpn_fd_set (sd, &reads); tv.tv_sec = timeout_sec; tv.tv_usec =0 ;
@@ -319,7 +319,7 @@ recv_socks_reply (socket_descriptor_t sd,char c;FD_ZERO (&reads); -FD_SET (sd, &reads); +openvpn_fd_set (sd, &reads); tv.tv_sec = timeout_sec; tv.tv_usec =0 ;
Signature · 1 line
2.5.0
ACK.
The FD_SET() calls in event.c should all be safe as there are checks already, but many of the others are not checked - "usually" we're well in the range of FD_SETSIZE with our file descriptors, and "usually" platforms are using poll() anyway, but in the exceptional case, having a clear ASSERT() is much better than silent malfunction or a crash.
As explanation for the archives why this is #ifndef WIN32 - the windows file handles do not follow the same "start with 0, count up" approach as unix file descriptors, so the windows FD_SET() API is actually more similar to unix poll() - the file descriptor is added to a list, instead of just setting the bit in an array (of FD_SETSIZE bits) that corresponds to the numeric value.
Your patch has been applied to the master and release/2.3 branch.
commit e0b3fd49e2b5bba8cb57419a13cb75b56ac91b94 (master) commit 1746908f66f5517a525ee2c114a0f7104c29dfad (release/2.3)
Author: Steffan Karger Date: Thu Mar 3 10:22:48 2016 +0100
hardening: add safe FD_SET() wrapper openvpn_fd_set()
Signed-off-by: Steffan Karger <[email protected]...>
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: http://article.gmane.org/gmane.network.openvpn.devel/11285
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering