datatype: fix crash if wrong integer type is passed

datatype: fix crash if wrong integer type is passed

[Patrick McHardy]

I'm going through older commits in nftables that I didn't review at the
time and I'm wondering about this one:

commit a320531e78f1bcb12b24da048f34592771392a9a
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Wed Jul 24 15:14:22 2013 +0200

    datatype: fix crash if wrong integer type is passed
    
    Eric Leblond reported that this command:
    
    nft add rule ip6 filter input position 4 meta protocol icmpv6 accept
    
    crashes nft. The problem is that 'icmpv6' is wrong there, as
    meta protocol is expecting an ethernet protocol, that can be
    expressed as an hexadecimal.
    
    Now this command displays the following error:
    
    <cmdline>:1:52-57: Error: This is not a valid Ethernet protocol
    add rule ip6 filter input position 4 meta protocol icmpv6 accept
                                                       ^^^^^^
We have generic type checks so I don't want to add special ones (with
different error messages) for special cases.

Additionally I don't see the error, reverting that patch and executing
the mentioned rule produces:

<cmdline>:1:52-57: Error: datatype mismatch, expected Ethernet protocol, expression has type Internet protocol
add rule ip6 filter input position 4 meta protocol icmpv6 accept
                                     ~~~~~~~~~~~~~ ^^^^^^

Looking at the bugzilla entry, the bug report states:

BUG: invalid input descriptor type 538976288
nft: src/erec.c:100: erec_print: Assertion `0' failed.
Abandon

So the problem wasn't related to data types at all but simply due to
some bug in error reporting. I'll queue up a revert in my tree.

Re: datatype: fix crash if wrong integer type is passed

[Pablo Neira Ayuso]

On Fri, Jan 10, 2014 at 08:04:25AM +0000, Patrick McHardy wrote:
> I'm going through older commits in nftables that I didn't review at the
> time and I'm wondering about this one:
>
> commit a320531e78f1bcb12b24da048f34592771392a9a
> Author: Pablo Neira Ayuso <pablo@netfilter.org>
> Date:   Wed Jul 24 15:14:22 2013 +0200
>
>     datatype: fix crash if wrong integer type is passed
>
>     Eric Leblond reported that this command:
>
>     nft add rule ip6 filter input position 4 meta protocol icmpv6 accept
>
>     crashes nft. The problem is that 'icmpv6' is wrong there, as
>     meta protocol is expecting an ethernet protocol, that can be
>     expressed as an hexadecimal.
>
>     Now this command displays the following error:
>
>     <cmdline>:1:52-57: Error: This is not a valid Ethernet protocol
>     add rule ip6 filter input position 4 meta protocol icmpv6 accept
>                                                        ^^^^^^
> We have generic type checks so I don't want to add special ones (with
> different error messages) for special cases.
>
> Additionally I don't see the error, reverting that patch and executing
> the mentioned rule produces:
>
> <cmdline>:1:52-57: Error: datatype mismatch, expected Ethernet protocol, expression has type Internet protocol
> add rule ip6 filter input position 4 meta protocol icmpv6 accept
>                                      ~~~~~~~~~~~~~ ^^^^^^
>
> Looking at the bugzilla entry, the bug report states:
>
> BUG: invalid input descriptor type 538976288
> nft: src/erec.c:100: erec_print: Assertion `0' failed.
> Abandon
>
> So the problem wasn't related to data types at all but simply due to
> some bug in error reporting. I'll queue up a revert in my tree.

static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
{
        struct error_record *erec;
        struct symbol *sym;
        struct set *set;
        struct expr *new; <------- note this is not initialized

        switch ((*expr)->symtype) {
        case SYMBOL_VALUE:
                (*expr)->dtype = ctx->ectx.dtype;
                erec = symbol_parse(*expr, &new); <---- passed here
                if (erec != NULL) {
                        erec_queue(erec, ctx->msgs);
                        return -1;
                }
                break;
        ...
        }
        expr_free(*expr);
        *expr = new; <--------- if symbol_parse() returns NULL.
                                this is set to an invalid memory position.

        return expr_evaluate(ctx, expr);
}

Let's see symbol_parse():

struct error_record *symbol_parse(const struct expr *sym,
                                  struct expr **res)
{
        const struct datatype *dtype = sym->dtype;

        assert(sym->ops->type == EXPR_SYMBOL);

        if (dtype == NULL)
                return error(&sym->location, "No symbol type information");
        do {
                if (dtype->parse != NULL)
                        return dtype->parse(sym, res); <-----


....
}

And now integer_type_parse(), which is the datatype that is called
(note that code below assumes that this patch is reverted):

static struct error_record *integer_type_parse(const struct expr *sym,
                                               struct expr **res)
{
        mpz_t v;
        int len;

