[PATCH net-next v2 0/3] netlink: strict attribute checking follow-up

Three follow-up patches for recent strict netlink validation series.

Patch 1 fixes dump handling for genetlink families which validate and parse
messages themselves (e.g. because they need different policies for diferent
commands).

Patch 2 sets bad_attr in extack in one place where this was omitted.

Patch 3 adds new NL_VALIDATE_NESTED flags for strict validation to enable
checking that NLA_F_NESTED value in received messages matches expectations
and includes this flag in NL_VALIDATE_STRICT. This would change userspace
visible behavior but the previous switching to NL_VALIDATE_STRICT for new
code is still only in net-next at the moment.

v2: change error messages to mention NLA_F_NESTED explicitly

Michal Kubecek (3):
  genetlink: do not validate dump requests if there is no policy
  netlink: set bad attribute also on maxtype check
  netlink: add validation of NLA_F_NESTED flag

 include/net/netlink.h   | 11 ++++++++++-
 lib/nlattr.c            | 18 +++++++++++++++++-
 net/netlink/genetlink.c | 24 ++++++++++++++----------
 3 files changed, 41 insertions(+), 12 deletions(-)

-- 
2.21.0

Unlike do requests, dump genetlink requests now perform strict validation
by default even if the genetlink family does not set policy and maxtype
because it does validation and parsing on its own (e.g. because it wants to
allow different message format for different commands). While the null
policy will be ignored, maxtype (which would be zero) is still checked so
that any attribute will fail validation.

The solution is to only call __nla_validate() from genl_family_rcv_msg()
if family->maxtype is set.

Fixes: ef6243acb478 ("genetlink: optionally validate strictly/dumps")
Signed-off-by: Michal Kubecek <redacted>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/netlink/genetlink.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 72668759cd2b..9814d6dbd2d6 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c

