Commit e34b9ed9 authored by Florian Westphal's avatar Florian Westphal Committed by Pablo Neira Ayuso
Browse files

netfilter: nf_tables: avoid skb access on nf_stolen 



When verdict is NF_STOLEN, the skb might have been freed.

When tracing is enabled, this can result in a use-after-free:
1. access to skb->nf_trace
2. access to skb->mark
3. computation of trace id
4. dump of packet payload

To avoid 1, keep a cached copy of skb->nf_trace in the
trace state struct.
Refresh this copy whenever verdict is != STOLEN.

Avoid 2 by skipping skb->mark access if verdict is STOLEN.

3 is avoided by precomputing the trace id.

Only dump the packet when verdict is not "STOLEN".

Reported-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: default avatarFlorian Westphal <fw@strlen.de>
Signed-off-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
parent 05907f10

include/net/netfilter/nf_tables.h

+10 −6
Original line number Diff line number Diff line
@@ -1338,24 +1338,28 @@ void nft_unregister_flowtable_type(struct nf_flowtable_type *type); 
/**

 *	struct nft_traceinfo - nft tracing information and state

 *

 *	@trace: other struct members are initialised

 *	@nf_trace: copy of skb->nf_trace before rule evaluation

 *	@type: event type (enum nft_trace_types)

 *	@skbid: hash of skb to be used as trace id

 *	@packet_dumped: packet headers sent in a previous traceinfo message

 *	@pkt: pktinfo currently processed

 *	@basechain: base chain currently processed

 *	@chain: chain currently processed

 *	@rule:  rule that was evaluated

 *	@verdict: verdict given by rule

 *	@type: event type (enum nft_trace_types)

 *	@packet_dumped: packet headers sent in a previous traceinfo message

 *	@trace: other struct members are initialised

 */

struct nft_traceinfo {

	bool				trace;

	bool				nf_trace;

	bool				packet_dumped;

	enum nft_trace_types		type:8;

	u32				skbid;

	const struct nft_pktinfo	*pkt;

	const struct nft_base_chain	*basechain;

	const struct nft_chain		*chain;

	const struct nft_rule_dp	*rule;

	const struct nft_verdict	*verdict;

	enum nft_trace_types		type;

	bool				packet_dumped;

	bool				trace;

};



