[4/6] netlink: add strict parsing for future attributes

Commit Message

Johannes Berg April 4, 2019, 6:54 a.m. UTC

From: Johannes Berg <[e-mail]>

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 have all possible validation applied, and NLA_UNSPEC
is rejected.

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 <[e-mail]>
---
 include/net/netlink.h | 18 ++++++++++++++++++
 lib/nlattr.c          |  4 ++++
 2 files changed, 22 insertions(+)

Comments

Nicolas Dichtel April 5, 2019, 3:22 p.m. UTC | #1

Le 04/04/2019 à 08:54, Johannes Berg a écrit :
[snip]
> 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.
OVS_TUNNEL_KEY_ATTR_ID seems to be one if I'm not wrong.


Regards,
Nicolas

Johannes Berg April 5, 2019, 3:31 p.m. UTC | #2

On Fri, 2019-04-05 at 17:22 +0200, Nicolas Dichtel wrote:
> Le 04/04/2019 à 08:54, Johannes Berg a écrit :
> [snip]
> > 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.
> 
> OVS_TUNNEL_KEY_ATTR_ID seems to be one if I'm not wrong.

Indeed, good find.

I guess I'll change the commit message, but all it really means is that
OVS can't use any validation function etc. for OVS_TUNNEL_KEY_ATTR_ID,
which seems like a reasonable trade-off.

johannes

Nicolas Dichtel April 5, 2019, 3:40 p.m. UTC | #3

Le 05/04/2019 à 17:31, Johannes Berg a écrit :
> On Fri, 2019-04-05 at 17:22 +0200, Nicolas Dichtel wrote:
>> Le 04/04/2019 à 08:54, Johannes Berg a écrit :
>> [snip]
>>> 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.
>>
>> OVS_TUNNEL_KEY_ATTR_ID seems to be one if I'm not wrong.
> 
> Indeed, good find.
> 
> I guess I'll change the commit message, but all it really means is that
> OVS can't use any validation function etc. for OVS_TUNNEL_KEY_ATTR_ID,
> which seems like a reasonable trade-off.
Yes I agree.

There is three others 0-attribute, but filled only by the kernel
(NETLINK_DIAG_NONE, PACKET_DIAG_NONE and UNIX_DIAG_NONE).

Nicolas

Patch

diff --git a/include/net/netlink.h b/include/net/netlink.h
index d75545c906b6..fdb39b0fd752 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 3ba54904256a..63691d40eccf 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -158,10 +158,14 @@  static int validate_nla(const struct nlattr *nla, int maxtype,
 			const struct nla_policy *policy, unsigned int validate,
 			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;
 
+	if (strict_start_type && type >= strict_start_type)
+		validate |= NL_VALIDATE_STRICT;
+
 	if (type <= 0 || type > maxtype)
 		return 0;