GTSM (TTL security)/RFC 5082 support?

[Alexander V. Chernikov]

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