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
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;
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;
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;
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
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
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
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
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
b.a.t.m.a.n@lists.open-mesh.org
-
Marek Lindner -
Sven Eckelmann