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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZhZUQoOuvNz8RVg8@hog>
Date: Wed, 10 Apr 2024 10:56:34 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antony Antony <antony@...nome.org>
Cc: Nicolas Dichtel <nicolas.dichtel@...nd.com>,
	Antony Antony <antony.antony@...unet.com>,
	Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org,
	devel@...ux-ipsec.org, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [devel-ipsec] [PATCH ipsec-next v6] xfrm: Add Direction to the
 SA in or out

2024-04-09, 19:23:04 +0200, Antony Antony wrote:
> On Mon, Apr 08, 2024 at 03:02:31PM +0200, Sabrina Dubroca wrote:
> > 2024-04-07, 10:23:21 +0200, Antony Antony wrote:
> > > Thank you for raising this point.  I thought that introducing a patch for 
> > > the replay window check could stir more controversy, which might delay the 
> > > acceptance of the essential 'dir' feature.
> > 
> > I'm not convinced it's *that* critical. People have someone managed to
> 
> Understood, but from a user's perspective, I've seen significant confusion 
> around this issue. Labeling it as 'historical' and unchangeable ignores its 
> real impact on usability. I feel adding "dir"  would help a lot.

Sure. I meant that also in relation to IPTFS development:

> > use IPsec without it for all those years. Is the intention to only
> > allow setting up IPTFS SAs when this new 'dir' attribute is provided?
> > If not, then this patch is not really blocking for IPTFS.


[...]
> > > For non-ESN scenarios, the outbound SA should have a replay window set to 0?
> > 
> > Looks ok.
> > 
> > > And for ESN 1?
> > 
> > Why 1 and not 0?
> 
> Current implemenation does not allow 0.

So we have to pass a replay window even if we know the SA is for
output? That's pretty bad.

> Though supporting 0 is higly desired 
> feature and probably a hard to implement feature in xfrm code. 

Why would it be hard for outgoing SAs? The replay window should never
be used on those. And xfrm_replay_check_esn and xfrm_replay_check_bmp
already have checks for 0-sized replay window.

> Supporting 0 
> is also a long standing argument:)
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/xfrm/xfrm_replay.c#n781
> 
> int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack)
> 
> 781                         if (replay_esn->replay_window == 0) {
> 782                                 NL_SET_ERR_MSG(extack, "ESN replay window must be > 0");
> 783                                 return -EINVAL;
> 784                         }
> 
> > Does setting xfrm_replay_state_esn->{oseq,oseq_hi} on an input SA (and
> > {seq,seq_hi} on output) make sense?
> 
> 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?

Also, this is a new bit of API. We don't have to accept strange
configs with it, userspace should adapt to the strict rules we
require.

> > And xfrm_state_update probably needs to check that the dir value
> > matches?  If we get this far we know the new state had matching
> > direction and properties, but maybe the old one didn't?
> 
> Yes. I will add this too.
> 
> > In XFRMA_SA_EXTRA_FLAGS, both XFRM_SA_XFLAG_DONT_ENCAP_DSCP and
> > XFRM_SA_XFLAG_OSEQ_MAY_WRAP look like they're only used on output. A
> > few of the flags defined with xfrm_usersa_info also seem to work only
> > in one direction (XFRM_STATE_DECAP_DSCP, XFRM_STATE_NOPMTUDISC,
> > XFRM_STATE_ICMP).
> 
> I'm familiar with one flag, but my knowledge on the rest is limited, still I 
> believe they aren't direction-specific. If anyone has more specific insight, 
> please do share. Are any of these flags or x flags direction specific?

[I typically wait for answers to my questions before I post the next
version of a patch. Otherwise, reviewers have to do more work, looking
at each version.]

BTW I just looked at all the flags defined in uapi, and asked cscope
where they were used. For some, the function names were clearly only
output path, for some just input, or both directions.

[...]
> > I think it would also make sense to only accept this attribute in
> > xfrm_add_sa, and not for any of the other message types. Sharing
> 
> > 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.

Thanks.

-- 
Sabrina