Re: [patch] cleaner checksum modification for pf

[Richard Procter]

On 29/09/2015, at 11:14 PM, Alexandr Nedvedicky wrote:

> Hello,
> 
> I've tried Richard's patch on sparc. I took a brief look at its source code.
> It's essentially what PF is doing on Solaris.

Thanks for doing that.

Just for the record, I look at PF's patched checksum handling as follows.

Before Henning's work, the checksum computation was spread (pseudo-header)
over different parts of the code and PF would face partially checksummed
packets. But there was no state indicating what the checksum covered, so when
PF altered the packet it could not determine whether or not it should also
modify the checksum, resulting in the rdr-to-localhost issue.

This problem goes away now that checksums either cover what they ought or are
flagged for computation. Now when PF alters a packet it need only modify the
checksum as appropriate for the protocol. And although that's unnecessary
when the checksum is as-yet uncomputed, it does no harm.

> The approach we take on Solaris is as follows:
> [...]

> The things are getting pretty wild in 2b, when PF is doing PBR (policy based
> routing) on outbound packets. Consider situation when IP stack routes packet
> via NIC, which is able to calculate chksum in HW.  IP stack sets flags and
> fields and passes packet to PF. PF changes interface, where packet is bound 
> to,
> to NIC, which is not able to calculate checksum, so the HW-cksum flags set by
> IP stack are no longer valid. In this case we always revert to calculation
> in SW.

As I see it in OpenBSD (maybe Solaris differs) the M_*_CSUM_OUT flags decouple
checksum policy ("whether to") from checksum mechanism ("how to"). So I don't
think of these as HW-cksum flags but as indicating that a needed checksum is
as-yet uncomputed. As that's not specific to an interface, they cannot become 
invalid if the output interface changes.

What they allow is the choice of checksumming method, whether by software or
by harware (or by small furry animals trained in ones-complement addition),
to be left until late in the output path when the capabilities of the output
interface are known.

> I currently have small suggestion to improve Richard's patch. The macro in
> PF_ALGNMNT() in pfvar.h uses modulo:
> 
>#define PF_HI (true)
>#define PF_LO (!PF_HI)
>#define PF_ALGNMNT(off) (((off) % 2) == 0 ? PF_HI : PF_LO)
> 
> I think we can get away with simple and operation (& 1), which will be faster
> than % on many platforms.
> 
>#define PF_ALGNMNT(off) (((off) & 1) == 0 ? PF_HI : PF_LO)

I've no strong feelings either way here. I note that gcc emits the same code
in either case on ppc and i386. 

best, 
Richard. 

Re: [patch] cleaner checksum modification for pf

[Alexandr Nedvedicky]

Hello,

I've tried Richard's patch on sparc. I took a brief look at its source code.
It's essentially what PF is doing on Solaris.

The checksum handling in PF on systems with HW assisted checksums is getting
tricky for local (out)bound packets. The approach we take on Solaris is as
follows:

- for inbound packets PF always trusts HW, if HW says chksum is
  correct, then checksum is correct. if HW is not able to verify
  checksum (HW checksum verification is off), PF falls back to SW
  verification (1)

- PF does not check (verify) checksum for outbound packets, outbound
  packet is either

- forwarded, so checksum has been verified in inbound side (2a)

- local outbound, then checksum is either valid or to be
  calculated by HW (2b)

The things are getting pretty wild in 2b, when PF is doing PBR (policy based
routing) on outbound packets. Consider situation when IP stack routes packet
via NIC, which is able to calculate chksum in HW.  IP stack sets flags and
fields and passes packet to PF. PF changes interface, where packet is bound to,
to NIC, which is not able to calculate checksum, so the HW-cksum flags set by
IP stack are no longer valid. In this case we always revert to calculation
in SW.

I have not looked at current checksum handling at PF on OpenBSD, so can't tell
exactly what's going on there. I feel PF does not bother too much with updating
the checksum, when it changes the packet. It seems to me the
in_proto_cksum_out() gets called as soon as outbound packet gets inspected by
pf_test() to calculate/fix checksums. It looks like in_proto_cksum_out() has to
recalculate checksum in SW for entire packet, when underlying HW does not offer
checksum offload. Is that right? Or am I missing some piece?

On the other hand Richard's patch adjusts checksums by delta caused by update.
The adjustment is of few operations (add/and/not) on very small chunk of
memory. The price should be same we pay for extra logic to decide if
HW will compute chksum for us or we have to do it on our own. However we will
save plenty of cycles, when we would have to revert to SW.


