Add DHE_RSA support to tlslite.

third_party/tlslite/tlslite/tlsconnection.py

Index: third_party/tlslite/tlslite/tlsconnection.py
diff --git a/third_party/tlslite/tlslite/tlsconnection.py b/third_party/tlslite/tlslite/tlsconnection.py
index 20cd85bc90f31b456ec45c5d23a746123b962346..13e82eacc8b2df71f124c39d95a038bdfd7cbe61 100644
--- a/third_party/tlslite/tlslite/tlsconnection.py
+++ b/third_party/tlslite/tlslite/tlsconnection.py
@@ -24,6 +24,90 @@ from .handshakesettings import HandshakeSettings
 from .utils.tackwrapper import *
 
 
+class KeyExchange(object):
+    def __init__(self, cipherSuite, clientHello, serverHello, privateKey):

[wtc, 2014/04/02 19:11:29]

Nit: it would be nice to document what privateKey is. This is only ambiguous
when the server has two key pairs. In that case I think this privateKey should
be the DH private key rather than the signing private key.

[wtc, 2014/04/02 19:13:46]

Sorry, I forgot to update this comment. I believe in that case this privateKey
should be the signing private key.

[davidben, 2014/04/03 18:45:48]

Done.

+        self.cipherSuite = cipherSuite
+        self.clientHello = clientHello
+        self.serverHello = serverHello
+        self.privateKey = privateKey
+
+    def makeServerKeyExchange():

[wtc, 2014/04/02 19:11:29]

Nit: document what this method returns, and what it means for this method to
return None.

[davidben, 2014/04/03 18:45:48]

Done.

+        raise NotImplementedError()
+    def processClientKeyExchange(clientKeyExchange):

[wtc, 2014/04/02 19:11:29]

Nit: document what this method returns (the premaster secret).

[davidben, 2014/04/03 18:45:48]

Done.

+        raise NotImplementedError()
+
+

[wtc, 2014/04/02 19:11:29]

Nit: use only one blank line here and before line 27?

[davidben, 2014/04/03 18:45:48]

Done. Although this code doesn't seem to be very consistent. :-)

+class RSAKeyExchange(KeyExchange):
+    def makeServerKeyExchange(self):
+        return None
+
+    def processClientKeyExchange(self, clientKeyExchange):
+        premasterSecret = self.privateKey.decrypt(\
+            clientKeyExchange.encryptedPreMasterSecret)
+
+        # On decryption failure randomize premaster secret to avoid
+        # Bleichenbacher's "million message" attack
+        randomPreMasterSecret = getRandomBytes(48)
+        versionCheck = (premasterSecret[0], premasterSecret[1])
+        if not premasterSecret:

[wtc, 2014/04/02 19:11:29]

Hmm... On line 51 we already dereferenced premasterSecret[0] and
premasterSecret[1], before checking premasterSecret is not null and has at least
two elements. This seems wrong.

It seems that the code should read:

    # On decryption failure randomize premaster secret to avoid
    # Bleichenbacher's "million message" attack
    randomPreMasterSecret = getRandomBytes(48)
    if not premasterSecret:
        premasterSecret = randomPreMasterSecret
    elif len(premasterSecret)!=48:
        premasterSecret = randomPreMasterSecret
    else:
        versionCheck = (premasterSecret[0], premasterSecret[1])
        if versionCheck != self.clientHello.client_version:
            #Tolerate buggy IE clients
            if versionCheck != self.serverHello.server_version:
                premasterSecret = randomPreMasterSecret
    return premasterSecret

[davidben, 2014/04/03 18:45:48]

Haha. I guess that code never actually worked then. Done.

