| Message ID | 20190716115120.21710-1-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] evaluate: bogus error when refering to existing non-base chain | expand |
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 = *
El 16 de julio de 2019 18:47:11 CEST, Phil Sutter <phil@nwl.cc> 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.
On Tue, Jul 16, 2019 at 01:51:20PM +0200, Pablo Neira Ayuso wrote: > add rule ip testNEW test6 jump test8 > ^^^^^ > Error: invalid verdict chain expression value Note that I can't reproduce this issue locally. [...] > - 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)); > - } So I guess the problem is that for an etype of EXPR_VALUE, symtype is still checked. The latter is used by EXPR_SYMBOL only, but since SYMBOL_VALUE is 0 (implicitly, it's the first item in enum symbol_types) this probably works by accident. I still don't understand why it doesn't work for you, but I guess the bug is found. So probably | if ((stmt->expr->chain->etype != EXPR_SYMBOL || | stmt->expr->chain->symtype != SYMBOL_VALUE) && | stmt->expr->chain->etype != EXPR_VALUE)) { is right. Cheers, Phil
Fernando Fernandez Mancera <ffmancera@riseup.net> wrote: > El 16 de julio de 2019 18:47:11 CEST, Phil Sutter <phil@nwl.cc> 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 ;-) Thanks, Florian
On Tue, Jul 16, 2019 at 08:00:04PM +0200, Florian Westphal wrote: > Fernando Fernandez Mancera <ffmancera@riseup.net> wrote: > > El 16 de julio de 2019 18:47:11 CEST, Phil Sutter <phil@nwl.cc> 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.
On Tue, Jul 16, 2019 at 08:12:53PM +0200, Pablo Neira Ayuso wrote: > On Tue, Jul 16, 2019 at 08:00:04PM +0200, Florian Westphal wrote: > > Fernando Fernandez Mancera <ffmancera@riseup.net> wrote: > > > El 16 de julio de 2019 18:47:11 CEST, Phil Sutter <phil@nwl.cc> 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. Having said this, if you want a test for this specific case, I really don't mind :-)
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > Having said this, if you want a test for this specific case, I really > don't mind :-) Fair enough, Fernando, if you think you have more important things to work on then just ignore this.
El 16 de julio de 2019 20:27:39 CEST, Florian Westphal <fw@strlen.de> escribió: >Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> Having said this, if you want a test for this specific case, I really >> don't mind :-) > >Fair enough, Fernando, if you think you have more important things to >work on then just ignore this. I am going to send a patch with a test for this case. It should be easy :-)
Hi, On 7/16/19 8:12 PM, Pablo Neira Ayuso wrote: > On Tue, Jul 16, 2019 at 08:00:04PM +0200, Florian Westphal wrote: >> Fernando Fernandez Mancera <ffmancera@riseup.net> wrote: >>> El 16 de julio de 2019 18:47:11 CEST, Phil Sutter <phil@nwl.cc> 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 have been taking a look to the shell tests and we already have some tests that cover these cases "tests/shell/testcases/chains/0001jumps_0", "tests/shell/testcases/nft-f/0018jump_variable_0", "tests/shell/testcases/nft-f/0019jump_variable_1", "tests/shell/testcases/nft-f/0020jump_variable_1". Also I have tested Pablo's v2 patch and it works fine for me. I would like to know how could I prevent this kind of situations in the future. Is there a way to automatically test your patch with other relevant kernel versions? Thanks! :-) > > 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. >
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)); - } - } + if (stmt->expr->chain != NULL && + expr_evaluate(ctx, &stmt->expr->chain) < 0) + return -1; break; case EXPR_MAP: break;
add rule ip testNEW test6 jump test8 ^^^^^ Error: invalid verdict chain expression value Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- src/evaluate.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)