Merge remote-tracking branch 'hanno/iotssl-1341-optional-certificate-verification-needs-ca-chain_backport-1.3' into mbedtls-1.3

* hanno/iotssl-1341-optional-certificate-verification-needs-ca-chain_backport-1.3:
  Add tests for missing CA chains and bad curves.
  Fix implementation of VERIFY_OPTIONAL verification mode

ChangeLog

@@ -15,6 +15,12 @@ Bugfix
    * Wipe stack buffers in RSA private key operations
      (rsa_rsaes_pkcs1_v15_decrypt(), rsa_rsaes_oaep_decrypt).
      Found by Laurent Simon.
+   * Accept empty trusted CA chain in authentication mode
+     SSL_VERIFY_OPTIONAL. Fixes #864. Found by jethrogb.
+   * Fix implementation of ssl_parse_certificate
+     to not annihilate fatal errors in authentication mode
+     SSL_VERIFY_OPTIONAL and to reflect bad EC curves
+     within verification result.
 
 = mbed TLS 1.3.19 branch released 2017-03-08
 

include/polarssl/x509.h

@@ -97,6 +97,8 @@
 #define BADCERT_KEY_USAGE         0x0800  /**< Usage does not match the keyUsage extension. */
 #define BADCERT_EXT_KEY_USAGE     0x1000  /**< Usage does not match the extendedKeyUsage extension. */
 #define BADCERT_NS_CERT_TYPE      0x2000  /**< Usage does not match the nsCertType extension. */
+#define BADCERT_BAD_KEY          0x10000  /**< Bad key (e.g. unsupported elliptic curve in use) */
+
 /* \} name */
 /* \} addtogroup x509_module */
 

library/ssl_tls.c

@@ -2841,12 +2841,6 @@ int ssl_parse_certificate( ssl_context *ssl )
 
     if( ssl->authmode != SSL_VERIFY_NONE )
     {
-        if( ssl->ca_chain == NULL )
-        {
-            SSL_DEBUG_MSG( 1, ( "got no CA chain" ) );
-            return( POLARSSL_ERR_SSL_CA_CHAIN_REQUIRED );
-        }
-
         /*
          * Main check: verify certificate
          */
@@ -2872,6 +2866,8 @@ int ssl_parse_certificate( ssl_context *ssl )
             if( pk_can_do( pk, POLARSSL_PK_ECKEY ) &&
                 ! ssl_curve_is_acceptable( ssl, pk_ec( *pk )->grp.id ) )
             {
+                ssl->session_negotiate->verify_result |= BADCERT_BAD_KEY;
+
                 SSL_DEBUG_MSG( 1, ( "bad certificate (EC key curve)" ) );
                 if( ret == 0 )
                     ret = POLARSSL_ERR_SSL_BAD_HS_CERTIFICATE;
@@ -2889,8 +2885,36 @@ int ssl_parse_certificate( ssl_context *ssl )
                 ret = POLARSSL_ERR_SSL_BAD_HS_CERTIFICATE;
         }
 
-        if( ssl->authmode != SSL_VERIFY_REQUIRED )
+        /* x509_crt_verify_with_profile is supposed to report a
+         * verification failure through POLARSSL_ERR_X509_CERT_VERIFY_FAILED,
+         * with details encoded in the verification flags. All other kinds
+         * of error codes, including those from the user provided f_vrfy
+         * functions, are treated as fatal and lead to a failure of
+         * ssl_parse_certificate even if verification was optional. */
+        if( ssl->authmode == SSL_VERIFY_OPTIONAL &&
+            ( ret == POLARSSL_ERR_X509_CERT_VERIFY_FAILED ||
+              ret == POLARSSL_ERR_SSL_BAD_HS_CERTIFICATE ) )
+        {
             ret = 0;
+        }
+
+        if( ssl->ca_chain == NULL && ssl->authmode == SSL_VERIFY_REQUIRED )
+        {
+            SSL_DEBUG_MSG( 1, ( "got no CA chain" ) );
+            ret = POLARSSL_ERR_SSL_CA_CHAIN_REQUIRED;
+        }
+
+#if defined(POLARSSL_DEBUG_C)
+        if( ssl->session_negotiate->verify_result != 0 )
+        {
+            SSL_DEBUG_MSG( 3, ( "! Certificate verification flags %x",
+                                ssl->session_negotiate->verify_result ) );
+        }
+        else
+        {
+            SSL_DEBUG_MSG( 3, ( "Certificate verification flags clear" ) );
+        }
+#endif /* POLARSSL_DEBUG_C */
     }
 
     SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) );