+            premasterSecret = randomPreMasterSecret
+        elif len(premasterSecret)!=48:
+            premasterSecret = randomPreMasterSecret
+        elif versionCheck != self.clientHello.client_version:
+            #Tolerate buggy IE clients
+            if versionCheck != self.serverHello.server_version:
+                premasterSecret = randomPreMasterSecret
+        return premasterSecret
+
+def _hexStringToNumber(s):
+    s = s.replace(" ", "").replace("\n", "")
+    if len(s) % 2 != 0:
+        raise ValueError("Length is not even")
+    return bytesToNumber(bytearray(s.decode("hex")))
+
+class DHE_RSAKeyExchange(KeyExchange):
+    # 2048-bit MODP Group (RFC 3526, Section 3)
+    dh_p = _hexStringToNumber("""
+FFFFFFFF FFFFFFFF C90FDAA2 2168C234 C4C6628B 80DC1CD1
+29024E08 8A67CC74 020BBEA6 3B139B22 514A0879 8E3404DD
+EF9519B3 CD3A431B 302B0A6D F25F1437 4FE1356D 6D51C245
+E485B576 625E7EC6 F44C42E9 A637ED6B 0BFF5CB6 F406B7ED
+EE386BFB 5A899FA5 AE9F2411 7C4B1FE6 49286651 ECE45B3D
+C2007CB8 A163BF05 98DA4836 1C55D39A 69163FA8 FD24CF5F
+83655D23 DCA3AD96 1C62F356 208552BB 9ED52907 7096966D
+670C354E 4ABC9804 F1746C08 CA18217C 32905E46 2E36CE3B
+E39E772C 180E8603 9B2783A2 EC07A28F B5C55DF0 6F4C52C9
+DE2BCBF6 95581718 3995497C EA956AE5 15D22618 98FA0510
+15728E5A 8AACAA68 FFFFFFFF FFFFFFFF""")

[wtc, 2014/04/02 19:11:29]

Nit: should lines 71-81 be indented?

[davidben, 2014/04/03 18:45:48]

*shrug* It does actually change what's in the string, but it doesn't really
matter I guess; _hexStringToNumber is going to strip it all away anyway. But I
suppose if I wanted to be really faithful to the string in the RFC, lines 70 and
71 should be merged.

+    dh_g = 2
+
+    # RFC 3526, Section 8.
+    strength = 160
+
+    def makeServerKeyExchange(self):
+        # Per RFC 3526, Section 1, the exponent should have double the entropy
+        # of the strength of the curve.
+        self.dh_Xs = bytesToNumber(getRandomBytes(self.strength * 2 / 8))
+        dh_Ys = powMod(self.dh_g, self.dh_Xs, self.dh_p)
+
+        serverKeyExchange = ServerKeyExchange(self.cipherSuite)
+        serverKeyExchange.createDH(self.dh_p, self.dh_g, dh_Ys)
+        serverKeyExchange.signature = self.privateKey.sign(
+            serverKeyExchange.hash(self.clientHello.random,
+                                   self.serverHello.random))
+        return serverKeyExchange
+
+    def processClientKeyExchange(self, clientKeyExchange):
+        dh_Yc = clientKeyExchange.dh_Yc
+
+        # First half of RFC 2631, Section 2.1.5.

[wtc, 2014/04/02 19:11:29]

Nit: Add "Validate the client's public key."

This DH group doesn't have a 'q' (subprime) parameter, so we can't do the second
half of the public key validation. Perhaps that check is not necessary for this
group.

[davidben, 2014/04/03 18:45:48]

Done.

+        if not 2 <= dh_Yc <= self.dh_p - 1:
+            raise TLSLocalAlert(AlertDescription.illegal_parameter,
+                                "Suspicious dh_Yc value")

[wtc, 2014/04/02 19:11:29]

Nit: Suspicious => Invalid

[davidben, 2014/04/03 18:45:48]

Done.

