Message-ID: <6659d71adc259_3f8cab29433@willemb.c.googlers.com.notmuch>
Date: Fri, 31 May 2024 09:56:42 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, 
 pabeni@...hat.com, 
 borisp@...dia.com, 
 gal@...dia.com, 
 cratiu@...dia.com, 
 rrameshbabu@...dia.com, 
 steffen.klassert@...unet.com, 
 tariqt@...dia.com
Subject: Re: [RFC net-next 01/15] psp: add documentation

Jakub Kicinski wrote:
> On Wed, 29 May 2024 20:47:02 -0400 Willem de Bruijn wrote:
> > Jakub Kicinski wrote:
> > > On Sun, 12 May 2024 21:24:23 -0400 Willem de Bruijn wrote:  
> > > > There is some value in using the same terminology in the code as in
> > > > the spec.
> > > > 
> > > > And the session keys are derived from a key. That is more precise than
> > > > state. Specifically, counter-mode KDF from an AES key.
> > > > 
> > > > Perhaps device key, instead of master key?   
> > > 
> > > Weak preference towards secret state, but device key works, too.  
> > 
> > Totally your choice. I just wanted to make sure this was considered.
> 
> Already run the sed, device key it is :)
> 
> > > > Consider clarifying the entire state diagram from when one pair
> > > > initiates upgrade.  
> > > 
> > > Not sure about state diagram, there are only 3 states. Or do you mean
> > > extend TCP state diagrams? I think a table may be better:
> > > 
> > > Event         | Normal TCP      | Rx PSP key present | Tx PSP key present |
> > > ---------------------------------------------------------------------------
> > > Rx plain text | accept          | accept             | drop               |
> > > 
> > > Rx PSP (good) | drop            | accept             | accept             |
> > > 
> > > Rx PSP (bad)  | drop            | drop               | drop               |
> > > 
> > > Tx            | plain text      | plain text         | encrypted *        |
> > > 
> > > * data enqueued before Tx key in installed will not be encrypted
> > >   (either initial send nor retranmissions)
> > > 
> > > 
> > > What should I add?  
> > 
> > I've mostly been concerned about the below edge cases.
> > 
> > If both peers are in TCP_ESTABLISHED for the during of the upgrade,
> > and data is aligned on message boundary, things are straightforward.
> > 
> > The retransmit logic is clear, as this is controlled by skb->decrypted
> > on the individual skbs on the retransmit queue.
> > 
> > That also solves another edge case: skb geometry changes on retransmit
> > (due to different MSS or segs, using tcp_fragment, tso_fragment,
> > tcp_retrans_try_collapse, ..) maintain skb->decrypted. It's not
> > possible that skb is accidentally created that combines plaintext and
> > ciphertext content.
> > 
> > Although.. does this require adding that skb->decrypted check to
> > tcp_skb_can_collapse?
> 
> Good catch. The TLS checks predate tcp_skb_can_collapse() (and MPTCP).
> We've grown the check in tcp_shift_skb_data() and the logic
> in tcp_grow_skb(), both missing the decrypted check.
> 
> I'll send some fixes, these are existing bugs :(
> 
> > > > And some edge cases:
> > > > 
> > > > - retransmits
> > > > - TCP fin handshake, if only one peer succeeds  
> > > 
> > > So FIN when one end is "locked down" and the other isn't?  
> > 
> > If one peer can enter the state where it drops all plaintext, while
> > the other decides to close the connection before completing the
> > upgrade, and thus sends a plaintext FIN.
> > 
> > If (big if) that can happen, then the connection cannot be cleanly
> > closed.
> 
> Hm. And we can avoid this by only enforcing encryption of data-less
> segments once we've seen some encrypted data?

That would help. It may also be needed to accept a pure ACK right at
the upgrade seqno. Depends on the upgrade process.

Which may be worth documenting explicitly: the system call and network
packet exchange from when one peer initiates (by generating its local
key) until the connection is fully encrypted. That also allows poking
at the various edge cases that may happen if packets are lost, or when
actions can race.

One unexpected example of the latter that I came across was Tx SADB
key insertion in tail edge cases taking longer than network RTT, for
instance.

The kernel API can be exercised in a variety of ways, not all of them
will uphold the correctness. Documenting how it should be used should
help.

Even better when it reduces the option space. As it already does by
failing a Tx key install until Rx is configured.

> > > > - TCP control socket response to encrypted pkt  
> > > 
> > > Control sock ignores PSP.  
> > 
> > Another example where a peer stays open and stays retrying if it has
> > upgraded and drops all plaintext.

May want to always allow plaintext RSTs. This is a potential DoS
vector. In all these cases, I suppose this has already been figured
out for TLS.

Powered by blists - more mailing lists