        mpz_init(v);
        if (gmp_sscanf(sym->identifier, "%Zu%n", v, &len) != 1 ||
            (int)strlen(sym->identifier) != len) {
                mpz_clear(v);
                if (sym->dtype != &integer_type)
                        return NULL; <-----


So integer_type_parse() returns NULL, note that **res is left
uninitialized.

Going back to expr_evaluate_symbol(), since NULL is returned, it
assumes that an expression has been allocated.

I guess this doesn't crash all the time as we're relying on an
uninitialized memory area and we can argue that the fix may not be
optimal, but I really think this is fixing something.

Re: datatype: fix crash if wrong integer type is passed

[Patrick McHardy]

On Fri, Jan 10, 2014 at 10:52:47AM +0100, Pablo Neira Ayuso wrote:
> On Fri, Jan 10, 2014 at 08:04:25AM +0000, Patrick McHardy wrote:
> >
> > Looking at the bugzilla entry, the bug report states:
> >
> > BUG: invalid input descriptor type 538976288
> > nft: src/erec.c:100: erec_print: Assertion `0' failed.
> > Abandon
> >
> > So the problem wasn't related to data types at all but simply due to
> > some bug in error reporting. I'll queue up a revert in my tree.
> 
> static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
> {
>         struct error_record *erec;
>         struct symbol *sym;
>         struct set *set;
>         struct expr *new; <------- note this is not initialized
> 
>         switch ((*expr)->symtype) {
>         case SYMBOL_VALUE:
>                 (*expr)->dtype = ctx->ectx.dtype;
>                 erec = symbol_parse(*expr, &new); <---- passed here
>                 if (erec != NULL) {
>                         erec_queue(erec, ctx->msgs);
>                         return -1;
>                 }
>                 break;
>         ...
>         }
>         expr_free(*expr);
>         *expr = new; <--------- if symbol_parse() returns NULL.
>                                 this is set to an invalid memory position.
> 
>         return expr_evaluate(ctx, expr);
> }
> 
> Let's see symbol_parse():
> 
> struct error_record *symbol_parse(const struct expr *sym,
>                                   struct expr **res)
> {
>         const struct datatype *dtype = sym->dtype;
> 
>         assert(sym->ops->type == EXPR_SYMBOL);
> 
>         if (dtype == NULL)
>                 return error(&sym->location, "No symbol type information");
>         do {
>                 if (dtype->parse != NULL)
>                         return dtype->parse(sym, res); <-----
> 
> 
> ....
> }
> 
> And now integer_type_parse(), which is the datatype that is called
> (note that code below assumes that this patch is reverted):
> 
> static struct error_record *integer_type_parse(const struct expr *sym,
>                                                struct expr **res)
> {
>         mpz_t v;
>         int len;
> 
>         mpz_init(v);
>         if (gmp_sscanf(sym->identifier, "%Zu%n", v, &len) != 1 ||
>             (int)strlen(sym->identifier) != len) {
>                 mpz_clear(v);
>                 if (sym->dtype != &integer_type)
>                         return NULL; <-----
> 
> 
> So integer_type_parse() returns NULL, note that **res is left
> uninitialized.

Right, but only if invoked from an upper layer datatype. This needs to
make sure we don't return NULL without an erec. This was actually a bug
in the meta expression that I just posted a fix for.

> Going back to expr_evaluate_symbol(), since NULL is returned, it
> assumes that an expression has been allocated.
> 
> I guess this doesn't crash all the time as we're relying on an
> uninitialized memory area and we can argue that the fix may not be
> optimal, but I really think this is fixing something.

Well, the proper fix seems to be to initialize *res to NULL in meta
before invoking the integer type parsing function. 

Re: datatype: fix crash if wrong integer type is passed

[Pablo Neira Ayuso]

On Fri, Jan 10, 2014 at 10:02:46AM +0000, Patrick McHardy wrote:
> On Fri, Jan 10, 2014 at 10:52:47AM +0100, Pablo Neira Ayuso wrote:
[...]
> > So integer_type_parse() returns NULL, note that **res is left
> > uninitialized.
> 
> Right, but only if invoked from an upper layer datatype. This needs to
> make sure we don't return NULL without an erec. This was actually a bug
> in the meta expression that I just posted a fix for.
>
> > Going back to expr_evaluate_symbol(), since NULL is returned, it
> > assumes that an expression has been allocated.
> > 
> > I guess this doesn't crash all the time as we're relying on an
> > uninitialized memory area and we can argue that the fix may not be
> > optimal, but I really think this is fixing something.
> 
> Well, the proper fix seems to be to initialize *res to NULL in meta
> before invoking the integer type parsing function.

I guess you mean this patch: http://patchwork.ozlabs.org/patch/309211/

I just think that patch descriptions should point to each other or be
reworded as the revert is taking as back to a crash that is fixed (in
a much more generic/nicer way) by your follow-up patch.

Re: datatype: fix crash if wrong integer type is passed

[Patrick McHardy]

On Fri, Jan 10, 2014 at 11:15:04AM +0100, Pablo Neira Ayuso wrote:
> On Fri, Jan 10, 2014 at 10:02:46AM +0000, Patrick McHardy wrote:
> > On Fri, Jan 10, 2014 at 10:52:47AM +0100, Pablo Neira Ayuso wrote:
> [...]
> > > So integer_type_parse() returns NULL, note that **res is left
> > > uninitialized.
> > 
> > Right, but only if invoked from an upper layer datatype. This needs to
> > make sure we don't return NULL without an erec. This was actually a bug
> > in the meta expression that I just posted a fix for.
> >
> > > Going back to expr_evaluate_symbol(), since NULL is returned, it
> > > assumes that an expression has been allocated.
> > > 
> > > I guess this doesn't crash all the time as we're relying on an
> > > uninitialized memory area and we can argue that the fix may not be
> > > optimal, but I really think this is fixing something.
> > 
> > Well, the proper fix seems to be to initialize *res to NULL in meta
> > before invoking the integer type parsing function.
> 
> I guess you mean this patch: http://patchwork.ozlabs.org/patch/309211/

Correct.