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/+/1706?usp=email
to review the following change.
Change subject: socket: assert buffer length before reading prepended sockaddr
family
......................................................................
socket: assert buffer length before reading prepended sockaddr family
read_sockaddr_from_packet() inspected sa->sa_family before any check
on buf->len, so a short delivery from the dco-win driver would have
produced a garbage peer address from uninitialized buffer memory.
The driver always prepends a full sockaddr and validates the family
before writing, so reaching any of the size/family checks would mean
something is severely wrong on the driver side - assert the three
preconditions instead of M_FATAL'ing on them.
GitHub: https://github.com/OpenVPN/openvpn-private-issues/issues/105
Change-Id: I2ce954aa5b74002be5e38d53783435736625bb2f
Signed-off-by: Lev Stipakov <[email protected]>
---
M src/openvpn/socket.c
1 file changed, 11 insertions(+), 18 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/06/1706/1
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 624ce4f..df2cc9e 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2813,38 +2813,31 @@
{
int sa_len = 0;
+ /* In dco-win multipeer mode the kernel driver always prepends a full
+ * sockaddr_in or sockaddr_in6 in front of the control-packet payload,
+ * so the buffer must hold at least sizeof(struct sockaddr_in) bytes
+ * before we may inspect sa_family. */
+ ASSERT(buf_len(buf) >= (int)sizeof(struct sockaddr_in));
+
const struct sockaddr *sa = (const struct sockaddr *)BPTR(buf);
switch (sa->sa_family)
{
case AF_INET:
sa_len = sizeof(struct sockaddr_in);
- if (buf_len(buf) < sa_len)
- {
- msg(M_FATAL,
- "ERROR: received incoming packet with too short length of
%d -- must be at least %d.",
- buf_len(buf), sa_len);
- }
- memcpy(dst, sa, sa_len);
- buf_advance(buf, sa_len);
break;
case AF_INET6:
sa_len = sizeof(struct sockaddr_in6);
- if (buf_len(buf) < sa_len)
- {
- msg(M_FATAL,
- "ERROR: received incoming packet with too short length of
%d -- must be at least %d.",
- buf_len(buf), sa_len);
- }
- memcpy(dst, sa, sa_len);
- buf_advance(buf, sa_len);
+ ASSERT(buf_len(buf) >= sa_len);
break;
default:
- msg(M_FATAL, "ERROR: received incoming packet with invalid address
family %d.",
- sa->sa_family);
+ ASSERT(0); /* driver validates the family before writing */
}
+ memcpy(dst, sa, sa_len);
+ buf_advance(buf, sa_len);
+
return sa_len;
}
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1706?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: I2ce954aa5b74002be5e38d53783435736625bb2f
Gerrit-Change-Number: 1706
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
