Adam,

I stand corrected. Of course I have to comment the 'if' statement as well ...
"Couldn't see the tree inside the forest"  ...

Tnx

Marcel


On Wed, Jul 30, 2008 at 4:51 PM, Marcel Wiget <mwiget at gmail dot com> wrote:
> actually I could have commented out the whole code block that does the
> additional checking. With the diff, I simply stopped the code from
> bailing out. Here's the complete code that got introduced at some
> point which I think needs modification or complete removal (from
> function nat_newrdr() in ip_nat.c):
>
>       /*
>         * Check to see if this redirect mapping already exists and if
>         * it does, return "failure" (allowing it to be created will just
>         * cause one or both of these "connections" to stop working.)
>         */
>
>        inb.s_addr = htonl(in.s_addr);
>        sp = fin->fin_data[0];
>        dp = fin->fin_data[1];
>        fin->fin_data[1] = fin->fin_data[0];
>        fin->fin_data[0] = ntohs(nport);
>        natl = nat_outlookup(fin, flags & ~(SI_WILDP|NAT_SEARCH),
>                             (u_int)fin->fin_p, inb, fin->fin_src);
>        fin->fin_data[0] = sp;
>        fin->fin_data[1] = dp;
>        if (natl != NULL)
> /*              return -1;      marcel skip for now */
>
>
> On Wed, Jul 30, 2008 at 4:42 PM, Adam Gibson <agibson at ptm dot com> wrote:
>> I don't know about the kernel specifics or freebsd for that matter so I am useless in that
respect but from a coding perspective I think you meant to comment out the if condition too didn't
you?
>>
>> The way you commented out that one line... the nat->nat_inip.s_addr... line becomes part of the
if condition which I do not think you intended to do.  So the assigning of inip.s_addr only happens
now if natl != NULL.  Hopefully the code further down does not expect that to be populated with
htonl(in.s_addr).
>>
>> You essentially changed it to:
>>
>>        fin->fin_data[1] = dp;
>>        if (natl != NULL)
>>        {
>>          nat->nat_inip.s_addr = htonl(in.s_addr);
>>        }
>>
>>
>> -----Original Message-----
>> From: Marcel Wiget [mailto:mwiget at gmail dot com]
>> Sent: Wednesday, July 30, 2008 9:49 AM
>> To: m0n0wall dash dev at lists dot m0n0 dot ch
>> Subject: [m0n0wall-dev] ipfilter and SIP invite problem and possible fix
>>
>> hi,
>>
>> I might have stumbled on a bug in ipfilter 4.1.28 (used since m0n0wall
>> 1.3b5) that wasn't in 4.1.13 (equals to m0n0wall 1.3b4) while
>> troubleshooting some SIP issues that have been reported a few times on
>> the mailing lists.
>>
>> Now I'm looking for feedback from you NAT experts. I have a fix that
>> works for me but probably breaks other things. I'm by no means a
>> kernel hacker.
>>
>> First a quick description of the problem:
>>
>> A SIP client behind NAT is registering with a SIP provider on the
>> internet. This results in a dynamic MAP entry similar to this (ipnat
>> -lv):
>>
>> List of active sessions:
>> MAP 192.168.1.129   5060  <- -> x.x.x.x  1025  [y.y.y.y 5060]
>>  ttl 1191 use 0 sumd 0x7187/0x7187 pr 17 bkt 12759/10439 flags 2
>>  ifp X,X bytes 0/20177 pkts 0/39 ipsumd 814a
>>
>> Now the SIP provider makes a call to the SIP client by sending an
>> INVITE, but it doesn't use symmetric port mapping as most SBC's do.
>> Instead it uses port 5060 as its source and destination. With m0n0wall
>> 1.3b13 (down to 1.3b5), such a packet doesn't make it thru sometimes,
>> even though there is an inbound NAT rule set up to force forward port
>> 5060 to the SIP client. If there is no dynamic mapping, the packet
>> goes thru and a RDR entry is dynamically added:
>>
>> List of active sessions:
>> RDR 192.168.1.129   5060  <- -> x.x.x.x  5060  [y.y.y.y 5060]
>>  ttl 1191 use 0 sumd 0x7eb5/0x7eb5 pr 17 bkt 12759/11225 flags 2
>>  ifp X,X bytes 30942/54312 pkts 27/129 ipsumd 7eb5
>>
>> When I try the same set up on m0n0wall 1.3b4, I do get a MAP and a RDR
>> active session reported and the invites are going thru.
>>
>> I did then some digging (comparing ip_nat.c versions, kernel
>> debugging, etc) and seem to have found the culprit in nat_newrdr(),
>> which gets called when the INVITE comes from the public side. The
>> current versions of ip_nat.c have a new check to see if this redirect
>> mapping already exists and returns failure if so. By disabling this
>> newly added check, it works again. The Invite's go thru and I do get 2
>> active sessions: MAP and RDR.
>>
>> Obviously the right fix would be to make this check smarter and not
>> confuse a mapping entry with a redirect.
>>
>> any comments?
>>
>> Marcel
>>
>>
>>
>> --- ip_nat.c.orig       2008-07-30 15:32:22.000000000 +0200
>> +++ ip_nat.c    2008-07-30 15:32:40.000000000 +0200
>> @@ -2270,6 +2270,7 @@
>>         * it does, return "failure" (allowing it to be created will just
>>         * cause one or both of these "connections" to stop working.)
>>         */
>> +
>>        inb.s_addr = htonl(in.s_addr);
>>        sp = fin->fin_data[0];
>>        dp = fin->fin_data[1];
>> @@ -2280,7 +2281,8 @@
>>        fin->fin_data[0] = sp;
>>        fin->fin_data[1] = dp;
>>        if (natl != NULL)
>> -               return -1;
>> +/*             return -1;      marcel skip for now */
>> +
>>
>>        nat->nat_inip.s_addr = htonl(in.s_addr);
>>        nat->nat_outip = fin->fin_dst;
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: m0n0wall dash dev dash unsubscribe at lists dot m0n0 dot ch
>> For additional commands, e-mail: m0n0wall dash dev dash help at lists dot m0n0 dot ch
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: m0n0wall dash dev dash unsubscribe at lists dot m0n0 dot ch
>> For additional commands, e-mail: m0n0wall dash dev dash help at lists dot m0n0 dot ch
>>
>>
>