From 8312ceb14c8067a030300b3c0da607ec0c953dfe Mon Sep 17 00:00:00 2001 From: Sean Parkinson Date: Fri, 28 Jun 2019 12:49:40 +1000 Subject: [PATCH 1/2] Improve hash and signature algorithm selection Return error when no hash-signature algorithm is possible. --- src/internal.c | 58 +++++++++++++++++++++++++++++++++------------- src/tls13.c | 5 ++-- wolfssl/internal.h | 17 ++++++++++++-- 3 files changed, 60 insertions(+), 20 deletions(-) diff --git a/src/internal.c b/src/internal.c index ea1b2e816..64dcc5b53 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1882,6 +1882,13 @@ static WC_INLINE void AddSuiteHashSigAlgo(Suites* suites, byte macAlgo, byte sig *inOutIdx += 1; suites->hashSigAlgo[*inOutIdx] = macAlgo; *inOutIdx += 1; +#ifdef WOLFSSL_TLS13 + /* Add the certificate algorithm as well */ + suites->hashSigAlgo[*inOutIdx] = sigAlgo; + *inOutIdx += 1; + suites->hashSigAlgo[*inOutIdx] = PSS_RSAE_TO_PSS_PSS(macAlgo); + *inOutIdx += 1; +#endif } else { suites->hashSigAlgo[*inOutIdx] = macAlgo; @@ -2868,13 +2875,6 @@ static WC_INLINE void DecodeSigAlg(const byte* input, byte* hashAlgo, byte* hsTy { switch (input[0]) { case NEW_SA_MAJOR: - #ifdef WC_RSA_PSS - /* PSS signatures: 0x080[4-6] */ - if (input[1] <= sha512_mac) { - *hsType = input[0]; - *hashAlgo = input[1]; - } - #endif #ifdef HAVE_ED25519 /* ED25519: 0x0807 */ if (input[1] == ED25519_SA_MINOR) { @@ -2882,8 +2882,21 @@ static WC_INLINE void DecodeSigAlg(const byte* input, byte* hashAlgo, byte* hsTy /* Hash performed as part of sign/verify operation. */ *hashAlgo = sha512_mac; } + else + #endif + #ifdef WC_RSA_PSS + /* PSS PSS signatures: 0x080[9-b] */ + if (input[1] >= pss_sha256 && input[1] <= pss_sha512) { + *hsType = rsa_pss_pss_algo; + *hashAlgo = PSS_PSS_HASH_TO_MAC(input[1]); + } + else #endif /* ED448: 0x0808 */ + { + *hsType = input[0]; + *hashAlgo = input[1]; + } break; default: *hashAlgo = input[0]; @@ -16742,10 +16755,10 @@ int SetCipherList(WOLFSSL_CTX* ctx, Suites* suites, const char* list) #if !defined(NO_WOLFSSL_SERVER) || !defined(NO_CERTS) -void PickHashSigAlgo(WOLFSSL* ssl, const byte* hashSigAlgo, - word32 hashSigAlgoSz) +int PickHashSigAlgo(WOLFSSL* ssl, const byte* hashSigAlgo, word32 hashSigAlgoSz) { word32 i; + int ret = MATCH_SUITE_ERROR; ssl->suites->sigAlgo = ssl->specs.sig_algo; @@ -16769,6 +16782,9 @@ void PickHashSigAlgo(WOLFSSL* ssl, const byte* hashSigAlgo, } #endif + if (hashSigAlgoSz == 0) + return 0; + /* i+1 since peek a byte ahead for type */ for (i = 0; (i+1) < hashSigAlgoSz; i += HELLO_EXT_SIGALGO_SZ) { byte hashAlgo = 0, sigAlgo = 0; @@ -16782,6 +16798,7 @@ void PickHashSigAlgo(WOLFSSL* ssl, const byte* hashSigAlgo, ssl->suites->sigAlgo == ecc_dsa_sa_algo) { ssl->suites->sigAlgo = sigAlgo; ssl->suites->hashAlgo = sha512_mac; + ret = 0; break; } #endif @@ -16801,7 +16818,8 @@ void PickHashSigAlgo(WOLFSSL* ssl, const byte* hashSigAlgo, if (digestSz == ssl->eccTempKeySz) { ssl->suites->hashAlgo = hashAlgo; ssl->suites->sigAlgo = sigAlgo; - return; /* done selected sig/hash algorithms */ + ret = 0; + break; /* done selected sig/hash algorithms */ } /* not strong enough, so keep checking hashSigAlso list */ if (digestSz < ssl->eccTempKeySz) @@ -16810,6 +16828,7 @@ void PickHashSigAlgo(WOLFSSL* ssl, const byte* hashSigAlgo, /* mark as highest and check remainder of hashSigAlgo list */ ssl->suites->hashAlgo = hashAlgo; ssl->suites->sigAlgo = sigAlgo; + ret = 0; } else #endif @@ -16828,8 +16847,10 @@ void PickHashSigAlgo(WOLFSSL* ssl, const byte* hashSigAlgo, case sha512_mac: #endif /* not strong enough, so keep checking hashSigAlso list */ - if (hashAlgo < ssl->suites->hashAlgo) + if (hashAlgo < ssl->suites->hashAlgo) { + ret = 0; continue; + } /* mark as highest and check remainder of hashSigAlgo list */ ssl->suites->hashAlgo = hashAlgo; ssl->suites->sigAlgo = sigAlgo; @@ -16837,12 +16858,16 @@ void PickHashSigAlgo(WOLFSSL* ssl, const byte* hashSigAlgo, default: continue; } + ret = 0; break; } - else if (ssl->specs.sig_algo == 0) { + else if (ssl->specs.sig_algo == 0 && !IsAtLeastTLSv1_3(ssl->version)) { ssl->suites->hashAlgo = ssl->specs.mac_algorithm; + ret = 0; } } + + return ret; } #endif /* !defined(NO_WOLFSSL_SERVER) || !defined(NO_CERTS) */ @@ -18056,7 +18081,7 @@ exit_dpk: if ((*inOutIdx - begin) + len > size) return BUFFER_ERROR; - PickHashSigAlgo(ssl, input + *inOutIdx, len); + (void)PickHashSigAlgo(ssl, input + *inOutIdx, len); *inOutIdx += len; #ifdef WC_RSA_PSS ssl->pssAlgo = 0; @@ -23250,9 +23275,10 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, ssl->options.cipherSuite0 = ssl->suites->suites[i]; ssl->options.cipherSuite = ssl->suites->suites[i+1]; result = SetCipherSpecs(ssl); - if (result == 0) - PickHashSigAlgo(ssl, peerSuites->hashSigAlgo, - peerSuites->hashSigAlgoSz); + if (result == 0) { + result = PickHashSigAlgo(ssl, peerSuites->hashSigAlgo, + peerSuites->hashSigAlgoSz); + } return result; } else { diff --git a/src/tls13.c b/src/tls13.c index a0b778a53..a6a8abf66 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -3233,7 +3233,7 @@ static int DoTls13CertificateRequest(WOLFSSL* ssl, const byte* input, *inOutIdx += OPAQUE16_LEN; if ((*inOutIdx - begin) + len > size) return BUFFER_ERROR; - PickHashSigAlgo(ssl, input + *inOutIdx, len); + (void)PickHashSigAlgo(ssl, input + *inOutIdx, len); *inOutIdx += len; /* Length of certificate authority data. */ @@ -3288,7 +3288,8 @@ static int DoTls13CertificateRequest(WOLFSSL* ssl, const byte* input, } *inOutIdx += len; - PickHashSigAlgo(ssl, peerSuites.hashSigAlgo, peerSuites.hashSigAlgoSz); + (void)PickHashSigAlgo(ssl, peerSuites.hashSigAlgo, + peerSuites.hashSigAlgoSz); #endif if (ssl->buffers.certificate && ssl->buffers.certificate->buffer && diff --git a/wolfssl/internal.h b/wolfssl/internal.h index b522ee543..4325e6cb2 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -1638,7 +1638,7 @@ WOLFSSL_LOCAL int DoServerHello(WOLFSSL* ssl, const byte* input, word32*, word32); WOLFSSL_LOCAL int CompleteServerHello(WOLFSSL *ssl); WOLFSSL_LOCAL int CheckVersion(WOLFSSL *ssl, ProtocolVersion pv); -WOLFSSL_LOCAL void PickHashSigAlgo(WOLFSSL* ssl, const byte* hashSigAlgo, +WOLFSSL_LOCAL int PickHashSigAlgo(WOLFSSL* ssl, const byte* hashSigAlgo, word32 hashSigAlgoSz); WOLFSSL_LOCAL int DecodePrivateKey(WOLFSSL *ssl, word16* length); #ifdef HAVE_PK_CALLBACKS @@ -2822,7 +2822,20 @@ enum SignatureAlgorithm { dsa_sa_algo = 2, ecc_dsa_sa_algo = 3, rsa_pss_sa_algo = 8, - ed25519_sa_algo = 9 + ed25519_sa_algo = 9, + rsa_pss_pss_algo = 10 +}; + +#define PSS_RSAE_TO_PSS_PSS(macAlgo) \ + (macAlgo + (pss_sha256 - sha256_mac)) + +#define PSS_PSS_HASH_TO_MAC(macAlgo) \ + (macAlgo - (pss_sha256 - sha256_mac)) + +enum SigAlgRsaPss { + pss_sha256 = 0x09, + pss_sha384 = 0x0a, + pss_sha512 = 0x0b, }; From 0e33e2d9ee6a0489f168e5bd3a7e2d8cef1c8636 Mon Sep 17 00:00:00 2001 From: Sean Parkinson Date: Tue, 2 Jul 2019 11:53:04 +1000 Subject: [PATCH 2/2] Check PickHashSigAlgo return when doing CerticateRequest Only check picking the hash and signature algorithm functions return when a certificate is available to send to peer. Include the ECC signature and hash algorithms in available list even when using ECDSA certificates signed with RSA. List is of capabilities not what is in certificate. Certificate request sent to peer doesn't have to be an ECDSA certificate signed with RSA. Same treatment for RSA. --- src/internal.c | 17 +++++++++++++++-- src/tls13.c | 18 +++++++++++++----- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/internal.c b/src/internal.c index 64dcc5b53..838a2cf12 100644 --- a/src/internal.c +++ b/src/internal.c @@ -2859,7 +2859,8 @@ void InitSuites(Suites* suites, ProtocolVersion pv, int keySz, word16 haveRSA, suites->suiteSz = idx; - InitSuitesHashSigAlgo(suites, haveECDSAsig, haveRSAsig, 0, tls1_2, keySz); + InitSuitesHashSigAlgo(suites, haveECDSAsig | haveECC, haveRSAsig | haveRSA, + 0, tls1_2, keySz); } #if !defined(NO_WOLFSSL_SERVER) || !defined(NO_CERTS) || \ @@ -18081,7 +18082,19 @@ exit_dpk: if ((*inOutIdx - begin) + len > size) return BUFFER_ERROR; - (void)PickHashSigAlgo(ssl, input + *inOutIdx, len); + if (PickHashSigAlgo(ssl, input + *inOutIdx, len) != 0 && + ssl->buffers.certificate && + ssl->buffers.certificate->buffer) { + #ifdef HAVE_PK_CALLBACKS + if (wolfSSL_CTX_IsPrivatePkSet(ssl->ctx)) { + WOLFSSL_MSG("Using PK for client private key"); + return INVALID_PARAMETER; + } + #endif + if (ssl->buffers.key && ssl->buffers.key->buffer) { + return INVALID_PARAMETER; + } + } *inOutIdx += len; #ifdef WC_RSA_PSS ssl->pssAlgo = 0; diff --git a/src/tls13.c b/src/tls13.c index a6a8abf66..ae11c1ab1 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -3233,7 +3233,11 @@ static int DoTls13CertificateRequest(WOLFSSL* ssl, const byte* input, *inOutIdx += OPAQUE16_LEN; if ((*inOutIdx - begin) + len > size) return BUFFER_ERROR; - (void)PickHashSigAlgo(ssl, input + *inOutIdx, len); + if (PickHashSigAlgo(ssl, input + *inOutIdx, len) != 0 && + ssl->buffers.certificate && ssl->buffers.certificate->buffer && + ssl->buffers.key && ssl->buffers.key->buffer) { + return INVALID_PARAMETER; + } *inOutIdx += len; /* Length of certificate authority data. */ @@ -3287,14 +3291,18 @@ static int DoTls13CertificateRequest(WOLFSSL* ssl, const byte* input, return ret; } *inOutIdx += len; - - (void)PickHashSigAlgo(ssl, peerSuites.hashSigAlgo, - peerSuites.hashSigAlgoSz); #endif if (ssl->buffers.certificate && ssl->buffers.certificate->buffer && - ssl->buffers.key && ssl->buffers.key->buffer) + ssl->buffers.key && ssl->buffers.key->buffer) { +#ifndef WOLFSSL_TLS13_DRAFT_18 + if (PickHashSigAlgo(ssl, peerSuites.hashSigAlgo, + peerSuites.hashSigAlgoSz) != 0) { + return INVALID_PARAMETER; + } +#endif ssl->options.sendVerify = SEND_CERT; + } else ssl->options.sendVerify = SEND_BLANK_CERT;