From 31ac379c4f5533c1ff98b495ce2c1c49ddafc2fc Mon Sep 17 00:00:00 2001 From: Sean Parkinson Date: Thu, 6 Jul 2017 14:44:20 +1000 Subject: [PATCH] Code review fixes Change verify depth and set curve to be compiled in whe using: OPENSSL_EXTRA Fix comparison of curve name strings to use ecc function. Fix verify depth check when compiling with both OPENSSL_EXTRA and WOLFSSL_TRUST_PEER_CERT. --- src/internal.c | 25 +++++++++++++++------- src/ssl.c | 53 +++++++++++++++++++++++++++++++++------------- src/tls.c | 4 ++-- src/tls13.c | 8 ++----- wolfssl/internal.h | 11 +++++----- wolfssl/ssl.h | 3 ++- 6 files changed, 66 insertions(+), 38 deletions(-) diff --git a/src/internal.c b/src/internal.c index 8a6bbe285..42588a274 100755 --- a/src/internal.c +++ b/src/internal.c @@ -1359,7 +1359,7 @@ int InitSSL_Ctx(WOLFSSL_CTX* ctx, WOLFSSL_METHOD* method, void* heap) ctx->minEccKeySz = MIN_ECCKEY_SZ; ctx->eccTempKeySz = ECDHE_SIZE; #endif -#ifdef WOLFSSL_NGINX +#ifdef OPENSSL_EXTRA ctx->verifyDepth = MAX_CHAIN_DEPTH; #endif @@ -3812,7 +3812,7 @@ int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) #ifdef HAVE_ECC ssl->options.minEccKeySz = ctx->minEccKeySz; #endif -#ifdef WOLFSSL_NGINX +#ifdef OPENSSL_EXTRA ssl->options.verifyDepth = ctx->verifyDepth; #endif @@ -7474,7 +7474,7 @@ typedef struct ProcPeerCertArgs { #ifdef WOLFSSL_TRUST_PEER_CERT byte haveTrustPeer; /* was cert verified by loaded trusted peer cert */ #endif -#ifdef WOLFSSL_NGINX +#ifdef OPENSSL_EXTRA char untrustedDepth; #endif } ProcPeerCertArgs; @@ -7778,6 +7778,9 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, "Checking CAs"); FreeDecodedCert(args->dCert); args->dCertInit = 0; + #ifdef OPENSSL_EXTRA + args->untrustedDepth = 1; + #endif } else if (MatchTrustedPeer(tp, args->dCert)){ WOLFSSL_MSG("Found matching trusted peer cert"); haveTrustPeer = 1; @@ -7785,10 +7788,16 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, WOLFSSL_MSG("Trusted peer cert did not match!"); FreeDecodedCert(args->dCert); args->dCertInit = 0; + #ifdef OPENSSL_EXTRA + args->untrustedDepth = 1; + #endif } } #endif /* WOLFSSL_TRUST_PEER_CERT */ - #ifdef WOLFSSL_NGINX + #ifdef OPENSSL_EXTRA + #ifdef WOLFSSL_TRUST_PEER_CERT + else + #endif if (args->certIdx == 0) { byte* subjectHash; @@ -7801,13 +7810,13 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, } ret = ParseCertRelative(args->dCert, CERT_TYPE, 0, - ssl->ctx->cm); + ssl->ctx->cm); if (ret != 0) { #ifdef WOLFSSL_ASYNC_CRYPT if (ret == WC_PENDING_E) { ret = wolfSSL_AsyncPush(ssl, - args->dCert->sigCtx.asyncDev, - WC_ASYNC_FLAG_CALL_AGAIN); + args->dCert->sigCtx.asyncDev, + WC_ASYNC_FLAG_CALL_AGAIN); } #endif goto exit_ppc; @@ -8483,7 +8492,7 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, ret = args->lastErr; } - #ifdef WOLFSSL_NGINX + #ifdef OPENSSL_EXTRA if (args->untrustedDepth > ssl->options.verifyDepth) { ssl->peerVerifyRet = X509_V_ERR_CERT_CHAIN_TOO_LONG; ret = MAX_CHAIN_ERROR; diff --git a/src/ssl.c b/src/ssl.c index 393ec21fc..195b08fb8 100755 --- a/src/ssl.c +++ b/src/ssl.c @@ -382,6 +382,8 @@ void wolfSSL_free(WOLFSSL* ssl) int wolfSSL_is_server(WOLFSSL* ssl) { + if (ssl == NULL) + return BAD_FUNC_ARG; return ssl->options.side == WOLFSSL_SERVER_END; } @@ -6276,7 +6278,7 @@ long wolfSSL_get_verify_depth(WOLFSSL* ssl) if(ssl == NULL) { return BAD_FUNC_ARG; } -#ifndef WOLFSSL_NGINX +#ifndef OPENSSL_EXTRA return MAX_CHAIN_DEPTH; #else return ssl->options.verifyDepth; @@ -6290,7 +6292,7 @@ long wolfSSL_CTX_get_verify_depth(WOLFSSL_CTX* ctx) if(ctx == NULL) { return BAD_FUNC_ARG; } -#ifndef WOLFSSL_NGINX +#ifndef OPENSSL_EXTRA return MAX_CHAIN_DEPTH; #else return ctx->verifyDepth; @@ -22769,7 +22771,7 @@ void* wolfSSL_GetRsaDecCtx(WOLFSSL* ssl) void wolfSSL_CTX_set_verify_depth(WOLFSSL_CTX *ctx, int depth) { WOLFSSL_ENTER("wolfSSL_CTX_set_verify_depth"); -#ifndef WOLFSSL_NGINX +#ifndef OPENSSL_EXTRA (void)ctx; (void)depth; WOLFSSL_STUB("wolfSSL_CTX_set_verify_depth"); @@ -22780,7 +22782,7 @@ void* wolfSSL_GetRsaDecCtx(WOLFSSL* ssl) void wolfSSL_set_verify_depth(WOLFSSL *ssl, int depth) { WOLFSSL_ENTER("wolfSSL_set_verify_depth"); -#ifndef WOLFSSL_NGINX +#ifndef OPENSSL_EXTRA (void)ssl; (void)depth; WOLFSSL_STUB("wolfSSL_set_verify_depth"); @@ -25008,33 +25010,54 @@ void wolfSSL_get0_next_proto_negotiated(const WOLFSSL *s, const unsigned char ** } #endif /* HAVE_ALPN */ +#endif /* WOLFSSL_NGINX / WOLFSSL_HAPROXY */ + +#if defined(OPENSSL_EXTRA) && defined(HAVE_ECC) WOLFSSL_API int wolfSSL_CTX_set1_curves_list(WOLFSSL_CTX* ctx, char* names) { - int idx, start = 0; + int idx, start = 0, len; int curve; + char name[MAX_CURVE_NAME_SZ]; + /* Disable all curves so that only the ones the user wants are enabled. */ ctx->disabledCurves = (word32)-1; for (idx = 1; names[idx-1] != '\0'; idx++) { if (names[idx] != ':' && names[idx] != '\0') continue; - if (XSTRNCMP(names, "prime256v1", idx - 1 - start) == 0) - curve = WOLFSSL_ECC_SECP256R1; - else if (XSTRNCMP(names, "secp384r1", idx - 1 - start) == 0) - curve = WOLFSSL_ECC_SECP384R1; - else if (XSTRNCMP(names, "X25519", idx - 1 - start) == 0) - curve = WOLFSSL_ECC_X25519; - else + len = idx - 1 - start; + if (len > MAX_CURVE_NAME_SZ - 1) return SSL_FAILURE; - ctx->disabledCurves ^= 1 << curve; + XMEMCPY(name, names + start, len); + name[len] = 0; + + if ((XSTRNCMP(name, "prime256v1", len) == 0) || + (XSTRNCMP(name, "secp256r1", len) == 0) || + (XSTRNCMP(name, "P-256", len) == 0)) { + curve = WOLFSSL_ECC_SECP256R1; + } + else if ((XSTRNCMP(name, "secp384r1", len) == 0) || + (XSTRNCMP(name, "P-384", len) == 0)) { + curve = WOLFSSL_ECC_SECP384R1; + } + else if ((XSTRNCMP(name, "secp521r1", len) == 0) || + (XSTRNCMP(name, "P-521", len) == 0)) { + curve = WOLFSSL_ECC_SECP521R1; + } + else if (XSTRNCMP(name, "X25519", len) == 0) + curve = WOLFSSL_ECC_X25519; + else if ((curve = wc_ecc_get_curve_id_from_name(name)) < 0) + return SSL_FAILURE; + + /* Switch the bit to off and therefore is enabled. */ + ctx->disabledCurves &= ~(1 << curve); start = idx + 1; } return SSL_SUCCESS; } - -#endif /* WOLFSSL_NGINX / WOLFSSL_HAPROXY */ +#endif #ifdef OPENSSL_EXTRA int wolfSSL_CTX_set_msg_callback(WOLFSSL_CTX *ctx, SSL_Msg_Cb cb) diff --git a/src/tls.c b/src/tls.c index 5a715e1d4..0787abca0 100755 --- a/src/tls.c +++ b/src/tls.c @@ -3016,7 +3016,7 @@ int TLSX_ValidateEllipticCurves(WOLFSSL* ssl, byte first, byte second) { curve && !(sig && key); curve = curve->next) { - #ifdef WOLFSSL_NGINX + #ifdef OPENSSL_EXTRA if (ssl->ctx->disabledCurves & (1 << curve->name)) continue; #endif @@ -5865,7 +5865,7 @@ int TLSX_KeyShare_Establish(WOLFSSL *ssl) if (!TLSX_SupportedGroups_Find(ssl, clientKSE->group)) return BAD_KEY_SHARE_DATA; - #ifdef WOLFSSL_NGINX + #ifdef OPENSSL_EXTRA /* Check if server supports group. */ if (ssl->ctx->disabledCurves & (1 << clientKSE->group)) continue; diff --git a/src/tls13.c b/src/tls13.c index 6d3046b9d..add73ca16 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -3298,12 +3298,8 @@ int DoTls13ClientHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, /* Session id - empty in TLS v1.3 */ sessIdSz = input[i++]; if (sessIdSz > 0) { - ssl->version.major = pv.major; - ssl->version.minor = pv.minor; - ret = DoClientHello(ssl, input, inOutIdx, helloSz); - if (ret != 0) - return ret; - return HashInput(ssl, input + begin, helloSz); + WOLFSSL_MSG("Client sent session id - not supported"); + return BUFFER_ERROR; } /* Cipher suites */ diff --git a/wolfssl/internal.h b/wolfssl/internal.h index ef65a5fcf..d77da93de 100755 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -1096,6 +1096,7 @@ enum Misc { ECDHE_SIZE = 32, /* ECHDE server size defaults to 256 bit */ MAX_EXPORT_ECC_SZ = 256, /* Export ANS X9.62 max future size */ + MAX_CURVE_NAME_SZ = 16, /* Maximum size of curve name string */ NEW_SA_MAJOR = 8, /* Most signicant byte used with new sig algos */ ED25519_SA_MAJOR = 8, /* Most significant byte for ED25519 */ @@ -2244,12 +2245,10 @@ struct WOLFSSL_CTX { #if defined(HAVE_ECC) || defined(HAVE_ED25519) short minEccKeySz; /* minimum ECC key size */ #endif -#ifdef WOLFSSL_NGINX - word32 disabledCurves; /* curves disabled by user */ - byte verifyDepth; /* maximum verification depth */ -#endif #ifdef OPENSSL_EXTRA - unsigned long mask; /* store SSL_OP_ flags */ + word32 disabledCurves; /* curves disabled by user */ + byte verifyDepth; /* maximum verification depth */ + unsigned long mask; /* store SSL_OP_ flags */ #endif CallbackIORecv CBIORecv; CallbackIOSend CBIOSend; @@ -2871,7 +2870,7 @@ typedef struct Options { #if defined(HAVE_ECC) || defined(HAVE_ED25519) short minEccKeySz; /* minimum ECC key size */ #endif -#ifdef WOLFSSL_NGINX +#ifdef OPENSSL_EXTRA byte verifyDepth; /* maximum verification depth */ #endif #ifdef WOLFSSL_EARLY_DATA diff --git a/wolfssl/ssl.h b/wolfssl/ssl.h index 38f00a7fb..8082aded2 100644 --- a/wolfssl/ssl.h +++ b/wolfssl/ssl.h @@ -2318,6 +2318,8 @@ WOLFSSL_API int wolfSSL_CTX_AsyncPoll(WOLFSSL_CTX* ctx, WOLF_EVENT** events, int #endif /* WOLFSSL_ASYNC_CRYPT */ #ifdef OPENSSL_EXTRA +WOLFSSL_API int wolfSSL_CTX_set1_curves_list(WOLFSSL_CTX* ctx, char* names); + typedef void (*SSL_Msg_Cb)(int write_p, int version, int content_type, const void *buf, size_t len, WOLFSSL *ssl, void *arg); @@ -2398,7 +2400,6 @@ WOLFSSL_API char* wolfSSL_sk_WOLFSSL_STRING_value( WOLFSSL_API int PEM_write_bio_WOLFSSL_X509(WOLFSSL_BIO *bio, WOLFSSL_X509 *cert); -WOLFSSL_API int wolfSSL_CTX_set1_curves_list(WOLFSSL_CTX* ctx, char* names); #endif /* WOLFSSL_NGINX */ WOLFSSL_API void wolfSSL_get0_alpn_selected(const WOLFSSL *ssl,