library/x509_crt.c

@@ -1403,6 +1403,7 @@ static const struct x509_crt_verify_string x509_crt_verify_strings[] = {
     { BADCERT_KEY_USAGE,     "Usage does not match the keyUsage extension" },
     { BADCERT_EXT_KEY_USAGE, "Usage does not match the extendedKeyUsage extension" },
     { BADCERT_NS_CERT_TYPE,  "Usage does not match the nsCertType extension" },
+    { BADCERT_BAD_KEY,       "The certificate uses an invalid key (e.g. unsupported elliptic curve)" },
     { 0, NULL }
 };
 

programs/ssl/ssl_client2.c

@@ -92,6 +92,7 @@
 #define DFL_RECO_DELAY          0
 #define DFL_TICKETS             SSL_SESSION_TICKETS_ENABLED
 #define DFL_ALPN_STRING         NULL
+#define DFL_CURVES              NULL
 #define DFL_FALLBACK            -1
 #define DFL_EXTENDED_MS         -1
 #define DFL_ETM                 -1
@@ -169,6 +170,17 @@
 #define USAGE_ALPN ""
 #endif /* POLARSSL_SSL_ALPN */
 
+#if defined(POLARSSL_SSL_SET_CURVES)
+#define USAGE_CURVES \
+    "    curves=a,b,c,d      default: \"default\" (library default)\n"  \
+    "                        example: \"secp521r1,brainpoolP512r1\"\n"  \
+    "                        - use \"none\" for empty list\n"           \
+    "                        - see ecp_curve_list()\n"                  \
+    "                          for acceptable curve names\n"
+#else
+#define USAGE_CURVES ""
+#endif /* POLARSSL_SSL_SET_CURVES */
+
 #if defined(POLARSSL_SSL_FALLBACK_SCSV)
 #define USAGE_FALLBACK \
     "    fallback=0/1        default: (library default: off)\n"
@@ -226,6 +238,7 @@
     USAGE_MAX_FRAG_LEN                                      \
     USAGE_TRUNC_HMAC                                        \
     USAGE_ALPN                                              \
+    USAGE_CURVES                                            \
     USAGE_FALLBACK                                          \
     USAGE_EMS                                               \
     USAGE_ETM                                               \
@@ -285,6 +298,7 @@ struct options
     int reconnect;              /* attempt to resume session                */
     int reco_delay;             /* delay in seconds before resuming session */
     int tickets;                /* enable / disable session tickets         */
+    const char *curves;         /* list of supported elliptic curves        */
     const char *alpn_string;    /* ALPN supported protocols                 */
     int fallback;               /* is this a fallback connection?           */
     int extended_ms;            /* negotiate extended master secret?        */
@@ -373,6 +387,11 @@ int main( int argc, char *argv[] )
 #if defined(POLARSSL_SSL_ALPN)
     const char *alpn_list[10];
 #endif
+#if defined(POLARSSL_SSL_SET_CURVES)
+    ecp_group_id curve_list[20];
+    const ecp_curve_info *curve_cur;
+#endif
+
     const char *pers = "ssl_client2";
 
     entropy_context entropy;
@@ -453,6 +472,7 @@ int main( int argc, char *argv[] )
     opt.reco_delay          = DFL_RECO_DELAY;
     opt.tickets             = DFL_TICKETS;
     opt.alpn_string         = DFL_ALPN_STRING;
+    opt.curves              = DFL_CURVES;
     opt.fallback            = DFL_FALLBACK;
     opt.extended_ms         = DFL_EXTENDED_MS;
     opt.etm                 = DFL_ETM;
@@ -584,6 +604,8 @@ int main( int argc, char *argv[] )
                 default: goto usage;
             }
         }
