alfred: allow startup without network interface

alfred: allow startup without network interface

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

Hi,

alfred could be used without any interface at all and operate as local data 
storage between 2 processes on the same system or the interface could be 
configured at a later time (via unix socket).

To allow for these additional use cases and keep the current behavior, the 
interface command line parameter shall accept 'none' as interface name 
(similar to the batman-adv interface).

Cheers,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/3] alfred: move interface check into helper function

[Marek Lindner]

Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
---
 alfred.h |  1 +
 server.c |  4 ++--
 util.c   | 11 +++++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/alfred.h b/alfred.h
index 0e4dd26..c595b06 100644
--- a/alfred.h
+++ b/alfred.h
@@ -204,5 +204,6 @@ int time_diff(struct timespec *tv1, struct timespec *tv2,
 void time_random_seed(void);
 uint16_t get_random_id(void);
 bool is_valid_ether_addr(uint8_t *addr);
+bool is_iface_disabled(char *iface);
 int ipv4_arp_request(struct interface *interface, const alfred_addr *addr,
 		     struct ether_addr *mac);
diff --git a/server.c b/server.c
index 85bf453..1efc211 100644
--- a/server.c
+++ b/server.c
@@ -205,7 +205,7 @@ static void update_server_info(struct globals *globals)
 	if (globals->opmode == OPMODE_PRIMARY)
 		return;
 
-	if (strcmp(globals->mesh_iface, "none") != 0) {
+	if (!is_iface_disabled(globals->mesh_iface)) {
 		tg_hash = tg_hash_new(globals->mesh_iface);
 		if (!tg_hash) {
 			fprintf(stderr, "Failed to create translation hash\n");
@@ -385,7 +385,7 @@ int alfred_server(struct globals *globals)
 		return -1;
 	}
 
-	if (strcmp(globals->mesh_iface, "none") != 0 &&
+	if (!is_iface_disabled(globals->mesh_iface) &&
 	    batadv_interface_check(globals->mesh_iface) < 0 &&
 	    !globals->force) {
 		fprintf(stderr, "Can't start server: batman-adv interface %s not found\n",
diff --git a/util.c b/util.c
index 42a625a..eabef57 100644
--- a/util.c
+++ b/util.c
@@ -67,6 +67,17 @@ bool is_valid_ether_addr(uint8_t addr[ETH_ALEN])
 	return true;
 }
 
+bool is_iface_disabled(char *iface)
+{
+	if (!iface)
+		return false;
+
+	if (strcmp(iface, "none") != 0)
+		return false;
+
+	return true;
+}
+
 static void ipv4_request_mac_resolve(const alfred_addr *addr)
 {
 	const struct sockaddr *sockaddr;
-- 
2.32.0.rc0

[PATCH 2/3] alfred: Allow operating without any interface specified

[Marek Lindner]

The '-i' commandline parameter to specify interface names
no longer is mandatory. Specifying interface 'none' or
sending a 'none' interface string within the
ALFRED_CHANGE_INTERFACE unix socket command disables
all interfaces operations at runtime.

Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
---
 README.rst   |  6 +++++-
 alfred.h     |  2 +-
 client.c     |  8 ++++----
 main.c       |  6 +++---
 man/alfred.8 |  6 +++++-
 netsock.c    |  4 ++++
 server.c     | 39 ++++++++++++++++++++++++---------------
 7 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/README.rst b/README.rst
index 33200e4..7f44db6 100644
--- a/README.rst
+++ b/README.rst
@@ -82,7 +82,8 @@ documentation how to configure alfred in this case. In any event, you can
 still run alfred from the command line. The relevant options are (for a full
 list of options, run alfred -h):
 
-  -i, --interface             specify the interface to listen on
+  -i, --interface             specify the interface to listen on. use 'none'
+                              to disable interface operations
   -b                          specify the batman-adv interface configured on
                               the system (default: bat0). use 'none' to disable
                               the batman-adv based best server selection
@@ -90,6 +91,9 @@ list of options, run alfred -h):
                               accepts data from secondaries and syncs it with
                               other primaries
 
+The interface option '-i' is optional. If interface 'none' is specified, the
+alfred daemon will not communicate with other alfred instances on the
+network unless the interface list is modified at runtime via the unix socket.
 The -b option is optional, and only needed if you run alfred on a batman-adv
 interface not called bat0, or if you don't use batman-adv at all
 (use '-b none'). In this case, alfred will still work but will not be able to
diff --git a/alfred.h b/alfred.h
index c595b06..9ab92a2 100644
--- a/alfred.h
+++ b/alfred.h
@@ -112,7 +112,7 @@ struct interface {
 struct globals {
 	struct list_head interfaces;
 
-	char *change_interface;
+	char *net_iface;
 	struct server *best_server;	/* NULL if we are a server ourselves */
 	char *mesh_iface;
 	enum opmode opmode;
diff --git a/client.c b/client.c
index d0d19fb..b5d8943 100644
--- a/client.c
+++ b/client.c
@@ -252,7 +252,7 @@ int alfred_client_change_interface(struct globals *globals)
 	if (unix_sock_open_client(globals))
 		return -1;
 
-	interface_len = strlen(globals->change_interface);
+	interface_len = strlen(globals->net_iface);
 	if (interface_len > sizeof(change_interface.ifaces)) {
 		fprintf(stderr, "%s: interface name list too long, not changing\n",
 			__func__);
@@ -264,15 +264,15 @@ int alfred_client_change_interface(struct globals *globals)
 	change_interface.header.type = ALFRED_CHANGE_INTERFACE;
 	change_interface.header.version = ALFRED_VERSION;
 	change_interface.header.length = FIXED_TLV_LEN(change_interface);
-	strncpy(change_interface.ifaces, globals->change_interface,
+	strncpy(change_interface.ifaces, globals->net_iface,
 		sizeof(change_interface.ifaces));
 	change_interface.ifaces[sizeof(change_interface.ifaces) - 1] = '\0';
 
 	/* test it before sending
-	 * globals->change_interface is now saved in change_interface.ifaces
+	 * globals->net_iface is now saved in change_interface.ifaces
 	 * and can be modified by strtok_r
 	 */
-	input = globals->change_interface;
+	input = globals->net_iface;
 	while ((token = strtok_r(input, ",", &saveptr))) {
 		input = NULL;
 
diff --git a/main.c b/main.c
index 2cb6d44..d40a0cc 100644
--- a/main.c
+++ b/main.c
@@ -179,7 +179,7 @@ static struct globals *alfred_init(int argc, char *argv[])
 	memset(globals, 0, sizeof(*globals));
 
 	INIT_LIST_HEAD(&globals->interfaces);
-	globals->change_interface = NULL;
+	globals->net_iface = NULL;
 	globals->opmode = OPMODE_SECONDARY;
 	globals->clientmode = CLIENT_NONE;
 	globals->best_server = NULL;
@@ -224,7 +224,7 @@ static struct globals *alfred_init(int argc, char *argv[])
 			globals->opmode = OPMODE_PRIMARY;
 			break;
 		case 'i':
-			netsock_set_interfaces(globals, optarg);
+			globals->net_iface = strdup(optarg);
 			break;
 		case 'b':
 			globals->mesh_iface = strdup(optarg);
@@ -252,7 +252,7 @@ static struct globals *alfred_init(int argc, char *argv[])
 			break;
 		case 'I':
 			globals->clientmode = CLIENT_CHANGE_INTERFACE;
-			globals->change_interface = strdup(optarg);
+			globals->net_iface = strdup(optarg);
 			break;
 		case 'B':
 			globals->clientmode = CLIENT_CHANGE_BAT_IFACE;
diff --git a/man/alfred.8 b/man/alfred.8
index 4e002f0..74814e0 100644
--- a/man/alfred.8
+++ b/man/alfred.8
@@ -98,12 +98,16 @@ Change the alfred server to use the new \fBbatman-adv interface\fP
 .SH SERVER OPTIONS
 .TP
 \fB\-i\fP, \fB\-\-interface\fP \fIiface\fP
-Specify the interface (or comma separated list of interfaces) to listen on
+Specify the interface (or comma separated list of interfaces) to listen on.
+Use 'none' to disable interface operations.
 .TP
 \fB\-b\fP \fIbatmanif\fP
 Specify the batman-adv interface configured on the system (default: bat0).
 Use 'none' to disable the batman-adv based best server selection.
 
+The interface option \fB\-i\fP is optional. If interface 'none' is specified, the
+alfred daemon will not communicate with other alfred instances on the
+network unless the interface list is modified at runtime via the unix socket.
 The \fB\-b\fP option is optional, and only needed if you run alfred on a
 batman-adv interface not called bat0, or if you don't use batman-adv at all
 (use '\fB\-b\fP none'). In this case, alfred will still work but will not be
diff --git a/netsock.c b/netsock.c
index 84b0ec3..128e768 100644
--- a/netsock.c
+++ b/netsock.c
@@ -116,6 +116,10 @@ int netsock_set_interfaces(struct globals *globals, char *interfaces)
 
 	netsock_close_all(globals);
 
+	/* interface 'none' disables all interface operations */
+	if (is_iface_disabled(interfaces))
+		return 0;
+
 	input = interfaces;
 	while ((token = strtok_r(input, ",", &saveptr))) {
 		input = NULL;
diff --git a/server.c b/server.c
index 1efc211..bfc37bc 100644
--- a/server.c
+++ b/server.c
@@ -380,9 +380,30 @@ int alfred_server(struct globals *globals)
 	if (unix_sock_open_daemon(globals))
 		return -1;
 
-	if (list_empty(&globals->interfaces)) {
-		fprintf(stderr, "Can't start server: interface missing\n");
-		return -1;
+	if (!is_iface_disabled(globals->net_iface)) {
+		if (!globals->net_iface) {
+			fprintf(stderr, "Can't start server: interface missing\n");
+			return -1;
+		}
+
+		netsock_set_interfaces(globals, globals->net_iface);
+
+		if (list_empty(&globals->interfaces) && !globals->force) {
+			fprintf(stderr, "Can't start server: valid interface missing\n");
+			return -1;
+		}
+
+		num_socks = netsock_open_all(globals);
+		if (num_socks <= 0 && !globals->force) {
+			fprintf(stderr, "Failed to open interfaces\n");
+			return -1;
+		}
+
+		num_interfaces = netsocket_count_interfaces(globals);
+		if (num_interfaces > 1 && globals->opmode == OPMODE_SECONDARY) {
+			fprintf(stderr, "More than one interface specified in secondary mode\n");
+			return -1;
+		}
 	}
 
 	if (!is_iface_disabled(globals->mesh_iface) &&
@@ -393,18 +414,6 @@ int alfred_server(struct globals *globals)
 		return -1;
 	}
 
-	num_socks = netsock_open_all(globals);
-	if (num_socks <= 0 && !globals->force) {
-		fprintf(stderr, "Failed to open interfaces\n");
-		return -1;
-	}
-
-	num_interfaces = netsocket_count_interfaces(globals);
-	if (num_interfaces > 1 && globals->opmode == OPMODE_SECONDARY) {
-		fprintf(stderr, "More than one interface specified in secondary mode\n");
-		return -1;
-	}
-
 	clock_gettime(CLOCK_MONOTONIC, &last_check);
 	globals->if_check = last_check;
 
-- 
2.32.0.rc0

[PATCH 3/3] alfred: properly initialize stack buffer before sending over unix socket

[Marek Lindner]

Without explicitely initializing the buffer with null bytes, the stack
variables may contain process information which may be leaked when
transmitted via unix socket.
Also, the size of the variables sitting on the stack can be reduced.

Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
---
 client.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/client.c b/client.c
index b5d8943..cf15ff4 100644
--- a/client.c
+++ b/client.c
@@ -35,6 +35,7 @@ int alfred_client_request_data(struct globals *globals)
 		return -1;
 
 	len = sizeof(request);
+	memset(&request, 0, len);
 
 	request.header.type = ALFRED_REQUEST;
 	request.header.version = ALFRED_VERSION;
@@ -184,6 +185,7 @@ int alfred_client_modeswitch(struct globals *globals)
 		return -1;
 
 	len = sizeof(modeswitch);
+	memset(&modeswitch, 0, len);
 
 	modeswitch.header.type = ALFRED_MODESWITCH;
 	modeswitch.header.version = ALFRED_VERSION;
@@ -260,6 +262,7 @@ int alfred_client_change_interface(struct globals *globals)
 	}
 
 	len = sizeof(change_interface);
+	memset(&change_interface, 0, len);
 
 	change_interface.header.type = ALFRED_CHANGE_INTERFACE;
 	change_interface.header.version = ALFRED_VERSION;
@@ -308,6 +311,7 @@ int alfred_client_change_bat_iface(struct globals *globals)
 	}
 
 	len = sizeof(change_bat_iface);
+	memset(&change_bat_iface, 0, len);
 
 	change_bat_iface.header.type = ALFRED_CHANGE_BAT_IFACE;
 	change_bat_iface.header.version = ALFRED_VERSION;
-- 
2.32.0.rc0

Re: alfred: allow startup without network interface

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Wednesday, 12 January 2022 22:02:58 CET Marek Lindner wrote:
> To allow for these additional use cases and keep the current behavior, the
> interface command line parameter shall accept 'none' as interface name
> (similar to the batman-adv interface).

Forgot to mention that these patches build upon master and:

* alfred: Avoid large send buffer for fixed size IPC commands
* alfred: Simplify calculation of fixed size IPC TLV length

Cheers,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] alfred: properly initialize stack buffer before sending over unix socket

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

On Wednesday, 12 January 2022 22:05:06 CET Marek Lindner wrote:
[...]
> diff --git a/client.c b/client.c
> index b5d8943..cf15ff4 100644
> --- a/client.c
> +++ b/client.c
> @@ -35,6 +35,7 @@ int alfred_client_request_data(struct globals *globals)
>  		return -1;
>  
>  	len = sizeof(request);
> +	memset(&request, 0, len);
>  
>  	request.header.type = ALFRED_REQUEST;
>  	request.header.version = ALFRED_VERSION;

All bytes (also all bits) are overwritten in the lines below the memset. So I 
don't see why memset would be required here.

> @@ -184,6 +185,7 @@ int alfred_client_modeswitch(struct globals *globals)
>  		return -1;
>  
>  	len = sizeof(modeswitch);
> +	memset(&modeswitch, 0, len);
>  
>  	modeswitch.header.type = ALFRED_MODESWITCH;
>  	modeswitch.header.version = ALFRED_VERSION;

Same here - with a minor exception. When mode is not written then the data is 
not written to the socket.

> @@ -260,6 +262,7 @@ int alfred_client_change_interface(struct globals *globals)
>  	}
>  
>  	len = sizeof(change_interface);
> +	memset(&change_interface, 0, len);
>  
>  	change_interface.header.type = ALFRED_CHANGE_INTERFACE;
>  	change_interface.header.version = ALFRED_VERSION;\

Same here.

> @@ -308,6 +311,7 @@ int alfred_client_change_bat_iface(struct globals *globals)
>  	}
>  
>  	len = sizeof(change_bat_iface);
> +	memset(&change_bat_iface, 0, len);
>  
>  	change_bat_iface.header.type = ALFRED_CHANGE_BAT_IFACE;
>  	change_bat_iface.header.version = ALFRED_VERSION;
> 

Same here.

Kind regards,
	Sven


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] alfred: properly initialize stack buffer before sending over unix socket

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]

On Friday, 21 January 2022 16:34:50 CET Sven Eckelmann wrote:
> > @@ -260,6 +262,7 @@ int alfred_client_change_interface(struct globals
> > *globals) }
> > 
> > len = sizeof(change_interface);
> > +       memset(&change_interface, 0, len);
> > 
> > change_interface.header.type = ALFRED_CHANGE_INTERFACE;
> > change_interface.header.version = ALFRED_VERSION;\
> 
> Same here.
> 
> > @@ -308,6 +311,7 @@ int alfred_client_change_bat_iface(struct globals
> > *globals) }
> > 
> > len = sizeof(change_bat_iface);
> > +       memset(&change_bat_iface, 0, len);
> > 
> > change_bat_iface.header.type = ALFRED_CHANGE_BAT_IFACE;
> > change_bat_iface.header.version = ALFRED_VERSION;
> 
> Same here.

The struct alfred_change_interface_v0 -> ifaces[IFNAMSIZ * 16] may be written 
to but not fully initialized. The interface name may be much shorter than the 
buffer holding it. Same applies struct alfred_change_bat_iface_v0 -> 
bat_iface[IFNAMSIZ] but to a lesser extent because the buffer is smaller.

This patch is based on your earlier observation that stack data may be leaked 
due to the lack of (complete) initialization.

You are correct that the structs struct alfred_request_v0 & 
alfred_modeswitch_v0 technically don't require initialization because all 
fields are set manually. I added those for completeness sake for the next 
person coming along copy & pasting the code (as I had done).

Kind regards,
Marek Lindner

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] alfred: properly initialize stack buffer before sending over unix socket

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On Saturday, 22 January 2022 01:41:36 CET Marek Lindner wrote:
[...]
> The struct alfred_change_interface_v0 -> ifaces[IFNAMSIZ * 16] may be written 
> to but not fully initialized. The interface name may be much shorter than the 
> buffer holding it. Same applies struct alfred_change_bat_iface_v0 -> 
> bat_iface[IFNAMSIZ] but to a lesser extent because the buffer is smaller.

But strncpy writes n bytes (second parameter of n). So the name + n-
strlen(name) 0-bytes. I thought I've corrected my earlier statement about 
strncpy but maybe I forgot it. So strlcpy will take care of always writing a 
single 0-byte at the end of a non-0-length buffer - but is not writing more than 
1x 0-byte (so half of the buffer might be uninitialised). strncpy will fill 
the remaining bytes with 0 but might end up writing NO 0-bytes when the buffer 
was already full.

But in your status patch, not all 16 name entries were written. So it leaks 
things from the stack and the receiver might parse things which are not 
actually written by the sender. And your code was not explicitly making sure
that the buffer ends with a 0-byte.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] alfred: properly initialize stack buffer before sending over unix socket

[Marek Lindner]

[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]

On Saturday, 22 January 2022 09:03:12 CET Sven Eckelmann wrote:
> > The struct alfred_change_interface_v0 -> ifaces[IFNAMSIZ * 16] may be
> > written to but not fully initialized. The interface name may be much
> > shorter than the buffer holding it. Same applies struct
> > alfred_change_bat_iface_v0 -> bat_iface[IFNAMSIZ] but to a lesser extent
> > because the buffer is smaller.
>
> But strncpy writes n bytes (second parameter of n). So the name + n-
> strlen(name) 0-bytes. I thought I've corrected my earlier statement about
> strncpy but maybe I forgot it. So strlcpy will take care of always writing a
> single 0-byte at the end of a non-0-length buffer - but is not writing more
> than 1x 0-byte (so half of the buffer might be uninitialised). strncpy will
> fill the remaining bytes with 0 but might end up writing NO 0-bytes when
> the buffer was already full.

Thanks for highlighting this difference between strncpy() and strlcpy(). I see 
your point.


> But in your status patch, not all 16 name entries were written. So it leaks
> things from the stack and the receiver might parse things which are not
> actually written by the sender. And your code was not explicitly making sure
> that the buffer ends with a 0-byte.

The server status patch iterates over the list of interfaces and performs 
individual strncpy() calls, so that strncpy() can't initialize the entire 
buffer unless there are 16 interfaces to begin with. Ok!

Then drop this patch.

Kind regards,
Marek Lindner

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]