Skip to content

Commit fecc5cd

Browse files
pvts-matpvts-mat
committed
netfilter: nf_tables: upfront validation of data via nft_data_init()
jira VULN-430 cve-pre CVE-2023-4244 commit-author Pablo Neira Ayuso <[email protected]> commit 341b694 upstream-diff Used the cleanly applying 9.4 backport 2313c52 Instead of parsing the data and then validate that type and length are correct, pass a description of the expected data so it can be validated upfront before parsing it to bail out earlier. This patch adds a new .size field to specify the maximum size of the data area. The .len field is optional and it is used as an input/output field, it provides the specific length of the expected data in the input path. If then .len field is not specified, then obtained length from the netlink attribute is stored. This is required by cmp, bitwise, range and immediate, which provide no netlink attribute that describes the data length. The immediate expression uses the destination register type to infer the expected data type. Relying on opencoded validation of the expected data might lead to subtle bugs as described in 7e6bc1f ("netfilter: nf_tables: stricter validation of element data"). Signed-off-by: Pablo Neira Ayuso <[email protected]> (cherry picked from commit 341b694) Signed-off-by: Marcin Wcisło <[email protected]>
1 parent b39b3ba commit fecc5cd

File tree

6 files changed

+123
-114
lines changed

6 files changed

+123
-114
lines changed

