Date: Fri, 26 Apr 2002 19:44:27 +0300 From: Ruslan Ermilov <ru@FreeBSD.ORG> To: Igor M Podlesny <poige@morning.ru> Cc: net@FreeBSD.ORG, freebsd-isp@FreeBSD.ORG Subject: Re: patch -- An ingress filter (RFC2827) Message-ID: <20020426164427.GA82505@sunbay.com> In-Reply-To: <20020426213657.D85230@mars-gw.morning.ru> References: <20020414180447.A93954@mars-gw.morning.ru> <20020426091620.GA18917@sunbay.com> <20020426213657.D85230@mars-gw.morning.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
--EeQfGwPcQSOJBaQU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Apr 26, 2002 at 09:36:57PM +0800, Igor M Podlesny wrote: > On Fri, Apr 26, 2002 at 12:16:20PM +0300, Ruslan Ermilov wrote: > > On Sun, Apr 14, 2002 at 06:04:47PM +0800, Igor M Podlesny wrote: > > >=20 > > > Hello! > > >=20 > > > I'd like to know your opinion about this patch > > >=20 > > > http://www.morning.ru/~poige/patchzone/ingressfiltering.patch > > >=20 > > > which is mine attempt to implement an ingress filter being inspired by > > > RFC2827 "Network Ingress Filtering: Defeating Denial of Service Attac= ks > > > which employ IP Source Address Spoofing". > > >=20 > > > (http://www.ietf.org/rfc/rfc2827.txt) > > >=20 > > > It should be mentioned IMHO that this code makes another one in ip_in= put.c a > > > kind of redundant -- I mean code checking/blocking the 127/8 network = "on > > > wire". BTW, I suggest if not removing it completely then adding (sys)= logging > > > into, -- 127/8-spoofing certainly should be logged. :) > > >=20 > > > Another thing to pay an attention to: I deem it'd be better if a such= filter > > > was built-in into ip_fw.c, allowing such syntax for ipfw(8): > > >=20 > > > deny log ip from any to any in via fxp0 spoofed > > >=20 > > > But AFAIS in ip_fw.h: > > >=20 > > > #define IP_FW_F_IN 0x00000100 > > > ... > > > #define IP_FW_F_DME 0x40000000 /* destination =3D me */ > > >=20 > > > #define IP_FW_F_MASK 0x7FFFFFFF /* All possible flag bits mas= k */ > > >=20 > > > and u_int32_t fw_flg; > > >=20 > > > there is no free space for any additional flags... > > >=20 > > > So, I was a bit unsure whether should I expand fw_flg to u_int64_t, a= nd do > > > any other extensions. For now I decided just to wrote something like a > > > draft, test it (it seems to be working ;), and asking you, people, fo= r your > > > comments/ideas on it. > > >=20 > > > P.S. A bit more info on this patch is at http://www.morning.ru/~poige= /patchzone/ > > >=20 >=20 > At first, thank you, Ruslan for your answer, it's quite fertile! >=20 > > Style comments: >=20 > I should had read the manual or just had known it by heart before writing > out the patch in case I was a commiter going to commit it into the > repository. But I'm not. Moreover, I don't suggest using _this_ code AS IS > in FreeBSD. I said before -- this is a draft. Other people also agree this > should be better done (ingress filtering I mean) at ip_fw.c not ip_input.= c. >=20 > So yeah, it works someway, but again -- it's a draft. ;) >=20 > Well, thank you for time you spent telling me (us all, reading that) about > style, but I think that was done in a wrong time. And just one question > to you (if we're anyway have been talking about the style) as to a really > knowledgeable person: is the whole FreeBSD's kernel code is style(9)d fro= m A > to Z?... :) >=20 Just tell me next time you don't want to hear about style, it's okay with m= e. > > 1. There are many unnecessary whitespace changes. >=20 > (I consider using whitespaces similar to commenting, BTW. It's a good > C-style I heard. ;)) >=20 This makes patches very hard to read as there are many unrelated to the functionality changes. > > 2. Don't use the `register' keyword. >=20 > (Haven't found anything bout it in style(9)...) >=20 There was a huge sweep in -CURRENT that "removed 'register' keyword". This comment was inspired by this. > > 3. Double `const' doesn't do any good. (I was once confused about thi= s too.) >=20 > (const char *const ptr? >=20 > Why? I deem `const' can't make a code worse, only better, cause it makes = an > additional description of variables/functions/code/algo...) >=20 Because this is merely equivalent to "const char *ptr". > > 4. ip_fw.c part of the patch has some cruft in it. >=20 > Namely what/where? >=20 I misread the diffs, in the part that const'ified some variables in ipfw_report(). The change also included the bogus whitespace change, after "int len;". These are whitespace changes that make patches unreadable. > > Functional comments: > >=20 > > 1. The use and externalization of ipfw_report() wasn't a good > > idea. Your patch makes ingressfilter dependent on `options > > IPFIREWALL' because ip_fw.c is only compiled if this option > > is present. >=20 > Not such a big deal cause this can be easily solved in various ways > dependent on what we want... (yeah, that's again about `draft'-approach :) >=20 Well, you were pushy in asking me to comment on your patch, here they go. What's up? > > 2. Comment for ipf_rtaddr() is bogus -- the function returns > > the pointer to an interface not address. >=20 > (A pointer is a value keeping an address. Am I wrong?) >=20 "addr" =3D=3D (AF_INET family address), "ifp" =3D=3D (pointer to "struct if= "). > > 3. Function name is not good either, the more natural name > > would be ip_rtifp(). I would also suggest reimplementing > > the already existing ip_rtaddr() into ip_rtifp(), and > > implementing ip_rtaddr() in terms of ip_rtifp(). >=20 > (This'd be a good thing, I also thought about it, but this wasn't the ma= in > point of the patch -- the main idea stills be the same and it is ingress > filtering. :)) >=20 Well, again, you asked for a feedback, and I just tried to do my best. > > General notes: > >=20 > > Ingress filtering in unacceptable in many cases. >=20 > And is acceptable in many others :) >=20 > Don't you agree with that? >=20 "In many cases" does not of couse mean "never acceptable". Just wanted to point this out. And of course this patch doesn't replace the 127/8 check which should also occur in ip_output(). > > For example, > > our site is connected to two ISPs, ISP-A and ISP-B. Each ISP > > has allocated a network (NET-A and NET-B). Both channels are > > connected to a single gateway box, and are reachable through > > interfaces IF-A and IF-B, respectively. The `default' route > > on this box point to ISP-A through IF-A. > >=20 > > Now imagine that you want to ping(8) one of our addresses in > > NET-B. This packet will appear on our gateway box through > > IF-B, but ingress filter would discard it because ip_rtifp() > > lookup would return the IF-A interface for your address > > (ip_src in the packet). > > We solve the problem with multiple default routes with `ipfw > > fwd'. All outgoing packets with the source IP address in > > NET-B we forward through the IF-B interface. >=20 > 1) in case of ip_fw.c integrated version you could easily specify > not using ingress filtering on such NICs. >=20 That's true. > 2) this patch _AS_ _IS_ is already useful for both back bone routers maki= ng > up an internal network infrastructure and border gateways with single > _default_ route. I wrote about asymmetric routing incompatible with the p= atch, > and this point is quite similar to the situation, represented by you. >=20 I haven't seen this in your original mail to which I replied, perhaps I just missed it. > > Cheers, >=20 > P.S. I'd be very grateful if you could point out reasonable ways of > integration ingress filter with ip_fw.c... Or you consider this is not wo= rth > doing at all?... >=20 Maybe introducing the new "non-routable" keyword. That would filter all matching incoming packets. Specifying "deny ip from any to any non-routable" would mean "ingress filter on all interfaces". The keyword should only be accepted for "in" or "in out" rules. It is meaningless for "out" rules. Cheers, --=20 Ruslan Ermilov Sysadmin and 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 --EeQfGwPcQSOJBaQU Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.6 (FreeBSD) Comment: For info see http://www.gnupg.org iD8DBQE8yYPrUkv4P6juNwoRAqBtAKCC94QAqr9hSFs98Vcg7pYq5XYT7QCcC5JX 73B6uA3XwqWPxeQ2vYLw4e4= =Q+wx -----END PGP SIGNATURE----- --EeQfGwPcQSOJBaQU-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020426164427.GA82505>