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
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 <[email protected]>
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
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 <[email protected]>
Reviewed-by: Johannes Berg <[email protected]>
Reviewed-by: David Ahern <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Johannes Berg <[email protected]>
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 <[email protected]>
>
> 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 <[email protected]>
From: Michal Kubecek <[email protected]>
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 <[email protected]>
>
> 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:
> > 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 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
On Thu, 2 May 2019 16:15:10 +0200 (CEST)
Michal Kubecek <[email protected]> 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 <[email protected]>
>
> 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.