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

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Thu, Jul 16, 2015 at 02:05:12PM +0200, Michal Kubecek wrote:
> 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

Erf, indeed. I totally confused it and thought they would be equal on
both directions.

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

Yes, that's pretty much it. Original code reads better here then.

Thanks,
Marcelo