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

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

[Michal Kubecek]

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

[PATCH net-next v2 1/3] genetlink: do not validate dump requests if there is no policy

[Michal Kubecek]

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 <mkubecek@suse.cz>
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

[PATCH net-next v2 3/3] netlink: add validation of NLA_F_NESTED flag

[Michal Kubecek]

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 <mkubecek@suse.cz>

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

Re: [PATCH net-next v2 3/3] netlink: add validation of NLA_F_NESTED flag

[Johannes Berg]

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 <mkubecek@suse.cz>

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

johannes

Re: [PATCH net-next v2 3/3] netlink: add validation of NLA_F_NESTED flag

[David Ahern]

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 <mkubecek@suse.cz>
> 
> 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 <dsahern@gmail.com>

Re: [PATCH net-next v2 3/3] netlink: add validation of NLA_F_NESTED flag

[Thomas Haller]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

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 <mkubecek@suse.cz>
> 
> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v2 3/3] netlink: add validation of NLA_F_NESTED flag

[Michal Kubecek]

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

Re: [PATCH net-next v2 3/3] netlink: add validation of NLA_F_NESTED flag

[Thomas Haller]

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]

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:
> > 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v2 3/3] netlink: add validation of NLA_F_NESTED flag

[David Ahern]

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.

Re: [PATCH net-next v2 3/3] netlink: add validation of NLA_F_NESTED flag

[Stephen Hemminger]

On Thu,  2 May 2019 16:15:10 +0200 (CEST)
Michal Kubecek <mkubecek@suse.cz> 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 <mkubecek@suse.cz>
> 
> 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)?

Re: [PATCH net-next v2 3/3] netlink: add validation of NLA_F_NESTED flag

[Johannes Berg]

On Tue, 2019-07-23 at 11:02 -0700, Stephen Hemminger wrote:
> 
> 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)?


There are *tons* of places where this (and other things) wasn't done
right, but the validation is only added for

 * all attributes on _new operations_ (that old userspace couldn't have
   been using since they're introduced after this patch)
 * _new attributes_ (dito, if the policy 'strict start' is filled)

johannes

[PATCH net-next v2 2/3] netlink: set bad attribute also on maxtype check

[Michal Kubecek]

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 <mkubecek@suse.cz>
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Reviewed-by: David Ahern <dsahern@gmail.com>
---
 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

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

[David Miller]

From: Michal Kubecek <mkubecek@suse.cz>
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.