netdev - [RFC v2 4/6] netlink: add strict parsing for future attributes

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20190322130840.26601-5-johannes@sipsolutions.net>
Date:   Fri, 22 Mar 2019 14:08:38 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     netdev@...r.kernel.org
Cc:     David Ahern <dsa@...ulusnetworks.com>,
        Johannes Berg <johannes.berg@...el.com>
Subject: [RFC v2 4/6] netlink: add strict parsing for future attributes

From: Johannes Berg <johannes.berg@...el.com>

Unfortunately, we cannot add strict parsing for all attributes, as
that would break existing userspace. We currently warn about it, but
that's about all we can do.

For new attributes, however, the story is better: nobody is using
them, so we can reject bad sizes.

Also, for new attributes, we need not accept them when the policy
doesn't declare their usage.

David Ahern and I went back and forth on how to best encode this, and
the best way we found was to have a "boundary type", from which point
on new attributes
 * are strictly checked for length etc.
 * NLA_UNSPEC is rejected as invalid, rather than accepting all.

As we didn't want to add another argument to all functions that get a
netlink policy, the workaround is to encode that boundary in the first
entry of the policy array (which is for type 0 and thus probably not
really valid anyway). I put it into the validation union for the rare
possibility that somebody is actually using attribute 0, which would
continue to work fine unless they tried to use the extended validation,
which isn't likely. We also didn't find any in-tree users with type 0.

The reason for setting the "start strict here" attribute is that we
never really need to start strict from 0, which is invalid anyway (or
in legacy families where that isn't true, it cannot be set to strict),
so we can thus reserve the value 0 for "don't do this check" and don't
have to add the tag to all policies right now.

Thus, policies can now opt in to this validation, which we should do
for all existing policies, at least when adding new attributes.

Note that entirely *new* policies won't need to set it, as the use
of that should be using nla_parse()/nlmsg_parse() etc. which anyway
do fully strict validation now, regardless of this.

So in effect, this patch only covers the "existing command with new
attribute" case.

Signed-off-by: Johannes Berg <johannes.berg@...el.com>
---
 include/net/netlink.h | 18 ++++++++++++++++++
 lib/nlattr.c          |  7 +++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 7f368dc13e3d..c6c0f689292a 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -299,6 +299,24 @@ struct nla_policy {
 		};
 		int (*validate)(const struct nlattr *attr,
 				struct netlink_ext_ack *extack);
+		/* This entry is special, and used for the attribute at index 0
+		 * only, and specifies special data about the policy, namely it
+		 * specifies the "boundary type" where strict length validation
+		 * starts for any attribute types >= this value, also, strict
+		 * nesting validation starts here.
+		 *
+		 * Additionally, it means that NLA_UNSPEC is actually NLA_REJECT
+		 * for any types >= this, so need to use NLA_MIN_LEN to get the
+		 * previous pure { .len = xyz } behaviour. The advantage of this
+		 * is that types not specified in the policy will be rejected.
+		 *
+		 * For completely new families it should be set to 1 so that the
+		 * validation is enforced for all attributes. For existing ones
+		 * it should be set at least when new attributes are added to
+		 * the enum used by the policy, and be set to the new value that
+		 * was added to enforce strict validation from thereon.
+		 */
+		u16 strict_start_type;
 	};
 };

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 17c94a518e8a..28ac8f83996a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -158,12 +158,15 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 			const struct nla_policy *policy, bool strict,
 			struct netlink_ext_ack *extack)
 {
+	u16 strict_start_type = policy[0].strict_start_type;
 	const struct nla_policy *pt;
 	int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
 	int err = -ERANGE;
+	enum netlink_validation validate;
+
+	strict = strict || (strict_start_type && type >= strict_start_type);
 	/* We don't need to worry about MSG_STRICT here, it was never done */
-	enum netlink_validation validate =
-		strict ? NL_VALIDATE_STRICT : NL_VALIDATE_LIBERAL;
+	validate = strict ? NL_VALIDATE_STRICT : NL_VALIDATE_LIBERAL;

 	if (type <= 0 || type > maxtype)
 		return 0;
-- 
2.17.2