+        else if( strcmp( p, "curves" ) == 0 )
+            opt.curves = q;
         else if( strcmp( p, "etm" ) == 0 )
         {
             switch( atoi( q ) )
@@ -775,6 +797,64 @@ int main( int argc, char *argv[] )
     }
 #endif /* POLARSSL_KEY_EXCHANGE__SOME__PSK_ENABLED */
 
+#if defined(POLARSSL_SSL_SET_CURVES)
+    if( opt.curves != NULL )
+    {
+        p = (char *) opt.curves;
+        i = 0;
+
+        if( strcmp( p, "none" ) == 0 )
+        {
+            curve_list[0] = POLARSSL_ECP_DP_NONE;
+        }
+        else if( strcmp( p, "default" ) != 0 )
+        {
+            /* Leave room for a final NULL in curve list */
+            while( i < (int) ( sizeof( curve_list ) / sizeof( *curve_list ) ) - 1
+                   && *p != '\0' )
+            {
+                q = p;
+
+                /* Terminate the current string */
+                while( *p != ',' && *p != '\0' )
+                    p++;
+                if( *p == ',' )
+                    *p++ = '\0';
+
+                if( ( curve_cur = ecp_curve_info_from_name( q ) ) != NULL )
+                {
+                    curve_list[i++] = curve_cur->grp_id;
+                }
+                else
+                {
+                    polarssl_printf( "unknown curve %s\n", q );
+                    polarssl_printf( "supported curves: " );
+                    for( curve_cur = ecp_curve_list();
+                         curve_cur->grp_id != POLARSSL_ECP_DP_NONE;
+                         curve_cur++ )
+                    {
+                        polarssl_printf( "%s ", curve_cur->name );
+                    }
+                    polarssl_printf( "\n" );
+                    goto exit;
+                }
+            }
+
+            polarssl_printf( "Number of curves: %d\n", i );
+
+            if( i == (int) ( sizeof( curve_list ) / sizeof( *curve_list ) ) - 1
+                && *p != '\0' )
+            {
+                polarssl_printf( "curves list too long, maximum %zu",
+                                 (size_t) ( sizeof( curve_list ) / sizeof( *curve_list ) - 1 ) );
+                goto exit;
+            }
+
+            curve_list[i] = POLARSSL_ECP_DP_NONE;
+        }
+    }
+#endif /* POLARSSL_SSL_SET_CURVES */
+
 #if defined(POLARSSL_SSL_ALPN)
     if( opt.alpn_string != NULL )
     {
@@ -996,6 +1076,14 @@ int main( int argc, char *argv[] )
         }
 #endif
 
+#if defined(POLARSSL_SSL_SET_CURVES)
+    if( opt.curves != NULL &&
+        strcmp( opt.curves, "default" ) != 0 )
+    {
+        ssl_set_curves( &ssl, curve_list );
+    }
+#endif
+
     ssl_set_rng( &ssl, ctr_drbg_random, &ctr_drbg );
     ssl_set_dbg( &ssl, my_debug, stdout );
 

programs/ssl/ssl_server2.c

@@ -106,6 +106,7 @@
 #define DFL_CACHE_TIMEOUT       -1
 #define DFL_SNI                 NULL
 #define DFL_ALPN_STRING         NULL
+#define DFL_CURVES              NULL
 #define DFL_DHM_FILE            NULL
 #define DFL_EXTENDED_MS         -1
 #define DFL_ETM                 -1
@@ -215,6 +216,17 @@
 #define USAGE_ALPN ""
 #endif /* POLARSSL_SSL_ALPN */
 
+#if defined(POLARSSL_SSL_SET_CURVES)
+#define USAGE_CURVES \
+    "    curves=a,b,c,d      default: \"default\" (library default)\n"  \
+    "                        example: \"secp521r1,brainpoolP512r1\"\n"  \
+    "                        - use \"none\" for empty list\n"           \
+    "                        - see ecp_curve_list()\n"                  \
+    "                          for acceptable curve names\n"
+#else
+#define USAGE_CURVES ""
+#endif /* POLARSSL_SSL_SET_CURVES */
+
 #if defined(POLARSSL_SSL_EXTENDED_MASTER_SECRET)
 #define USAGE_EMS \
     "    extended_ms=0/1     default: (library default: on)\n"
