diff mbox

[1/9] wpa_supplicant: use getaddrinfo() when UDP ctrl interface

Message ID 1443009859-15327-2-git-send-email-janusz.dziedzic@tieto.com
State Changes Requested
Headers show

Commit Message

Janusz.Dziedzic@tieto.com Sept. 23, 2015, 12:04 p.m. UTC 
Kill not needed #ifdefs and make code common
for IPv4/IPv6 when registering UDP server for
ctrl interface.

Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
---
 wpa_supplicant/ctrl_iface_udp.c | 113 +++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 58 deletions(-)

Comments

Jouni Malinen Oct. 4, 2015, 4 p.m. UTC | #1 
On Wed, Sep 23, 2015 at 02:04:11PM +0200, Janusz Dziedzic wrote:
> Kill not needed #ifdefs and make code common
> for IPv4/IPv6 when registering UDP server for
> ctrl interface.

> @@ -560,20 +556,6 @@ static void wpa_supplicant_global_ctrl_iface_receive(int sock, void *eloop_ctx,
>  		return;
>  	}
>  
> -#ifndef CONFIG_CTRL_IFACE_UDP_REMOTE
> -	if (from.sin_addr.s_addr != htonl((127 << 24) | 1)) {
> -		/*
> -		 * The OS networking stack is expected to drop this kind of
> -		 * frames since the socket is bound to only localhost address.
> -		 * Just in case, drop the frame if it is coming from any other
> -		 * address.
> -		 */
> -		wpa_printf(MSG_DEBUG, "CTRL: Drop packet from unexpected "
> -			   "source %s", inet_ntoa(from.sin_addr));
> -		return;
> -	}
> -#endif /* CONFIG_CTRL_IFACE_UDP_REMOTE */
> -

Huh? Why was this removed? Same applies for patch 2/9
wpa_supplicant_ctrl_iface_receive() changes.

This can be a critical check on some platforms to protect the interface
from remote connection and as such, it must not be removed without very
good justification and certainly not in a patch that claims to be just
cleanup..
Janusz.Dziedzic@tieto.com Oct. 5, 2015, 5:14 a.m. UTC | #2 
On 4 October 2015 at 18:00, Jouni Malinen <j@w1.fi> wrote:
> On Wed, Sep 23, 2015 at 02:04:11PM +0200, Janusz Dziedzic wrote:
>> Kill not needed #ifdefs and make code common
>> for IPv4/IPv6 when registering UDP server for
>> ctrl interface.
>
>> @@ -560,20 +556,6 @@ static void wpa_supplicant_global_ctrl_iface_receive(int sock, void *eloop_ctx,
>>               return;
>>       }
>>
>> -#ifndef CONFIG_CTRL_IFACE_UDP_REMOTE
>> -     if (from.sin_addr.s_addr != htonl((127 << 24) | 1)) {
>> -             /*
>> -              * The OS networking stack is expected to drop this kind of
>> -              * frames since the socket is bound to only localhost address.
>> -              * Just in case, drop the frame if it is coming from any other
>> -              * address.
>> -              */
>> -             wpa_printf(MSG_DEBUG, "CTRL: Drop packet from unexpected "
>> -                        "source %s", inet_ntoa(from.sin_addr));
>> -             return;
>> -     }
>> -#endif /* CONFIG_CTRL_IFACE_UDP_REMOTE */
>> -
>
> Huh? Why was this removed? Same applies for patch 2/9
> wpa_supplicant_ctrl_iface_receive() changes.
>
> This can be a critical check on some platforms to protect the interface
> from remote connection and as such, it must not be removed without very
> good justification and certainly not in a patch that claims to be just
> cleanup..
>

I am not sure this is really required while we will listen only on
127.0.0.1 and will not accept remote connection. I suspect IP stack
already will care about it.
Even without this check I wasn't able to connect to such UDP port from
remote machine - seems IP stack handle this correctly (hints.ai_flags
= AI_PASSIVE works fine).

udp        0      0 127.0.0.1:9877          0.0.0.0:*
udp        0      0 127.0.0.1:9878          0.0.0.0:*

I suspect this check wasn't needed, or I miss something?

BR
Janusz
Jouni Malinen Oct. 5, 2015, 9:20 a.m. UTC | #3 
On Mon, Oct 05, 2015 at 07:14:04AM +0200, Janusz Dziedzic wrote:
> I am not sure this is really required while we will listen only on
> 127.0.0.1 and will not accept remote connection. I suspect IP stack
> already will care about it.
> Even without this check I wasn't able to connect to such UDP port from
> remote machine - seems IP stack handle this correctly (hints.ai_flags
> = AI_PASSIVE works fine).
> 
> udp        0      0 127.0.0.1:9877          0.0.0.0:*
> udp        0      0 127.0.0.1:9878          0.0.0.0:*
> 
> I suspect this check wasn't needed, or I miss something?

