| Message ID | 20210126175502.9171-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] erec: Sanitize erec location indesc | expand |
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 >
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
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
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.
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
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.
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?
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.
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
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++; }
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(-)