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

[Antony Antony <antony@phenome.org>]

On Thu, Apr 11, 2024 at 11:24:06AM +0200, Sabrina Dubroca wrote:
> 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?

Corrrect, It is not allowed in v10.

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

Definitely see the value in such feature in general, but it seems ambitious 
for this patch set. Currently, only NEWSA, UPDSA, and ALLOCSPI need 
XFRMA_SA_DIR. I am wondering how to reject this atrribute in remaining 20-22 
messages.  Is there a precedent or example in xfrm_user.c for this kind of 
validation, or maybe a Netlink feature that lets us restrict NL attributes 
for a specific messages like DELSA.

If not, it feels like a  seperate patch set for general API cleanup.