* [nft PATCH] erec: Sanitize erec location indesc @ 2021-01-26 17:55 Phil Sutter 2021-02-03 0:38 ` Pablo Neira Ayuso 0 siblings, 1 reply; 10+ messages in thread From: Phil Sutter @ 2021-01-26 17:55 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel erec_print() unconditionally dereferences erec->locations->indesc, so make sure it is valid when either creating an erec or adding a location. Signed-off-by: Phil Sutter <phil@nwl.cc> --- src/erec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/erec.c b/src/erec.c index c550a596b38c8..5c3351a512464 100644 --- a/src/erec.c +++ b/src/erec.c @@ -38,7 +38,8 @@ void erec_add_location(struct error_record *erec, const struct location *loc) { assert(erec->num_locations < EREC_LOCATIONS_MAX); erec->locations[erec->num_locations] = *loc; - erec->locations[erec->num_locations].indesc = loc->indesc; + erec->locations[erec->num_locations].indesc = loc->indesc ? + : &internal_indesc; erec->num_locations++; } -- 2.28.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [nft PATCH] erec: Sanitize erec location indesc 2021-01-26 17:55 [nft PATCH] erec: Sanitize erec location indesc Phil Sutter @ 2021-02-03 0:38 ` Pablo Neira Ayuso 2021-02-03 10:45 ` Phil Sutter 0 siblings, 1 reply; 10+ messages in thread From: Pablo Neira Ayuso @ 2021-02-03 0:38 UTC (permalink / raw) To: Phil Sutter; +Cc: netfilter-devel Hi Phil, On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote: > erec_print() unconditionally dereferences erec->locations->indesc, so > make sure it is valid when either creating an erec or adding a location. I guess your're trigger a bug where erec is indesc is NULL, thing is that indesc should be always set on. Is there a reproducer for this bug? > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > src/erec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/erec.c b/src/erec.c > index c550a596b38c8..5c3351a512464 100644 > --- a/src/erec.c > +++ b/src/erec.c > @@ -38,7 +38,8 @@ void erec_add_location(struct error_record *erec, const struct location *loc) > { > assert(erec->num_locations < EREC_LOCATIONS_MAX); > erec->locations[erec->num_locations] = *loc; > - erec->locations[erec->num_locations].indesc = loc->indesc; > + erec->locations[erec->num_locations].indesc = loc->indesc ? > + : &internal_indesc; > erec->num_locations++; > } > > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [nft PATCH] erec: Sanitize erec location indesc 2021-02-03 0:38 ` Pablo Neira Ayuso @ 2021-02-03 10:45 ` Phil Sutter 2021-02-09 13:15 ` Pablo Neira Ayuso 0 siblings, 1 reply; 10+ messages in thread From: Phil Sutter @ 2021-02-03 10:45 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel Hi Pablo, On Wed, Feb 03, 2021 at 01:38:32AM +0100, Pablo Neira Ayuso wrote: > On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote: > > erec_print() unconditionally dereferences erec->locations->indesc, so > > make sure it is valid when either creating an erec or adding a location. > > I guess your're trigger a bug where erec is indesc is NULL, thing is > that indesc should be always set on. Is there a reproducer for this bug? Yes, exactly. I hit it when trying to clean up the netdev family reject support, while just "hacking around". You can trigger it with the following change: | --- a/src/evaluate.c | +++ b/src/evaluate.c | @@ -2718,7 +2718,7 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, | const struct proto_desc *desc; | | desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; | - if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) | + if (desc != &proto_eth && desc != &proto_vlan) | return stmt_binary_error(ctx, | &ctx->pctx.protocol[PROTO_BASE_LL_HDR], | stmt, "unsupported link layer protocol"); and this ruleset: | table netdev t { | chain c { | reject | } | } Cheers, Phil ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [nft PATCH] erec: Sanitize erec location indesc 2021-02-03 10:45 ` Phil Sutter @ 2021-02-09 13:15 ` Pablo Neira Ayuso 2021-02-09 13:22 ` Pablo Neira Ayuso 2021-02-09 14:11 ` Phil Sutter 0 siblings, 2 replies; 10+ messages in thread From: Pablo Neira Ayuso @ 2021-02-09 13:15 UTC (permalink / raw) To: Phil Sutter, netfilter-devel [-- Attachment #1: Type: text/plain, Size: 1676 bytes --] Hi Phil, On Wed, Feb 03, 2021 at 11:45:07AM +0100, Phil Sutter wrote: > Hi Pablo, > > On Wed, Feb 03, 2021 at 01:38:32AM +0100, Pablo Neira Ayuso wrote: > > On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote: > > > erec_print() unconditionally dereferences erec->locations->indesc, so > > > make sure it is valid when either creating an erec or adding a location. > > > > I guess your're trigger a bug where erec is indesc is NULL, thing is > > that indesc should be always set on. Is there a reproducer for this bug? > > Yes, exactly. I hit it when trying to clean up the netdev family reject > support, while just "hacking around". You can trigger it with the > following change: > > | --- a/src/evaluate.c > | +++ b/src/evaluate.c > | @@ -2718,7 +2718,7 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, > | const struct proto_desc *desc; > | > | desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; > | - if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) > | + if (desc != &proto_eth && desc != &proto_vlan) > | return stmt_binary_error(ctx, > | &ctx->pctx.protocol[PROTO_BASE_LL_HDR], > | stmt, "unsupported link layer protocol"); I'm attaching fix. Looks like call to stmt_binary_error() parameters are not in the right order, &ctx->pctx.protocol[PROTO_BASE_LL_HDR] has indesc. Probably add a bugtrap to erec to check that indesc is always set on accordingly instead? > and this ruleset: > > | table netdev t { > | chain c { > | reject > | } > | } > > Cheers, Phil [-- Attachment #2: x.patch --] [-- Type: text/x-diff, Size: 651 bytes --] diff --git a/src/evaluate.c b/src/evaluate.c index 030bbde4ab2c..771b71c83d01 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -2729,9 +2729,9 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) - return stmt_binary_error(ctx, + return stmt_binary_error(ctx, stmt, &ctx->pctx.protocol[PROTO_BASE_LL_HDR], - stmt, "unsupported link layer protocol"); + "unsupported link layer protocol"); desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc; if (desc != NULL && ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [nft PATCH] erec: Sanitize erec location indesc 2021-02-09 13:15 ` Pablo Neira Ayuso @ 2021-02-09 13:22 ` Pablo Neira Ayuso 2021-02-09 14:11 ` Phil Sutter 1 sibling, 0 replies; 10+ messages in thread From: Pablo Neira Ayuso @ 2021-02-09 13:22 UTC (permalink / raw) To: Phil Sutter, netfilter-devel [-- Attachment #1: Type: text/plain, Size: 1829 bytes --] On Tue, Feb 09, 2021 at 02:15:11PM +0100, Pablo Neira Ayuso wrote: > Hi Phil, > > On Wed, Feb 03, 2021 at 11:45:07AM +0100, Phil Sutter wrote: > > Hi Pablo, > > > > On Wed, Feb 03, 2021 at 01:38:32AM +0100, Pablo Neira Ayuso wrote: > > > On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote: > > > > erec_print() unconditionally dereferences erec->locations->indesc, so > > > > make sure it is valid when either creating an erec or adding a location. > > > > > > I guess your're trigger a bug where erec is indesc is NULL, thing is > > > that indesc should be always set on. Is there a reproducer for this bug? > > > > Yes, exactly. I hit it when trying to clean up the netdev family reject > > support, while just "hacking around". You can trigger it with the > > following change: > > > > | --- a/src/evaluate.c > > | +++ b/src/evaluate.c > > | @@ -2718,7 +2718,7 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, > > | const struct proto_desc *desc; > > | > > | desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; > > | - if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) > > | + if (desc != &proto_eth && desc != &proto_vlan) > > | return stmt_binary_error(ctx, > > | &ctx->pctx.protocol[PROTO_BASE_LL_HDR], > > | stmt, "unsupported link layer protocol"); > > I'm attaching fix. > > Looks like call to stmt_binary_error() parameters are not in the right > order, &ctx->pctx.protocol[PROTO_BASE_LL_HDR] has indesc. New patch fix, just do not use ctx->pctx.protocol[PROTO_BASE_LL_HDR] since it's an internal generated dependency, it is not visible from the rule, so stmt_binary_error() cannot really help with the error printing. [-- Attachment #2: x.patch --] [-- Type: text/x-diff, Size: 684 bytes --] diff --git a/src/evaluate.c b/src/evaluate.c index 030bbde4ab2c..782a5bca98bb 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -2729,9 +2729,8 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) - return stmt_binary_error(ctx, - &ctx->pctx.protocol[PROTO_BASE_LL_HDR], - stmt, "unsupported link layer protocol"); + return __stmt_binary_error(ctx, &stmt->location, NULL, + "cannot reject from this link layer protocol"); desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc; if (desc != NULL && ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [nft PATCH] erec: Sanitize erec location indesc 2021-02-09 13:15 ` Pablo Neira Ayuso 2021-02-09 13:22 ` Pablo Neira Ayuso @ 2021-02-09 14:11 ` Phil Sutter 2021-02-09 15:50 ` Pablo Neira Ayuso 1 sibling, 1 reply; 10+ messages in thread From: Phil Sutter @ 2021-02-09 14:11 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel Hi Pablo, On Tue, Feb 09, 2021 at 02:15:11PM +0100, Pablo Neira Ayuso wrote: > On Wed, Feb 03, 2021 at 11:45:07AM +0100, Phil Sutter wrote: > > On Wed, Feb 03, 2021 at 01:38:32AM +0100, Pablo Neira Ayuso wrote: > > > On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote: > > > > erec_print() unconditionally dereferences erec->locations->indesc, so > > > > make sure it is valid when either creating an erec or adding a location. > > > > > > I guess your're trigger a bug where erec is indesc is NULL, thing is > > > that indesc should be always set on. Is there a reproducer for this bug? > > > > Yes, exactly. I hit it when trying to clean up the netdev family reject > > support, while just "hacking around". You can trigger it with the > > following change: > > > > | --- a/src/evaluate.c > > | +++ b/src/evaluate.c > > | @@ -2718,7 +2718,7 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, > > | const struct proto_desc *desc; > > | > > | desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; > > | - if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) > > | + if (desc != &proto_eth && desc != &proto_vlan) > > | return stmt_binary_error(ctx, > > | &ctx->pctx.protocol[PROTO_BASE_LL_HDR], > > | stmt, "unsupported link layer protocol"); > > I'm attaching fix. > > Looks like call to stmt_binary_error() parameters are not in the right > order, &ctx->pctx.protocol[PROTO_BASE_LL_HDR] has indesc. Thanks for addressing the root problem! > Probably add a bugtrap to erec to check that indesc is always set on > accordingly instead? Is it better than just sanitizing input to error functions? After all we just want to make sure users see the error message, right? Catching the programming mistake (wrong args passed to __stmt_binary_error()) IMHO is useful only if we can compile-time assert it. Otherwise we risk hiding error info from user. Cheers, Phil ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [nft PATCH] erec: Sanitize erec location indesc 2021-02-09 14:11 ` Phil Sutter @ 2021-02-09 15:50 ` Pablo Neira Ayuso 2021-02-09 15:53 ` Pablo Neira Ayuso 0 siblings, 1 reply; 10+ messages in thread From: Pablo Neira Ayuso @ 2021-02-09 15:50 UTC (permalink / raw) To: Phil Sutter, netfilter-devel On Tue, Feb 09, 2021 at 03:11:51PM +0100, Phil Sutter wrote: > Hi Pablo, > > On Tue, Feb 09, 2021 at 02:15:11PM +0100, Pablo Neira Ayuso wrote: > > On Wed, Feb 03, 2021 at 11:45:07AM +0100, Phil Sutter wrote: > > > On Wed, Feb 03, 2021 at 01:38:32AM +0100, Pablo Neira Ayuso wrote: > > > > On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote: > > > > > erec_print() unconditionally dereferences erec->locations->indesc, so > > > > > make sure it is valid when either creating an erec or adding a location. > > > > > > > > I guess your're trigger a bug where erec is indesc is NULL, thing is > > > > that indesc should be always set on. Is there a reproducer for this bug? > > > > > > Yes, exactly. I hit it when trying to clean up the netdev family reject > > > support, while just "hacking around". You can trigger it with the > > > following change: > > > > > > | --- a/src/evaluate.c > > > | +++ b/src/evaluate.c > > > | @@ -2718,7 +2718,7 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, > > > | const struct proto_desc *desc; > > > | > > > | desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; > > > | - if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) > > > | + if (desc != &proto_eth && desc != &proto_vlan) > > > | return stmt_binary_error(ctx, > > > | &ctx->pctx.protocol[PROTO_BASE_LL_HDR], > > > | stmt, "unsupported link layer protocol"); > > > > I'm attaching fix. > > > > Looks like call to stmt_binary_error() parameters are not in the right > > order, &ctx->pctx.protocol[PROTO_BASE_LL_HDR] has indesc. > > Thanks for addressing the root problem! > > > Probably add a bugtrap to erec to check that indesc is always set on > > accordingly instead? > > Is it better than just sanitizing input to error functions? After all we > just want to make sure users see the error message, right? Catching > the programming mistake (wrong args passed to __stmt_binary_error()) > IMHO is useful only if we can compile-time assert it. Otherwise we risk > hiding error info from user. I see. I don't see a way to catch this at compile time. Push out your patch and I'll push mine too for correctness. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [nft PATCH] erec: Sanitize erec location indesc 2021-02-09 15:50 ` Pablo Neira Ayuso @ 2021-02-09 15:53 ` Pablo Neira Ayuso 2021-02-09 15:54 ` Pablo Neira Ayuso 0 siblings, 1 reply; 10+ messages in thread From: Pablo Neira Ayuso @ 2021-02-09 15:53 UTC (permalink / raw) To: Phil Sutter, netfilter-devel On Tue, Feb 09, 2021 at 04:50:30PM +0100, Pablo Neira Ayuso wrote: > On Tue, Feb 09, 2021 at 03:11:51PM +0100, Phil Sutter wrote: > > Hi Pablo, > > > > On Tue, Feb 09, 2021 at 02:15:11PM +0100, Pablo Neira Ayuso wrote: > > > On Wed, Feb 03, 2021 at 11:45:07AM +0100, Phil Sutter wrote: > > > > On Wed, Feb 03, 2021 at 01:38:32AM +0100, Pablo Neira Ayuso wrote: > > > > > On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote: > > > > > > erec_print() unconditionally dereferences erec->locations->indesc, so > > > > > > make sure it is valid when either creating an erec or adding a location. > > > > > > > > > > I guess your're trigger a bug where erec is indesc is NULL, thing is > > > > > that indesc should be always set on. Is there a reproducer for this bug? > > > > > > > > Yes, exactly. I hit it when trying to clean up the netdev family reject > > > > support, while just "hacking around". You can trigger it with the > > > > following change: > > > > > > > > | --- a/src/evaluate.c > > > > | +++ b/src/evaluate.c > > > > | @@ -2718,7 +2718,7 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, > > > > | const struct proto_desc *desc; > > > > | > > > > | desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; > > > > | - if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) > > > > | + if (desc != &proto_eth && desc != &proto_vlan) > > > > | return stmt_binary_error(ctx, > > > > | &ctx->pctx.protocol[PROTO_BASE_LL_HDR], > > > > | stmt, "unsupported link layer protocol"); > > > > > > I'm attaching fix. > > > > > > Looks like call to stmt_binary_error() parameters are not in the right > > > order, &ctx->pctx.protocol[PROTO_BASE_LL_HDR] has indesc. > > > > Thanks for addressing the root problem! > > > > > Probably add a bugtrap to erec to check that indesc is always set on > > > accordingly instead? > > > > Is it better than just sanitizing input to error functions? After all we > > just want to make sure users see the error message, right? Catching > > the programming mistake (wrong args passed to __stmt_binary_error()) > > IMHO is useful only if we can compile-time assert it. Otherwise we risk > > hiding error info from user. > > I see. I don't see a way to catch this at compile time. > > Push out your patch and I'll push mine too for correctness. Hm, one second: Probably set internal_indesc for autogenerated dependencies? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [nft PATCH] erec: Sanitize erec location indesc 2021-02-09 15:53 ` Pablo Neira Ayuso @ 2021-02-09 15:54 ` Pablo Neira Ayuso 2021-02-09 16:01 ` Phil Sutter 0 siblings, 1 reply; 10+ messages in thread From: Pablo Neira Ayuso @ 2021-02-09 15:54 UTC (permalink / raw) To: Phil Sutter, netfilter-devel On Tue, Feb 09, 2021 at 04:53:19PM +0100, Pablo Neira Ayuso wrote: > On Tue, Feb 09, 2021 at 04:50:30PM +0100, Pablo Neira Ayuso wrote: > > On Tue, Feb 09, 2021 at 03:11:51PM +0100, Phil Sutter wrote: > > > Hi Pablo, > > > > > > On Tue, Feb 09, 2021 at 02:15:11PM +0100, Pablo Neira Ayuso wrote: > > > > On Wed, Feb 03, 2021 at 11:45:07AM +0100, Phil Sutter wrote: > > > > > On Wed, Feb 03, 2021 at 01:38:32AM +0100, Pablo Neira Ayuso wrote: > > > > > > On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote: > > > > > > > erec_print() unconditionally dereferences erec->locations->indesc, so > > > > > > > make sure it is valid when either creating an erec or adding a location. > > > > > > > > > > > > I guess your're trigger a bug where erec is indesc is NULL, thing is > > > > > > that indesc should be always set on. Is there a reproducer for this bug? > > > > > > > > > > Yes, exactly. I hit it when trying to clean up the netdev family reject > > > > > support, while just "hacking around". You can trigger it with the > > > > > following change: > > > > > > > > > > | --- a/src/evaluate.c > > > > > | +++ b/src/evaluate.c > > > > > | @@ -2718,7 +2718,7 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, > > > > > | const struct proto_desc *desc; > > > > > | > > > > > | desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; > > > > > | - if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) > > > > > | + if (desc != &proto_eth && desc != &proto_vlan) > > > > > | return stmt_binary_error(ctx, > > > > > | &ctx->pctx.protocol[PROTO_BASE_LL_HDR], > > > > > | stmt, "unsupported link layer protocol"); > > > > > > > > I'm attaching fix. > > > > > > > > Looks like call to stmt_binary_error() parameters are not in the right > > > > order, &ctx->pctx.protocol[PROTO_BASE_LL_HDR] has indesc. > > > > > > Thanks for addressing the root problem! > > > > > > > Probably add a bugtrap to erec to check that indesc is always set on > > > > accordingly instead? > > > > > > Is it better than just sanitizing input to error functions? After all we > > > just want to make sure users see the error message, right? Catching > > > the programming mistake (wrong args passed to __stmt_binary_error()) > > > IMHO is useful only if we can compile-time assert it. Otherwise we risk > > > hiding error info from user. > > > > I see. I don't see a way to catch this at compile time. > > > > Push out your patch and I'll push mine too for correctness. > > Hm, one second: Probably set internal_indesc for autogenerated > dependencies? Either way, it's just changing where internal_indesc is set. Probably not worth spending more cycles on this issue. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [nft PATCH] erec: Sanitize erec location indesc 2021-02-09 15:54 ` Pablo Neira Ayuso @ 2021-02-09 16:01 ` Phil Sutter 0 siblings, 0 replies; 10+ messages in thread From: Phil Sutter @ 2021-02-09 16:01 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel On Tue, Feb 09, 2021 at 04:54:33PM +0100, Pablo Neira Ayuso wrote: > On Tue, Feb 09, 2021 at 04:53:19PM +0100, Pablo Neira Ayuso wrote: > > On Tue, Feb 09, 2021 at 04:50:30PM +0100, Pablo Neira Ayuso wrote: > > > On Tue, Feb 09, 2021 at 03:11:51PM +0100, Phil Sutter wrote: > > > > Hi Pablo, > > > > > > > > On Tue, Feb 09, 2021 at 02:15:11PM +0100, Pablo Neira Ayuso wrote: > > > > > On Wed, Feb 03, 2021 at 11:45:07AM +0100, Phil Sutter wrote: > > > > > > On Wed, Feb 03, 2021 at 01:38:32AM +0100, Pablo Neira Ayuso wrote: > > > > > > > On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote: > > > > > > > > erec_print() unconditionally dereferences erec->locations->indesc, so > > > > > > > > make sure it is valid when either creating an erec or adding a location. > > > > > > > > > > > > > > I guess your're trigger a bug where erec is indesc is NULL, thing is > > > > > > > that indesc should be always set on. Is there a reproducer for this bug? > > > > > > > > > > > > Yes, exactly. I hit it when trying to clean up the netdev family reject > > > > > > support, while just "hacking around". You can trigger it with the > > > > > > following change: > > > > > > > > > > > > | --- a/src/evaluate.c > > > > > > | +++ b/src/evaluate.c > > > > > > | @@ -2718,7 +2718,7 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, > > > > > > | const struct proto_desc *desc; > > > > > > | > > > > > > | desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; > > > > > > | - if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) > > > > > > | + if (desc != &proto_eth && desc != &proto_vlan) > > > > > > | return stmt_binary_error(ctx, > > > > > > | &ctx->pctx.protocol[PROTO_BASE_LL_HDR], > > > > > > | stmt, "unsupported link layer protocol"); > > > > > > > > > > I'm attaching fix. > > > > > > > > > > Looks like call to stmt_binary_error() parameters are not in the right > > > > > order, &ctx->pctx.protocol[PROTO_BASE_LL_HDR] has indesc. > > > > > > > > Thanks for addressing the root problem! > > > > > > > > > Probably add a bugtrap to erec to check that indesc is always set on > > > > > accordingly instead? > > > > > > > > Is it better than just sanitizing input to error functions? After all we > > > > just want to make sure users see the error message, right? Catching > > > > the programming mistake (wrong args passed to __stmt_binary_error()) > > > > IMHO is useful only if we can compile-time assert it. Otherwise we risk > > > > hiding error info from user. > > > > > > I see. I don't see a way to catch this at compile time. > > > > > > Push out your patch and I'll push mine too for correctness. DONE. > > Hm, one second: Probably set internal_indesc for autogenerated > > dependencies? > > Either way, it's just changing where internal_indesc is set. > > Probably not worth spending more cycles on this issue. ACK. :) Cheers, Phil ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-09 16:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-26 17:55 [nft PATCH] erec: Sanitize erec location indesc Phil Sutter 2021-02-03 0:38 ` Pablo Neira Ayuso 2021-02-03 10:45 ` Phil Sutter 2021-02-09 13:15 ` Pablo Neira Ayuso 2021-02-09 13:22 ` Pablo Neira Ayuso 2021-02-09 14:11 ` Phil Sutter 2021-02-09 15:50 ` Pablo Neira Ayuso 2021-02-09 15:53 ` Pablo Neira Ayuso 2021-02-09 15:54 ` Pablo Neira Ayuso 2021-02-09 16:01 ` Phil Sutter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.