| 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: <1512547571.26976.34.camel@sipsolutions.net>
Date: Wed, 06 Dec 2017 09:06:11 +0100
From: Johannes Berg <johannes@...solutions.net>
To: David Ahern <dsahern@...il.com>, netdev@...r.kernel.org
Subject: Re: [net] netlink: Relax attr validation for fixed length types
On Tue, 2017-12-05 at 12:55 -0700, David Ahern wrote:
> @@ -70,10 +78,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> BUG_ON(pt->type > NLA_TYPE_MAX);
>
> /* for data types NLA_U* and NLA_S* require exact length */
You should update the comment now :-)
And the comment on nla_attr_len as well.
With the comments fixed, this looks fine to me.
Reviewed-by: Johannes Berg <johannes@...solutions.net>
Since we already have two tables now and may want to add attribute
types with exact enforcement in the future (like the BITFIELD32 one),
I'd actually do things a bit more data driven, but I haven't tested it
right now, and it's better done in net-next after this fix.
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 8bf78b4b78f0..e65eb5400a1a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -15,21 +15,67 @@
#include <linux/types.h>
#include <net/netlink.h>
-/* for these data types attribute length must be exactly given size */
-static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
- [NLA_U8] = sizeof(u8),
- [NLA_U16] = sizeof(u16),
- [NLA_U32] = sizeof(u32),
- [NLA_U64] = sizeof(u64),
- [NLA_S8] = sizeof(s8),
- [NLA_S16] = sizeof(s16),
- [NLA_S32] = sizeof(s32),
- [NLA_S64] = sizeof(s64),
-};
-
-static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
- [NLA_MSECS] = sizeof(u64),
- [NLA_NESTED] = NLA_HDRLEN,
+/* netlink validation flags */
+#define NLVF_WARN_WRONG_LEN BIT(0)
+#define NLVF_REJECT_WRONG_LEN BIT(1)
+
+static const struct {
+ u8 len, flags;
+} nla_attr_val[NLA_TYPE_MAX + 1] = {
+ [NLA_FLAG] = {
+ .len = 0,
+ .flags = NLVF_REJECT_WRONG_LEN,
+ },
+ [NLA_BITFIELD32] = {
+ /* further checks below */
+ .len = sizeof(struct nla_bitfield32),
+ .flags = NLVF_REJECT_WRONG_LEN,
+ },
+ [NLA_U8] = {
+ .len = sizeof(u8),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_U16] = {
+ .len = sizeof(u16),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_U32] = {
+ .len = sizeof(u32),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_U64] = {
+ .len = sizeof(u64),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_S8] = {
+ .len = sizeof(s8),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_S16] = {
+ .len = sizeof(s16),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_S32] = {
+ .len = sizeof(s32),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_S64] = {
+ .len = sizeof(s64),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_MSECS] = {
+ .len = sizeof(s64),
+ .flags = NLVF_WARN_WRONG_LEN,
+ },
+ [NLA_NESTED] = {
+ /* minimum length */
+ .len = NLA_HDRLEN,
+ },
+ [NLA_STRING] = {
+ /* minimum length, further checks below */
+ .len = 1,
+ },
+ /* others have .len = 0 and no flags */
};
static int validate_nla_bitfield32(const struct nlattr *nla,
@@ -60,7 +106,7 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy)
{
const struct nla_policy *pt;
- int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
+ int minlen, attrlen = nla_len(nla), type = nla_type(nla);
if (type <= 0 || type > maxtype)
return 0;
@@ -69,23 +115,51 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
BUG_ON(pt->type > NLA_TYPE_MAX);
- /* for data types NLA_U* and NLA_S* require exact length */
- if (nla_attr_len[pt->type]) {
- if (attrlen != nla_attr_len[pt->type])
+ switch (pt->type) {
+ case NLA_BINARY:
+ if (pt->len && attrlen > pt->len)
return -ERANGE;
- return 0;
- }
+ break;
- switch (pt->type) {
- case NLA_FLAG:
- if (attrlen > 0)
+ case NLA_NESTED_COMPAT:
+ if (attrlen < pt->len)
+ return -ERANGE;
+ if (attrlen < NLA_ALIGN(pt->len))
+ break;
+ if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN)
+ return -ERANGE;
+ nla = nla_data(nla) + NLA_ALIGN(pt->len);
+ if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN + nla_len(nla))
return -ERANGE;
break;
- case NLA_BITFIELD32:
- if (attrlen != sizeof(struct nla_bitfield32))
+ case NLA_NESTED:
+ /* a nested attributes is allowed to be empty; if its not,
+ * it must have a size of at least NLA_HDRLEN.
+ */
+ if (attrlen == 0)
+ break;
+ /* fall through */
+ default:
+ minlen = nla_attr_val[pt->type].len;
+
+ if (nla_attr_val[pt->type].flags & NLVF_REJECT_WRONG_LEN &&
+ minlen != attrlen)
+ return -ERANGE;
+ if (nla_attr_val[pt->type].flags & NLVF_WARN_WRONG_LEN &&
+ minlen != attrlen)
+ pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
+ current->comm, type);
+
+ if (pt->len)
+ minlen = pt->len;
+
+ if (attrlen < minlen)
return -ERANGE;
+ }
+ switch (pt->type) {
+ case NLA_BITFIELD32:
return validate_nla_bitfield32(nla, pt->validation_data);
case NLA_NUL_STRING:
@@ -99,9 +173,6 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
/* fall through */
case NLA_STRING:
- if (attrlen < 1)
- return -ERANGE;
-
if (pt->len) {
char *buf = nla_data(nla);
@@ -112,37 +183,6 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
return -ERANGE;
}
break;
-
- case NLA_BINARY:
- if (pt->len && attrlen > pt->len)
- return -ERANGE;
- break;
-
- case NLA_NESTED_COMPAT:
- if (attrlen < pt->len)
- return -ERANGE;
- if (attrlen < NLA_ALIGN(pt->len))
- break;
- if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN)
- return -ERANGE;
- nla = nla_data(nla) + NLA_ALIGN(pt->len);
- if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN + nla_len(nla))
- return -ERANGE;
- break;
- case NLA_NESTED:
- /* a nested attributes is allowed to be empty; if its not,
- * it must have a size of at least NLA_HDRLEN.
- */
- if (attrlen == 0)
- break;
- default:
- if (pt->len)
- minlen = pt->len;
- else if (pt->type != NLA_UNSPEC)
- minlen = nla_attr_minlen[pt->type];
-
- if (attrlen < minlen)
- return -ERANGE;
}
return 0;
johannes
Powered by blists - more mailing lists