Re: [PATCH net] nfnetlink_osf: validate individual option lengths in fingerprints

[Weiming Shi]

On 26-03-19 09:04, Florian Westphal wrote:
> bestswngs@xxxxxxxxx <bestswngs@xxxxxxxxx> wrote:
> > From: Weiming Shi <bestswngs@xxxxxxxxx>
> > 
> > nfnl_osf_add_callback() validates opt_num bounds and string
> > NUL-termination but does not check individual option length fields.
> > A zero-length option causes nf_osf_match_one() to enter the option
> > matching loop even when foptsize sums to zero, which matches packets
> > with no TCP options where ctx->optp is NULL:
> 
> Would you mind if i squash:
> 
> diff --git a/net/netfilter/nfnetlink_osf.c b/net/netfilter/nfnetlink_osf.c
> --- a/net/netfilter/nfnetlink_osf.c
> +++ b/net/netfilter/nfnetlink_osf.c
> @@ -302,6 +302,7 @@ static int nfnl_osf_add_callback(struct sk_buff *skb,
>  {
>         struct nf_osf_user_finger *f;
>         struct nf_osf_finger *kf = NULL, *sf;
> +       unsigned int tot_opt_len = 0;
>         int err = 0;
>         int i;
> 
> @@ -320,10 +321,14 @@ static int nfnl_osf_add_callback(struct sk_buff *skb,
>                 return -EINVAL;
> 
>         for (i = 0; i < f->opt_num; i++) {
> -               if (!f->opt[i].length)
> +               if (!f->opt[i].length || f->opt[i].length > MAX_IPOPTLEN)
>                         return -EINVAL;
>                 if (f->opt[i].kind == OSFOPT_MSS && f->opt[i].length < 4)
>                         return -EINVAL;
> +
> +               tot_opt_len += f->opt[i].length;
> +               if (tot_opt_len > MAX_IPOPTLEN)
> +                       return -EINVAL;
>         }
> 
>         if (!memchr(f->genre, 0, MAXGENRELEN) ||
> 
> There is a runtime check (WTF) for this already, but arguably it
> better belongs here.
Hi Florian,

No problem, please go ahead. Thanks for improving it.

Weiming Shi