@@ -537,21 +537,25 @@ static int genl_family_rcv_msg(const struct genl_family *family,
 			return -EOPNOTSUPP;
 
 		if (!(ops->validate & GENL_DONT_VALIDATE_DUMP)) {
-			unsigned int validate = NL_VALIDATE_STRICT;
 			int hdrlen = GENL_HDRLEN + family->hdrsize;
 
-			if (ops->validate & GENL_DONT_VALIDATE_DUMP_STRICT)
-				validate = NL_VALIDATE_LIBERAL;
-
 			if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
 				return -EINVAL;
 
-			rc = __nla_validate(nlmsg_attrdata(nlh, hdrlen),
-					    nlmsg_attrlen(nlh, hdrlen),
-					    family->maxattr, family->policy,
-					    validate, extack);
-			if (rc)
-				return rc;
+			if (family->maxattr) {
+				unsigned int validate = NL_VALIDATE_STRICT;
+
+				if (ops->validate &
+				    GENL_DONT_VALIDATE_DUMP_STRICT)
+					validate = NL_VALIDATE_LIBERAL;
+				rc = __nla_validate(nlmsg_attrdata(nlh, hdrlen),
+						    nlmsg_attrlen(nlh, hdrlen),
+						    family->maxattr,
+						    family->policy,
+						    validate, extack);
+				if (rc)
+					return rc;
+			}
 		}
 
 		if (!family->parallel_ops) {

-- 
2.21.0

The check that attribute type is within 0...maxtype range in
__nla_validate_parse() sets only error message but not bad_attr in extack.
Set also bad_attr to tell userspace which attribute failed validation.

Signed-off-by: Michal Kubecek <redacted>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Reviewed-by: David Ahern <redacted>
---
 lib/nlattr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 29f6336e2422..adc919b32bf9 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c

@@ -356,7 +356,8 @@ static int __nla_validate_parse(const struct nlattr *head, int len, int maxtype,
 
 		if (type == 0 || type > maxtype) {
 			if (validate & NL_VALIDATE_MAXTYPE) {
-				NL_SET_ERR_MSG(extack, "Unknown attribute type");
+				NL_SET_ERR_MSG_ATTR(extack, nla,
+						    "Unknown attribute type");
 				return -EINVAL;
 			}
 			continue;

-- 
2.21.0

Add new validation flag NL_VALIDATE_NESTED which adds three consistency
checks of NLA_F_NESTED_FLAG:

  - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
  - the flag is not set on attributes with other policies except NLA_UNSPEC
  - the flag is set on attribute passed to nla_parse_nested()

Signed-off-by: Michal Kubecek <redacted>

v2: change error messages to mention NLA_F_NESTED explicitly
---
 include/net/netlink.h | 11 ++++++++++-
 lib/nlattr.c          | 15 +++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 679f649748d4..395b4406f4b0 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h

@@ -401,6 +401,8 @@ struct nl_info {
  *	are enforced going forward.
  * @NL_VALIDATE_STRICT_ATTRS: strict attribute policy parsing (e.g.
  *	U8, U16, U32 must have exact size, etc.)
+ * @NL_VALIDATE_NESTED: Check that NLA_F_NESTED is set for NLA_NESTED(_ARRAY)
+ *	and unset for other policies.
  */
 enum netlink_validation {
 	NL_VALIDATE_LIBERAL = 0,

@@ -408,6 +410,7 @@ enum netlink_validation {
 	NL_VALIDATE_MAXTYPE = BIT(1),
 	NL_VALIDATE_UNSPEC = BIT(2),
 	NL_VALIDATE_STRICT_ATTRS = BIT(3),
+	NL_VALIDATE_NESTED = BIT(4),
 };
 
 #define NL_VALIDATE_DEPRECATED_STRICT (NL_VALIDATE_TRAILING |\

@@ -415,7 +418,8 @@ enum netlink_validation {
 #define NL_VALIDATE_STRICT (NL_VALIDATE_TRAILING |\
 			    NL_VALIDATE_MAXTYPE |\
 			    NL_VALIDATE_UNSPEC |\
-			    NL_VALIDATE_STRICT_ATTRS)
+			    NL_VALIDATE_STRICT_ATTRS |\
+			    NL_VALIDATE_NESTED)
 
 int netlink_rcv_skb(struct sk_buff *skb,
 		    int (*cb)(struct sk_buff *, struct nlmsghdr *,

@@ -1132,6 +1136,11 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
 				   const struct nla_policy *policy,
 				   struct netlink_ext_ack *extack)
 {
+	if (!(nla->nla_type & NLA_F_NESTED)) {
+		NL_SET_ERR_MSG_ATTR(extack, nla, "NLA_F_NESTED is missing");
+		return -EINVAL;
+	}
+
 	return __nla_parse(tb, maxtype, nla_data(nla), nla_len(nla), policy,
 			   NL_VALIDATE_STRICT, extack);
 }

diff --git a/lib/nlattr.c b/lib/nlattr.c
index adc919b32bf9..cace9b307781 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c

@@ -184,6 +184,21 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 		}
 	}
 
+	if (validate & NL_VALIDATE_NESTED) {
+		if ((pt->type == NLA_NESTED || pt->type == NLA_NESTED_ARRAY) &&
+		    !(nla->nla_type & NLA_F_NESTED)) {
+			NL_SET_ERR_MSG_ATTR(extack, nla,
+					    "NLA_F_NESTED is missing");
+			return -EINVAL;
+		}
+		if (pt->type != NLA_NESTED && pt->type != NLA_NESTED_ARRAY &&
+		    pt->type != NLA_UNSPEC && (nla->nla_type & NLA_F_NESTED)) {
+			NL_SET_ERR_MSG_ATTR(extack, nla,
+					    "NLA_F_NESTED not expected");
+			return -EINVAL;
+		}
+	}
+
 	switch (pt->type) {
 	case NLA_EXACT_LEN:
 		if (attrlen != pt->len)

-- 
2.21.0

On Thu, 2019-05-02 at 16:15 +0200, Michal Kubecek wrote:

Add new validation flag NL_VALIDATE_NESTED which adds three consistency
checks of NLA_F_NESTED_FLAG:

  - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
  - the flag is not set on attributes with other policies except NLA_UNSPEC
  - the flag is set on attribute passed to nla_parse_nested()

Signed-off-by: Michal Kubecek <redacted>

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

johannes

On 5/2/19 8:15 AM, Michal Kubecek wrote:

Add new validation flag NL_VALIDATE_NESTED which adds three consistency
checks of NLA_F_NESTED_FLAG:

  - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
  - the flag is not set on attributes with other policies except NLA_UNSPEC
  - the flag is set on attribute passed to nla_parse_nested()

Signed-off-by: Michal Kubecek <redacted>

v2: change error messages to mention NLA_F_NESTED explicitly
---
 include/net/netlink.h | 11 ++++++++++-
 lib/nlattr.c          | 15 +++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

Reviewed-by: David Ahern <redacted>

From: Michal Kubecek <redacted>
Date: Thu,  2 May 2019 16:15:10 +0200 (CEST)

Three follow-up patches for recent strict netlink validation series.

Patch 1 fixes dump handling for genetlink families which validate and parse
messages themselves (e.g. because they need different policies for diferent
commands).

Patch 2 sets bad_attr in extack in one place where this was omitted.

Patch 3 adds new NL_VALIDATE_NESTED flags for strict validation to enable
checking that NLA_F_NESTED value in received messages matches expectations
and includes this flag in NL_VALIDATE_STRICT. This would change userspace
visible behavior but the previous switching to NL_VALIDATE_STRICT for new
code is still only in net-next at the moment.

v2: change error messages to mention NLA_F_NESTED explicitly

Series applied.

On Thu, 2019-05-02 at 16:15 +0200, Michal Kubecek wrote:

Add new validation flag NL_VALIDATE_NESTED which adds three
consistency
checks of NLA_F_NESTED_FLAG:

  - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
  - the flag is not set on attributes with other policies except
NLA_UNSPEC
  - the flag is set on attribute passed to nla_parse_nested()

Signed-off-by: Michal Kubecek <redacted>

v2: change error messages to mention NLA_F_NESTED explicitly
---
 include/net/netlink.h | 11 ++++++++++-
 lib/nlattr.c          | 15 +++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

Hi,


libnl3 does currently not ever set NLA_F_NESTED flag.

That means, nla_nest_start() will not work as it used to.
https://github.com/thom311/libnl/blob/65b3dd5ac2d5de4c7a0c64e430596d9d27973527/lib/attr.c#L902

As workaround, one could call

  nla_nest_start(msg, NLA_F_NESTED | attr);


Of course, that is a bug in libnl3 that should be fixed. But it seems
quite unfortunate to me.


Does this flag and strict validation really provide any value? Commonly a netlink message
is a plain TLV blob, and the meaning depends entirely on the policy.

What I mean is that for example

  NLA_PUT_U32 (msg, ATTR_IFINDEX, (uint32_t) ifindex)
  NLA_PUT_STRING (msg, ATTR_IFNAME, "net")

results in a 4 bytes payload that does not encode whether the data is a number or
a string.

Why is it valuable in this case to encode additional type information inside the message,
when it's commonly not done and also not necessary?


best,
Thomas

On Tue, Jul 23, 2019 at 10:57:54AM +0200, Thomas Haller wrote:

Does this flag and strict validation really provide any value?
Commonly a netlink message is a plain TLV blob, and the meaning
depends entirely on the policy.

What I mean is that for example

  NLA_PUT_U32 (msg, ATTR_IFINDEX, (uint32_t) ifindex)
  NLA_PUT_STRING (msg, ATTR_IFNAME, "net")

results in a 4 bytes payload that does not encode whether the data is
a number or a string.

Why is it valuable in this case to encode additional type information
inside the message, when it's commonly not done and also not
necessary?

One big advantage of having nested attributes explicitly marked is that
it allows parsers not aware of the semantics to recognize nested
attributes and parse their inner structure.

This is very important e.g. for debugging purposes as without the flag,
wireshark can only recurse into nested attributes if it understands the
protocol and knows they are nested, otherwise it displays them only as
an opaque blob (which is what happens for most netlink based protocols).
Another example is mnl_nlmsg_fprintf() function from libmnl which is
also a valuable debugging aid but without NLA_F_NESTED flags it cannot
show message structure properly.

Michal Kubecek

On Tue, 2019-07-23 at 11:09 +0200, Michal Kubecek wrote:

On Tue, Jul 23, 2019 at 10:57:54AM +0200, Thomas Haller wrote:

quoted

Does this flag and strict validation really provide any value?
Commonly a netlink message is a plain TLV blob, and the meaning
depends entirely on the policy.

What I mean is that for example

  NLA_PUT_U32 (msg, ATTR_IFINDEX, (uint32_t) ifindex)
  NLA_PUT_STRING (msg, ATTR_IFNAME, "net")

results in a 4 bytes payload that does not encode whether the data
is
a number or a string.

Why is it valuable in this case to encode additional type
information
inside the message, when it's commonly not done and also not
necessary?

One big advantage of having nested attributes explicitly marked is
that
it allows parsers not aware of the semantics to recognize nested
attributes and parse their inner structure.

This is very important e.g. for debugging purposes as without the
flag,
wireshark can only recurse into nested attributes if it understands
the
protocol and knows they are nested, otherwise it displays them only
as
an opaque blob (which is what happens for most netlink based
protocols).
Another example is mnl_nlmsg_fprintf() function from libmnl which is
also a valuable debugging aid but without NLA_F_NESTED flags it
cannot
show message structure properly.

Hi,

I don't question the use of the flag. I question whether it's necessary
for kernel to strictly require the sending side to aid debuggability.

"e.g. for debugging purposes" makes it sound like it would be important
for something else. I wonder what else.


Anyway. What you elaborate makes sense!! Thanks


My main point was to raise awareness that this is a problem for libnl3.


best,
Thomas

On Thu,  2 May 2019 16:15:10 +0200 (CEST)
Michal Kubecek [off-list ref] wrote:

Add new validation flag NL_VALIDATE_NESTED which adds three consistency
checks of NLA_F_NESTED_FLAG:

  - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
  - the flag is not set on attributes with other policies except NLA_UNSPEC
  - the flag is set on attribute passed to nla_parse_nested()

Signed-off-by: Michal Kubecek <redacted>

v2: change error messages to mention NLA_F_NESTED explicitly

There are some cases where netlink related to IPv4 does not send nested
flag. You risk breaking older iproute2 and other tools being used on newer
kernel. I.e this patch may break binary compatibility. Have you tried running
with this on a very old distro (like Redhat Linux 9)?

On 7/23/19 1:57 AM, Thomas Haller wrote:

Does this flag and strict validation really provide any value? Commonly a netlink message
is a plain TLV blob, and the meaning depends entirely on the policy.

Strict checking enables kernel side filtering and other features that
require passing attributes as part of the dump request - like address
dumps in a specific namespace.