@@ -263,6 +275,7 @@
     USAGE_MAX_FRAG_LEN                                      \
     USAGE_TRUNC_HMAC                                        \
     USAGE_ALPN                                              \
+    USAGE_CURVES                                            \
     USAGE_EMS                                               \
     USAGE_ETM                                               \
     "\n"                                                    \
@@ -327,6 +340,7 @@ struct options
     int cache_max;              /* max number of session cache entries      */
     int cache_timeout;          /* expiration delay of session cache entries */
     char *sni;                  /* string describing sni information        */
+    const char *curves;         /* list of supported elliptic curves        */
     const char *alpn_string;    /* ALPN supported protocols                 */
     const char *dhm_file;       /* the file with the DH parameters          */
     int extended_ms;            /* allow negotiation of extended MS?        */
@@ -685,6 +699,10 @@ int main( int argc, char *argv[] )
 #if defined(POLARSSL_SSL_ALPN)
     const char *alpn_list[10];
 #endif
+#if defined(POLARSSL_SSL_SET_CURVES)
+    ecp_group_id curve_list[20];
+    const ecp_curve_info *curve_cur;
+#endif
 #if defined(POLARSSL_MEMORY_BUFFER_ALLOC_C)
     unsigned char alloc_buf[100000];
 #endif
@@ -780,6 +798,7 @@ int main( int argc, char *argv[] )
     opt.cache_timeout       = DFL_CACHE_TIMEOUT;
     opt.sni                 = DFL_SNI;
     opt.alpn_string         = DFL_ALPN_STRING;
+    opt.curves              = DFL_CURVES;
     opt.dhm_file            = DFL_DHM_FILE;
     opt.extended_ms         = DFL_EXTENDED_MS;
     opt.etm                 = DFL_ETM;
@@ -987,6 +1006,8 @@ int main( int argc, char *argv[] )
                 default: goto usage;
             }
         }
+        else if( strcmp( p, "curves" ) == 0 )
+            opt.curves = q;
         else if( strcmp( p, "etm" ) == 0 )
         {
             switch( atoi( q ) )
@@ -1118,6 +1139,64 @@ int main( int argc, char *argv[] )
     }
 #endif /* POLARSSL_KEY_EXCHANGE__SOME__PSK_ENABLED */
 
+#if defined(POLARSSL_SSL_SET_CURVES)
+    if( opt.curves != NULL )
+    {
+        p = (char *) opt.curves;
+        i = 0;
+
+        if( strcmp( p, "none" ) == 0 )
+        {
+            curve_list[0] = POLARSSL_ECP_DP_NONE;
+        }
+        else if( strcmp( p, "default" ) != 0 )
+        {
+            /* Leave room for a final NULL in curve list */
+            while( i < (int) ( sizeof( curve_list ) / sizeof( *curve_list ) ) - 1
+                   && *p != '\0' )
+            {
+                q = p;
+
+                /* Terminate the current string */
+                while( *p != ',' && *p != '\0' )
+                    p++;
+                if( *p == ',' )
+                    *p++ = '\0';
+
+                if( ( curve_cur = ecp_curve_info_from_name( q ) ) != NULL )
+                {
+                    curve_list[i++] = curve_cur->grp_id;
+                }
+                else
+                {
+                    polarssl_printf( "unknown curve %s\n", q );
+                    polarssl_printf( "supported curves: " );
+                    for( curve_cur = ecp_curve_list();
+                         curve_cur->grp_id != POLARSSL_ECP_DP_NONE;
+                         curve_cur++ )
+                    {
+                        polarssl_printf( "%s ", curve_cur->name );
+                    }
+                    polarssl_printf( "\n" );
+                    goto exit;
+                }
+            }
+
+            polarssl_printf( "Number of curves: %d\n", i );
+
+            if( i == (int) ( sizeof( curve_list ) / sizeof( *curve_list ) ) - 1
+                && *p != '\0' )
+            {
+                polarssl_printf( "curves list too long, maximum %zu",
+                                 (size_t) ( sizeof( curve_list ) / sizeof( *curve_list ) - 1 ) );
+                goto exit;
+            }
+
+            curve_list[i] = POLARSSL_ECP_DP_NONE;
+        }
+    }
+#endif /* POLARSSL_SSL_SET_CURVES */
+
 #if defined(POLARSSL_SSL_ALPN)
     if( opt.alpn_string != NULL )
     {
@@ -1397,6 +1476,14 @@ int main( int argc, char *argv[] )
         }
 #endif
 
