Bug 1260046 - check for invalid ecc points in ssl handshake, r=mt

2 files changed, 81 insertions, 6 deletions

diff --git a/external_tests/ssl_gtest/ssl_loopback_unittest.cc b/external_tests/ssl_gtest/ssl_loopback_unittest.cc
index db246d89a..86ae2a8c6 100644
--- a/external_tests/ssl_gtest/ssl_loopback_unittest.cc
+++ b/external_tests/ssl_gtest/ssl_loopback_unittest.cc
@@ -4,6 +4,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+#include "secerr.h"
 #include "ssl.h"
 #include "sslerr.h"
 #include "sslproto.h"
@@ -1112,6 +1113,64 @@ TEST_P(TlsConnectGenericPre13, AuthCompleteBeforeFinishedWithFalseStart) {
   Receive(10);
 }
 
+// Replace the point in the client key exchange message with an empty one
+class ECCClientKEXFilter : public TlsHandshakeFilter {
+public:
+  ECCClientKEXFilter() {}
+
+protected:
+  virtual PacketFilter::Action FilterHandshake(const HandshakeHeader &header,
+                                               const DataBuffer &input,
+                                               DataBuffer *output) {
+    if (header.handshake_type() != kTlsHandshakeClientKeyExchange) {
+      return KEEP;
+    }
+
+    // Replace the client key exchange message with an empty point
+    output->Allocate(1);
+    output->Write(0, 0U, 1); // set point length 0
+    return CHANGE;
+  }
+};
+
+// Replace the point in the server key exchange message with an empty one
+class ECCServerKEXFilter : public TlsHandshakeFilter {
+public:
+  ECCServerKEXFilter() {}
+
+protected:
+  virtual PacketFilter::Action FilterHandshake(const HandshakeHeader &header,
+                                               const DataBuffer &input,
+                                               DataBuffer *output) {
+    if (header.handshake_type() != kTlsHandshakeServerKeyExchange) {
+      return KEEP;
+    }
+
+    // Replace the server key exchange message with an empty point
+    output->Allocate(4);
+    output->Write(0, 3U, 1); // named curve
+    uint32_t curve;
+    EXPECT_TRUE(input.Read(1, 2, &curve)); // get curve id
+    output->Write(1, curve, 2); // write curve id
+    output->Write(3, 0U, 1); // point length 0
+    return CHANGE;
+  }
+};
+
+TEST_P(TlsConnectGenericPre13, ConnectECDHEmptyServerPoint) {
+  // add packet filter
+  server_->SetPacketFilter(new ECCServerKEXFilter());
+  ConnectExpectFail();
+  client_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_SERVER_KEY_EXCH);
+}
+
+TEST_P(TlsConnectGenericPre13, ConnectECDHEmptyClientPoint) {
+  // add packet filter
+  client_->SetPacketFilter(new ECCClientKEXFilter());
+  ConnectExpectFail();
+  server_->CheckErrorCode(SSL_ERROR_RX_MALFORMED_CLIENT_KEY_EXCH);
+}
+
 INSTANTIATE_TEST_CASE_P(GenericStream, TlsConnectGeneric,
                         ::testing::Combine(
                           TlsConnectTestBase::kTlsModesStream,
diff --git a/lib/ssl/ssl3ecc.c b/lib/ssl/ssl3ecc.c
index c8e9d06b5..a28362773 100644
--- a/lib/ssl/ssl3ecc.c
+++ b/lib/ssl/ssl3ecc.c
@@ -424,6 +424,7 @@ ssl3_HandleECDHClientKeyExchange(sslSocket *ss, SSL3Opaque *b,
     SECKEYPublicKey clntPubKey;
     CK_MECHANISM_TYPE target;
     PRBool isTLS, isTLS12;
+    int errCode = SSL_ERROR_RX_MALFORMED_CLIENT_KEY_EXCH;
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
@@ -437,8 +438,15 @@ ssl3_HandleECDHClientKeyExchange(sslSocket *ss, SSL3Opaque *b,
     rv = ssl3_ConsumeHandshakeVariable(ss, &clntPubKey.u.ec.publicValue,
                                        1, &b, &length);
     if (rv != SECSuccess) {
-        SEND_ALERT
-        return SECFailure; /* XXX Who sets the error code?? */
+        PORT_SetError(errCode);
+        return SECFailure;
+    }
+
+    // we have to catch the case when the client's public key has length 0
+    if (!clntPubKey.u.ec.publicValue.len) {
+        (void)SSL3_SendAlert(ss, alert_fatal, illegal_parameter);
+        PORT_SetError(errCode);
+        return SECFailure;
     }
 
     isTLS = (PRBool)(ss->ssl3.prSpec->version > SSL_LIBRARY_VERSION_3_0);
@@ -459,15 +467,16 @@ ssl3_HandleECDHClientKeyExchange(sslSocket *ss, SSL3Opaque *b,
 
     if (pms == NULL) {
         /* last gasp.  */
-        ssl_MapLowLevelError(SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE);
+        errCode = ssl_MapLowLevelError(SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE);
+        PORT_SetError(errCode);
         return SECFailure;
     }
 
     rv = ssl3_InitPendingCipherSpec(ss, pms);
     PK11_FreeSymKey(pms);
     if (rv != SECSuccess) {
-        SEND_ALERT
-        return SECFailure; /* error code set by ssl3_InitPendingCipherSpec */
+        /* error code set by ssl3_InitPendingCipherSpec */
+        return SECFailure;
     }
     return SECSuccess;
 }
@@ -786,7 +795,14 @@ ssl3_HandleECDHServerKeyExchange(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
     if (rv != SECSuccess) {
         goto loser; /* malformed. */
     }
-    /* Fail if the ec point uses compressed representation */
+
+    /* Fail if the provided point has length 0. */
+    if (!ec_point.len) {
+        /* desc and errCode are initialized already */
+        goto alert_loser;
+    }
+
+    /* Fail if the ec point uses compressed representation. */
     if (ec_point.data[0] != EC_POINT_FORM_UNCOMPRESSED) {
         errCode = SEC_ERROR_UNSUPPORTED_EC_POINT_FORM;
         desc = handshake_failure;