* [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.