-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Ondrej Zajicek wrote:
On Sun, Aug 14, 2011 at 02:47:27PM +0400, Alexander V. Chernikov wrote:
Review/comments are welcome
Patch adds:
* new sk_set_min_ttl() function to set minimum received TTL * new BGP (cisco-like) config option: ttl_secutity hops <value>
Tested on FreeBSD, however linux part should work too.
Great! We could merge it soon. Several comments inside.
Index: proto/bgp/bgp.c =================================================================== --- proto/bgp/bgp.c (revision 4980) +++ proto/bgp/bgp.c (working copy) @@ -574,7 +574,11 @@ bgp_connect(struct bgp_proto *p) /* Enter Connect s->saddr = p->source_addr; s->daddr = p->cf->remote_ip; s->dport = BGP_PORT; - s->ttl = p->cf->multihop ? : 1; + /* TTL security support */ + if (p->cf->min_ttl > 0) + s->ttl = 255; + else + s->ttl = p->cf->multihop ? : 1; s->rbsize = BGP_RX_BUFFER_SIZE; s->tbsize = BGP_TX_BUFFER_SIZE; s->tos = IP_PREC_INTERNET_CONTROL; @@ -589,6 +593,17 @@ bgp_connect(struct bgp_proto *p) /* Enter Connect bgp_sock_err(s, 0); return; } + /* Set minimal receive TTL if needed */ + if (p->cf->min_ttl > 0) + {
+ log(L_ERR "Setting minimum received TTL to %d", p->cf->min_ttl);
Probably should not be here (there is no error), some fugitive debugging message? Ups. It should be removed or converted to DBG() :)
+ if (sk_set_min_ttl(s, p->cf->min_ttl) != 0) + { + log(L_ERR "TTL security configuration failed, closing session"); + bgp_sock_err(s, 0); + return; + } + }
Shouldn't be better to set min TTL before sk_open? Not sure. Not many callers need this, so adding another min_ttl field seems unnecessary IMHO. Anyway, you will need to specify minimum ttl directly in case of new connection from listening socket.
Also a common idiom is to test negative error return values using 'fn() < 0'.
Okay :) There are so many idioms/coding styles in different software projects that sometimes it's hard to switch.
Both these comments are probably unnecessary nitpicky. ;-)
No problem :)
Index: proto/bgp/config.Y =================================================================== --- proto/bgp/config.Y (revision 4980) +++ proto/bgp/config.Y (working copy) @@ -25,7 +25,7 @@ CF_KEYWORDS(BGP, LOCAL, NEIGHBOR, AS, HOLD, TIME, ADVERTISE, IPV4, CAPABILITIES, LIMIT, PASSIVE, PREFER, OLDER, MISSING, LLADDR, DROP, IGNORE, ROUTE, REFRESH, INTERPRET, COMMUNITIES, BGP_ORIGINATOR_ID, BGP_CLUSTER_LIST, IGP, TABLE, - GATEWAY, DIRECT, RECURSIVE, MED) + GATEWAY, DIRECT, RECURSIVE, MED, TTL_SECURITY, HOPS)
CF_GRAMMAR
@@ -49,6 +49,7 @@ bgp_proto_start: proto_start BGP { BGP_CFG->advertise_ipv4 = 1; BGP_CFG->interpret_communities = 1; BGP_CFG->default_local_pref = 100; + BGP_CFG->min_ttl = 0;
BGP_CFG fields are already zeroed (somewhere in proto_config_new()), no need to set that.
Ups :)
| bgp_proto IGP TABLE rtable ';' { BGP_CFG->igp_table = $4; } + | bgp_proto TTL_SECURITY HOPS expr ';' { BGP_CFG->min_ttl = $4; }
It is very unBIRDy to have a config option with underscore.
Em, well, BGP still heavily uses underscores: BGP_ORIGINATOR_ID, BGP_CLUSTER_LIST, BGP_PATH, BGP_LOCAL_PREF, BGP_MED, BGP_ORIGIN, BGP_NEXT_HOP, BGP_ATOMIC_AGGR, BGP_AGGREGATOR,BGP_COMMUNITY,
Perhaps TTL SECURITY HOPS, or just MIN TTL? 'TTL SECURITY HOPS' sounds good and is at least used by cisco.
(MIN TTL is probably much better name as we do not specify the number of hops, but the complement (255 - hops), if i understand it correctly.)
Well, actually we're specifying minimal TTL packet needs to have in its packet header to be accepted. Packets with lower TTL are silently dropped. If we name this option 'min ttl' or 'min hops' it will: * be confised with 'multihop' option * not be associated with enabling TTL security We can also make 'TTL SECURITY' boolean option and use 'multihop' option value (like 255 - hops + 1)
+static int +sk_set_min_ttl4(sock *s, int ttl) +{ +#ifdef IP_MINTTL + if (setsockopt(s->fd, IPPROTO_IP, IP_MINTTL, &ttl, sizeof(ttl)) == -1) + { + log(L_ERR "sk_set_min_ttl4: setsockopt: %m"); + return -1; + } + + return 0; +#else + log(L_ERR "IP_MINTTL options is not supported by your kernel"); + return -1; +#endif +}
It is much better to define IP_MINTTL if it is missing:
#ifndef IP_MINTTL #define IP_MINTTL XXX #endif
Compile time kernel feature autodetection is IMHO a pretty poor policy, when software is usually distributed in binary form (as is common in Linux distributions), compiled on some unknown machine, where user chooses kernel versions independently. Understood.
+ +static int +sk_set_min_ttl6(sock *s, int ttl) +{ +#ifdef IPV6_MINHOPCNT + if (setsockopt(s->fd, IPPROTO_IPV6, IPV6_MINHOPCNT, &ttl, sizeof(ttl)) == -1) + { + log(L_ERR "sk_set_min_ttl6: setsockopt: %m"); + return -1; + } + + return 0; +#else + log(L_ERR "IPV6_MINHOPCNT options is not supported by your kernel"); + return -1; +#endif
ditto
The new config option should be also documented in doc/bird.sgml .
Should I supply updated patch?
-----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.14 (FreeBSD) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk5H6SsACgkQwcJ4iSZ1q2nwBwCgkNLG9iFeu/pSckH3vtn2n67B ITQAnAuIG34ZleiFDUskWoSXLIPPPS/h =oe0H -----END PGP SIGNATURE-----