I currently have small suggestion to improve Richard's patch. The macro in
PF_ALGNMNT() in pfvar.h uses modulo:

#define PF_HI (true)
#define PF_LO (!PF_HI)
#define PF_ALGNMNT(off) (((off) % 2) == 0 ? PF_HI : PF_LO)

I think we can get away with simple and operation (& 1), which will be faster
than % on many platforms.

#define PF_ALGNMNT(off) (((off) & 1) == 0 ? PF_HI : PF_LO)

regards
sasha

Re: [patch] cleaner checksum modification for pf

[Henning Brauer]

* Alexandr Nedvedicky  [2015-09-29 12:17]:
> I have not looked at current checksum handling at PF on OpenBSD, so can't tell
> exactly what's going on there. I feel PF does not bother too much with 
> updating
> the checksum, when it changes the packet. It seems to me the
> in_proto_cksum_out() gets called as soon as outbound packet gets inspected by
> pf_test() to calculate/fix checksums. It looks like in_proto_cksum_out() has 
> to
> recalculate checksum in SW for entire packet, when underlying HW does not 
> offer
> checksum offload. Is that right? Or am I missing some piece?

Basically. Packets that are modified by pf or are locally originated
get "needs checksumming" flags (there are a few actually).
in_proto_cksum_out basically emulates the hw cksum engine if we don't
have one. I consider having one the norm these days.

-- 
Henning Brauer, h...@bsws.de, henn...@openbsd.org
BS Web Services GmbH, http://bsws.de, Full-Service ISP
Secure Hosting, Mail and DNS. Virtual & Dedicated Servers, Root to Fully Managed
Henning Brauer Consulting, http://henningbrauer.com/

Re: [patch] cleaner checksum modification for pf

[Richard Procter]

On 25/09/2015, at 9:33 PM, Stuart Henderson wrote:
> 
> [snip comment; I completely agree]
> 
>> Another (home) router I administer was seeing IIRC five [bad TCP checksums] a
>> day on average over 42 days, something we expect of a globe-spanning 
>> internetwork.
>> Passing one of these damaged segments to the user sufficies to break MACs 
>> and drop
>> secure connections.
> 
> While I do generally support this diff as long as it doesn't have
> a big negative impact on performance, the implication of mentioning
> this is that these are packets which PF would pass on to other
> hosts with a re-calculated checksum if the packets were modified
> (nat, scrub etc). But that's not the case because they would be
> checked on input to PF so wouldn't make it that far.

If I implied PF would mask these damaged packets I didn't mean to.  As you
say, PF would drop them when it tried to alter them (nat, scrub, etc).

Rather, the stats show that router faults can't be dismissed as irrelevant in
practice. And just one passed to the user in a secure stream will have a
significant impact, independently of its payload, by breaking the connection.

As to the patch, I have edited it for length and coherence but will hold off 
posting it for a week or two, as I hear there's much work going on elsewhere 
in the network stack. If anyone would like a copy in the meantime please 
contact me privately.

I don't expect a big performance hit, if any, as it works the same way as 5.3 
did
plus or minus a few function calls. Nor could I measure a difference netcatting
a largish file over 100BaseT via my Alix 2d2 running PF doing nat, scrub, etc. 
I'd appreciate more data or reports, positive or negative, though.

best, 
Richard. 
 
P.S. I earlier recommended EWD1023 from memory, that should have been EWD1036. 

 

Re: [patch] cleaner checksum modification for pf

[Stuart Henderson]

On 2015/09/25 13:05, Richard Procter wrote:
> > Well we've been thru it more than once; the argument presented here
> > was that modifying the cksum instead of verify + recalc is better as
> > it wouldn't hide cksum mismatches if the cksum engines on the NICs we
> > offload to misbehave. After many years with the verify + recalc
> > approach I think it is pretty safe to say that this is of no
> > concern...
> 
> I'm sorry, that's not what I'm saying. Even supposing perfection of every
> offload engine, offload engines have no monopoly on faults; regenerated
> checksums also mask bus, stack and memory faults in pf-altered packets.

This is definitely not just a theoretical problem.

It's one thing having corrupted packets delivered to the router's own
TCP/UDP stack (or to another host but in a state where the corruption
can be detected), but it's something totally different to do this for
forwarded packets without providing the endpoint a way to detect it
before they reach the application layer. SSH/TLS typical response is
to terminate the whole session.

