Re: [devel-ipsec] [PATCH ipsec-next v6] xfrm: Add Direction to the SA in or out

[Sabrina Dubroca <sd@queasysnail.net>]

2024-04-10, 18:59:00 +0200, Antony Antony wrote:
> On Wed, Apr 10, 2024 at 10:56:34AM +0200, Sabrina Dubroca wrote:
> > 2024-04-09, 19:23:04 +0200, Antony Antony wrote:
> > > Good point. I will add  {seq,seq_hi} validation. I don't think we add a for 
> > > {oseq,oseq_hi} as it might be used by strongSwan with: ESN  replay-window 1, 
> > > and migrating an SA.
> > 
> > I'm not at all familiar with that. Can you explain the problem?
> 
> strongSwan sets ESN and replay-window 1 on "out" SA. Then to migrgate, when 
> IKEv2  mobike exchange succeds, it use GETSA read {oseq,oseq_hi} and the 
> attributes, delete this SA.  Then create a new SA, with a different end 
> point, and with old SA's {oseq,oseq_hi} and other parameters(curlft..).  
> While Libreswan and Android use XFRM_MSG_MIGRATE.

Ok, thanks. But that's still an output SA. Setting {oseq,oseq_hi} on
an input SA is bogus I would think?


> > > > xfrma_policy is convenient but not all attributes are valid for all
> > > > requests. Old attributes can't be changed, but we should try to be
> > > > more strict when we introduce new attributes.
> > > 
> > > To clarify your feedback, are you suggesting the API should not permit 
> > > XFRMA_SA_DIR for methods like XFRM_MSG_DELSA, and only allow it for 
> > > XFRM_MSG_NEWSA and XFRM_MSG_UPDSA? I added XFRM_MSG_UPDSA, as it's used 
> > > equivalently to XFRM_MSG_NEWSA by *swan.
> > 
> > Not just DELSA, also all the *POLICY, ALLOCSPI, FLUSHSA, etc. NEWSA
> > and UPDSA should accept it, but I'm thinking none of the other
> > operations should. It's a property of SAs, not of other xfrm objects.
> 
> For instance, there isn't a validation for unused XFRMA_SA_EXTRA_FLAGS in 
> DELSA; if set, it's simply ignored. Similarly, if XFRMA_SA_DIR were set in 
> DELSA, it would also be disregarded. Attempting to introduce validations for 
> DELSA and other methods seems like an extensive cleanup task. Do we consider 
> this level of validation within the scope of our current patch? It feels 
> like we are going too far.

No, I wouldn't introduce validation of other attributes. It doesn't
belong in this patch(set), and I'm not sure we can add it now as it
might break userspace (I don't see why userspace would pass
XFRMA_ALG_AEAD etc on a DELSA request, but if we never rejected it,
they could).

But rejecting this new attribute from messages that don't handle it
would be good, and should be done in this patch/series.

-- 
Sabrina