‎include/net/netfilter/nf_tables.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -197,12 +197,12 @@ enum nft_data_desc_flags {
197197

198198
struct nft_data_desc {
199199
enum nft_data_types type;
200+
unsigned int size;
200201
unsigned int len;
201202
unsigned int flags;
202203
};
203204

204-
int nft_data_init(const struct nft_ctx *ctx,
205-
struct nft_data *data, unsigned int size,
205+
int nft_data_init(const struct nft_ctx *ctx, struct nft_data *data,
206206
struct nft_data_desc *desc, const struct nlattr *nla);
207207
void nft_data_hold(const struct nft_data *data, enum nft_data_types type);
208208
void nft_data_release(const struct nft_data *data, enum nft_data_types type);

‎net/netfilter/nf_tables_api.c‎

Lines changed: 41 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -5420,19 +5420,13 @@ static int nft_setelem_parse_flags(const struct nft_set *set,
54205420
static int nft_setelem_parse_key(struct nft_ctx *ctx, struct nft_set *set,
54215421
struct nft_data *key, struct nlattr *attr)
54225422
{
5423-
struct nft_data_desc desc;
5424-
int err;
5425-
5426-
err = nft_data_init(ctx, key, NFT_DATA_VALUE_MAXLEN, &desc, attr);
5427-
if (err < 0)
5428-
return err;
5429-
5430-
if (desc.type != NFT_DATA_VALUE || desc.len != set->klen) {
5431-
nft_data_release(key, desc.type);
5432-
return -EINVAL;
5433-
}
5423+
struct nft_data_desc desc = {
5424+
.type = NFT_DATA_VALUE,
5425+
.size = NFT_DATA_VALUE_MAXLEN,
5426+
.len = set->klen,
5427+
};
54345428

5435-
return 0;
5429+
return nft_data_init(ctx, key, &desc, attr);
54365430
}
54375431

54385432
static int nft_setelem_parse_data(struct nft_ctx *ctx, struct nft_set *set,
@@ -5441,26 +5435,18 @@ static int nft_setelem_parse_data(struct nft_ctx *ctx, struct nft_set *set,
54415435
struct nlattr *attr)
54425436
{
54435437
u32 dtype;
5444-
int err;
5445-
5446-
desc->flags = NFT_DATA_DESC_SETELEM;
5447-
5448-
err = nft_data_init(ctx, data, NFT_DATA_VALUE_MAXLEN, desc, attr);
5449-
if (err < 0)
5450-
return err;
54515438

54525439
if (set->dtype == NFT_DATA_VERDICT)
54535440
dtype = NFT_DATA_VERDICT;
54545441
else
54555442
dtype = NFT_DATA_VALUE;
54565443

5457-
if (dtype != desc->type ||
5458-
set->dlen != desc->len) {
5459-
nft_data_release(data, desc->type);
5460-
return -EINVAL;
5461-
}
5444+
desc->type = dtype;
5445+
desc->size = NFT_DATA_VALUE_MAXLEN;
5446+
desc->len = set->dlen;
5447+
desc->flags = NFT_DATA_DESC_SETELEM;
54625448

5463-
return 0;
5449+
return nft_data_init(ctx, data, desc, attr);
54645450
}
54655451

54665452
static void *nft_setelem_catchall_get(const struct net *net,
@@ -9931,7 +9917,7 @@ static int nft_verdict_init(const struct nft_ctx *ctx, struct nft_data *data,
99319917
}
99329918

99339919
desc->len = sizeof(data->verdict);
9934-
desc->type = NFT_DATA_VERDICT;
9920+
99359921
return 0;
99369922
}
99379923

@@ -9974,20 +9960,25 @@ int nft_verdict_dump(struct sk_buff *skb, int type, const struct nft_verdict *v)
99749960
}
99759961

99769962
static int nft_value_init(const struct nft_ctx *ctx,
9977-
struct nft_data *data, unsigned int size,
9978-
struct nft_data_desc *desc, const struct nlattr *nla)
9963+
struct nft_data *data, struct nft_data_desc *desc,
9964+
const struct nlattr *nla)
99799965
{
99809966
unsigned int len;
99819967

99829968
len = nla_len(nla);
99839969
if (len == 0)
99849970
return -EINVAL;
9985-
if (len > size)
9971+
if (len > desc->size)
99869972
return -EOVERFLOW;
9973+
if (desc->len) {
9974+
if (len != desc->len)
9975+
return -EINVAL;
9976+
} else {
9977+
desc->len = len;
9978+
}
99879979

99889980
nla_memcpy(data->data, nla, len);
9989-
desc->type = NFT_DATA_VALUE;
9990-
desc->len = len;
9981+
99919982
return 0;
99929983
}
99939984

@@ -10007,7 +9998,6 @@ static const struct nla_policy nft_data_policy[NFTA_DATA_MAX + 1] = {
100079998
*
100089999
* @ctx: context of the expression using the data
1000910000
* @data: destination struct nft_data
10010-
* @size: maximum data length
1001110001
* @desc: data description
1001210002
* @nla: netlink attribute containing data
1001310003
*
@@ -10017,24 +10007,35 @@ static const struct nla_policy nft_data_policy[NFTA_DATA_MAX + 1] = {
1001710007
* The caller can indicate that it only wants to accept data of type
1001810008
* NFT_DATA_VALUE by passing NULL for the ctx argument.
1001910009
*/
10020-
int nft_data_init(const struct nft_ctx *ctx,
10021-
struct nft_data *data, unsigned int size,
10010+
int nft_data_init(const struct nft_ctx *ctx, struct nft_data *data,
1002210011
struct nft_data_desc *desc, const struct nlattr *nla)
1002310012
{
1002410013
struct nlattr *tb[NFTA_DATA_MAX + 1];
1002510014
int err;
1002610015

10016+
if (WARN_ON_ONCE(!desc->size))
10017+
return -EINVAL;
10018+
1002710019
err = nla_parse_nested_deprecated(tb, NFTA_DATA_MAX, nla,
1002810020
nft_data_policy, NULL);
1002910021
if (err < 0)
1003010022
return err;
1003110023

10032-
if (tb[NFTA_DATA_VALUE])
10033-
return nft_value_init(ctx, data, size, desc,
10034-
tb[NFTA_DATA_VALUE]);
10035-
if (tb[NFTA_DATA_VERDICT] && ctx != NULL)
10036-
return nft_verdict_init(ctx, data, desc, tb[NFTA_DATA_VERDICT]);
10037-
return -EINVAL;
10024+
if (tb[NFTA_DATA_VALUE]) {
10025+
if (desc->type != NFT_DATA_VALUE)
10026+
return -EINVAL;
10027+
10028+
err = nft_value_init(ctx, data, desc, tb[NFTA_DATA_VALUE]);
10029+
} else if (tb[NFTA_DATA_VERDICT] && ctx != NULL) {
10030+
if (desc->type != NFT_DATA_VERDICT)
10031+
return -EINVAL;
10032+
10033+
err = nft_verdict_init(ctx, data, desc, tb[NFTA_DATA_VERDICT]);
10034+
} else {
10035+
err = -EINVAL;
10036+
}
10037+
10038+
return err;
1003810039
}
1003910040
EXPORT_SYMBOL_GPL(nft_data_init);
1004010041

‎net/netfilter/nft_bitwise.c‎

Lines changed: 34 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,16 @@ static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
9393
static int nft_bitwise_init_bool(struct nft_bitwise *priv,
9494
const struct nlattr *const tb[])
9595
{
96-
struct nft_data_desc mask, xor;
96+
struct nft_data_desc mask = {
97+
.type = NFT_DATA_VALUE,
98+
.size = sizeof(priv->mask),
99+
.len = priv->len,
100+
};
101+
struct nft_data_desc xor = {
102+
.type = NFT_DATA_VALUE,
103+
.size = sizeof(priv->xor),
104+
.len = priv->len,
105+
};
97106
int err;
98107

99108
if (tb[NFTA_BITWISE_DATA])
@@ -103,36 +112,30 @@ static int nft_bitwise_init_bool(struct nft_bitwise *priv,
103112
!tb[NFTA_BITWISE_XOR])
104113
return -EINVAL;
105114

106-
err = nft_data_init(NULL, &priv->mask, sizeof(priv->mask), &mask,
107-
tb[NFTA_BITWISE_MASK]);
115+
err = nft_data_init(NULL, &priv->mask, &mask, tb[NFTA_BITWISE_MASK]);
108116
if (err < 0)
109117
return err;
110-
if (mask.type != NFT_DATA_VALUE || mask.len != priv->len) {
111-
err = -EINVAL;
112-
goto err1;
113-
}
114118

115-
err = nft_data_init(NULL, &priv->xor, sizeof(priv->xor), &xor,
116-
tb[NFTA_BITWISE_XOR]);
119+
err = nft_data_init(NULL, &priv->xor, &xor, tb[NFTA_BITWISE_XOR]);
117120
if (err < 0)
118-
goto err1;
119-
if (xor.type != NFT_DATA_VALUE || xor.len != priv->len) {
120-
err = -EINVAL;
121-
goto err2;
122-
}
121+
goto err_xor_err;
123122

124123
return 0;
125-
err2:
126-
nft_data_release(&priv->xor, xor.type);
127-
err1:
124+
125+
err_xor_err:
128126
nft_data_release(&priv->mask, mask.type);
127+
129128
return err;
130129
}
131130

132131
static int nft_bitwise_init_shift(struct nft_bitwise *priv,
133132
const struct nlattr *const tb[])
134133
{
135-
struct nft_data_desc d;
134+
struct nft_data_desc desc = {
135+
.type = NFT_DATA_VALUE,
136+
.size = sizeof(priv->data),
137+
.len = sizeof(u32),
138+
};
136139
int err;
137140

138141
if (tb[NFTA_BITWISE_MASK] ||
@@ -142,13 +145,12 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
142145
if (!tb[NFTA_BITWISE_DATA])
143146
return -EINVAL;
144147

145-
err = nft_data_init(NULL, &priv->data, sizeof(priv->data), &d,
146-
tb[NFTA_BITWISE_DATA]);
148+
err = nft_data_init(NULL, &priv->data, &desc, tb[NFTA_BITWISE_DATA]);
147149
if (err < 0)
148150
return err;
149-
if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
150-
priv->data.data[0] >= BITS_PER_TYPE(u32)) {
151-
nft_data_release(&priv->data, d.type);
151+
152+
if (priv->data.data[0] >= BITS_PER_TYPE(u32)) {
153+
nft_data_release(&priv->data, desc.type);
152154
return -EINVAL;
153155
}
154156

@@ -291,22 +293,21 @@ static const struct nft_expr_ops nft_bitwise_ops = {
291293
static int
292294
nft_bitwise_extract_u32_data(const struct nlattr * const tb, u32 *out)
293295
{
294-
struct nft_data_desc desc;
295296
struct nft_data data;
296-
int err = 0;
297+
struct nft_data_desc desc = {
298+
.type = NFT_DATA_VALUE,
299+
.size = sizeof(data),
300+
.len = sizeof(u32),
301+
};
302+
int err;
297303

298-
err = nft_data_init(NULL, &data, sizeof(data), &desc, tb);
304+
err = nft_data_init(NULL, &data, &desc, tb);
299305
if (err < 0)
300306
return err;
301307

302-
if (desc.type != NFT_DATA_VALUE || desc.len != sizeof(u32)) {
303-
err = -EINVAL;
304-
goto err;
305-
}
306308
*out = data.data[0];
307-
err:
308-
nft_data_release(&data, desc.type);
309-
return err;
309+
310+
return 0;
310311
}
311312

312313
static int nft_bitwise_fast_init(const struct nft_ctx *ctx,

‎net/netfilter/nft_cmp.c‎

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -73,20 +73,16 @@ static int nft_cmp_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
7373
const struct nlattr * const tb[])
7474
{
7575
struct nft_cmp_expr *priv = nft_expr_priv(expr);
76-
struct nft_data_desc desc;
76+
struct nft_data_desc desc = {
77+
.type = NFT_DATA_VALUE,
78+
.size = sizeof(priv->data),
79+
};
7780
int err;
7881

79-
err = nft_data_init(NULL, &priv->data, sizeof(priv->data), &desc,
80-
tb[NFTA_CMP_DATA]);
82+
err = nft_data_init(NULL, &priv->data, &desc, tb[NFTA_CMP_DATA]);
8183
if (err < 0)
8284
return err;
8385

84-
if (desc.type != NFT_DATA_VALUE) {
85-
err = -EINVAL;
86-
nft_data_release(&priv->data, desc.type);
87-
return err;
88-
}
89-
9086
err = nft_parse_register_load(tb[NFTA_CMP_SREG], &priv->sreg, desc.len);
9187
if (err < 0)
9288
return err;
@@ -202,12 +198,14 @@ static int nft_cmp_fast_init(const struct nft_ctx *ctx,
202198
const struct nlattr * const tb[])
203199
{
204200
struct nft_cmp_fast_expr *priv = nft_expr_priv(expr);
205-
struct nft_data_desc desc;
206201
struct nft_data data;
202+
struct nft_data_desc desc = {
203+
.type = NFT_DATA_VALUE,
204+
.size = sizeof(data),
205+
};
207206
int err;
208207

209-
err = nft_data_init(NULL, &data, sizeof(data), &desc,
210-
tb[NFTA_CMP_DATA]);
208+
err = nft_data_init(NULL, &data, &desc, tb[NFTA_CMP_DATA]);
211209
if (err < 0)
212210
return err;
213211

@@ -277,8 +275,11 @@ const struct nft_expr_ops nft_cmp_fast_ops = {
277275
static const struct nft_expr_ops *
278276
nft_cmp_select_ops(const struct nft_ctx *ctx, const struct nlattr * const tb[])
279277
{
280-
struct nft_data_desc desc;
281278
struct nft_data data;
279+
struct nft_data_desc desc = {
280+
.type = NFT_DATA_VALUE,
281+
.size = sizeof(data),
282+
};
282283
enum nft_cmp_ops op;
283284
int err;
284285

@@ -300,21 +301,14 @@ nft_cmp_select_ops(const struct nft_ctx *ctx, const struct nlattr * const tb[])
300301
return ERR_PTR(-EINVAL);
301302
}
302303

303-
err = nft_data_init(NULL, &data, sizeof(data), &desc,
304-
tb[NFTA_CMP_DATA]);
304+
err = nft_data_init(NULL, &data, &desc, tb[NFTA_CMP_DATA]);
305305
if (err < 0)
306306
return ERR_PTR(err);
307307

308-
if (desc.type != NFT_DATA_VALUE)
309-
goto err1;
310-
311308
if (desc.len <= sizeof(u32) && (op == NFT_CMP_EQ || op == NFT_CMP_NEQ))
312309
return &nft_cmp_fast_ops;
313310

314311
return &nft_cmp_ops;
315-
err1:
316-
nft_data_release(&data, desc.type);
317-
return ERR_PTR(-EINVAL);
318312
}
319313

320314
struct nft_expr_type nft_cmp_type __read_mostly = {

0 commit comments

Comments
 (0)