Re: [External] : Re: pf.conf(5) clarify ICMP sloppy state handling
On 2022/05/09 23:16, Alexandr Nedvedicky wrote: > Hello, > > I'm sorry I was too fast with commit. I've just committed > what's been suggested by bluhm@: That's totally ok, my diff is on top and wasn't written until you committed yours :-) > @@ -2186,6 +2186,7 @@ It cannot be used with > .Cm modulate state > or > .Cm synproxy state . > +With this option ICMP replies can create states. > .It Ar timeout seconds > Changes the > .Ar timeout > > > > This is helpful, but because it's so surprising that "pass proto icmp" > > doesn't pass all icmp traffic, I think it would help to mention it where > > "proto icmp" is described too. > > > > Also, the top of the text about "sloppy" just talks about the sloppy > > TCP connection tracker, I think perhaps it would be better to lead > > with something that suggests it has multiple functions for different > > protocols? > > I don't object to any of your enhancements. > > reads OK sashan Thanks.
Re: [External] : Re: pf.conf(5) clarify ICMP sloppy state handling
On Mon, May 09, 2022 at 10:08:24PM +0100, Stuart Henderson wrote: > This is helpful, but because it's so surprising that "pass proto icmp" > doesn't pass all icmp traffic, I think it would help to mention it where > "proto icmp" is described too. > > Also, the top of the text about "sloppy" just talks about the sloppy > TCP connection tracker, I think perhaps it would be better to lead > with something that suggests it has multiple functions for different > protocols? OK bluhm@ > Index: man5/pf.conf.5 > === > RCS file: /cvs/src/share/man/man5/pf.conf.5,v > retrieving revision 1.594 > diff -u -p -r1.594 pf.conf.5 > --- man5/pf.conf.59 May 2022 20:29:23 - 1.594 > +++ man5/pf.conf.59 May 2022 21:05:48 - > @@ -594,6 +594,13 @@ or > .Pc > must match. > .Pp > +ICMP responses are not permitted unless they either match an > +existing request, or unless > +.Cm no state > +or > +.Cm keep state (sloppy) > +is specified. > +.Pp > .It Cm label Ar string > Adds a label to the rule, which can be used to identify the rule. > For instance, > @@ -2177,7 +2184,7 @@ States created by this rule are exported > .Xr pflow 4 > interface. > .It Cm sloppy > -Uses a sloppy TCP connection tracker that does not check sequence > +For TCP, uses a sloppy connection tracker that does not check sequence > numbers at all, which makes insertion and ICMP teardown attacks way > easier. > This is intended to be used in situations where one does not see all > @@ -2186,7 +2193,8 @@ It cannot be used with > .Cm modulate state > or > .Cm synproxy state . > -With this option ICMP replies can create states. > +For ICMP, this option allows states to be created from replies, > +not just requests. > .It Ar timeout seconds > Changes the > .Ar timeout
Re: [External] : Re: pf.conf(5) clarify ICMP sloppy state handling
Hello, I'm sorry I was too fast with commit. I've just committed what's been suggested by bluhm@: @@ -2186,6 +2186,7 @@ It cannot be used with .Cm modulate state or .Cm synproxy state . +With this option ICMP replies can create states. .It Ar timeout seconds Changes the .Ar timeout > This is helpful, but because it's so surprising that "pass proto icmp" > doesn't pass all icmp traffic, I think it would help to mention it where > "proto icmp" is described too. > > Also, the top of the text about "sloppy" just talks about the sloppy > TCP connection tracker, I think perhaps it would be better to lead > with something that suggests it has multiple functions for different > protocols? I don't object to any of your enhancements. reads OK sashan > > Index: man5/pf.conf.5 > === > RCS file: /cvs/src/share/man/man5/pf.conf.5,v > retrieving revision 1.594 > diff -u -p -r1.594 pf.conf.5 > --- man5/pf.conf.59 May 2022 20:29:23 - 1.594 > +++ man5/pf.conf.59 May 2022 21:05:48 - > @@ -594,6 +594,13 @@ or > .Pc > must match. > .Pp > +ICMP responses are not permitted unless they either match an > +existing request, or unless > +.Cm no state > +or > +.Cm keep state (sloppy) > +is specified. > +.Pp > .It Cm label Ar string > Adds a label to the rule, which can be used to identify the rule. > For instance, > @@ -2177,7 +2184,7 @@ States created by this rule are exported > .Xr pflow 4 > interface. > .It Cm sloppy > -Uses a sloppy TCP connection tracker that does not check sequence > +For TCP, uses a sloppy connection tracker that does not check sequence > numbers at all, which makes insertion and ICMP teardown attacks way > easier. > This is intended to be used in situations where one does not see all > @@ -2186,7 +2193,8 @@ It cannot be used with > .Cm modulate state > or > .Cm synproxy state . > -With this option ICMP replies can create states. > +For ICMP, this option allows states to be created from replies, > +not just requests. > .It Ar timeout seconds > Changes the > .Ar timeout >
Re: [External] : Re: pf.conf(5) clarify ICMP sloppy state handling
This is helpful, but because it's so surprising that "pass proto icmp" doesn't pass all icmp traffic, I think it would help to mention it where "proto icmp" is described too. Also, the top of the text about "sloppy" just talks about the sloppy TCP connection tracker, I think perhaps it would be better to lead with something that suggests it has multiple functions for different protocols? Index: man5/pf.conf.5 === RCS file: /cvs/src/share/man/man5/pf.conf.5,v retrieving revision 1.594 diff -u -p -r1.594 pf.conf.5 --- man5/pf.conf.5 9 May 2022 20:29:23 - 1.594 +++ man5/pf.conf.5 9 May 2022 21:05:48 - @@ -594,6 +594,13 @@ or .Pc must match. .Pp +ICMP responses are not permitted unless they either match an +existing request, or unless +.Cm no state +or +.Cm keep state (sloppy) +is specified. +.Pp .It Cm label Ar string Adds a label to the rule, which can be used to identify the rule. For instance, @@ -2177,7 +2184,7 @@ States created by this rule are exported .Xr pflow 4 interface. .It Cm sloppy -Uses a sloppy TCP connection tracker that does not check sequence +For TCP, uses a sloppy connection tracker that does not check sequence numbers at all, which makes insertion and ICMP teardown attacks way easier. This is intended to be used in situations where one does not see all @@ -2186,7 +2193,8 @@ It cannot be used with .Cm modulate state or .Cm synproxy state . -With this option ICMP replies can create states. +For ICMP, this option allows states to be created from replies, +not just requests. .It Ar timeout seconds Changes the .Ar timeout
Re: [External] : Re: pf.conf(5) clarify ICMP sloppy state handling
On Sun, May 08, 2022 at 09:58:47PM +0200, Alexandr Nedvedicky wrote: > Hello, > > On Sun, May 08, 2022 at 08:06:57PM +0200, Alexander Bluhm wrote: > > On Sun, May 08, 2022 at 06:37:57PM +0200, Alexandr Nedvedicky wrote: > > > this tiny update to pf.conf(5) has been prompted here [1] on > > > pf mailing list. By default only ICMP queries are allowed > > > to create state in pf(4). The sloppy option relaxes that > > > so also ICMP replies can create a state. I think this should > > > be also mentioned in pf.conf(5) > > > > > > OK to my suggestion below? > > > > I would make it a bit shorter. pf.conf(5) is very long already. > > > > With this option ICMP replies can create states. > > > > Does this describe everything? > > yes, it does. I Like it. Updated diff below. OK bluhm@ > 8<---8<---8<--8< > diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5 > index fe4b117994a..e4af2a37c5e 100644 > --- a/share/man/man5/pf.conf.5 > +++ b/share/man/man5/pf.conf.5 > @@ -2186,6 +2186,7 @@ It cannot be used with > .Cm modulate state > or > .Cm synproxy state . > +With this option ICMP replies can create states. > .It Ar timeout seconds > Changes the > .Ar timeout
Re: [External] : Re: pf.conf(5) clarify ICMP sloppy state handling
Hello, On Sun, May 08, 2022 at 08:06:57PM +0200, Alexander Bluhm wrote: > On Sun, May 08, 2022 at 06:37:57PM +0200, Alexandr Nedvedicky wrote: > > this tiny update to pf.conf(5) has been prompted here [1] on > > pf mailing list. By default only ICMP queries are allowed > > to create state in pf(4). The sloppy option relaxes that > > so also ICMP replies can create a state. I think this should > > be also mentioned in pf.conf(5) > > > > OK to my suggestion below? > > I would make it a bit shorter. pf.conf(5) is very long already. > > With this option ICMP replies can create states. > > Does this describe everything? yes, it does. I Like it. Updated diff below. thanks and regards sashan 8<---8<---8<--8< diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5 index fe4b117994a..e4af2a37c5e 100644 --- a/share/man/man5/pf.conf.5 +++ b/share/man/man5/pf.conf.5 @@ -2186,6 +2186,7 @@ It cannot be used with .Cm modulate state or .Cm synproxy state . +With this option ICMP replies can create states. .It Ar timeout seconds Changes the .Ar timeout
Re: pf.conf(5) clarify ICMP sloppy state handling
On Sun, May 08, 2022 at 06:37:57PM +0200, Alexandr Nedvedicky wrote: > this tiny update to pf.conf(5) has been prompted here [1] on > pf mailing list. By default only ICMP queries are allowed > to create state in pf(4). The sloppy option relaxes that > so also ICMP replies can create a state. I think this should > be also mentioned in pf.conf(5) > > OK to my suggestion below? I would make it a bit shorter. pf.conf(5) is very long already. With this option ICMP replies can create states. Does this describe everything? > 8<---8<---8<--8< > diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5 > index fe4b117994a..7389d231fe2 100644 > --- a/share/man/man5/pf.conf.5 > +++ b/share/man/man5/pf.conf.5 > @@ -2186,6 +2186,9 @@ It cannot be used with > .Cm modulate state > or > .Cm synproxy state . > +The option also relaxes handling of ICMP such that also ICMP replies > +are allowed to create state. > +By default ICMP queries only are allowed to create state. > .It Ar timeout seconds > Changes the > .Ar timeout
pf.conf(5) clarify ICMP sloppy state handling
Hello, this tiny update to pf.conf(5) has been prompted here [1] on pf mailing list. By default only ICMP queries are allowed to create state in pf(4). The sloppy option relaxes that so also ICMP replies can create a state. I think this should be also mentioned in pf.conf(5) OK to my suggestion below? thanks and regards sashan [1] https://marc.info/?l=openbsd-pf=165160086423472=2 8<---8<---8<--8< diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5 index fe4b117994a..7389d231fe2 100644 --- a/share/man/man5/pf.conf.5 +++ b/share/man/man5/pf.conf.5 @@ -2186,6 +2186,9 @@ It cannot be used with .Cm modulate state or .Cm synproxy state . +The option also relaxes handling of ICMP such that also ICMP replies +are allowed to create state. +By default ICMP queries only are allowed to create state. .It Ar timeout seconds Changes the .Ar timeout