+#if defined(POLARSSL_SSL_SET_CURVES)
+    if( opt.curves != NULL &&
+        strcmp( opt.curves, "default" ) != 0 )
+    {
+        ssl_set_curves( &ssl, curve_list );
+    }
+#endif
+
     ssl_set_rng( &ssl, ctr_drbg_random, &ctr_drbg );
     ssl_set_dbg( &ssl, my_debug, stdout );
 

tests/ssl-opt.sh

@@ -1432,6 +1432,54 @@ run_test    "Authentication: server badcert, client optional" \
             -C "! ssl_handshake returned" \
             -C "X509 - Certificate verification failed"
 
+run_test    "Authentication: server goodcert, client optional, no trusted CA" \
+            "$P_SRV" \
+            "$P_CLI debug_level=3 auth_mode=optional ca_file=none ca_path=none" \
+            0 \
+            -c "x509_verify_cert() returned" \
+            -c "! The certificate is not correctly signed by the trusted CA" \
+            -c "! Certificate verification flags"\
+            -C "! ssl_handshake returned" \
+            -C "X509 - Certificate verification failed" \
+            -C "SSL - No CA Chain is set, but required to operate"
+
+run_test    "Authentication: server goodcert, client required, no trusted CA" \
+            "$P_SRV" \
+            "$P_CLI debug_level=3 auth_mode=required ca_file=none ca_path=none" \
+            1 \
+            -c "x509_verify_cert() returned" \
+            -c "! The certificate is not correctly signed by the trusted CA" \
+            -c "! Certificate verification flags"\
+            -c "! ssl_handshake returned" \
+            -c "SSL - No CA Chain is set, but required to operate"
+
+# The purpose of the next two tests is to test the client's behaviour when receiving a server
+# certificate with an unsupported elliptic curve. This should usually not happen because
+# the client informs the server about the supported curves - it does, though, in the
+# corner case of a static ECDH suite, because the server doesn't check the curve on that
+# occasion (to be fixed). If that bug's fixed, the test needs to be altered to use a
+# different means to have the server ignoring the client's supported curve list.
+
+requires_config_enabled POLARSSL_SSL_SET_CURVES
+run_test    "Authentication: server ECDH p256v1, client required, p256v1 unsupported" \
+            "$P_SRV debug_level=1 key_file=data_files/server5.key \
+             crt_file=data_files/server5.ku-ka.crt" \
+            "$P_CLI debug_level=3 auth_mode=required curves=secp521r1" \
+            1 \
+            -c "bad certificate (EC key curve)"\
+            -c "! Certificate verification flags"\
+            -C "bad server certificate (ECDH curve)" # Expect failure at earlier verification stage
+
+requires_config_enabled POLARSSL_SSL_SET_CURVES
+run_test    "Authentication: server ECDH p256v1, client optional, p256v1 unsupported" \
+            "$P_SRV debug_level=1 key_file=data_files/server5.key \
+             crt_file=data_files/server5.ku-ka.crt" \
+            "$P_CLI debug_level=3 auth_mode=optional curves=secp521r1" \
+            1 \
+            -c "bad certificate (EC key curve)"\
+            -c "! Certificate verification flags"\
+            -c "bad server certificate (ECDH curve)" # Expect failure only at ECDH params check
+
 run_test    "Authentication: server badcert, client none" \
             "$P_SRV crt_file=data_files/server5-badsign.crt \
              key_file=data_files/server5.key" \
@@ -2420,7 +2468,7 @@ run_test    "Small packet TLS 1.2 AEAD shorter tag" \
 
 # A test for extensions in SSLv3
 
-requires_config_enabled MBEDTLS_SSL_PROTO_SSL3
+requires_config_enabled POLARSSL_SSL_PROTO_SSL3
 run_test    "SSLv3 with extensions, server side" \
             "$P_SRV min_version=ssl3 debug_level=3" \
             "$P_CLI force_version=ssl3 tickets=1 max_frag_len=4096 alpn=abc,1234" \