Re: [PATCH nf-next] netfilter: nf_ct_sctp: minimal multihoming support

[Michal Kubecek <mkubecek@suse.cz>]

On Wed, Jul 15, 2015 at 05:35:08PM -0300, Marcelo Ricardo Leitner wrote:
> Hi,
> 
> On Tue, Jul 14, 2015 at 06:42:25PM +0200, Michal Kubecek wrote:
> > On Tue, Jul 14, 2015 at 03:42:03PM +0200, Florian Westphal wrote:
> > > Michal Kubecek <mkubecek@suse.cz> wrote:
> > > > +	case SCTP_CID_HEARTBEAT:
> > > > +		pr_debug("SCTP_CID_HEARTBEAT");
> > > > +		i = 9;
> > > > +		break;
> > > > +	case SCTP_CID_HEARTBEAT_ACK:
> > > > +		pr_debug("SCTP_CID_HEARTBEAT_ACK");
> > > > +		i = 10;
> > > > +		break;
> > > >  	default:
> > > >  		/* Other chunks like DATA, SACK, HEARTBEAT and
> > > >  		its ACK do not cause a change in state */
> > > > @@ -329,6 +351,8 @@ static int sctp_packet(struct nf_conn *ct,
> > > >  	    !test_bit(SCTP_CID_COOKIE_ECHO, map) &&
> > > >  	    !test_bit(SCTP_CID_ABORT, map) &&
> > > >  	    !test_bit(SCTP_CID_SHUTDOWN_ACK, map) &&
> > > > +	    !test_bit(SCTP_CID_HEARTBEAT, map) &&
> > > > +	    !test_bit(SCTP_CID_HEARTBEAT_ACK, map) &&
> > > >  	    sh->vtag != ct->proto.sctp.vtag[dir]) {
> > > >  		pr_debug("Verification tag check failed\n");
> > > >  		goto out;
> > > > @@ -357,6 +381,16 @@ static int sctp_packet(struct nf_conn *ct,
> > > >  			/* Sec 8.5.1 (D) */
> > > >  			if (sh->vtag != ct->proto.sctp.vtag[dir])
> > > >  				goto out_unlock;
> > > > +		} else if (sch->type == SCTP_CID_HEARTBEAT ||
> > > > +			   sch->type == SCTP_CID_HEARTBEAT_ACK) {
> > > > +			if (ct->proto.sctp.vtag[dir] == 0) {
> > > > +				pr_debug("Setting vtag %x for dir %d\n",
> > > > +					 sh->vtag, dir);
> > > > +				ct->proto.sctp.vtag[dir] = sh->vtag;
> > > 
> > > Could you please elaborate on the [dir] == 0 test?
> > > 
> > > I see this might happen for SCTP_CID_HEARTBEAT_ACK, but why is this
> > > needed for SCTP_CID_HEARTBEAT ?
> > > 
> > > We found a conntrack entry so shouldn't the vtag[dir] already be > 0?
> > 
> > Yes, you are right. This was originally intended to handle the case when
> > a HEARTBEAT in the reply direction is seen before the HEARTBEAT-ACK but
> > such HEARTBEAT would be dropped anyway in current version.
> 
> And we have to keep the first vtag attempted because otherwise an
> attacker could just probe for the right one until she gets a reply.
> 
> IOW, if a different vtag is attempted, we should drop it as the packet
> doesn't belong to that association/conntrack entry.
> 
> As vtags are always != 0 in such case, that's a way to know if we
> already have that information or not.
> 
> > On the other hand, an alternative would be
> > 
> > 		} else if (sch->type == SCTP_CID_HEARTBEAT_ACK &&
> > 			   ct->proto.sctp.vtag[dir] == 0) {
> > 			pr_debug("Setting vtag %x for dir %d\n",
> > 				 sh->vtag, dir);
> > 			ct->proto.sctp.vtag[dir] = sh->vtag;
> > 		} else if ((sch->type == SCTP_CID_HEARTBEAT ||
> > 			    sch->type == SCTP_CID_HEARTBEAT_ACK) &&
> > 			   sh->vtag != ct->proto.sctp.vtag[dir]) {
> > 			pr_debug("Verification tag check failed\n");
> > 			goto out_unlock;
> > 		}
> > 
> > I'm not sure it looks better.
> 
> Now it seems swapped, we should save the tag on HB and check on
> HB_ACK only and would have to check against !dir entry. Like:

I forgot to include the explanation of vtag setting/checking logic into
the commit message. It is supposed to work like this:

Normally, vtag is set from the INIT chunk for the reply direction and
from the INIT-ACK chunk for the originating direction (i.e. each of
these defines vtag value for the opposite direction). For secondary
conntracks, we can't rely on seeing INIT/INIT-ACK and even if we have
seen them, we would need to connect two different conntracks. Therefore
simplified logic is applied: vtag of first packet in each direction
(HEARTBEAT in the originating and HEARTBEAT-ACK in reply direction) is
saved and all following packets in that direction are compared with this
saved value. While INIT and INIT-ACK define vtag for the opposite
direction (that's where "!dir" comes from), vtags extracted from
HEARTBEAT and HEARTBEAT-ACK are always for their direction. And we have
to check vtags on packets with HEARTBEAT chunks as well because their
vtags should match vtag of the first (set in sctp_new()).

                                                          Michal Kubecek