Merge pull request #388 from tomato42/tls13-ecdh-fixes

TLS 1.3 ECDH and key_shares fixes

Author: simo5

tlslite/keyexchange.py

@@ -23,6 +23,7 @@
 from .utils.x25519 import x25519, x448, X25519_G, X448_G, X25519_ORDER_SIZE, \
         X448_ORDER_SIZE
 from .utils.compat import int_types
+from .utils.codec import DecodeError
 
 
 class KeyExchange(object):
@@ -907,7 +908,7 @@ def calc_shared_key(self, private, peer_share):
             try:
                 ecdhYc = decodeX962Point(peer_share,
                                          curve)
-            except AssertionError:
+            except (AssertionError, DecodeError):
                 raise TLSIllegalParameterException("Invalid ECC point")
 
             S = ecdhYc * private

tlslite/tlsconnection.py

@@ -691,20 +691,6 @@ def _clientSendClientHello(self, settings, session, srpUsername,
             extensions.append(TLSExtension().create(ExtensionType.
                                                     extended_master_secret,
                                                     bytearray(0)))
-        groups = []
-        #Send the ECC extensions only if we advertise ECC ciphers
-        if next((cipher for cipher in cipherSuites \
-                if cipher in CipherSuite.ecdhAllSuites), None) is not None:
-            groups.extend(self._curveNamesToList(settings))
-            extensions.append(ECPointFormatsExtension().\
-                              create([ECPointFormat.uncompressed]))
-        # Advertise FFDHE groups if we have DHE ciphers
-        if next((cipher for cipher in cipherSuites
-                 if cipher in CipherSuite.dhAllSuites), None) is not None:
-            groups.extend(self._groupNamesToList(settings))
-        # Send the extension only if it will be non empty
-        if groups:
-            extensions.append(SupportedGroupsExtension().create(groups))
         # In TLS1.2 advertise support for additional signature types
         if settings.maxVersion >= (3, 3):
             sigList = self._sigHashesToList(settings)
@@ -717,6 +703,7 @@ def _clientSendClientHello(self, settings, session, srpUsername,
 
         session_id = bytearray()
         # when TLS 1.3 advertised, add key shares, set fake session_id
+        shares = None
         if next((i for i in settings.versions if i > (3, 3)), None):
             # if we have a client cert configured, do indicate we're willing
             # to perform Post Handshake Authentication
@@ -746,6 +733,27 @@ def _clientSendClientHello(self, settings, session, srpUsername,
                 [getattr(PskKeyExchangeMode, i) for i in settings.psk_modes])
             extensions.append(ext)
 
+        groups = []
+        #Send the ECC extensions only if we advertise ECC ciphers
+        if next((cipher for cipher in cipherSuites \
+                if cipher in CipherSuite.ecdhAllSuites), None) is not None:
+            groups.extend(self._curveNamesToList(settings))
+            extensions.append(ECPointFormatsExtension().\
+                              create([ECPointFormat.uncompressed]))
+        # Advertise FFDHE groups if we have DHE ciphers
+        if next((cipher for cipher in cipherSuites
+                 if cipher in CipherSuite.dhAllSuites), None) is not None:
+            groups.extend(self._groupNamesToList(settings))
+        # Send the extension only if it will be non empty
+        if groups:
+            if shares:
+                # put the groups used for key shares first, and in order
+                # (req. from RFC 8446, section 4.2.8)
+                share_ids = [i.group for i in shares]
+                diff = set(groups) - set(share_ids)
+                groups = share_ids + [i for i in groups if i in diff]
+            extensions.append(SupportedGroupsExtension().create(groups))
+
         if settings.use_heartbeat_extension:
             extensions.append(HeartbeatExtension().create(
                 HeartbeatMode.PEER_ALLOWED_TO_SEND))
@@ -3119,6 +3127,23 @@ def _serverGetClientHello(self, settings, private_key, cert_chain,
                             .format(GroupName.toStr(mismatch))):
                         yield result
 
+                key_share_ids = [i.group for i in key_share.client_shares]
+                if len(set(key_share_ids)) != len(key_share_ids):
+                    for result in self._sendError(
+                            AlertDescription.illegal_parameter,
+                            "Client sent multiple key shares for the same "
+                            "group"):
+                        yield result
+
+                group_ids = sup_groups.groups
+                diff = set(group_ids) - set(key_share_ids)
+                if key_share_ids != [i for i in group_ids if i not in diff]:
+                    for result in self._sendError(
+                            AlertDescription.illegal_parameter,
+                            "Client sent key shares in different order than "
+                            "the advertised groups."):
+                        yield result
+
                 sig_algs = clientHello.getExtension(
                     ExtensionType.signature_algorithms)
                 if (not psk_modes or not psk) and sig_algs:

tlslite/utils/ecc.py

@@ -3,21 +3,26 @@
 # See the LICENSE file for legal information regarding use of this file.
 """Methods for dealing with ECC points"""
 
-from .codec import Parser, Writer
+from .codec import Parser, Writer, DecodeError
 from .cryptomath import bytesToNumber, numberToByteArray, numBytes
 from .compat import ecdsaAllCurves
 import ecdsa
 
+
 def decodeX962Point(data, curve=ecdsa.NIST256p):
     """Decode a point from a X9.62 encoding"""
     parser = Parser(data)
     encFormat = parser.get(1)
-    assert encFormat == 4
+    if encFormat != 4:
+        raise DecodeError("Not an uncompressed point encoding")
     bytelength = getPointByteSize(curve)
     xCoord = bytesToNumber(parser.getFixBytes(bytelength))
     yCoord = bytesToNumber(parser.getFixBytes(bytelength))
+    if parser.getRemainingLength():
+        raise DecodeError("Invalid length of point encoding for curve")
     return ecdsa.ellipticcurve.Point(curve.curve, xCoord, yCoord)
 
+
 def encodeX962Point(point):
     """Encode a point in X9.62 format"""
     bytelength = numBytes(point.curve().p())