Message ID | 20220112210506.3488775-3-mareklindner@neomailbox.ch |
---|---|
State | Rejected, archived |
Delegated to: | Simon Wunderlich |
Headers |
Return-Path: <b.a.t.m.a.n-bounces@lists.open-mesh.org> X-Original-To: patchwork@open-mesh.org Delivered-To: patchwork@open-mesh.org Received: from diktynna.open-mesh.org (localhost [IPv6:::1]) by diktynna.open-mesh.org (Postfix) with ESMTP id 5E0A883E57; Wed, 12 Jan 2022 22:06:01 +0100 (CET) Received: from s2.neomailbox.net (s2.neomailbox.net [5.148.176.60]) by diktynna.open-mesh.org (Postfix) with ESMTPS id 7143483E46 for <b.a.t.m.a.n@lists.open-mesh.org>; Wed, 12 Jan 2022 22:05:58 +0100 (CET) From: Marek Lindner <mareklindner@neomailbox.ch> To: b.a.t.m.a.n@lists.open-mesh.org Subject: [PATCH 3/3] alfred: properly initialize stack buffer before sending over unix socket Date: Wed, 12 Jan 2022 22:05:06 +0100 Message-Id: <20220112210506.3488775-3-mareklindner@neomailbox.ch> In-Reply-To: <20220112210506.3488775-1-mareklindner@neomailbox.ch> References: <10410848.OOsao9LFFs@rousseau> <20220112210506.3488775-1-mareklindner@neomailbox.ch> MIME-Version: 1.0 ARC-Authentication-Results: i=1; diktynna.open-mesh.org; dkim=none; spf=pass (diktynna.open-mesh.org: domain of mareklindner@neomailbox.ch designates 5.148.176.60 as permitted sender) smtp.mailfrom=mareklindner@neomailbox.ch; dmarc=none ARC-Seal: i=1; s=20121; d=open-mesh.org; t=1642021558; a=rsa-sha256; cv=none; b=qcsDfe7U65/mEbFVwPQzrJeQHFiadyae7GP8dyYK2eygh+6ETNFDOqsR67K8U8h0I9jE4C 2Ou9SpigD/q5KHd0139VYOGbLW7a1HtlHQa/638dbSspPM0mgOlgeWgw7vuaF+4lS8fUaA UwdmHwFC/OlhYjpbzNToYGQi7pOWQAs= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=open-mesh.org; s=20121; t=1642021558; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=NVI4cMzS+FQAMGCh0y9pZ/Cu8V2R+vchBJrn/PpDH5A=; b=YGm1UoJy5A1soOl3zTwkJoKYb+3UFXNrVUopix4D1YKpCXWnZkNK+skharXk1giPfYpDKl JG3TJlguDkaFnHDr2exKv/Wo7NwrIu9ynpqO4zmy7CnEpqYS4mEnm1iUSjNMyaIQSnwxYv vnfoVCUss82rVPs5BYFPnlmCfr0ZnRc= Content-Transfer-Encoding: quoted-printable Message-ID-Hash: KAQQACDKPGCW3TNG7PAYE52EAGQJZA2A X-Message-ID-Hash: KAQQACDKPGCW3TNG7PAYE52EAGQJZA2A X-MailFrom: mareklindner@neomailbox.ch X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-b.a.t.m.a.n.lists.open-mesh.org-0; header-match-b.a.t.m.a.n.lists.open-mesh.org-1; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header CC: Marek Lindner <mareklindner@neomailbox.ch> X-Mailman-Version: 3.2.1 Precedence: list Reply-To: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n@lists.open-mesh.org> List-Id: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n.lists.open-mesh.org> Archived-At: <https://lists.open-mesh.org/mailman3/hyperkitty/list/b.a.t.m.a.n@lists.open-mesh.org/message/KAQQACDKPGCW3TNG7PAYE52EAGQJZA2A/> List-Archive: <https://lists.open-mesh.org/mailman3/hyperkitty/list/b.a.t.m.a.n@lists.open-mesh.org/> List-Help: <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=help> List-Post: <mailto:b.a.t.m.a.n@lists.open-mesh.org> List-Subscribe: <mailto:b.a.t.m.a.n-join@lists.open-mesh.org> List-Unsubscribe: <mailto:b.a.t.m.a.n-leave@lists.open-mesh.org> |
Series |
[1/3] alfred: move interface check into helper function
|
|
Commit Message
Marek Lindner
Jan. 12, 2022, 9:05 p.m. UTC
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(+)
Comments
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
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;