netdev - Re: [PATCH nf-next] netfilter: add support for matching IPv4 options

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 31 May 2019 15:35:58 -0400
From:   Stephen Suryaputra <ssuryaextr@...il.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH nf-next] netfilter: add support for matching IPv4 options

On Fri, May 31, 2019 at 07:11:01PM +0200, Pablo Neira Ayuso wrote:
> > +/* find the offset to specified option or the header beyond the options
> > + * if target < 0.
> > + *
> > + * Note that *offset is used as input/output parameter, and if it is not zero,
> > + * then it must be a valid offset to an inner IPv4 header. This can be used
> > + * to explore inner IPv4 header, eg. ICMP error messages.
> 
> In other extension headers (IPv6 and TCP options) this offset is used
> to match for a field inside the extension / option.
> 
> So this semantics you describe here are slightly different, right?

It is the same as the IPv6 one. The offset returned is the offset to the
specific option (target) or the byte beyond the options if the target
isn't specified (< 0).

> Align parameters to parens when breaking too long lines.
> Please, also break lines at 80 chars.
> Please, define variables using reverse xmas tree, ie.

OK on these.

> > +	optlen = iph->ihl * 4 - (int)sizeof(struct iphdr);
> > +	if (optlen <= 0)
> > +		return -ENOENT;
> 
> You could just:
> 
>                 return -1;
> 
> in all these errors in ipv4_find_option() since nft_exthdr_ipv4_eval()
> does not use it.

Yes, but I followed the pattern in ipv6_find_hdr().

> I'd suggest:
> 
>                 if (!opt->srr)
>                         break;
> 
> So you save one level of indentation below.
> 
> same here:
> 
>                 if (!opt->rr)
>                         break;
> 
> and same thing for other extensions.

OK on these.
> 
> > +	if (!found)
> > +		target = -ENOENT;
> > +	return target;
> 
> Hm, 'target' value is never used, right?

Again, this follows ipv6_find_hdr().

> This should also work for the NFPROTO_INET (inet tables), NFPROTO_BRIDGE
> and the NFPROTO_NETDEV families.
> 
> I would turn this into:
> 
> 		if (ctx->family != NFPROTO_IPV6) {
> 

OK.

> > +			if (tb[NFTA_EXTHDR_DREG])
> > +				return &nft_exthdr_ipv4_ops;
> > +		}
> > +		break;
> 
> Then, from the _eval() path:
> 
> You have to replace iph->version == 4 check abive, you could use
> skb->protocol instead, and check for htons(ETH_P_IP) packets.

A bit lost. Did you mean this?

static int ipv4_find_option(struct net *net, struct sk_buff *skb,
»       »       »           unsigned int *offset, int target,
»       »       »           unsigned short *fragoff, int *flags)
{
	...
»       iph = skb_header_pointer(skb, *offset, sizeof(_iph), &_iph);
»       if (!iph || skb->protocol != htons(ETH_P_IP))
»       »       return -EBADMSG;

Thank you.