As more and more traffic moves to secure connections (I'm seeing 40%
of requests at my web proxies using CONNECT vs GET/HEAD, and the true
count will be higher because it doesn't count keepalives as separate
hits), errors that previously weren't really noticeable (some minor
corruption in a web page or glitch in an image) now cause sessions
to fail.

> Another (home) router I administer was seeing IIRC five a day on average over
> 42 days, something we expect of a globe-spanning internetwork. Passing one of
> these damaged segments to the user sufficies to break MACs and drop secure
> connections.

While I do generally support this diff as long as it doesn't have
a big negative impact on performance, the implication of mentioning
this is that these are packets which PF would pass on to other
hosts with a re-calculated checksum if the packets were modified
(nat, scrub etc). But that's not the case because they would be
checked on input to PF so wouldn't make it that far.

Re: [patch] cleaner checksum modification for pf

[Richard Procter]

On 14/09/2015, at 11:51 PM, Henning Brauer wrote:
> * Martin Pieuchot  [2015-09-11 13:54]:
>> On 11/09/15(Fri) 13:28, Henning Brauer wrote:
>>> Ryan pointed me to this diff and we briefly discussed it; we remain
>>> convinced that the in-tree approach is better than this.
>> Could you elaborate why?
> 
> [ elaboration, quoted further down ] 
> 
> And given that, the approach that has less and simpler code and makes
> better use of offloading wins.

Well, I agree that less and simpler code is better, all else being equal. 
In fact if you'd like my opinion, the in-tree approach actually places a
greater burden of complexity on the programmer. The patch needs one line 
to alter a value

rewrite += pf_change_16(pd, >hdr.icmp->icmp_id, icmpid);

; the in-tree code requires all packet alterations to occur between 
two calls, analogous to the way locking must enclose a critical section:

if (pd->csum_status == PF_CSUM_UNKNOWN)
   pf_check_proto_cksum(pd, pd->off, pd->tot_len - pd->off,
   pd->proto, pd->af); 
   [...]
   header->field = new_value; 
   [...]
pf_cksum(pd, pd->m);

This is the stuff mild headaches are made of. Especially as 1) pf_cksum()
usually occurs elsewhere and as 2) the idiom tempts programmers to rely on
existing pf_check_proto_cksum() calls. This last occurs three places in the
existing code. I presume these are correct but it is much easier to avoid the
question by replacing them with the one-liner.

The new interface also provides a place to put all the altered-value guards,
replacing, e.g.:


if (icmpid != pd->hdr.icmp->icmp_id) {
if (pd->csum_status == PF_CSUM_UNKNOWN)
pf_check_proto_cksum(pd, pd->off,
pd->tot_len - pd->off, pd->proto,
pd->af);
pd->hdr.icmp->icmp_id = icmpid;
rewrite = 1;
}

with:

rewrite += pf_change_16(pd, >hdr.icmp->icmp_id, icmpid);

This saves code. Here are some numbers. First, non-comment[0] 
lines of code for all affected files:

pf.c:1.944   pfvar.h:1.420   pf_norm.c:1.182
current 5724 16131030 
patched 5682 16141028 
 
 -88   +1  -2

(amd64) pf.o  pf_norm.o
current 123520 bytes  30816 bytes
patched 123584 bytes  30912 bytes
- -
  +64   +96

(i386)  pf.o  pf_norm.o 
current 95904 bytes   21096 bytes
patched 94956 bytes   21156 bytes
- -
 -948   -60

The patch's numbers include the new pf_change_* interface and the checksum 
modification code. So the proposed interface pays its way. Ok, let's move 
on to handling checksums:

> Well we've been thru it more than once; the argument presented here
> was that modifying the cksum instead of verify + recalc is better as
> it wouldn't hide cksum mismatches if the cksum engines on the NICs we
> offload to misbehave. After many years with the verify + recalc
> approach I think it is pretty safe to say that this is of no
> concern...

I'm sorry, that's not what I'm saying. Even supposing perfection of every
offload engine, offload engines have no monopoly on faults; regenerated
checksums also mask bus, stack and memory faults in pf-altered packets.

Regeneration always hides faults in /something/, unfortunately. 
pf_check_proto_cksum() takes care to mitigate that by preventing it from
hiding faults in the routers a packet passes through --- except for the current
one, which it can't.

