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@xxxxxxxxxxxxx>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
