pf scrub modifying IP header without fixing checksum

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

pf scrub modifying IP header without fixing checksum

Daniel Hartmeier
See http://www.freebsd.org/cgi/query-pr.cgi?pr=93849 for background.

Max' patch is correct, but there are other modifications which also
require checksum fixing, unless I'm mistaken.

I suggest the patch below. Proof-reading and testing welcome :)

Daniel


Index: pf_norm.c
===================================================================
RCS file: /cvs/src/sys/net/pf_norm.c,v
retrieving revision 1.104
diff -u -r1.104 pf_norm.c
--- pf_norm.c 18 Jan 2006 22:03:21 -0000 1.104
+++ pf_norm.c 8 Mar 2006 16:27:57 -0000
@@ -864,8 +864,12 @@
  goto drop;
 
  /* Clear IP_DF if the rule uses the no-df option */
- if (r->rule_flag & PFRULE_NODF)
+ if (r->rule_flag & PFRULE_NODF && h->ip_off & htons(IP_DF)) {
+ u_int16_t ip_off = h->ip_off;
+
  h->ip_off &= htons(~IP_DF);
+ h->ip_sum = pf_cksum_fixup(h->ip_sum, ip_off, h->ip_off, 0);
+ }
 
  /* We will need other tests here */
  if (!fragoff && !mff)
@@ -968,11 +972,20 @@
 
  no_fragment:
  /* At this point, only IP_DF is allowed in ip_off */
- h->ip_off &= htons(IP_DF);
+ if (h->ip_off & ~htons(IP_DF)) {
+ u_int16_t ip_off = h->ip_off;
+
+ h->ip_off &= htons(IP_DF);
+ h->ip_sum = pf_cksum_fixup(h->ip_sum, ip_off, h->ip_off, 0);
+ }
 
  /* Enforce a minimum ttl, may cause endless packet loops */
- if (r->min_ttl && h->ip_ttl < r->min_ttl)
+ if (r->min_ttl && h->ip_ttl < r->min_ttl) {
+ u_int16_t ip_ttl = h->ip_ttl;
+
  h->ip_ttl = r->min_ttl;
+ h->ip_sum = pf_cksum_fixup(h->ip_sum, ip_ttl, h->ip_ttl, 0);
+ }
 
  if (r->rule_flag & PFRULE_RANDOMID) {
  u_int16_t ip_id = h->ip_id;
@@ -987,8 +1000,12 @@
 
  fragment_pass:
  /* Enforce a minimum ttl, may cause endless packet loops */
- if (r->min_ttl && h->ip_ttl < r->min_ttl)
+ if (r->min_ttl && h->ip_ttl < r->min_ttl) {
+ u_int16_t ip_ttl = h->ip_ttl;
+
  h->ip_ttl = r->min_ttl;
+ h->ip_sum = pf_cksum_fixup(h->ip_sum, ip_ttl, h->ip_ttl, 0);
+ }
  if ((r->rule_flag & (PFRULE_FRAGCROP|PFRULE_FRAGDROP)) == 0)
  pd->flags |= PFDESC_IP_REAS;
  return (PF_PASS);

Reply | Threaded
Open this post in threaded view
|

Re: pf scrub modifying IP header without fixing checksum

Jon Hart-2
On Wed, Mar 08, 2006 at 05:33:28PM +0100, Daniel Hartmeier wrote:
> See http://www.freebsd.org/cgi/query-pr.cgi?pr=93849 for background.
>
> Max' patch is correct, but there are other modifications which also
> require checksum fixing, unless I'm mistaken.
>
> I suggest the patch below. Proof-reading and testing welcome :)

Any reason to believe this problem shows itself on non-bridged setups?
I've had an intermittent problem on a 3.8-current box from october where
packets are dropped because of invalid checksums.  Up until now, I've
suspected faulty cards, cables or setups on the sending machines (Debian
with 2.4 kernel).

My setup scrubs in and out, hence my question on whether this would work
in setups other than bridged.

-jon

Reply | Threaded
Open this post in threaded view
|

Re: pf scrub modifying IP header without fixing checksum

Daniel Hartmeier
On Wed, Mar 08, 2006 at 11:52:42AM -0500, Jon Hart wrote:

> Any reason to believe this problem shows itself on non-bridged setups?
> I've had an intermittent problem on a 3.8-current box from october where
> packets are dropped because of invalid checksums.  Up until now, I've
> suspected faulty cards, cables or setups on the sending machines (Debian
> with 2.4 kernel).
>
> My setup scrubs in and out, hence my question on whether this would work
> in setups other than bridged.

IP forwarding generally calls ip_output() which recalculates the
checksum, but there might be cases of hardware checksumming and/or
pf routing where that is not done.

The problem only affects the no-df and min-ttl options of scrub. If you
have confirmed broken IP checksums (at the recipient) with either of
those two options, and correct checksums with the option(s) disabled,
then, yes, the patch should fix it.

Daniel