On Tue, Dec 12, 2017 at 03:32:50PM +0100, Patrick Wildt wrote:
> On Wed, Nov 29, 2017 at 07:50:21PM +0100, Claudio Jeker wrote:
> > More or less. It seems that msg_local is garbage:
> > $3 = {msg_data = 0x1b4e541fa4c0, msg_offset = 0, msg_local = {
> > ss_len = 128 '\200', ss_family = 192 'À',
> > __ss_pad1 = 0x7f7ffffda8c2 "Ì]\001", __ss_pad2 = 30024042514159,
> > __ss_pad3 = 0x7f7ffffda8d0 "Ьýÿ\177\177"}, msg_locallen = 128,
> > msg_peer = {ss_len = 16 '\020', ss_family = 2 '\002',
> > __ss_pad1 = 0x7f7ffffda9ca "\001ô>0\036\005", __ss_pad2 = 0,
> > __ss_pad3 = 0x7f7ffffda9d8 ""}, msg_peerlen = 16,
> >
> > Why so I don't know.
>
> I found it. socket_getaddr is called to retrieve information about the
> socket behind the file descriptor. If I understand correctly, we need
> to pass the length of the struct to getsockname() so that it knows how
> far it can fill the struct. So far we always passed stack garbage, so
> sometimes it worked, sometimes not. Then getsockname() does not change
> the struct, so sockaddr_storage ss is still stack garbarge, and that
> ends up in msg_local.
>
> I think recvfromto() suffers the same. The caller ikev2_msg_cb() does
> pass the correct size to recvfromto(), but then recvfromto() uncondi-
> tionally overwrites it with zero before calling getsockname().
>
> Opinions?
>
> Patrick
Markus points out that socket_getaddr() is promised a pointer to a
sockaddr_storage, so we don't need to pass the length and can just
initialize sslen correctly.
diff --git a/sbin/iked/util.c b/sbin/iked/util.c
index 1d4cee2c424..e67dae27d8a 100644
--- a/sbin/iked/util.c
+++ b/sbin/iked/util.c
@@ -93,7 +93,7 @@ socket_setport(struct sockaddr *sa, in_port_t port)
int
socket_getaddr(int s, struct sockaddr_storage *ss)
{
- socklen_t sslen;
+ socklen_t sslen = sizeof(*ss);
return (getsockname(s, (struct sockaddr *)ss, &sslen));
}
@@ -366,7 +366,6 @@ recvfromto(int s, void *buf, size_t len, int flags, struct
sockaddr *from,
return (-1);
*fromlen = from->sa_len;
- *tolen = 0;
if (getsockname(s, to, tolen) != 0)
*tolen = 0;
