Re: [nft PATCH] erec: Sanitize erec location indesc

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Tue, Feb 09, 2021 at 02:15:11PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Feb 03, 2021 at 11:45:07AM +0100, Phil Sutter wrote:
> > On Wed, Feb 03, 2021 at 01:38:32AM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote:
> > > > erec_print() unconditionally dereferences erec->locations->indesc, so
> > > > make sure it is valid when either creating an erec or adding a location.
> > > 
> > > I guess your're trigger a bug where erec is indesc is NULL, thing is
> > > that indesc should be always set on. Is there a reproducer for this bug?
> > 
> > Yes, exactly. I hit it when trying to clean up the netdev family reject
> > support, while just "hacking around". You can trigger it with the
> > following change:
> > 
> > | --- a/src/evaluate.c
> > | +++ b/src/evaluate.c
> > | @@ -2718,7 +2718,7 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt,
> > |         const struct proto_desc *desc;
> > |  
> > |         desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
> > | -       if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev)
> > | +       if (desc != &proto_eth && desc != &proto_vlan)
> > |                 return stmt_binary_error(ctx,
> > |                                          &ctx->pctx.protocol[PROTO_BASE_LL_HDR],
> > |                                          stmt, "unsupported link layer protocol");
> 
> I'm attaching fix.
> 
> Looks like call to stmt_binary_error() parameters are not in the right
> order, &ctx->pctx.protocol[PROTO_BASE_LL_HDR] has indesc.

Thanks for addressing the root problem!

> Probably add a bugtrap to erec to check that indesc is always set on
> accordingly instead?

Is it better than just sanitizing input to error functions? After all we
just want to make sure users see the error message, right? Catching
the programming mistake (wrong args passed to __stmt_binary_error())
IMHO is useful only if we can compile-time assert it. Otherwise we risk
hiding error info from user.

Cheers, Phil