From 1428a5e3ae3d07961ded1f30987fccc90c6e8015 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 24 May 2017 22:20:08 +0900 Subject: [PATCH 1/2] nghttpx: Verify OCSP response At least we should make sure that the OCSP response is targeted to the expected certificate. This is important because we pass the file path to the external script, and if the file is replaced because of renewal, and nghttpx has not reloaded its configuration, the certificate nghttpx has loaded and the one included in the file differ. Verifying the OCSP response detects this, and avoids to send wrong OCSP response. --- src/shrpx_connection_handler.cc | 7 +-- src/shrpx_tls.cc | 79 +++++++++++++++++++++++++++++++++ src/shrpx_tls.h | 5 +++ 3 files changed, 88 insertions(+), 3 deletions(-) diff --git a/src/shrpx_connection_handler.cc b/src/shrpx_connection_handler.cc index ae55701c..07c012f6 100644 --- a/src/shrpx_connection_handler.cc +++ b/src/shrpx_connection_handler.cc @@ -620,8 +620,9 @@ void ConnectionHandler::handle_ocsp_complete() { << " finished successfully"; } + if (tls::verify_ocsp_response(ssl_ctx, ocsp_.resp.data(), + ocsp_.resp.size()) == 0) { #ifndef OPENSSL_IS_BORINGSSL - { #ifdef HAVE_ATOMIC_STD_SHARED_PTR std::atomic_store_explicit( &tls_ctx_data->ocsp_data, @@ -632,10 +633,10 @@ void ConnectionHandler::handle_ocsp_complete() { tls_ctx_data->ocsp_data = std::make_shared>(std::move(ocsp_.resp)); #endif // !HAVE_ATOMIC_STD_SHARED_PTR - } #else // OPENSSL_IS_BORINGSSL - SSL_CTX_set_ocsp_response(ssl_ctx, ocsp_.resp.data(), ocsp_.resp.size()); + SSL_CTX_set_ocsp_response(ssl_ctx, ocsp_.resp.data(), ocsp_.resp.size()); #endif // OPENSSL_IS_BORINGSSL + } ++ocsp_.next; proceed_next_cert_ocsp(); diff --git a/src/shrpx_tls.cc b/src/shrpx_tls.cc index 1b4f8dba..171f2092 100644 --- a/src/shrpx_tls.cc +++ b/src/shrpx_tls.cc @@ -45,6 +45,7 @@ #include #include #include +#include #include @@ -1818,6 +1819,84 @@ int proto_version_from_string(const StringRef &v) { return -1; } +int verify_ocsp_response(SSL_CTX *ssl_ctx, const uint8_t *ocsp_resp, + size_t ocsp_resplen) { +#if !defined(LIBRESSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x10002000L + int rv; + + STACK_OF(X509) * chain_certs; + SSL_CTX_get0_chain_certs(ssl_ctx, &chain_certs); + + auto resp = d2i_OCSP_RESPONSE(nullptr, &ocsp_resp, ocsp_resplen); + if (resp == nullptr) { + LOG(ERROR) << "d2i_OCSP_RESPONSE failed"; + return -1; + } + auto resp_deleter = defer(OCSP_RESPONSE_free, resp); + + ERR_clear_error(); + + auto bs = OCSP_response_get1_basic(resp); + if (bs == nullptr) { + LOG(ERROR) << "OCSP_response_get1_basic failed: " + << ERR_error_string(ERR_get_error(), nullptr); + return -1; + } + auto bs_deleter = defer(OCSP_BASICRESP_free, bs); + + ERR_clear_error(); + + rv = OCSP_basic_verify(bs, chain_certs, nullptr, OCSP_TRUSTOTHER); + + if (rv != 1) { + LOG(ERROR) << "OCSP_basic_verify failed: " + << ERR_error_string(ERR_get_error(), nullptr); + return -1; + } + + auto sresp = OCSP_resp_get0(bs, 0); + if (sresp == nullptr) { + LOG(ERROR) << "OCSP response verification failed: no single response"; + return -1; + } + +#if OPENSSL_1_1_API + auto certid = OCSP_SINGLERESP_get0_id(sresp); +#else // !OPENSSL_1_1_API + auto certid = sresp->certId; +#endif // !OPENSSL_1_1_API + assert(certid != nullptr); + + ASN1_INTEGER *serial; + rv = OCSP_id_get0_info(nullptr, nullptr, nullptr, &serial, + const_cast(certid)); + if (rv != 1) { + LOG(ERROR) << "OCSP_id_get0_info failed"; + return -1; + } + + if (serial == nullptr) { + LOG(ERROR) << "OCSP response does not contain serial number"; + return -1; + } + + auto cert = SSL_CTX_get0_certificate(ssl_ctx); + auto cert_serial = X509_get_serialNumber(cert); + + if (ASN1_INTEGER_cmp(cert_serial, serial)) { + LOG(ERROR) << "OCSP verification serial numbers do not match"; + return -1; + } + + if (LOG_ENABLED(INFO)) { + LOG(INFO) << "OCSP verification succeeded"; + } +#endif // !defined(LIBRESSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= + // 0x10002000L + + return 0; +} + } // namespace tls } // namespace shrpx diff --git a/src/shrpx_tls.h b/src/shrpx_tls.h index 7775d18e..20be9607 100644 --- a/src/shrpx_tls.h +++ b/src/shrpx_tls.h @@ -264,6 +264,11 @@ X509 *load_certificate(const char *filename); // TLS version string. int proto_version_from_string(const StringRef &v); +// Verifies OCSP response |ocsp_resp| of length |ocsp_resplen|. This +// function returns 0 if it succeeds, or -1. +int verify_ocsp_response(SSL_CTX *ssl_ctx, const uint8_t *ocsp_resp, + size_t ocsp_resplen); + } // namespace tls } // namespace shrpx From 74c2f1257a800f25181c7945edab8f788a7980c2 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 25 May 2017 22:12:54 +0900 Subject: [PATCH 2/2] nghttpx: Add --no-verify-ocsp to disable OCSP response verification --- gennghttpxfun.py | 1 + src/shrpx.cc | 8 ++++++++ src/shrpx_config.cc | 9 +++++++++ src/shrpx_config.h | 3 +++ src/shrpx_connection_handler.cc | 6 +++++- 5 files changed, 26 insertions(+), 1 deletion(-) diff --git a/gennghttpxfun.py b/gennghttpxfun.py index f46fac78..f741920a 100755 --- a/gennghttpxfun.py +++ b/gennghttpxfun.py @@ -167,6 +167,7 @@ OPTIONS = [ "no-add-x-forwarded-proto", "no-strip-incoming-x-forwarded-proto", "ocsp-startup", + "no-verify-ocsp", ] LOGVARS = [ diff --git a/src/shrpx.cc b/src/shrpx.cc index c6f6a322..2d5f339d 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -2240,6 +2240,8 @@ SSL/TLS: the attempts fail. This feature is useful if OCSP responses must be available before accepting connections. + --no-verify-ocsp + nghttpx does not verify OCSP response. --no-ocsp Disable OCSP stapling. --tls-session-cache-memcached=,[;tls] Specify address of memcached server to store session @@ -3191,6 +3193,7 @@ int main(int argc, char **argv) { {SHRPX_OPT_BACKEND_NO_TLS.c_str(), no_argument, &flag, 27}, {SHRPX_OPT_OCSP_STARTUP.c_str(), no_argument, &flag, 28}, {SHRPX_OPT_FRONTEND_NO_TLS.c_str(), no_argument, &flag, 29}, + {SHRPX_OPT_NO_VERIFY_OCSP.c_str(), no_argument, &flag, 30}, {SHRPX_OPT_BACKEND_TLS_SNI_FIELD.c_str(), required_argument, &flag, 31}, {SHRPX_OPT_DH_PARAM_FILE.c_str(), required_argument, &flag, 33}, {SHRPX_OPT_READ_RATE.c_str(), required_argument, &flag, 34}, @@ -3550,6 +3553,11 @@ int main(int argc, char **argv) { cmdcfgs.emplace_back(SHRPX_OPT_FRONTEND_NO_TLS, StringRef::from_lit("yes")); break; + case 30: + // --no-verify-ocsp + cmdcfgs.emplace_back(SHRPX_OPT_NO_VERIFY_OCSP, + StringRef::from_lit("yes")); + break; case 31: // --backend-tls-sni-field cmdcfgs.emplace_back(SHRPX_OPT_BACKEND_TLS_SNI_FIELD, diff --git a/src/shrpx_config.cc b/src/shrpx_config.cc index f8b696d0..e0ff9de2 100644 --- a/src/shrpx_config.cc +++ b/src/shrpx_config.cc @@ -1651,6 +1651,11 @@ int option_lookup_token(const char *name, size_t namelen) { return SHRPX_OPTID_NO_SERVER_PUSH; } break; + case 'p': + if (util::strieq_l("no-verify-ocs", name, 13)) { + return SHRPX_OPTID_NO_VERIFY_OCSP; + } + break; case 's': if (util::strieq_l("backend-no-tl", name, 13)) { return SHRPX_OPTID_BACKEND_NO_TLS; @@ -3429,6 +3434,10 @@ int parse_config(Config *config, int optid, const StringRef &opt, case SHRPX_OPTID_OCSP_STARTUP: config->tls.ocsp.startup = util::strieq_l("yes", optarg); + return 0; + case SHRPX_OPTID_NO_VERIFY_OCSP: + config->tls.ocsp.no_verify = util::strieq_l("yes", optarg); + return 0; case SHRPX_OPTID_CONF: LOG(WARN) << "conf: ignored"; diff --git a/src/shrpx_config.h b/src/shrpx_config.h index 820f90fa..392904d4 100644 --- a/src/shrpx_config.h +++ b/src/shrpx_config.h @@ -342,6 +342,7 @@ constexpr auto SHRPX_OPT_NO_ADD_X_FORWARDED_PROTO = constexpr auto SHRPX_OPT_NO_STRIP_INCOMING_X_FORWARDED_PROTO = StringRef::from_lit("no-strip-incoming-x-forwarded-proto"); constexpr auto SHRPX_OPT_OCSP_STARTUP = StringRef::from_lit("ocsp-startup"); +constexpr auto SHRPX_OPT_NO_VERIFY_OCSP = StringRef::from_lit("no-verify-ocsp"); constexpr size_t SHRPX_OBFUSCATED_NODE_LENGTH = 8; @@ -563,6 +564,7 @@ struct TLSConfig { StringRef fetch_ocsp_response_file; bool disabled; bool startup; + bool no_verify; } ocsp; // Client verification configurations @@ -1045,6 +1047,7 @@ enum { SHRPX_OPTID_NO_SERVER_PUSH, SHRPX_OPTID_NO_SERVER_REWRITE, SHRPX_OPTID_NO_STRIP_INCOMING_X_FORWARDED_PROTO, + SHRPX_OPTID_NO_VERIFY_OCSP, SHRPX_OPTID_NO_VIA, SHRPX_OPTID_NPN_LIST, SHRPX_OPTID_OCSP_STARTUP, diff --git a/src/shrpx_connection_handler.cc b/src/shrpx_connection_handler.cc index 07c012f6..cf078a12 100644 --- a/src/shrpx_connection_handler.cc +++ b/src/shrpx_connection_handler.cc @@ -620,7 +620,11 @@ void ConnectionHandler::handle_ocsp_complete() { << " finished successfully"; } - if (tls::verify_ocsp_response(ssl_ctx, ocsp_.resp.data(), + auto config = get_config(); + auto &tlsconf = config->tls; + + if (tlsconf.ocsp.no_verify || + tls::verify_ocsp_response(ssl_ctx, ocsp_.resp.data(), ocsp_.resp.size()) == 0) { #ifndef OPENSSL_IS_BORINGSSL #ifdef HAVE_ATOMIC_STD_SHARED_PTR