Hi Pablo,
On Wed, May 15, 2019 at 05:21:32PM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 15, 2019 at 01:46:17PM +0200, Phil Sutter wrote:
> > On Wed, May 15, 2019 at 01:12:32PM +0200, Pablo Neira Ayuso wrote:
> > > On Wed, May 15, 2019 at 01:02:05PM +0200, Fernando Fernandez Mancera
> > > wrote:
> > > >
> > > >
> > > > On 5/15/19 12:58 PM, Phil Sutter wrote:
> > > > > Hey,
> > > > >
> > > > > On Tue, May 14, 2019 at 11:13:40PM +0200, Fernando Fernandez Mancera
> > > > > wrote:
> > > > > [...]
> > > > >> diff --git a/src/datatype.c b/src/datatype.c
> > > > >> index 6aaf9ea..7e9ec5e 100644
> > > > >> --- a/src/datatype.c
> > > > >> +++ b/src/datatype.c
> > > > >> @@ -297,11 +297,22 @@ static void verdict_type_print(const struct
> > > > >> expr *expr, struct output_ctx *octx)
> > > > >> }
> > > > >> }
> > > > >>
> > > > >> +static struct error_record *verdict_type_parse(const struct expr
> > > > >> *sym,
> > > > >> + struct expr **res)
> > > > >> +{
> > > > >> + *res = constant_expr_alloc(&sym->location, &string_type,
> > > > >> + BYTEORDER_HOST_ENDIAN,
> > > > >> + (strlen(sym->identifier) + 1) *
> > > > >> BITS_PER_BYTE,
> > > > >> + sym->identifier);
> > > > >> + return NULL;
> > > > >> +}
> > > > >
> > > > > One more thing: The above lacks error checking of any kind. I *think*
> > > > > this is the place where one should make sure the symbol expression is
> > > > > actually a string (but I'm not quite sure how you do that).
> > > > >
> > > > > In any case, please try to exploit that variable support in the
> > > > > testcase
> > > > > (or maybe a separate one), just to make sure we don't allow weird
> > > > > things.
> > > > >
> > > >
> > > > I think I can get the symbol type and check if it is a string. I will
> > > > check this on the testcase as you said. Thanks!
> > >
> > > There's not much we can do in this case I think, have a look at
> > > string_type_parse().
> >
> > OK, maybe it's not as bad as I feared, symbol_parse() is called only if
> > we do have a symbol expr. Still I guess we should make sure nft
> > complains if the variable points to any other primary_expr or a set
> > reference ('@<something>').
>
> '@<something>' is currently allowed, as any arbitrary string can be
> placed in between strings - although in some way this is taking us
> back to the quote debate that needs to be addressed. If we want to
> disallow something enclosed in quotes then we'll have to apply this
> function everywhere we allow variables.
Oh, sorry. I put those ticks in there just to quote the value, not as
part of the value. The intention was to point out that something like:
| define foo = @set1
| add rule ip t c jump $foo
Might pass evaluation stage and since there is a special case for things
starting with '@' in symbol_expr, the added rule would turn into
| add rule ip t c jump set1
We could detect this situation by checking expr->symtype.
On the other hand, can we maybe check if given string points to an
*existing* chain in verdict_type_parse()? Or will that happen later
anyway?
Cheers, Phil
