On Tue, Jul 16, 2019 at 08:00:04PM +0200, Florian Westphal wrote:
> Fernando Fernandez Mancera <ffmancera@xxxxxxxxxx> wrote:
> > El 16 de julio de 2019 18:47:11 CEST, Phil Sutter <phil@xxxxxx> escribió:
> > >Hi Pablo,
> > >
> > >On Tue, Jul 16, 2019 at 01:51:20PM +0200, Pablo Neira Ayuso wrote:
> > >[...]
> > >> diff --git a/src/evaluate.c b/src/evaluate.c
> > >> index f95f42e1067a..cd566e856a11 100644
> > >> --- a/src/evaluate.c
> > >> +++ b/src/evaluate.c
> > >> @@ -1984,17 +1984,9 @@ static int stmt_evaluate_verdict(struct
> > >eval_ctx *ctx, struct stmt *stmt)
> > >> case EXPR_VERDICT:
> > >> if (stmt->expr->verdict != NFT_CONTINUE)
> > >> stmt->flags |= STMT_F_TERMINAL;
> > >> - if (stmt->expr->chain != NULL) {
> > >> - if (expr_evaluate(ctx, &stmt->expr->chain) < 0)
> > >> - return -1;
> > >> - if ((stmt->expr->chain->etype != EXPR_SYMBOL &&
> > >> - stmt->expr->chain->etype != EXPR_VALUE) ||
> > >> - stmt->expr->chain->symtype != SYMBOL_VALUE) {
> > >> - return stmt_error(ctx, stmt,
> > >> - "invalid verdict chain expression %s\n",
> > >> - expr_name(stmt->expr->chain));
> > >> - }
> > >> - }
> > >
> > >According to my logs, this bit was added by Fernando to cover for
> > >invalid variable values[1]. So I fear we can't just drop this check.
> > >
> > >Cheers, Phil
> > >
> > >[1] I didn't check with current sources, but back then the following
> > > variable contents were problematic:
> > >
> > > * define foo = @set1 (a set named 'set1' must exist)
> > > * define foo = { 1024 }
> > > * define foo = *
> >
> > Yes I am looking to the report and why current version fails when the jump is to a non-base chain because I tested that some months ago.
> >
> > I will catch up with more details in a few hours. Sorry for the inconveniences.
>
> Fernando, in case Pablos patch v2 fixes the reported bug, could you
> followup with a test case? It would help when someone tries to remove
> "unneeded code" in the future ;-)
I'm not sure it's worth a test for this unlikely corner case.
There are thousands of paths where we're not performing strict
expression validation as in this case... and if you really want to get
this right.
