Bug 753136 - Fixing ssl3_HandleServerNameXtn, r=ekr

1 files changed, 19 insertions, 20 deletions

diff --git a/lib/ssl/ssl3ext.c b/lib/ssl/ssl3ext.c
index 36608669d..e88b60a77 100644
--- a/lib/ssl/ssl3ext.c
+++ b/lib/ssl/ssl3ext.c
@@ -1,3 +1,4 @@
+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /*
  * SSL3 Protocol
  *
@@ -398,13 +399,7 @@ ssl3_HandleServerNameXtn(sslSocket * ss, PRUint16 ex_type, SECItem *data)
     PRInt32  listLenBytes = 0;
 
     if (!ss->sec.isServer) {
-        /* Verify extension_data is empty. */
-        if (data->data || data->len ||
-            !ssl3_ExtensionNegotiated(ss, ssl_server_name_xtn)) {
-            /* malformed or was not initiated by the client.*/
-            return SECFailure;
-        }
-        return SECSuccess;
+        return SECSuccess; /* ignore extension */
     }
 
     /* Server side - consume client data and register server sender. */
@@ -414,33 +409,38 @@ ssl3_HandleServerNameXtn(sslSocket * ss, PRUint16 ex_type, SECItem *data)
     }
     /* length of server_name_list */
     listLenBytes = ssl3_ConsumeHandshakeNumber(ss, 2, &data->data, &data->len);
-    if (listLenBytes == 0 || listLenBytes != data->len) {
+    if (listLenBytes < 0 || listLenBytes != data->len) {
+        (void)ssl3_DecodeError(ss);
         return SECFailure;
     }
+    if (listLenBytes == 0) {
+        return SECSuccess; /* ignore an empty extension */
+    }
     ldata = *data;
     /* Calculate the size of the array.*/
     while (listLenBytes > 0) {
         SECItem litem;
         SECStatus rv;
-        PRInt32  type;
-        /* Name Type (sni_host_name) */
+        PRInt32 type;
+        /* Skip Name Type (sni_host_name); checks are on the second pass */
         type = ssl3_ConsumeHandshakeNumber(ss, 1, &ldata.data, &ldata.len);
-        if (!ldata.len) {
+        if (type < 0) { /* i.e., SECFailure cast to PRint32 */
             return SECFailure;
         }
         rv = ssl3_ConsumeHandshakeVariable(ss, &litem, 2, &ldata.data, &ldata.len);
         if (rv != SECSuccess) {
-            return SECFailure;
+            return rv;
         }
-        /* Adjust total length for cunsumed item, item len and type.*/
+        /* Adjust total length for consumed item, item len and type.*/
         listLenBytes -= litem.len + 3;
         if (listLenBytes > 0 && !ldata.len) {
+            (void)ssl3_DecodeError(ss);
             return SECFailure;
         }
         listCount += 1;
     }
     if (!listCount) {
-        return SECFailure;
+        return SECFailure;  /* nothing we can act on */
     }
     names = PORT_ZNewArray(SECItem, listCount);
     if (!names) {
@@ -455,6 +455,7 @@ ssl3_HandleServerNameXtn(sslSocket * ss, PRUint16 ex_type, SECItem *data)
         type = ssl3_ConsumeHandshakeNumber(ss, 1, &data->data, &data->len);
         /* Check if we have such type in the list */
         for (j = 0;j < listCount && names[j].data;j++) {
+            /* TODO bug 998524: .type is not assigned a value */
             if (names[j].type == type) {
                 nametypePresent = PR_TRUE;
                 break;
@@ -464,7 +465,10 @@ ssl3_HandleServerNameXtn(sslSocket * ss, PRUint16 ex_type, SECItem *data)
         rv = ssl3_ConsumeHandshakeVariable(ss, &names[namesPos], 2,
                                            &data->data, &data->len);
         if (rv != SECSuccess) {
-            goto loser;
+            PORT_Assert(0);
+            PORT_Free(names);
+            PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
+            return rv;
         }
         if (nametypePresent == PR_FALSE) {
             namesPos += 1;
@@ -479,10 +483,6 @@ ssl3_HandleServerNameXtn(sslSocket * ss, PRUint16 ex_type, SECItem *data)
     xtnData->negotiated[xtnData->numNegotiated++] = ssl_server_name_xtn;
 
     return SECSuccess;
-
-loser:
-    PORT_Free(names);
-    return SECFailure;
 }
 
 /* Called by both clients and servers.
@@ -2520,4 +2520,3 @@ loser:
 
     return SECSuccess;
 }
-