| Message ID | 1471017610-3473-5-git-send-email-phil@nwl.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Fri, Aug 12, 2016 at 06:00:10PM +0200, Phil Sutter wrote: > Looking at expr_evaluate_concat(), 'off' might be zero and the error > checks not triggering (by having dtype != NULL and i->dtype->size > 0). > Decrementing it will then lead to casting -1 to unsigned during the call > to concat_subtype_lookup() will lead to bit-shifting in > concat_subtype_id() by a value bigger than the number of bits in 'type' > (which is 32bit). > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > This patch is just an ugly sanitization hack and should probably be > substituted by an additional error check in expr_evaluate_concat() > giving an explanation of what went wrong. I agree. It would be good to look back and see what condition can indeed trigger this instead of papering the problem with this check. Thanks! > src/evaluate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/evaluate.c b/src/evaluate.c > index 523eedabe84ac..c8568690f6338 100644 > --- a/src/evaluate.c > +++ b/src/evaluate.c > @@ -950,7 +950,7 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr) > "expressions", > i->dtype->name); > > - tmp = concat_subtype_lookup(type, --off); > + tmp = concat_subtype_lookup(type, off > 0 ? --off : 0); > expr_set_context(&ctx->ectx, tmp, tmp->size); > > if (list_member_evaluate(ctx, &i) < 0) > -- > 2.8.2 > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/evaluate.c b/src/evaluate.c index 523eedabe84ac..c8568690f6338 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -950,7 +950,7 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr) "expressions", i->dtype->name); - tmp = concat_subtype_lookup(type, --off); + tmp = concat_subtype_lookup(type, off > 0 ? --off : 0); expr_set_context(&ctx->ectx, tmp, tmp->size); if (list_member_evaluate(ctx, &i) < 0)
Looking at expr_evaluate_concat(), 'off' might be zero and the error checks not triggering (by having dtype != NULL and i->dtype->size > 0). Decrementing it will then lead to casting -1 to unsigned during the call to concat_subtype_lookup() will lead to bit-shifting in concat_subtype_id() by a value bigger than the number of bits in 'type' (which is 32bit). Signed-off-by: Phil Sutter <phil@nwl.cc> --- This patch is just an ugly sanitization hack and should probably be substituted by an additional error check in expr_evaluate_concat() giving an explanation of what went wrong. --- src/evaluate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)