That is, either router faults are a concern or they're not. pf can't
have it both ways, concerned to detect faults in every router other than 
the one on which it is running. The proposed patch fixes that.

And, as I wrote in a previous post, we know that routers corrupt data:

>> Right now my home firewall shows 30 TCP segments dropped for bad checksums.
>> As checks at least as strong are used by every sane link-layer this
>> virtually implies the dropped packets suffered router or end-point faults.

Another (home) router I administer was seeing IIRC five a day on average over
42 days, something we expect of a globe-spanning internetwork. Passing one of
these damaged segments to the user sufficies to break MACs and drop secure
connections.

Nonetheless, one might still argue that the reduced reliability is acceptable.
If there were some compelling upside, perhaps, maybe it might be, but I've
seen no evidence of performance benefits and the proposed patch addresses the
code complexity from which the more reliable method suffered.


Lastly, I don't want to sound like I'm trying to beat up on your checksum
work. I know very well how much effort is involved and it's very much
appreciated; you've cleaned up the code considerably. The 

Re: [patch] cleaner checksum modification for pf

[Henning Brauer]

* Martin Pieuchot  [2015-09-11 13:54]:
> On 11/09/15(Fri) 13:28, Henning Brauer wrote:
> > Ryan pointed me to this diff and we briefly discussed it; we remain
> > convinced that the in-tree approach is better than this.
> Could you elaborate why?

Well we've been thru it more than once; the argument presented here
was that modifying the cksum instead of verify + recalc is better as
it wouldn't hide cksum mismatches if the cksum engines on the NICs we
offload to misbehave. After many years with the verify + recalc
approach I think it is pretty safe to say that this is of no
concern...

And given that, the approach that has less and simpler code and makes
better use of offloading wins.

there's a more elaborate discussion with exactly the same people in
teh archives from around the time the cksum rewrite hit the tree, with
the same conclusion.

-- 
Henning Brauer, h...@bsws.de, henn...@openbsd.org
BS Web Services GmbH, http://bsws.de, Full-Service ISP
Secure Hosting, Mail and DNS. Virtual & Dedicated Servers, Root to Fully Managed
Henning Brauer Consulting, http://henningbrauer.com/

Re: [patch] cleaner checksum modification for pf

[Henning Brauer]

Ryan pointed me to this diff and we briefly discussed it; we remain
convinced that the in-tree approach is better than this.

-- 
Henning Brauer, h...@bsws.de, henn...@openbsd.org
BS Web Services GmbH, http://bsws.de, Full-Service ISP
Secure Hosting, Mail and DNS. Virtual & Dedicated Servers, Root to Fully Managed
Henning Brauer Consulting, http://henningbrauer.com/

Re: [patch] cleaner checksum modification for pf

[Martin Pieuchot]

On 11/09/15(Fri) 13:28, Henning Brauer wrote:
> Ryan pointed me to this diff and we briefly discussed it; we remain
> convinced that the in-tree approach is better than this.

Could you elaborate why?

Re: [patch] cleaner checksum modification for pf

[Richard Procter]

Hi, 

On 16/06/2015, at 1:09 PM, Richard Procter wrote:
 - I was unable to test af-to, which does a lot of packet fiddling.
 I've now tested this without obvious issue. 

I neglected checksum regeneration within icmp af-to, which masked a 
couple of icmp af-to errata in my last patch.

I've re-included the entire patch refreshed against HEAD below. 
(Thanks to whoever mentioned 'quilt' the other day!) 

Two further diffs then 0) fix the errata and 1) reintroduce checksum 
modification for icmp af-to. 

I see no remaining regeneration cases in PF. 

Note: Checksumless IPv4 UDP packets, illegal under IPv6, are now 
checksummed on af-to IPv6. This improves on HEAD. 

Note: pf_translate_af() flushes pd-pcksum to mbuf by flushing the  
entire transport header. Simple but possibly suboptimal; you may
wish to do it another way.  

testing: 

$4 IPv4 - $6 IPv6 
TCP:ssh $4 -- af-to $6 [good]
ICMPv4-v6: ping $4 -- af-to $6 [good]
UDP, ICMPv6-v4 quoting UDP: traceroute $4 -- af-to $6 [good] 
Checksumless UDP:   traceroute -x $4 -- af-to $6 [good] 

