GTSM (TTL security)/RFC 5082 support?

[Ondrej Zajicek]

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?

> +    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?

Also a common idiom is to test negative error return values using 'fn() < 0'.

Both these comments are probably unnecessary nitpicky. ;-)

> 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.

>   | 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.
Perhaps TTL SECURITY HOPS, or just MIN TTL?

(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.)

> +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.

> +
> +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 .

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago at crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://trubka.network.cz/pipermail/bird-users/attachments/20110814/a67cbde4/attachment-0001.asc>