microsoft / PQCrypto-VPN

Post-quantum Cryptography VPN
https://www.microsoft.com/research/project/post-quantum-crypto-vpn/
MIT License
319 stars 74 forks source link

Broken implementation of kyber512, kyber768 and kyber1024 as KEX. #19

Closed ReverseControl closed 4 years ago

ReverseControl commented 4 years ago

I tested kyber512, kyber768 and kyber1024. The test is simple:

  1. Establish a VPN using each of those algorithms as KEX.
  2. Observe the raw network traffic.
  3. Confirm both parties exchange the keys.
  4. Confirm the number of bytes for said keys are correct.

Kyber512 ( click to see the Kyber NIST submission paper for round 2 with the Kyber specs):

  1. Here is what happens with openssl s_server/s_client testing. kyber512_working_KEX_s_server
  1. Here is what happens with openvpn --config server/client(.ovpn):

Kyber768:

kyber1024:

kyber1024_not_working_KEX

kevinmkane commented 4 years ago

I'm seeing what you're seeing in my repro. We'll investigate. Thanks for the report.

kevinmkane commented 4 years ago

I think I see what's going on here. There are two issues:

  1. It turns out the ecdh-curve configuration directive in OpenVPN is only respected server-side. It's ignored on the client. In the normal case this makes sense: the client offers up all the groups it supports and the server picks (based on the ecdh-curve directive if provided, or its own default list if not). So the client has x25519 at the top of its list, and starts offering that. In our case, it might make sense to have the client also process the directive, so it sends a supported group list of exactly one element. OpenVPN's options interface doesn't currently support providing more than one curve; we could extend this to allow the client to set a list of supported groups, much as we allowed for a list of supported TLS 1.2 ciphersuites in the previous version.

  2. Wireshark is leading us astray by not actually parsing everything correctly. There's a Change Cipher Spec, and then there's Continuation Data. The data being split up like this is confusing Wireshark. In my trace, the Change Cipher Spec's size shows as 1242 bytes, which is absurdly oversized for that type. That type is 6 bytes. What follows in that packet and the Continuation Data is another Client Hello, and this time its Key Share extension has the correct group and key exchange data. Wireshark just isn't picking this apart, probably because the Kyber keys are bigger than sikep434. The key exchange data for sikep434 in my trace is 330 bytes, whereas kyber768 is 1008 bytes. Between this and OpenVPN's protocol I think Wireshark is losing track. But, going through the bytes by hand I can pick out the second ClientHello. The rest of my trace is showing as more Continuation Data, but I expect if I pick it apart I'll find another Server Hello to complete the negotiation.

So what's actually happening is:

If I look at a trace where sikep434 is selected, the same thing is happening: the client first offers an x25519 key share, and the server comes back with a Hello Retry Request with another key share extension that only has the group it wants. In this case, Wireshark manages to see and parse the second Client Hello that comes back with sikep434 group and key exchange data. It's just not seeing it in the Kyber case.

So in summary: Everything looks like it's working as it should, and we are getting a key negotiated with the desired algorithm. Wireshark just seems to be getting confused and isn't parsing all the pieces of the TLS 1.3 negotiation, possibly as a consequence of them being fragmented and/or encapsulated in OpenVPN's protocol.

ReverseControl commented 4 years ago

I think allowing the client to set group for KEX from the ecdh-curve directive would be helpful; even one choice is fine with the goal of extending it to more than one.

Is there any way you could write a plugin, or modify the wireshark TLS decoder, to handle this? :D

kevinmkane commented 4 years ago

It looks like making the client respect the ecdh-curve directive is pretty straightforward, so I'll look into doing that.

I don't think I'm up for trying to fix Wireshark's bugs, though. :) Have you considered opening a bug in their bug tracker, and supplying one of these captures to show what's going on? They'd be much better equipped to fix the parsing than me.

ReverseControl commented 4 years ago

I tested ecdh-curve on the client side and it works; kyber512 shows up as expected from looking at the s_client/s_server. However, for kyber768 wireshark cannot parse it at all. Could you please confirm that for kyber768 the keys are exchanged and that they are of the right size?

kevinmkane commented 4 years ago

That was what I tracked down before the analysis I posted yesterday, with Wireshark missing the second ClientHello. The key share extension in there had the kyber768 group number and a key share of the right size.