$6 IPv6 - $4 IPv4
TCP:ssh $6 -- af-to $4 [good]
ICMPv6: ping6 $6 -- af-to $4 [good]
UDP, ICMPv4-v6 quoting UDP: traceroute6 $6 -- af-to $4 [good]

best, 
Richard. 

To apply: 
# cd /src/sys/net
# cat - | patch 

--- Rename pf_change_a() - pf_change_32_unaligned() to 
prepare for address-specific pf_change_a()

Index: net/pf.c
===
--- net.orig/pf.c
+++ net/pf.c
@@ -1664,7 +1664,7 @@ pf_change_ap(struct pf_pdesc *pd, struct
 
 /* Changes a u_int32_t.  Uses a void * so there are no align restrictions */
 void
-pf_change_a(struct pf_pdesc *pd, void *a, u_int32_t an)
+pf_change_32_unaligned(struct pf_pdesc *pd, void *a, u_int32_t an)
 {
if (pd-csum_status == PF_CSUM_UNKNOWN)
pf_check_proto_cksum(pd, pd-off, pd-tot_len - pd-off,
@@ -2273,10 +2273,10 @@ pf_modulate_sack(struct pf_pdesc *pd, st
for (i = 2; i + TCPOLEN_SACK = olen;
i += TCPOLEN_SACK) {
memcpy(sack, opt[i], sizeof(sack));
-   pf_change_a(pd, sack.start,
+   pf_change_32_unaligned(pd, sack.start,
htonl(ntohl(sack.start) -
dst-seqdiff));
-   pf_change_a(pd, sack.end,
+   pf_change_32_unaligned(pd, sack.end,
htonl(ntohl(sack.end) -
dst-seqdiff));
memcpy(opt[i], sack, sizeof(sack));
@@ -3484,7 +3484,7 @@ pf_create_state(struct pf_pdesc *pd, str
if ((s-src.seqdiff = pf_tcp_iss(pd) - s-src.seqlo) ==
0)
s-src.seqdiff = 1;
-   pf_change_a(pd, th-th_seq,
+   pf_change_32_unaligned(pd, th-th_seq,
htonl(s-src.seqlo + s-src.seqdiff));
*rewrite = 1;
} else
@@ -3680,12 +3680,12 @@ pf_translate(struct pf_pdesc *pd, struct
 #endif /* INET6 */
} else {
if (PF_ANEQ(saddr, pd-src, pd-af)) {
-   pf_change_a(pd, pd-src-v4.s_addr,
+   pf_change_32_unaligned(pd, pd-src-v4.s_addr,
saddr-v4.s_addr);
rewrite = 1;
}
if (PF_ANEQ(daddr, pd-dst, pd-af)) {
-   pf_change_a(pd, pd-dst-v4.s_addr,
+   pf_change_32_unaligned(pd, pd-dst-v4.s_addr,
daddr-v4.s_addr);
rewrite = 1;
}
@@ -3745,12 +3745,12 @@ pf_translate(struct pf_pdesc *pd, struct
switch (pd-af) {
case AF_INET:
if (!afto  PF_ANEQ(saddr, pd-src, pd-af)) {
-   pf_change_a(pd, pd-src-v4.s_addr,
+   pf_change_32_unaligned(pd, pd-src-v4.s_addr,
saddr-v4.s_addr);
rewrite = 1;
}
if (!afto  PF_ANEQ(daddr, pd-dst, pd-af)) {
-   pf_change_a(pd, pd-dst-v4.s_addr,
+   pf_change_32_unaligned(pd, pd-dst-v4.s_addr,
daddr-v4.s_addr);
rewrite = 1;
}
@@ -3813,8 +3813,8 @@ pf_tcp_track_full(struct pf_pdesc *pd, s
while ((src-seqdiff = arc4random() - 

Re: [patch] cleaner checksum modification for pf

[Richard Procter]

On 16/06/2015, at 1:09 PM, Richard Procter wrote:

 - I was unable to test af-to, which does a lot of packet fiddling.

I've now tested this without obvious issue. Note: a couple of 
one-line changes in icmp af-translation remain untested. Details 
attached.

 - I haven't tested modification of unaligned TCP options,
   for SACK and timestamp. 

This tested without issue, too.

Also, some points raised in off-list comment suggested I was unclear in my
patch post, which I'll address here.

- Note this diff does not revert all of Henning's checksum work, just returns
to checksum modification for pf-altered packets as in 5.3[0] but without the 
ugly nested pf_checksum_fixup() calls and without (I expect) loss of 
efficiency. I've aimed for the minimal necessary changes only.

And the patch is made much easier now that pseudo-header checksums are
calculated late in the output path, as pf now never sees a partial checksum.
Worst-case, pf does some unnecessary arithmetic when altering locally generated
packets, but no more than the original 5.3 code did, as the 5.3 modification
code didn't distinguish between local and forwarded packets either.

- The first patch renames pf_change_a() to pf_change_32_unaligned() as it was
not being used for addresses alone, and I needed to move it aside for an
address-specific pf_change_a().

- Some commented that, although the patch re-enables end-to-end verification of
transport payloads traversing pf, it is unimportant as we now have far stronger
end-to-end checks in ssh and TLS.

These protocols, however, assume a reliable transport[1]: they terminate the
connection on a failed MAC as evidence of malicious tampering[*]. If they
didn't they would need to reimplement TCP's retransmission strategies, as 
the Internet can be expected to damage packets as a matter of course. So TCP 
checksums continue to play an important role for secure streams built atop TCP
and this diff bears upon them, too. 


As usual, comment, questions, or criticism is welcome.

best, 
Richard.
 
[0] 5.3, not 5.4 as I stated
[1] for those interested in the fine print: 
rfc4253 (2006) SSH p3
rfc5246 (2008) TLSv1.2 p3
[*] IIRC someone last year reported mysterious ssh disconnects due to corrupted
MACs which Stuart Henderson pointed out could have been due to a flakey NAT
router.

Test details:

- unaligned SACK, timestamp options
- tested on i386, and I expect no issues for architectures with stronger 
  alignment constraints but aiming to test this in the weekend on socppc 
  for kicks. 

- pf_translate_icmp_af
These two remain untested due to my lack of test nodes:
nextmtu for ICMP6_PACKET_TOO_BIG 
pptr for ICMP_PARAMPROB
but low risk as these are one line substitutions with tested primitives, 
and nextmtu is always changed.

- translate quoted address family (af-to)
via pf_change_icmp_af pf_change_ap

Testing af-to 4 - 6 
pass in on ral0 inet from any to 192.168.2.2 af-to inet6 from 
  fec0:0:0:2::1 to fec0:0:0:2::2

   af-to TCP
- inet4 telnet to inet6 netcat host [TESTED]
   af-to ICMP
- unassociated with connection
 - src ICMP4 ping [TESTED]
- associated with connection
 - traceroute elicits ICMP6 port unreachable
   from dest; translated result inspected with wireshark [TESTED]

Testing af-to 6 - 4 
pass in on vr0 inet6 from any to 64:ff9b::/96 af-to inet from 192.168.2.3

   af-to TCP
- inet6 telnet to inet4 netcat host [TESTED]
   af-to ICMP
- unassociated with connection
 - src ICMP6 ping [TESTED]
- associated with connection
 - traceroute6 elicits ICMP4 port unreachable
   from dest; translated result inspected with wireshark. [TESTED]

Re: [patch] cleaner checksum modification for pf

[Mike Belopuhov]

On 16 June 2015 at 03:09, Richard Procter richard.n.proc...@gmail.com wrote:
  - I was unable to test af-to, which does a lot of packet fiddling.
I've never used it before and was unable to get it working on a
generic kernel. I figure I'm just missing something. I used the line

pass out on vr0 inet af-to inet6 from fec0:0:0:2::1 to fec0:0:0:2::2

but although inet4 tcp connection attempts were translated
to fec0:0:0:2::2, its SYN replies received RST from the
router, fec0:0:02:::1.


You didn't read the pf.conf manual page carefully:

   [...] Because address family translation
   overrides the routing table, it's only possible to use af-to on
   inbound rules, and a source address for the resulting
   translation must always be specified.

And all example rules after that use pass in.

Re: [patch] cleaner checksum modification for pf

[Richard Procter]

On 17/06/2015, at 4:26 AM, Mike Belopuhov wrote:
 
 You didn't read the pf.conf manual page carefully:
 
   [...] Because address family translation
   overrides the routing table, it's only possible to use af-to on
   inbound rules, and a source address for the resulting
   translation must always be specified.
 
 And all example rules after that use pass in.

Thanks for the pointer, I'll have another go at testing this then. 

best, 
Richard. 

[patch] cleaner checksum modification for pf

[Richard Procter]

Hi,

These patches against HEAD re-instate the pf algorithm of OpenBSD 5.4
for preserving payload checksums end-to-end but rewritten without the
ugly and error-prone (but speedy!) nested pf_cksum_fixup calls.

I have been running this code on a small Alix (i386) IPv4 gateway for a
month with no obvious issues. To test as many of the affected features
as possible, its pf.conf included:

  match scrub (random-id) 
  match on egress scrub (max-mss 1440, reassemble tcp)
  match out on egress from !egress:network nat-to egress:0 
  pass out on egress inet proto tcp modulate state 
  pass in inet proto tcp from any to egress port ... rdr-to ...

I've tried to avoid significant performance impact on modern hardware,
and don't expect any, but have not tested this. I've aimed for
simplicity in the first instance and there is scope for optimisation if
necessary.

I've attached my test notes below, covering every change. Note:

 - I was unable to test af-to, which does a lot of packet fiddling.
   I've never used it before and was unable to get it working on a 
   generic kernel. I figure I'm just missing something. I used the line

   pass out on vr0 inet af-to inet6 from fec0:0:0:2::1 to fec0:0:0:2::2

   but although inet4 tcp connection attempts were translated 
   to fec0:0:0:2::2, its SYN replies received RST from the 
   router, fec0:0:02:::1.

 - My inet6 testing was limited to two nodes connected via the alix
   router.

 - I've assumed that rdr-to is good if nat-to tests ok as the code
   paths look virtually identical.

 - I haven't tested modification of unaligned TCP options,
   for SACK and timestamp, but have tested the unaligned paths of the
   change primitives.

 - the patch includes a small fix for pf_pdesc_setup to setup the
   pdesc protocol checksum for ICMPv6

The patch is in three commits to ease review, to be applied in order:

 0. rename pf_change_a - pf_change_32_unaligned to better reflect its
 use 
 1. reinstate pf_cksum_fixup sans nesting 
 2. avoid unnecessary calls to pf_change_32_unaligned

...but if another format is easier, let me know.

This patch should be examined closely --- it's been a while since I've
worked at this level and I've never worked on OpenBSD code.

I'm keen to hear comments, questions or criticisms.

best, 
Richard.  

 - [G] means that errors here are expected 
to have been exposed in the course of running on the gateway. 

 - [O] means these changes involve primitives tested elsewhere.

pf_tcp_track_full, pf_create_state [G]
- modulate sequence number  (modulate state)
 pf_change_a, old unaligned copy - pf_change_32
 new pf_change_a

pf_translate 
- translate address and port, if any 
AF_INET [G]
AF_INET6 [TESTED between two addresses UDP, TCP, ICMP6]
 
- translate ICMP icmp_id for ICMP_ECHO [TESTED]
pf_change_16
- translate ICMP6 icmp6_id for ICMP6_ECHO [TESTED]
pf_change_16
- translate address for non TCP,UDP,ICMP,ICMP6 protocol [TESTED]

 pf_change_a, old unaligned copy - pf_change_32
new pf_change_a

pf_modulate_sack 
- modulate SACK sequence numbers [G]
pf_change_32_unaligned [need to test unaligned options]
minor refactoring to support fixup

pf_test_state_icmp
- translate ICMP unrelated to another connection, e.g ECHO [TESTED]
- translate icmp_id
- translate address
new pf_change_a 

- ICMP error related to another connection, e.g dest unreachable
- translate address, quoted address, port (nat, rdr-to) 
via pf_change_icmp, see this 

- translate quoted address family (af-to) [*]
via pf_change_icmp_af
pf_change_ap

TCP 
   - demodulate quoted TCP sequence number [TESTED]
pf_change_a, old unaligned - pf_change_32
UDP [TESTED]
   - zero quoted UDP checksum
pf_change_16

pf_change_icmp [O]
- change quoted protocol port and address, if any 
- change outer ip address 
pf_cksum_fixup_a
pf_cksum_fixup
pf_change_a   

pf_change_icmp_af
- replace quoted IPv4 / IPv6 headers with converse AF. 
pf_cksum_cover / uncover

pf_translate_icmp_af [O] 
- to AF_INET
- sets icmp type, code
  nextmtu for ICMP6_PACKET_TOO_BIG
  pptr for ICMP_PARAMPROB
- to AF_INET6
- sets icmp type, code
  nextmtu for ICMP_UNREACH_NEEDFRAG
  ptr for ICMP_PARAMPROB

pf_change_8 [TESTED when testing alternate