+
+        S = powMod(dh_Yc, self.dh_Xs, self.dh_p)
+        return numberToByteArray(S)
+
 class TLSConnection(TLSRecordLayer):
     """
     This class wraps a socket and provides TLS handshaking and data
@@ -500,6 +584,8 @@ class TLSConnection(TLSRecordLayer):
             cipherSuites += CipherSuite.getSrpAllSuites(settings)
         elif certParams:
             cipherSuites += CipherSuite.getCertSuites(settings)
+            # Client DHE_RSA not supported.

[wtc, 2014/04/02 19:11:29]

Nit: Add "TODO" to this comment.

[davidben, 2014/04/03 18:45:48]

Done.

+            # cipherSuites += CipherSuite.getDheCertSuites(settings)
         elif anonParams:
             cipherSuites += CipherSuite.getAnonSuites(settings)
         else:
@@ -1204,10 +1290,23 @@ class TLSConnection(TLSRecordLayer):
                 else: break
             premasterSecret = result
 
-        # Perform the RSA key exchange
-        elif cipherSuite in CipherSuite.certSuites:
+        # Perform the RSA or DHE_RSA key exchange
+        elif (cipherSuite in CipherSuite.certSuites or
+              cipherSuite in CipherSuite.dheCertSuites):
+            if cipherSuite in CipherSuite.certSuites:
+                keyExchange = RSAKeyExchange(cipherSuite,
+                                             clientHello,
+                                             serverHello,
+                                             privateKey)
+            elif cipherSuite in CipherSuite.dheCertSuites:
+                keyExchange = DHE_RSAKeyExchange(cipherSuite,
+                                                 clientHello,
+                                                 serverHello,
+                                                 privateKey)
+            else:
+                assert(False)
             for result in self._serverCertKeyExchange(clientHello, serverHello, 
-                                        certChain, privateKey,
+                                        certChain, keyExchange,
                                         reqCert, reqCAs, cipherSuite,
                                         settings, ocspResponse):
                 if result in (0,1): yield result
@@ -1268,6 +1367,7 @@ class TLSConnection(TLSRecordLayer):
             cipherSuites += CipherSuite.getSrpSuites(settings)
         elif certChain:
             cipherSuites += CipherSuite.getCertSuites(settings)
+            cipherSuites += CipherSuite.getDheCertSuites(settings)
         elif anon:
             cipherSuites += CipherSuite.getAnonSuites(settings)
         else:
@@ -1483,11 +1583,11 @@ class TLSConnection(TLSRecordLayer):
 
 
     def _serverCertKeyExchange(self, clientHello, serverHello, 
-                                serverCertChain, privateKey,
+                                serverCertChain, keyExchange,
                                 reqCert, reqCAs, cipherSuite,
                                 settings, ocspResponse):
-        #Send ServerHello, Certificate[, CertificateRequest],
-        #ServerHelloDone
+        #Send ServerHello, Certificate[, ServerKeyExchange]
+        #[, CertificateRequest], ServerHelloDone
         msgs = []
 
         # If we verify a client cert chain, return it
@@ -1497,6 +1597,9 @@ class TLSConnection(TLSRecordLayer):
         msgs.append(Certificate(CertificateType.x509).create(serverCertChain))
         if serverHello.status_request:
             msgs.append(CertificateStatus().create(ocspResponse))
+        serverKeyExchange = keyExchange.makeServerKeyExchange()

[wtc, 2014/04/02 19:11:29]

Nit: the function call you use to create the ServerKeyExchange message is
different from how the other messages are created. The other messages are all
created with a .create() method call on the message class.

I don't know how hard it is to follow that pattern.

[davidben, 2014/04/03 18:45:48]

Hrm. Yeah, I'm not sure we can follow that pattern very well and still allow
keyExchange to decide whether or not to send a ServerKeyExchange. I didn't want
to duplicate _serverCertKeyExchange for every key exchange mechanism; if
anything, probably the other _server*KeyExchange functions should get adapted to
that interface. The constructor probably will need changing through.

+        if serverKeyExchange is not None:
+            msgs.append(serverKeyExchange)
         if reqCert and reqCAs:
             msgs.append(CertificateRequest().create(\
                 [ClientCertificateType.rsa_sign], reqCAs))
@@ -1555,21 +1658,13 @@ class TLSConnection(TLSRecordLayer):
             else: break
         clientKeyExchange = result
 
-        #Decrypt ClientKeyExchange
-        premasterSecret = privateKey.decrypt(\
-            clientKeyExchange.encryptedPreMasterSecret)
-
-        # On decryption failure randomize premaster secret to avoid
-        # Bleichenbacher's "million message" attack
-        randomPreMasterSecret = getRandomBytes(48)
-        versionCheck = (premasterSecret[0], premasterSecret[1])
-        if not premasterSecret:
-            premasterSecret = randomPreMasterSecret
-        elif len(premasterSecret)!=48:
-            premasterSecret = randomPreMasterSecret
-        elif versionCheck != clientHello.client_version:
-            if versionCheck != self.version: #Tolerate buggy IE clients
-                premasterSecret = randomPreMasterSecret
+        #Process ClientKeyExchange
+        try:
+            premasterSecret = \
+                keyExchange.processClientKeyExchange(clientKeyExchange)
+        except TLSLocalAlert, e:
+            for result in self._sendError(e.description, e.message):

[wtc, 2014/04/02 19:11:29]

Nit: rename |e| to |alert|?

[davidben, 2014/04/03 18:45:48]

Done.

+                yield result
 
         #Get and check CertificateVerify, if relevant
         if clientCertChain: