| 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
| ||
|
Message-Id: <20190321220504.7642-6-johannes@sipsolutions.net>
Date: Thu, 21 Mar 2019 23:05:03 +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 5/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.
Signed-off-by: Johannes Berg <johannes.berg@...el.com>
---
include/net/netlink.h | 18 ++++++++++++++++++
lib/nlattr.c | 35 +++++++++++++++++++++++++++++------
2 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index ba79dd0cd23c..2f759867a264 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -301,6 +301,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 34d53e772779..439ac33d5692 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -69,7 +69,8 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
const struct nla_policy *policy,
- struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack,
+ bool strict)
{
const struct nlattr *entry;
int rem;
@@ -86,8 +87,13 @@ static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
return -ERANGE;
}
- ret = nla_validate(nla_data(entry), nla_len(entry),
- maxtype, policy, extack);
+ if (strict)
+ ret = nla_validate_strict(nla_data(entry),
+ nla_len(entry),
+ maxtype, policy, extack);
+ else
+ ret = nla_validate(nla_data(entry), nla_len(entry),
+ maxtype, policy, extack);
if (ret < 0)
return ret;
}
@@ -157,9 +163,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy,
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;
+ bool strict = strict_start_type && type >= strict_start_type;
if (type <= 0 || type > maxtype)
return 0;
@@ -172,6 +180,8 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
(pt->type == NLA_EXACT_LEN_WARN && attrlen != pt->len)) {
pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
current->comm, type);
+ if (strict)
+ return -EINVAL;
}
switch (pt->type) {
@@ -244,8 +254,15 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
if (attrlen < NLA_HDRLEN)
goto out_err;
if (pt->validation_data) {
- err = nla_validate(nla_data(nla), nla_len(nla), pt->len,
- pt->validation_data, extack);
+ if (strict)
+ err = nla_validate_strict(nla_data(nla),
+ nla_len(nla), pt->len,
+ pt->validation_data,
+ extack);
+ else
+ err = nla_validate(nla_data(nla), nla_len(nla),
+ pt->len, pt->validation_data,
+ extack);
if (err < 0) {
/*
* return directly to preserve the inner
@@ -268,7 +285,7 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
err = nla_validate_array(nla_data(nla), nla_len(nla),
pt->len, pt->validation_data,
- extack);
+ extack, strict);
if (err < 0) {
/*
* return directly to preserve the inner
@@ -280,6 +297,12 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
break;
case NLA_UNSPEC:
+ if (strict) {
+ NL_SET_ERR_MSG_ATTR(extack, nla,
+ "Attribute not understood");
+ return -EINVAL;
+ }
+ /* fall through */
case NLA_MIN_LEN:
if (attrlen < pt->len)
goto out_err;
--
2.17.2
Powered by blists - more mailing lists