Please check with all other platforms than Linux.. There was a reason
for me adding that check, but it was a long time ago, so I don't
remember the details. Like I said, this most certainly must not be
removed as part of a cleanup patch.

Based on this, I'm dropping this patch set and the following patch set
that was based on this. Feel free to resubmit if this type of issues are
addressed. If _any_ of the existing security related checks are removed,
such removal need to be clearly presented in separate patches that
describe why the check is removed.
diff mbox

Patch

diff --git a/wpa_supplicant/ctrl_iface_udp.c b/wpa_supplicant/ctrl_iface_udp.c
index 76f69f2..12b4d45 100644
--- a/wpa_supplicant/ctrl_iface_udp.c
+++ b/wpa_supplicant/ctrl_iface_udp.c
@@ -15,7 +15,7 @@ 
 #include "wpa_supplicant_i.h"
 #include "ctrl_iface.h"
 #include "common/wpa_ctrl.h"
-
+#include <netdb.h>
 
 #define COOKIE_LEN 8
 
@@ -338,13 +338,9 @@  wpa_supplicant_ctrl_iface_init(struct wpa_supplicant *wpa_s)
 {
 	struct ctrl_iface_priv *priv;
 	int port = WPA_CTRL_IFACE_PORT;
-#ifdef CONFIG_CTRL_IFACE_UDP_IPV6
-	struct sockaddr_in6 addr;
-	int domain = PF_INET6;
-#else /* CONFIG_CTRL_IFACE_UDP_IPV6 */
-	struct sockaddr_in addr;
-	int domain = PF_INET;
-#endif /* CONFIG_CTRL_IFACE_UDP_IPV6 */
+	char p[32]={0};
+	struct addrinfo hints = { 0 }, *res, *saveres;
+	int n;
 
 	priv = os_zalloc(sizeof(*priv));
 	if (priv == NULL)
@@ -356,35 +352,33 @@  wpa_supplicant_ctrl_iface_init(struct wpa_supplicant *wpa_s)
 	if (wpa_s->conf->ctrl_interface == NULL)
 		return priv;
 
-	priv->sock = socket(domain, SOCK_DGRAM, 0);
+#ifdef CONFIG_CTRL_IFACE_UDP_REMOTE
+	hints.ai_flags = AI_PASSIVE;
+#endif
+
+#ifdef CONFIG_CTRL_IFACE_UDP_IPV6
+	hints.ai_family = AF_INET6;
+#else
+	hints.ai_family = AF_INET;
+#endif
+	hints.ai_socktype = SOCK_DGRAM;
+
+try_again:
+	os_snprintf(p, sizeof(p), "%d", port);
+	n = getaddrinfo(NULL, p, &hints, &res);
+	if (n) {
+		wpa_printf(MSG_ERROR, "getaddrinfo(): %s", gai_strerror(n));
+		goto fail;
+	}
+
+	saveres = res;
+	priv->sock = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
 	if (priv->sock < 0) {
 		wpa_printf(MSG_ERROR, "socket(PF_INET): %s", strerror(errno));
 		goto fail;
 	}
 
-	os_memset(&addr, 0, sizeof(addr));
-#ifdef CONFIG_CTRL_IFACE_UDP_IPV6
-	addr.sin6_family = AF_INET6;
-#ifdef CONFIG_CTRL_IFACE_UDP_REMOTE
-	addr.sin6_addr = in6addr_any;
-#else /* CONFIG_CTRL_IFACE_UDP_REMOTE */
-	inet_pton(AF_INET6, "::1", &addr.sin6_addr);
-#endif /* CONFIG_CTRL_IFACE_UDP_REMOTE */
-#else /* CONFIG_CTRL_IFACE_UDP_IPV6 */
-	addr.sin_family = AF_INET;
-#ifdef CONFIG_CTRL_IFACE_UDP_REMOTE
-	addr.sin_addr.s_addr = INADDR_ANY;
-#else /* CONFIG_CTRL_IFACE_UDP_REMOTE */
-	addr.sin_addr.s_addr = htonl((127 << 24) | 1);
-#endif /* CONFIG_CTRL_IFACE_UDP_REMOTE */
-#endif /* CONFIG_CTRL_IFACE_UDP_IPV6 */
-try_again:
-#ifdef CONFIG_CTRL_IFACE_UDP_IPV6
-	addr.sin6_port = htons(port);
-#else /* CONFIG_CTRL_IFACE_UDP_IPV6 */
-	addr.sin_port = htons(port);
-#endif /* CONFIG_CTRL_IFACE_UDP_IPV6 */
-	if (bind(priv->sock, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
+	if (bind(priv->sock, res->ai_addr, res->ai_addrlen) < 0) {
 		port--;
 		if ((WPA_CTRL_IFACE_PORT - port) < WPA_CTRL_IFACE_PORT_LIMIT)
 			goto try_again;
@@ -392,6 +386,8 @@  try_again:
 		goto fail;
 	}
 
+	freeaddrinfo(saveres);
+
 #ifdef CONFIG_CTRL_IFACE_UDP_REMOTE
 	wpa_msg(wpa_s, MSG_DEBUG, "ctrl_iface_init UDP port: %d", port);
 #endif /* CONFIG_CTRL_IFACE_UDP_REMOTE */
@@ -560,20 +556,6 @@  static void wpa_supplicant_global_ctrl_iface_receive(int sock, void *eloop_ctx,
 		return;
 	}
 
-#ifndef CONFIG_CTRL_IFACE_UDP_REMOTE
-	if (from.sin_addr.s_addr != htonl((127 << 24) | 1)) {
-		/*
-		 * The OS networking stack is expected to drop this kind of
-		 * frames since the socket is bound to only localhost address.
-		 * Just in case, drop the frame if it is coming from any other
-		 * address.
-		 */
-		wpa_printf(MSG_DEBUG, "CTRL: Drop packet from unexpected "
-			   "source %s", inet_ntoa(from.sin_addr));
-		return;
-	}
-#endif /* CONFIG_CTRL_IFACE_UDP_REMOTE */
-
 	buf[res] = '\0';
 
 	if (os_strcmp(buf, "GET_COOKIE") == 0) {
@@ -622,8 +604,10 @@  struct ctrl_iface_global_priv *
 wpa_supplicant_global_ctrl_iface_init(struct wpa_global *global)
 {
 	struct ctrl_iface_global_priv *priv;
-	struct sockaddr_in addr;
 	int port = WPA_GLOBAL_CTRL_IFACE_PORT;
+	char p[32]={0};
+	struct addrinfo hints = { 0 }, *res, *saveres;
+	int n;
 
 	priv = os_zalloc(sizeof(*priv));
 	if (priv == NULL)
@@ -637,22 +621,33 @@  wpa_supplicant_global_ctrl_iface_init(struct wpa_global *global)
 	wpa_printf(MSG_DEBUG, "Global control interface '%s'",
 		   global->params.ctrl_interface);
 
-	priv->sock = socket(PF_INET, SOCK_DGRAM, 0);
+#ifdef CONFIG_CTRL_IFACE_UDP_REMOTE
+	hints.ai_flags = AI_PASSIVE;
+#endif
+
+#ifdef CONFIG_CTRL_IFACE_UDP_IPV6
+	hints.ai_family = AF_INET6;
+#else
+	hints.ai_family = AF_INET;
+#endif
+	hints.ai_socktype = SOCK_DGRAM;
+
+try_again:
+	os_snprintf(p, sizeof(p), "%d", port);
+	n = getaddrinfo(NULL, p, &hints, &res);
+	if (n) {
+		wpa_printf(MSG_ERROR, "getaddrinfo(): %s", gai_strerror(n));
+		goto fail;
+	}
+
+	saveres = res;
+	priv->sock = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
 	if (priv->sock < 0) {
 		wpa_printf(MSG_ERROR, "socket(PF_INET): %s", strerror(errno));
 		goto fail;
 	}
 
-	os_memset(&addr, 0, sizeof(addr));
-	addr.sin_family = AF_INET;
-#ifdef CONFIG_CTRL_IFACE_UDP_REMOTE
-	addr.sin_addr.s_addr = INADDR_ANY;
-#else /* CONFIG_CTRL_IFACE_UDP_REMOTE */
-	addr.sin_addr.s_addr = htonl((127 << 24) | 1);
-#endif /* CONFIG_CTRL_IFACE_UDP_REMOTE */
-try_again:
-	addr.sin_port = htons(port);
-	if (bind(priv->sock, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
+	if (bind(priv->sock, res->ai_addr, res->ai_addrlen) < 0) {
 		port++;
 		if ((port - WPA_GLOBAL_CTRL_IFACE_PORT) <
 		    WPA_GLOBAL_CTRL_IFACE_PORT_LIMIT)
@@ -661,6 +656,8 @@  try_again:
 		goto fail;
 	}
 
+	freeaddrinfo(saveres);
+
 #ifdef CONFIG_CTRL_IFACE_UDP_REMOTE
 	wpa_printf(MSG_DEBUG, "global_ctrl_iface_init UDP port: %d", port);
 #endif /* CONFIG_CTRL_IFACE_UDP_REMOTE */