Bug 20877
| Summary: | ICMP error msg on UDP port unreachable is incorrect | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Frank Volf <volf> | ||||
| Component: | kern | Assignee: | ru <ru> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | CC: | volf | ||||
| Priority: | Normal | ||||||
| Version: | 4.1-STABLE | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
State Changed From-To: open->closed Duplicate of PR 16240. On Sun, 27 Aug 2000 19:45:22 +0200, Frank Volf wrote:
> I disagree with the fact that you simply close this pr as being a duplicate
> case of PR 16240.
>
> PR 16240 tries to address the generic problem, which is indeed present in
> many network implementations and may or maynot be difficult to fix.
Do you not agree that the resolution to PR 16240 will resolve PR 20877
as a side-effect?
I know that we're not altogether keen on applying hack-arounds for more
general problems unless the more general problems can't be resolved in
the foreseeable future.
Ciao,
Sheldon.
On Sun, Aug 27, 2000 at 07:45:22PM +0200, Frank Volf wrote: > > I disagree with the fact that you simply close this pr as being a duplicate > case of PR 16240. > > PR 16240 tries to address the generic problem, which is indeed present in > many network implementations and may or maynot be difficult to fix. > > Here, a very simple patch is presented for a special instance of 16240 > (an instance that occurs a lot, e.g. using udp based tracerouted). I see no > reason why this patch cannot be applied to FreeBSD. > The reason is simple -- your patch is wrong and incomplete. > If there *are* issues that I overlooked I would like to hear about them, > and have them properly discussed. > You overlooked (amongst other things) that ip_off field is also vulnerable. The basic idea is that all IP header fields SHOULD BE in host byte order right after the start of ip_input(), and ip_output() converts them back to network byte order. So in icmp_error() the bytes should still be in host byte order, this is even implied by the following piece of code: /* * Don't send error if not the first fragment of message. * Don't error if the old packet protocol was ICMP * error message, only known informational types. */ if (oip->ip_off &~ (IP_MF|IP_DF)) goto freeit; Attached is the patch that fixes part of problems with ICMP error generation. It could be applied to both 5.0-CURRENT and 4.1-STABLE. This patch is still incomplete, it misses the ip_output() portion of fixes. I will develop and test the remaining bits tomorrow and commit it along with this patch. Cheers, -- Ruslan Ermilov Oracle Developer/DBA, ru@sunbay.com Sunbay Software AG, ru@FreeBSD.org FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age Sheldon Hearn wrote: > > On Sun, 27 Aug 2000 19:45:22 +0200, Frank Volf wrote: > > > I disagree with the fact that you simply close this pr as being a duplicate > > case of PR 16240. > > > > PR 16240 tries to address the generic problem, which is indeed present in > > many network implementations and may or maynot be difficult to fix. > > Do you not agree that the resolution to PR 16240 will resolve PR 20877 > as a side-effect? Yes, I agree. But the impression that I got from PR 16240 is, that the general problem is too difficult to solve and I got the feeling that it would not be solved at all.... The reason that I want this fixed are twofold: 1) OS finger printing. Because FreeBSD returns the wrong data, you can use that information to 2) I'm working on fixing a bug in IP-Filter that has to do with NAT and ICMP error messages associated with the NAT-ed connection. However, before I can fix that, I must first make sure that the host OS behaves properly :-) > I know that we're not altogether keen on applying hack-arounds for more > general problems unless the more general problems can't be resolved in > the foreseeable future. If someone is going to tackle the generic problem, that would be great. But, then you should say: hey we are working on fixing the general case so we are closing your PR.... Frank On Mon, 28 Aug 2000 23:59:50 +0200, Frank Volf wrote:
> If someone is going to tackle the generic problem, that would be great. But,
> then you should say: hey we are working on fixing the general case so we
> are closing your PR....
You'll find that there are people even more qualified to criticise the
way the PR database is handled, namely the volunteers who handle it.
For example, check this out... ;-)
You're quite right. Your PR could definitely have been closed with a
message better geared toward putting you at ease.
Now...
All we can really offer you as consolation is that there are lots of
PRs and not as many people working on them. So if we can at least
limit the database to one PR per problem, the folks doing the work of
tackling problems are more likely to find their investment of time
paying wortwhile dividends.
So please try to be understanding of the volunteers who do what they can
to ensure that FreeBSD works as well as possible for as many people as
possible. Even when we make mistakes while doing so. :-)
Ciao,
Sheldon.
Ruslan Ermilov wrote:
> On Sun, Aug 27, 2000 at 07:45:22PM +0200, Frank Volf wrote:
> >
> > I disagree with the fact that you simply close this pr as being a duplicate
> > case of PR 16240.
> >
> > PR 16240 tries to address the generic problem, which is indeed present in
> > many network implementations and may or maynot be difficult to fix.
> >
> > Here, a very simple patch is presented for a special instance of 16240
> > (an instance that occurs a lot, e.g. using udp based tracerouted). I see no
> > reason why this patch cannot be applied to FreeBSD.
> >
> The reason is simple -- your patch is wrong and incomplete.
I agree that I overlooked the ip_off part. At the place where I inserted my
patch the packet is reassambled so ip_off must be zero. But I forgot about
the MF and DF flags that could be set, and therefore my patch does not work
for that case. Sorry for that.
I'll try your patch and see how it works.
Thanks,
Frank
Sheldon, I know everything about the huge effort that an organization needs to make to handle the problems of its users (I'm working far too long in that area.... :-). I know that FreeBSD volunteers put in a huge effort to help everybody out, so no problem their either. I just had the feeling that I might not be taken seriously. While, in my opinion, the people that are willing to make PR's are often the people that care about FreeBSD! But never mind, I think we have things on the road now, and we will be able to tackle this problem once and for all! Frank Ruslan,
I verified your patch, and it seems to work for udp based traceroutes.
I do have two questions though:
1) You might have broken ipfw, ipfilter and possibly other packet filtering
systems that are called in ip_input(); if they use icmp_error() and they
did the right thing (fixing ip_id e.a) then they do now the wrong thing
:-)
2) I do not understand why you remove HTONS(ip->ip_id) from ip_forward().
I have the feeling, without being able to pin point it, that you have
broken something: because now you change the ip_id for every ip packet
that goes through ip_forward()! If this was not broken before it must
be broken now (or I must get some sleep....).
Frank
Ruslan Ermilov wrote:
> On Sun, Aug 27, 2000 at 07:45:22PM +0200, Frank Volf wrote:
> >
> > I disagree with the fact that you simply close this pr as being a duplicate
> > case of PR 16240.
> >
> > PR 16240 tries to address the generic problem, which is indeed present in
> > many network implementations and may or maynot be difficult to fix.
> >
> > Here, a very simple patch is presented for a special instance of 16240
> > (an instance that occurs a lot, e.g. using udp based tracerouted). I see no
> > reason why this patch cannot be applied to FreeBSD.
> >
> The reason is simple -- your patch is wrong and incomplete.
>
> > If there *are* issues that I overlooked I would like to hear about them,
> > and have them properly discussed.
> >
> You overlooked (amongst other things) that ip_off field is also vulnerable.
>
> The basic idea is that all IP header fields SHOULD BE in host byte order
> right after the start of ip_input(), and ip_output() converts them back
> to network byte order. So in icmp_error() the bytes should still be in
> host byte order, this is even implied by the following piece of code:
>
> /*
> * Don't send error if not the first fragment of message.
> * Don't error if the old packet protocol was ICMP
> * error message, only known informational types.
> */
> if (oip->ip_off &~ (IP_MF|IP_DF))
> goto freeit;
>
>
> Attached is the patch that fixes part of problems with ICMP error generation.
> It could be applied to both 5.0-CURRENT and 4.1-STABLE. This patch is still
> incomplete, it misses the ip_output() portion of fixes. I will develop and
> test the remaining bits tomorrow and commit it along with this patch.
>
>
> Cheers,
> --
> Ruslan Ermilov Oracle Developer/DBA,
> ru@sunbay.com Sunbay Software AG,
> ru@FreeBSD.org FreeBSD committer,
> +380.652.512.251 Simferopol, Ukraine
>
> http://www.FreeBSD.org The Power To Serve
> http://www.oracle.com Enabling The Information Age
[ Attachment, skipping... ]
On Tue, Aug 29, 2000 at 01:03:44AM +0200, Frank Volf wrote: > > Ruslan, > > I verified your patch, and it seems to work for udp based traceroutes. > I do have two questions though: > > 1) You might have broken ipfw, ipfilter and possibly other packet filtering > systems that are called in ip_input(); if they use icmp_error() and they > did the right thing (fixing ip_id e.a) then they do now the wrong thing > :-) > No they do not "fix" it, and so they were broken as well. It could be checked by putting the following ipfw(8) rule: ipfw add 10 unreach host ip from foo to bar > 2) I do not understand why you remove HTONS(ip->ip_id) from ip_forward(). > I have the feeling, without being able to pin point it, that you have > broken something: because now you change the ip_id for every ip packet > that goes through ip_forward()! If this was not broken before it must > be broken now (or I must get some sleep....). > Because HTONS(ip->ip_id) has migrated to icmp_error() where it should be. Try `traceroute -m1 1.2.3.4' via FreeBSD router from one-hop-away host. BTW NetBSD had this bug fixed since January 1999 in a similar fashion. -- Ruslan Ermilov Oracle Developer/DBA, ru@sunbay.com Sunbay Software AG, ru@FreeBSD.org FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age Responsible Changed From-To: freebsd-bugs->ru Prevent people from committing incomplete changes while I am working on a proper patch. |
When a UDP packet is send to a port that is unreachable (which is a common feature used in e.g. traceroute) an ICMP error message is returned (icmp port unreachable) which includes (part of) the original IP packet. The problem is that this (part of) the original IP packet is not completely correct. The ip_id field is not in network byte order (in ip_input(), ip_id is translated to host byte order), but this is never reverted before the IP packet is send to icmp_error for generation of the ICMP error message. The following patch (relative to 4.1 stable source code) fixes the problem: Index: udp_usrreq.c =================================================================== RCS file: /home2/CVS/src/sys/netinet/udp_usrreq.c,v retrieving revision 1.64.2.3 diff -u -u -r1.64.2.3 udp_usrreq.c --- udp_usrreq.c 2000/08/03 00:09:36 1.64.2.3 +++ udp_usrreq.c 2000/08/27 01:02:35 @@ -358,8 +358,10 @@ if (badport_bandlim(0) < 0) goto bad; #endif - if (!blackhole) + if (!blackhole) { + HTONS(ip->ip_id); icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_PORT, 0, 0); + } else goto bad; return; Please commit this patch to the FreeBSD source code repository. Many thanks, Frank