netdev - RE: [PATCH net] genetlink: reject use of nlmsg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 29 Sep 2022 17:22:51 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "Florent Fourcot" <florent.fourcot@...irst.fr>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Nicolas Dichtel <nicolas.dichtel@...nd.com>,
        "Johannes Berg" <johannes@...solutions.net>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Florian Westphal <fw@...len.de>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Guillaume Nault <gnault@...hat.com>,
        Hangbin Liu <liuhangbin@...il.com>
Subject: RE: [PATCH net] genetlink: reject use of nlmsg_flags for new commands



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Wednesday, September 28, 2022 11:35 AM
> To: davem@...emloft.net
> Cc: netdev@...r.kernel.org; edumazet@...gle.com; pabeni@...hat.com; Jakub
> Kicinski <kuba@...nel.org>; Florent Fourcot <florent.fourcot@...irst.fr>; Nikolay
> Aleksandrov <razor@...ckwall.org>; Nicolas Dichtel
> <nicolas.dichtel@...nd.com>; Johannes Berg <johannes@...solutions.net>;
> Pablo Neira Ayuso <pablo@...filter.org>; Florian Westphal <fw@...len.de>;
> Jamal Hadi Salim <jhs@...atatu.com>; Keller, Jacob E
> <jacob.e.keller@...el.com>; Guillaume Nault <gnault@...hat.com>; Hangbin Liu
> <liuhangbin@...il.com>
> Subject: [PATCH net] genetlink: reject use of nlmsg_flags for new commands
> 
> Commit 9c5d03d36251 ("genetlink: start to validate reserved header bytes")
> introduced extra validation for genetlink headers. We had to gate it
> to only apply to new commands, to maintain bug-wards compatibility.
> Use this opportunity (before the new checks make it to Linus's tree)
> to add more conditions.
> 
> Validate that Generic Netlink families do not use nlmsg_flags outside
> of the well-understood set.
> 
> Link: https://lore.kernel.org/all/20220928073709.1b93b74a@kernel.org/
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> --
> CC: Florent Fourcot <florent.fourcot@...irst.fr>
> CC: Nikolay Aleksandrov <razor@...ckwall.org>
> CC: Nicolas Dichtel <nicolas.dichtel@...nd.com>
> CC: Johannes Berg <johannes@...solutions.net>
> CC: Pablo Neira Ayuso <pablo@...filter.org>
> CC: Florian Westphal <fw@...len.de>
> CC: Jamal Hadi Salim <jhs@...atatu.com>
> CC: Jacob Keller <jacob.e.keller@...el.com>
> CC: Guillaume Nault <gnault@...hat.com>
> CC: Hangbin Liu <liuhangbin@...il.com>
> ---
>  net/netlink/genetlink.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 7c136de117eb..39b7c00e4cef 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -739,6 +739,36 @@ static int genl_family_rcv_msg_doit(const struct
> genl_family *family,
>  	return err;
>  }
> 
> +static int genl_header_check(const struct genl_family *family,
> +			     struct nlmsghdr *nlh, struct genlmsghdr *hdr,
> +			     struct netlink_ext_ack *extack)
> +{
> +	u16 flags;
> +
> +	/* Only for commands added after we started validating */
> +	if (hdr->cmd < family->resv_start_op)
> +		return 0;
> +
> +	if (hdr->reserved) {
> +		NL_SET_ERR_MSG(extack, "genlmsghdr.reserved field is not 0");
> +		return -EINVAL;
> +	}
> +
> +	/* Old netlink flags have pretty loose semantics, allow only the flags
> +	 * consumed by the core where we can enforce the meaning.
> +	 */
> +	flags = nlh->nlmsg_flags;
> +	if ((flags & NLM_F_DUMP) == NLM_F_DUMP) /* DUMP is 2 bits */
> +		flags &= ~NLM_F_DUMP;

Ok so this check is making sure that flags has BOTH NLM_F_ROOT and NLM_F_MATCH or neither, but not only one of them?

That's why it can't go in the following section, because we want to allow NLM_F_DUMP but not NLM_F_ROOT or NLM_F_MATCH on its own.

Ok.

> +	if (flags & ~(NLM_F_REQUEST | NLM_F_ACK | NLM_F_ECHO)) {
> +		NL_SET_ERR_MSG(extack,
> +			       "ambiguous or reserved bits set in nlmsg_flags");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int genl_family_rcv_msg(const struct genl_family *family,
>  			       struct sk_buff *skb,
>  			       struct nlmsghdr *nlh,
> @@ -757,7 +787,7 @@ static int genl_family_rcv_msg(const struct genl_family
> *family,
>  	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
>  		return -EINVAL;
> 
> -	if (hdr->cmd >= family->resv_start_op && hdr->reserved)
> +	if (genl_header_check(family, nlh, hdr, extack))
>  		return -EINVAL;
> 
>  	if (genl_get_cmd(hdr->cmd, family, &op))
> --
> 2.37.3

Looks good to me!

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>