void nft_trace_init(struct nft_traceinfo *info, const struct nft_pktinfo *pkt,

net/netfilter/nf_tables_core.c

+21 −3
Original line number Diff line number Diff line
@@ -25,9 +25,7 @@ static noinline void __nft_trace_packet(struct nft_traceinfo *info, 
					const struct nft_chain *chain,

					enum nft_trace_types type)

{

	const struct nft_pktinfo *pkt = info->pkt;



	if (!info->trace || !pkt->skb->nf_trace)

	if (!info->trace || !info->nf_trace)

		return;



	info->chain = chain;
@@ -42,11 +40,24 @@ static inline void nft_trace_packet(struct nft_traceinfo *info, 
				    enum nft_trace_types type)

{

	if (static_branch_unlikely(&nft_trace_enabled)) {

		const struct nft_pktinfo *pkt = info->pkt;



		info->nf_trace = pkt->skb->nf_trace;

		info->rule = rule;

		__nft_trace_packet(info, chain, type);

	}

}



static inline void nft_trace_copy_nftrace(struct nft_traceinfo *info)

{

	if (static_branch_unlikely(&nft_trace_enabled)) {

		const struct nft_pktinfo *pkt = info->pkt;



		if (info->trace)

			info->nf_trace = pkt->skb->nf_trace;

	}

}



static void nft_bitwise_fast_eval(const struct nft_expr *expr,

				  struct nft_regs *regs)

{
@@ -85,6 +96,7 @@ static noinline void __nft_trace_verdict(struct nft_traceinfo *info, 
					 const struct nft_chain *chain,

					 const struct nft_regs *regs)

{

	const struct nft_pktinfo *pkt = info->pkt;

	enum nft_trace_types type;



	switch (regs->verdict.code) {
@@ -92,8 +104,13 @@ static noinline void __nft_trace_verdict(struct nft_traceinfo *info, 
	case NFT_RETURN:

		type = NFT_TRACETYPE_RETURN;

		break;

	case NF_STOLEN:

		type = NFT_TRACETYPE_RULE;

		/* can't access skb->nf_trace; use copy */

		break;

	default:

		type = NFT_TRACETYPE_RULE;

		info->nf_trace = pkt->skb->nf_trace;

		break;

	}
@@ -254,6 +271,7 @@ nft_do_chain(struct nft_pktinfo *pkt, void *priv) 
		switch (regs.verdict.code) {

		case NFT_BREAK:

			regs.verdict.code = NFT_CONTINUE;

			nft_trace_copy_nftrace(&info);

			continue;

		case NFT_CONTINUE:

			nft_trace_packet(&info, chain, rule,

net/netfilter/nf_tables_trace.c

+24 −20
Original line number Diff line number Diff line
@@ -7,7 +7,7 @@ 
#include <linux/module.h>

#include <linux/static_key.h>

#include <linux/hash.h>

#include <linux/jhash.h>

#include <linux/siphash.h>

#include <linux/if_vlan.h>

#include <linux/init.h>

#include <linux/skbuff.h>
@@ -25,22 +25,6 @@ 
DEFINE_STATIC_KEY_FALSE(nft_trace_enabled);

EXPORT_SYMBOL_GPL(nft_trace_enabled);



static int trace_fill_id(struct sk_buff *nlskb, struct sk_buff *skb)

{

	__be32 id;



	/* using skb address as ID results in a limited number of

	 * values (and quick reuse).

	 *

	 * So we attempt to use as many skb members that will not

	 * change while skb is with netfilter.

	 */

	id = (__be32)jhash_2words(hash32_ptr(skb), skb_get_hash(skb),

				  skb->skb_iif);



	return nla_put_be32(nlskb, NFTA_TRACE_ID, id);

}



static int trace_fill_header(struct sk_buff *nlskb, u16 type,

			     const struct sk_buff *skb,

			     int off, unsigned int len)
@@ -186,6 +170,7 @@ void nft_trace_notify(struct nft_traceinfo *info) 
	struct nlmsghdr *nlh;

	struct sk_buff *skb;

	unsigned int size;

	u32 mark = 0;

	u16 event;



	if (!nfnetlink_has_listeners(nft_net(pkt), NFNLGRP_NFTRACE))
@@ -229,7 +214,7 @@ void nft_trace_notify(struct nft_traceinfo *info) 
	if (nla_put_be32(skb, NFTA_TRACE_TYPE, htonl(info->type)))

		goto nla_put_failure;



	if (trace_fill_id(skb, pkt->skb))

	if (nla_put_u32(skb, NFTA_TRACE_ID, info->skbid))

		goto nla_put_failure;



	if (nla_put_string(skb, NFTA_TRACE_CHAIN, info->chain->name))
@@ -249,16 +234,24 @@ void nft_trace_notify(struct nft_traceinfo *info) 
	case NFT_TRACETYPE_RULE:

		if (nft_verdict_dump(skb, NFTA_TRACE_VERDICT, info->verdict))

			goto nla_put_failure;



		/* pkt->skb undefined iff NF_STOLEN, disable dump */

		if (info->verdict->code == NF_STOLEN)

			info->packet_dumped = true;

		else

			mark = pkt->skb->mark;



		break;

	case NFT_TRACETYPE_POLICY:

		mark = pkt->skb->mark;



		if (nla_put_be32(skb, NFTA_TRACE_POLICY,

				 htonl(info->basechain->policy)))

			goto nla_put_failure;

		break;

	}



	if (pkt->skb->mark &&

	    nla_put_be32(skb, NFTA_TRACE_MARK, htonl(pkt->skb->mark)))

	if (mark && nla_put_be32(skb, NFTA_TRACE_MARK, htonl(mark)))

		goto nla_put_failure;



	if (!info->packet_dumped) {
@@ -283,9 +276,20 @@ void nft_trace_init(struct nft_traceinfo *info, const struct nft_pktinfo *pkt, 
		    const struct nft_verdict *verdict,

		    const struct nft_chain *chain)

{

	static siphash_key_t trace_key __read_mostly;

	struct sk_buff *skb = pkt->skb;



	info->basechain = nft_base_chain(chain);

	info->trace = true;

	info->nf_trace = pkt->skb->nf_trace;

	info->packet_dumped = false;

	info->pkt = pkt;

	info->verdict = verdict;



	net_get_random_once(&trace_key, sizeof(trace_key));



	info->skbid = (u32)siphash_3u32(hash32_ptr(skb),

					skb_get_hash(skb),

					skb->skb_iif,

					&trace_key);

}