Improve checking of received SSL2 records.

Bug 364319, bug 364323. r=rrelyea, wtchang

1 files changed, 73 insertions, 61 deletions

diff --git a/security/nss/lib/ssl/sslcon.c b/security/nss/lib/ssl/sslcon.c
index 48935671d..1cfe54220 100644
--- a/security/nss/lib/ssl/sslcon.c
+++ b/security/nss/lib/ssl/sslcon.c
@@ -230,7 +230,7 @@ ssl2_ConstructCipherSpecs(sslSocket *ss)
     	(ss->allowedByPolicy & ss->chosenPreference & SSL_CB_IMPLEMENTED);
     for (i = 0; i < ssl2_NUM_SUITES_IMPLEMENTED * 3; i += 3) {
 	const PRUint8 * hs = implementedCipherSuites + i;
-	int             ok = allowed & (1U << hs[0]);
+	unsigned int    ok = allowed & (1U << hs[0]);
 	if (ok) {
 	    cs[0] = hs[0];
 	    cs[1] = hs[1];
@@ -679,7 +679,7 @@ done:
  * Acquires and releases the socket's xmitBufLock.
  */
 static SECStatus
-ssl2_SendSessionKeyMessage(sslSocket *ss, int cipher, int keySize,
+ssl2_SendSessionKeyMessage(sslSocket *ss, int cipher, int keyBits,
 		      PRUint8 *ca, int caLen,
 		      PRUint8 *ck, int ckLen,
 		      PRUint8 *ek, int ekLen)
@@ -704,8 +704,8 @@ ssl2_SendSessionKeyMessage(sslSocket *ss, int cipher, int keySize,
     msg = ss->sec.ci.sendBuf.buf;
     msg[0] = SSL_MT_CLIENT_MASTER_KEY;
     msg[1] = cipher;
-    msg[2] = MSB(keySize);
-    msg[3] = LSB(keySize);
+    msg[2] = MSB(keyBits);
+    msg[3] = LSB(keyBits);
     msg[4] = MSB(ckLen);
     msg[5] = LSB(ckLen);
     msg[6] = MSB(ekLen);
@@ -1579,15 +1579,17 @@ ssl2_ServerSetupSessionCypher(sslSocket *ss, int cipher, unsigned int keyBits,
 			 PRUint8 *ek, unsigned int ekLen,
 			 PRUint8 *ca, unsigned int caLen)
 {
-    PRUint8           *kk = NULL;
+    PRUint8      *    dk   = NULL; /* decrypted master key */
     sslSessionID *    sid;
+    sslServerCerts *  sc   = ss->serverCerts + kt_rsa;
     PRUint8       *   kbuf = 0;	/* buffer for RSA decrypted data. */
-    unsigned int      el1;	/* length of RSA decrypted data in kbuf */
+    unsigned int      ddLen;	/* length of RSA decrypted data in kbuf */
     unsigned int      keySize;
-    unsigned int      modulusLen;
+    unsigned int      dkLen;    /* decrypted key length in bytes */
+    int               modulusLen;
     SECStatus         rv;
+    PRUint16          allowed;  /* cipher kinds enabled and allowed by policy */
     PRUint8           mkbuf[SSL_MAX_MASTER_KEY_BYTES];
-    sslServerCerts  * sc = ss->serverCerts + kt_rsa;
 
     PORT_Assert( ss->opt.noLocks || ssl_Have1stHandshakeLock(ss) );
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss)   );
@@ -1595,18 +1597,6 @@ ssl2_ServerSetupSessionCypher(sslSocket *ss, int cipher, unsigned int keyBits,
     PORT_Assert((ss->sec.ci.sid != 0));
     sid = ss->sec.ci.sid;
 
-    keySize = (keyBits + 7) >> 3;
-    /* Is the message just way too big? */
-    if (keySize > SSL_MAX_MASTER_KEY_BYTES) {
-	/* bummer */
-	SSL_DBG(("%d: SSL[%d]: keySize=%d ckLen=%d max session key size=%d",
-		 SSL_GETPID(), ss->fd, keySize, ckLen,
-		 SSL_MAX_MASTER_KEY_BYTES));
-	PORT_SetError(SSL_ERROR_BAD_CLIENT);
-	goto loser;
-    }
-
-
     /* Trying to cut down on all these switch statements that should be tables.
      * So, test cipherType once, here, and then use tables below. 
      */
@@ -1626,50 +1616,72 @@ ssl2_ServerSetupSessionCypher(sslSocket *ss, int cipher, unsigned int keyBits,
 	goto loser;
     }
 
-    /* For export ciphers, make sure they didn't send too much key data. */
+    allowed = ss->allowedByPolicy & ss->chosenPreference & SSL_CB_IMPLEMENTED;
+    if (!(allowed & (1U << cipher))) {
+    	/* client chose a kind we don't allow! */
+	SSL_DBG(("%d: SSL[%d]: disallowed cipher=%d",
+		 SSL_GETPID(), ss->fd, cipher));
+	PORT_SetError(SSL_ERROR_BAD_CLIENT);
+	goto loser;
+    }
+
+    keySize = ssl_Specs[cipher].keyLen;
+    if (keyBits != keySize * BPB) {
+	SSL_DBG(("%d: SSL[%d]: invalid master secret key length=%d (bits)!",
+		 SSL_GETPID(), ss->fd, keyBits));
+	PORT_SetError(SSL_ERROR_BAD_CLIENT);
+	goto loser;
+    }
+
     if (ckLen != ssl_Specs[cipher].pubLen) {
-	SSL_DBG(("%d: SSL[%d]: odd secret key size, keySize=%d ckLen=%d!",
-		 SSL_GETPID(), ss->fd, keySize, ckLen));
-	/* Somebody tried to sneak by a strange secret key */
+	SSL_DBG(("%d: SSL[%d]: invalid clear key length, ckLen=%d (bytes)!",
+		 SSL_GETPID(), ss->fd, ckLen));
+	PORT_SetError(SSL_ERROR_BAD_CLIENT);
+	goto loser;
+    }
+
+    if (caLen != ssl_Specs[cipher].ivLen) {
+	SSL_DBG(("%d: SSL[%d]: invalid key args length, caLen=%d (bytes)!",
+		 SSL_GETPID(), ss->fd, caLen));
+	PORT_SetError(SSL_ERROR_BAD_CLIENT);
+	goto loser;
+    }
+
+    modulusLen = PK11_GetPrivateModulusLen(sc->SERVERKEY);
+    if (modulusLen == -1) {
+	/* If the key is bad, then PK11_PubDecryptRaw will fail below. */
+	modulusLen = ekLen;
+    }
+    /* RSA modulus size presently limited to 8k bits maximum */
+    if (ekLen > modulusLen || ekLen > 1024 || ekLen + ckLen < keySize) {
+	SSL_DBG(("%d: SSL[%d]: invalid encrypted key length, ekLen=%d (bytes)!",
+		 SSL_GETPID(), ss->fd, ekLen));
 	PORT_SetError(SSL_ERROR_BAD_CLIENT);
 	goto loser;
     }
 
     /* allocate the buffer to hold the decrypted portion of the key. */
-    /* XXX Haven't done any range check on ekLen. */
-    kbuf = (PRUint8*) PORT_Alloc(ekLen);
+    kbuf = (PRUint8*)PORT_Alloc(modulusLen);
     if (!kbuf) {
 	goto loser;
     }
+    dkLen = keySize - ckLen;
+    dk    = kbuf + modulusLen - dkLen;
 
-    /*
-    ** Decrypt encrypted half of the key. Note that encrypted half has
-    ** been made to match the modulus size of our public key using
-    ** PKCS#1. keySize is the real size of the data that is interesting.
+    /* Decrypt encrypted half of the key. 
     ** NOTE: PK11_PubDecryptRaw will barf on a non-RSA key. This is
     ** desired behavior here.
     */
-    rv = PK11_PubDecryptRaw(sc->SERVERKEY, kbuf, &el1, ekLen, ek, ekLen);
+    rv = PK11_PubDecryptRaw(sc->SERVERKEY, kbuf, &ddLen, modulusLen, ek, ekLen);
     if (rv != SECSuccess) 
 	goto hide_loser;
 
-    modulusLen = PK11_GetPrivateModulusLen(sc->SERVERKEY);
-    if (modulusLen == -1) {
-	/* If the key was really bad, then PK11_pubDecryptRaw
-	 * would have failed, therefore the we must assume that the card
-	 * is just being a pain and not giving us the modulus... but it
-	 * should be the same size as the encrypted key length, so use it
-	 * and keep cranking */
-	modulusLen = ekLen;
-    }
-    /* Is the length of the decrypted data (el1) the expected value? */
-    if (modulusLen != el1) 
+    /* Is the length of the decrypted data (ddLen) the expected value? */
+    if (modulusLen != ddLen) 
 	goto hide_loser;
 
     /* Cheaply verify that PKCS#1 was used to format the encryption block */
-    kk = kbuf + modulusLen - (keySize - ckLen);
-    if ((kbuf[0] != 0x00) || (kbuf[1] != 0x02) || (kk[-1] != 0x00)) {
-	/* Tsk tsk. */
+    if ((kbuf[0] != 0x00) || (kbuf[1] != 0x02) || (dk[-1] != 0x00)) {
 	SSL_DBG(("%d: SSL[%d]: strange encryption block",
 		 SSL_GETPID(), ss->fd));
 	PORT_SetError(SSL_ERROR_BAD_CLIENT);
@@ -1678,10 +1690,10 @@ ssl2_ServerSetupSessionCypher(sslSocket *ss, int cipher, unsigned int keyBits,
 
     /* Make sure we're not subject to a version rollback attack. */
     if (ss->opt.enableSSL3 || ss->opt.enableTLS) {
-	PRUint8 threes[8] = { 0x03, 0x03, 0x03, 0x03,
-			      0x03, 0x03, 0x03, 0x03 };
+	static const PRUint8 threes[8] = { 0x03, 0x03, 0x03, 0x03,
+			                   0x03, 0x03, 0x03, 0x03 };
 	
-	if (PORT_Memcmp(kk - 8 - 1, threes, 8) == 0) {
+	if (PORT_Memcmp(dk - 8 - 1, threes, 8) == 0) {
 	    PORT_SetError(SSL_ERROR_BAD_CLIENT);
 	    goto hide_loser;
 	}
@@ -1693,10 +1705,7 @@ hide_loser:
 	 * was erroneous.  Don't send any error messages.
 	 * Instead, Generate a completely bogus master key .
 	 */
-	PK11_GenerateRandom(kbuf, ekLen);
-	if (!kk) {
-	    kk = kbuf + ekLen - (keySize-ckLen);
-	}
+	PK11_GenerateRandom(dk, dkLen);
     }
 
     /*
@@ -1705,7 +1714,7 @@ hide_loser:
     if (ckLen) {
 	PORT_Memcpy(mkbuf, ck, ckLen);
     }
-    PORT_Memcpy(mkbuf+ckLen, kk, keySize-ckLen);
+    PORT_Memcpy(mkbuf + ckLen, dk, dkLen);
 
     /* Fill in session-id */
     rv = ssl2_FillInSID(sid, cipher, mkbuf, keySize, ca, caLen,
@@ -1981,7 +1990,10 @@ ssl_FormatSSL2Block(unsigned modulusLen, SECItem *data)
     SECStatus rv;
     int i;
 
-    PORT_Assert (data->len <= (modulusLen - (3 + RSA_BLOCK_MIN_PAD_LEN)));
+    if (modulusLen < data->len + (3 + RSA_BLOCK_MIN_PAD_LEN)) {
+	PORT_SetError(SEC_ERROR_BAD_KEY);
+    	return NULL;
+    }
     block = (unsigned char *) PORT_Alloc(modulusLen);
     if (block == NULL)
 	return NULL;
@@ -3223,7 +3235,7 @@ ssl2_HandleClientSessionKeyMessage(sslSocket *ss)
     unsigned int     caLen;
     unsigned int     ckLen;
     unsigned int     ekLen;
-    unsigned int     keySize;
+    unsigned int     keyBits;
     int              cipher;
     SECStatus        rv;
 
@@ -3238,13 +3250,13 @@ ssl2_HandleClientSessionKeyMessage(sslSocket *ss)
 	goto bad_client;
     }
     cipher  = data[1];
-    keySize = (data[2] << 8) | data[3];
+    keyBits = (data[2] << 8) | data[3];
     ckLen   = (data[4] << 8) | data[5];
     ekLen   = (data[6] << 8) | data[7];
     caLen   = (data[8] << 8) | data[9];
 
-    SSL_TRC(5, ("%d: SSL[%d]: session-key, cipher=%d keySize=%d ckLen=%d ekLen=%d caLen=%d",
-		SSL_GETPID(), ss->fd, cipher, keySize, ckLen, ekLen, caLen));
+    SSL_TRC(5, ("%d: SSL[%d]: session-key, cipher=%d keyBits=%d ckLen=%d ekLen=%d caLen=%d",
+		SSL_GETPID(), ss->fd, cipher, keyBits, ckLen, ekLen, caLen));
 
     if (ss->gs.recordLen < 
     	    SSL_HL_CLIENT_MASTER_KEY_HBYTES + ckLen + ekLen + caLen) {
@@ -3254,8 +3266,7 @@ ssl2_HandleClientSessionKeyMessage(sslSocket *ss)
     }
 
     /* Use info from client to setup session key */
-    /* XXX should validate cipher&keySize are in our array */
-    rv = ssl2_ServerSetupSessionCypher(ss, cipher, keySize,
+    rv = ssl2_ServerSetupSessionCypher(ss, cipher, keyBits,
 		data + SSL_HL_CLIENT_MASTER_KEY_HBYTES,                 ckLen,
 		data + SSL_HL_CLIENT_MASTER_KEY_HBYTES + ckLen,         ekLen,
 		data + SSL_HL_CLIENT_MASTER_KEY_HBYTES + ckLen + ekLen, caLen);
@@ -3282,7 +3293,8 @@ ssl2_HandleClientSessionKeyMessage(sslSocket *ss)
     }
 
     SSL_TRC(5, ("%d: SSL[%d]: server: waiting for elements=0x%d",
-		SSL_GETPID(), ss->fd, ss->sec.ci.requiredElements ^ ss->sec.ci.elements));
+		SSL_GETPID(), ss->fd, 
+		ss->sec.ci.requiredElements ^ ss->sec.ci.elements));
     ss->handshake         = ssl_GatherRecord1stHandshake;
     ss->nextHandshake     = ssl2_HandleMessage;