On Tue, Dec 12 2017, Patrick Wildt <[email protected]> wrote:
> 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().
... and then cmsg handling should update *tolen.
>> Opinions?
The getsockname(2) doesn't seem to perform anything useful: the ikev2
process gets a fully initialized sockaddr (len, family, address,
port) along with the socket, and recvfromto() could just care about
filling the address using cmsg parsing.
>> 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.
ok jca@ for socket_getaddr(), also ok for recvfromto() since the code is
confusing as is, but if I'm not mistaken we could just remove the
getsockname(2) call.
> 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;
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
