Refactor ocspDoVerify a bit 81/242681/8
authorKonrad Lipinski <k.lipinski2@samsung.com>
Fri, 28 Aug 2020 17:25:28 +0000 (19:25 +0200)
committerKonrad Lipinski <k.lipinski2@samsung.com>
Wed, 16 Sep 2020 11:10:07 +0000 (13:10 +0200)
Change-Id: I717cf06ff6a7cbb34b12349ee305f19d2bab0deb

CMakeLists.txt
src/manager/service/ocsp.cpp

index cf33344..ae63e83 100644 (file)
@@ -29,11 +29,11 @@ INCLUDE(FindPkgConfig)
 ############################# compiler flags ##################################
 
 SET(CMAKE_C_FLAGS_PROFILING    "-g -O0 -pg -Wp,-U_FORTIFY_SOURCE")
-SET(CMAKE_CXX_FLAGS_PROFILING  "-g -std=c++14 -O0 -pg -Wp,-U_FORTIFY_SOURCE")
+SET(CMAKE_CXX_FLAGS_PROFILING  "-g -std=c++17 -O0 -pg -Wp,-U_FORTIFY_SOURCE")
 SET(CMAKE_C_FLAGS_DEBUG        "-g -O0 -ggdb -Wp,-U_FORTIFY_SOURCE")
-SET(CMAKE_CXX_FLAGS_DEBUG      "-g -std=c++14 -O0 -ggdb -Wp,-U_FORTIFY_SOURCE")
+SET(CMAKE_CXX_FLAGS_DEBUG      "-g -std=c++17 -O0 -ggdb -Wp,-U_FORTIFY_SOURCE")
 SET(CMAKE_C_FLAGS_RELEASE      "-g -O2")
-SET(CMAKE_CXX_FLAGS_RELEASE    "-g -std=c++14 -O2")
+SET(CMAKE_CXX_FLAGS_RELEASE    "-g -std=c++17 -O2")
 
 # Force PIE
 SET(CMAKE_POSITION_INDEPENDENT_CODE "True")
@@ -134,4 +134,4 @@ ADD_SUBDIRECTORY(misc)
 ADD_SUBDIRECTORY(upgrade)
 ENDIF (NOT DEFINED COVERAGE_ONLY)
 
-ADD_SUBDIRECTORY(unit-tests)
\ No newline at end of file
+ADD_SUBDIRECTORY(unit-tests)
index b0ae883..72b67e7 100644 (file)
 #include <ckm/ckm-error.h>
 #include <vconf.h>
 
-/* Maximum leeway in validity period: default 5 minutes */
-#define MAX_VALIDITY_PERIOD     (5 * 60)
-
-/* Timeout in seconds for ocsp response */
-#define OCSP_TIMEOUT 30
 
 namespace CKM {
 
 namespace {
-template <size_t S>
-constexpr size_t staticStringLen(const char (&)[S]) {
-       static_assert(S, "static string of zero size");
-       return S-1;
-}
 
-const char HTTP_PREFIX[] = "http://";
-const size_t HTTP_PREFIX_LEN = staticStringLen(HTTP_PREFIX);
-const char HTTPS_PREFIX[] = "https://";
-const size_t HTTPS_PREFIX_LEN = staticStringLen(HTTPS_PREFIX);
+/* Maximum leeway in validity period: default 5 minutes */
+constexpr long MAX_VALIDITY_PERIOD = 5 * 60;
 
-typedef std::unique_ptr<BIO, std::function<void(BIO *)>> BioUniquePtr;
+/* Timeout in seconds for ocsp response */
+constexpr int OCSP_TIMEOUT = 30;
 
 void BIO_write_and_free(BIO *bio)
 {
@@ -75,163 +64,152 @@ void BIO_write_and_free(BIO *bio)
        BIO_free_all(bio);
 }
 
-int ocspDoVerify(X509 *cert, X509 *issuer,
-                                                       STACK_OF(X509) *trustedCerts, const std::string &constUrl)
+template <class Pointee, class Free>
+auto uptr(Pointee *p, Free f)
 {
-       OCSP_REQUEST *req = NULL;
-       OCSP_RESPONSE *resp = NULL;
-       OCSP_BASICRESP *bs = NULL;
-       OCSP_CERTID *certid = NULL;
-       BIO *cbio = NULL;
-       SSL_CTX *use_ssl_ctx = NULL;
-       std::string host, port, path;
-       ASN1_GENERALIZEDTIME *rev = NULL;
-       ASN1_GENERALIZEDTIME *thisupd = NULL;
-       ASN1_GENERALIZEDTIME *nextupd = NULL;
-       int use_ssl = 0;
-       int ocspStatus = -1;
-       int i = 0;
-       long nsec = MAX_VALIDITY_PERIOD, maxage = -1;
-       char subj_buf[256];
-       int reason = 0;
-       //    const char *reason_str = NULL;0
-       X509_STORE *trustedStore = NULL;
-       BioUniquePtr bioLogger(BIO_new(BIO_s_mem()), BIO_write_and_free);
-
-       std::vector<char> url(constUrl.begin(), constUrl.end());
-       url.push_back(0);
-       std::string headerHost;
-
-       {
-               char *chost = NULL, *cport = NULL, *cpath = NULL;
-
-               if (!OCSP_parse_url(url.data(), &chost, &cport, &cpath, &use_ssl))
-                       /* report error */
-                       return CKM_API_OCSP_STATUS_INVALID_URL;
+       return std::unique_ptr<Pointee, Free>(p, f);
+}
 
-               if (chost) {
-                       host = chost;
-                       headerHost = chost;
-               }
-               if (cport) port = cport;
-               if (cpath) path = cpath;
+auto opensslStrUptr()
+{
+       return uptr<char>(NULL, [](auto p) { OPENSSL_free(p); });
+}
 
-               OPENSSL_free(chost);
-               OPENSSL_free(cport);
-               OPENSSL_free(cpath);
-       }
+using OpensslStrUptr = decltype(opensslStrUptr());
 
-       LogDebug("Host: " << host);
-       LogDebug("Port: " << port);
-       LogDebug("Path: " << path);
-       LogDebug("Use_ssl: " << use_ssl);
+int parseUrl(const char *url, OpensslStrUptr &host, OpensslStrUptr &port, OpensslStrUptr &path, int &use_ssl)
+{
+       char *phost = NULL, *pport = NULL, *ppath = NULL;
+       if (!OCSP_parse_url(url, &phost, &pport, &ppath, &use_ssl)) {
+               return 0;
+       }
+       assert(phost);
+       assert(pport);
+       assert(ppath);
 
-       std::unique_ptr<char, decltype(free)*> proxy(vconf_get_str(VCONFKEY_NETWORK_PROXY), free);
+       host.reset(phost);
+       port.reset(pport);
+       path.reset(ppath);
 
-       if (proxy && strlen(proxy.get()) > 0) {
-               char *phost = NULL, *pport = NULL, *ppath = NULL;
+       return 1;
+}
 
+int canonicalizeProxy(std::unique_ptr<char, decltype(free)*> &proxy)
+{
+       if (!proxy || proxy.get()[0] == '\0') {
+               proxy.reset();
+       } else {
                LogDebug("Using proxy: " << proxy.get());
 
-               if (strncmp(HTTP_PREFIX, proxy.get(), HTTP_PREFIX_LEN) != 0 &&
-                       strncmp(HTTPS_PREFIX, proxy.get(), HTTPS_PREFIX_LEN) != 0) {
+               static constexpr char HTTP_PREFIX[] = "http://";
+               static constexpr char HTTPS_PREFIX[] = "https://";
+
+               if (strncmp(HTTP_PREFIX, proxy.get(), std::size(HTTP_PREFIX) - 1) &&
+                               strncmp(HTTPS_PREFIX, proxy.get(), std::size(HTTPS_PREFIX) - 1)) {
                        LogDebug("No http/https prefix. Assuming http.");
                        char *tmp = NULL;
-                       if (asprintf(&tmp, "%s%s", HTTP_PREFIX, proxy.get()) == -1) {
+                       if (asprintf(&tmp, "%s%s", HTTP_PREFIX, proxy.get()) <= -1) {
                                LogError("Http prefix application failed.");
-                               return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
+                               return 0;
                        }
                        proxy.reset(tmp);
                }
+       }
+       return 1;
+}
 
-               if (!OCSP_parse_url(proxy.get(), &phost, &pport, &ppath, &use_ssl)) {
-                       return CKM_API_OCSP_STATUS_INVALID_URL;
-               }
+int ocspDoVerify(X509 *cert, X509 *issuer,
+               STACK_OF(X509) *trustedCerts, const std::string &url)
+{
+       const auto bioLogger = uptr(BIO_new(BIO_s_mem()), BIO_write_and_free);
+       if (!bioLogger) {
+               LogDebug("Error in BIO_new(BIO_s_mem())");
+               return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
+       }
 
-               path = url.data();
-               if (phost) host = phost;
-               if (pport) port = pport;
+       auto host = opensslStrUptr();
+       auto port = opensslStrUptr();
+       auto path = opensslStrUptr();
+       int use_ssl = 0;
 
-               OPENSSL_free(phost);
-               OPENSSL_free(pport);
-               OPENSSL_free(ppath);
+       if (!parseUrl(url.c_str(), host, port, path, use_ssl)) {
+               return CKM_API_OCSP_STATUS_INVALID_URL;
        }
 
-       cbio = BIO_new_connect(host.c_str());
+       std::string headerHost = host.get();
+
+       LogDebug("Host: " << host.get());
+       LogDebug("Port: " << port.get());
+       LogDebug("Path: " << path.get());
+       LogDebug("Use_ssl: " << use_ssl);
 
-       if (cbio == NULL) {
-               /*BIO_printf(bio_err, "Error creating connect BIO\n");*/
-               /* report error */
-               LogError("Connection to ocsp host failed: " << host);
+       auto proxy = uptr(vconf_get_str(VCONFKEY_NETWORK_PROXY), free);
+       if (!canonicalizeProxy(proxy)) {
                return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
        }
 
-       if (!port.empty())
-               BIO_set_conn_port(cbio, port.c_str());
+       if (proxy) {
+               auto dummyPath = opensslStrUptr();
+               if (!parseUrl(proxy.get(), host, port, dummyPath, use_ssl)) {
+                       return CKM_API_OCSP_STATUS_INVALID_URL;
+               }
+       }
 
-       if (use_ssl == 1) {
-               BIO *sbio = NULL;
-               use_ssl_ctx = SSL_CTX_new(SSLv23_client_method());
+       auto cbio = uptr(BIO_new_connect(host.get()), BIO_free_all);
+       if (!cbio) {
+               LogError("Connection to ocsp host failed: " << host.get());
+               return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
+       }
 
-               if (use_ssl_ctx == NULL) {
-                       /* report error */
-                       return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
-               }
+       if (port.get()[0] != '\0')
+               BIO_set_conn_port(cbio.get(), port.get());
 
-               SSL_CTX_set_mode(use_ssl_ctx, SSL_MODE_AUTO_RETRY);
-               sbio = BIO_new_ssl(use_ssl_ctx, 1);
+       auto use_ssl_ctx = uptr<SSL_CTX>(NULL, SSL_CTX_free);
 
-               if (sbio == NULL) {
-                       /* report error */
+       if (use_ssl) {
+               use_ssl_ctx.reset(SSL_CTX_new(SSLv23_client_method()));
+
+               if (!use_ssl_ctx) {
                        return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
                }
 
-               cbio = BIO_push(sbio, cbio);
+               SSL_CTX_set_mode(use_ssl_ctx.get(), SSL_MODE_AUTO_RETRY);
+               BIO *sbio = BIO_new_ssl(use_ssl_ctx.get(), 1);
 
-               if (cbio == NULL) {
-                       /* report error */
+               if (sbio == NULL) {
+                       LogDebug("Error in BIO_new_ssl");
                        return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
                }
+
+               cbio.reset(BIO_push(sbio, cbio.release()));
+               assert(cbio);
        }
 
-       if (BIO_do_connect(cbio) <= 0) {
+       if (BIO_do_connect(cbio.get()) <= 0) {
                LogDebug("Error in BIO_do_connect.");
                ERR_print_errors(bioLogger.get());
-               /* report error */
-
-               if (use_ssl && use_ssl_ctx)
-                       SSL_CTX_free(use_ssl_ctx);
-
-               use_ssl_ctx = NULL;
-
-               if (cbio != NULL)
-                       BIO_free_all(cbio);
-
-               cbio = NULL;
-
                return CKM_API_OCSP_STATUS_NET_ERROR;
        }
 
-       req = OCSP_REQUEST_new();
-
-       if (req == NULL) {
+       const auto req = uptr(OCSP_REQUEST_new(), OCSP_REQUEST_free);
+       if (!req) {
                LogDebug("Error in OCPS_REQUEST_new");
                return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
        }
 
-       certid = OCSP_cert_to_id(NULL, cert, issuer);
-
+       auto certid = OCSP_cert_to_id(NULL, cert, issuer);
        if (certid == NULL) {
                LogDebug("Error in OCSP_cert_to_id");
                return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
        }
 
-       if (OCSP_request_add0_id(req, certid) == NULL) {
+       if (OCSP_request_add0_id(req.get(), certid) == NULL) {
                LogDebug("Error in OCSP_request_add0_id");
                return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
        }
 
-       std::unique_ptr<OCSP_REQ_CTX, decltype(OCSP_REQ_CTX_free)*> ctx(OCSP_sendreq_new(cbio, path.c_str(), NULL, -1), OCSP_REQ_CTX_free);
+       const auto ctx = uptr(OCSP_sendreq_new(cbio.get(), proxy ? url.c_str() : path.get(), NULL, -1),
+                       OCSP_REQ_CTX_free);
        if (!ctx) {
                LogError("Error creating OCSP_REQ_CTX");
                return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
@@ -242,31 +220,34 @@ int ocspDoVerify(X509 *cert, X509 *issuer,
                return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
        }
 
-       if (!OCSP_REQ_CTX_set1_req(ctx.get(), req)) {
+       if (!OCSP_REQ_CTX_set1_req(ctx.get(), req.get())) {
                LogError("Error setting ocsp request");
                return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
        }
 
        int fd;
-       if (BIO_get_fd(cbio, &fd) < 0) {
+       if (BIO_get_fd(cbio.get(), &fd) < 0) {
                LogError("Error extracting fd from bio");
                return CKM_API_OCSP_STATUS_INTERNAL_ERROR;
        }
 
+       auto resp = uptr<OCSP_RESPONSE>(NULL, OCSP_RESPONSE_free);
        for (;;) {
-               fd_set confds;
-               int req_timeout = OCSP_TIMEOUT;
-               struct timeval tv;
-               int rv = OCSP_sendreq_nbio(&resp, ctx.get());
-               if (rv != -1)
+               OCSP_RESPONSE *tmpResp = NULL;
+               int rv = OCSP_sendreq_nbio(&tmpResp, ctx.get());
+               if (rv > -1) {
+                       resp.reset(tmpResp);
                        break;
+               }
+               fd_set confds;
                FD_ZERO(&confds);
                FD_SET(fd, &confds);
+               struct timeval tv;
                tv.tv_usec = 0;
-               tv.tv_sec = req_timeout;
-               if (BIO_should_read(cbio)) {
+               tv.tv_sec = OCSP_TIMEOUT;
+               if (BIO_should_read(cbio.get())) {
                        rv = select(fd + 1, &confds, NULL, NULL, &tv);
-               } else if (BIO_should_write(cbio)) {
+               } else if (BIO_should_write(cbio.get())) {
                        rv = select(fd + 1, NULL, &confds, NULL, &tv);
                } else {
                        LogError("Unexpected retry condition\n");
@@ -276,61 +257,36 @@ int ocspDoVerify(X509 *cert, X509 *issuer,
                        LogError("Timeout on request\n");
                        break;
                }
-               if (rv == -1) {
+               if (rv <= -1) {
                        LogError("Select error\n");
                        break;
                }
        }
 
-       if (use_ssl && use_ssl_ctx)
-               SSL_CTX_free(use_ssl_ctx);
-
-       use_ssl_ctx = NULL;
-
-       if (cbio != NULL)
-               BIO_free_all(cbio);
-
-       cbio = NULL;
-
        if (!resp) {
-               /*BIO_printf(bio_err, "Error querying OCSP responsder\n");*/
-               /* report error */
-               /* free stuff */
-               OCSP_REQUEST_free(req);
                return CKM_API_OCSP_STATUS_NET_ERROR;
        }
 
-       i = OCSP_response_status(resp);
-
-       if (i != OCSP_RESPONSE_STATUS_SUCCESSFUL) {
-               /* report error */
+       if (OCSP_response_status(resp.get()) != OCSP_RESPONSE_STATUS_SUCCESSFUL) {
                ERR_print_errors(bioLogger.get());
-               /* free stuff */
-               OCSP_REQUEST_free(req);
-               OCSP_RESPONSE_free(resp);
                return CKM_API_OCSP_STATUS_REMOTE_ERROR;
        }
 
-       bs = OCSP_response_get1_basic(resp);
-
+       const auto bs = uptr(OCSP_response_get1_basic(resp.get()), OCSP_BASICRESP_free);
        if (!bs) {
-               /* report error */
                ERR_print_errors(bioLogger.get());
-               /* free stuff */
-               OCSP_REQUEST_free(req);
-               OCSP_RESPONSE_free(resp);
-
                LogDebug("Error in OCSP_response_get1_basic");
                return CKM_API_OCSP_STATUS_INVALID_RESPONSE;
        }
 
+       auto trustedStore = uptr<X509_STORE>(NULL, X509_STORE_free);
        if (trustedCerts != NULL) {
-               trustedStore = X509_STORE_new();
+               trustedStore.reset(X509_STORE_new());
 
                for (int tmpIdx = 0; tmpIdx < sk_X509_num(trustedCerts); tmpIdx++)
-                       X509_STORE_add_cert(trustedStore, sk_X509_value(trustedCerts, tmpIdx));
+                       X509_STORE_add_cert(trustedStore.get(), sk_X509_value(trustedCerts, tmpIdx));
 
-               X509_STORE_add_cert(trustedStore, issuer);
+               X509_STORE_add_cert(trustedStore.get(), issuer);
        }
 
        // Additional certificates to search for signer.
@@ -339,47 +295,26 @@ int ocspDoVerify(X509 *cert, X509 *issuer,
        X509_STACK_PTR verifyOther = create_x509_stack();
        sk_X509_push(verifyOther.get(), issuer);
 
-       int response = OCSP_basic_verify(bs, verifyOther.get(), trustedStore, 0);
-
-       verifyOther.reset();
-
-       if (response <= 0) {
-               OCSP_REQUEST_free(req);
-               OCSP_RESPONSE_free(resp);
-               OCSP_BASICRESP_free(bs);
-               X509_STORE_free(trustedStore);
+       if (OCSP_basic_verify(bs.get(), verifyOther.get(), trustedStore.get(), 0) <= 0) {
                ERR_print_errors(bioLogger.get());
                return CKM_API_OCSP_STATUS_INVALID_RESPONSE;
        }
 
-       if ((i = OCSP_check_nonce(req, bs)) <= 0) {
-               if (i == -1) {
-                       ERR_print_errors(bioLogger.get());
-               } else {
-                       /* report error */
-                       ERR_print_errors(bioLogger.get());
-                       /* free stuff */
-                       OCSP_REQUEST_free(req);
-                       OCSP_RESPONSE_free(resp);
-                       OCSP_BASICRESP_free(bs);
-                       X509_STORE_free(trustedStore);
+       if (const int i = OCSP_check_nonce(req.get(), bs.get()); i <= 0) {
+               ERR_print_errors(bioLogger.get());
+               if (i == 0) {
                        LogDebug("Error in OCSP_check_nonce");
                        return CKM_API_OCSP_STATUS_INVALID_RESPONSE;
                }
        }
 
-       (void)X509_NAME_oneline(X509_get_subject_name(cert), subj_buf, 255);
+       int ocspStatus = -1;
+       ASN1_GENERALIZEDTIME *thisupd = NULL;
+       ASN1_GENERALIZEDTIME *nextupd = NULL;
 
-       if (!OCSP_resp_find_status(bs, certid, &ocspStatus, &reason,
-                                                          &rev, &thisupd, &nextupd)) {
-               /* report error */
+       if (int reason; !OCSP_resp_find_status(
+                               bs.get(), certid, &ocspStatus, &reason, NULL, &thisupd, &nextupd)) {
                ERR_print_errors(bioLogger.get());
-               /* free stuff */
-               OCSP_RESPONSE_free(resp);
-               OCSP_REQUEST_free(req);
-               OCSP_BASICRESP_free(bs);
-               X509_STORE_free(trustedStore);
-
                LogDebug("Error in OCSP_resp_find_status");
                return CKM_API_OCSP_STATUS_INVALID_RESPONSE;
        }
@@ -387,39 +322,12 @@ int ocspDoVerify(X509 *cert, X509 *issuer,
        /* Check validity: if invalid write to output BIO so we
         * know which response this refers to.
         */
-       if (!OCSP_check_validity(thisupd, nextupd, nsec, maxage)) {
-               /* report error */
+       if (!OCSP_check_validity(thisupd, nextupd, MAX_VALIDITY_PERIOD, -1)) {
                ERR_print_errors(bioLogger.get());
-               /* free stuff */
-               OCSP_REQUEST_free(req);
-               OCSP_RESPONSE_free(resp);
-               OCSP_BASICRESP_free(bs);
-               X509_STORE_free(trustedStore);
-
                LogDebug("Error in OCSP_check_validity");
                return CKM_API_OCSP_STATUS_INVALID_RESPONSE;
        }
 
-       if (req != NULL) {
-               OCSP_REQUEST_free(req);
-               req = NULL;
-       }
-
-       if (resp != NULL) {
-               OCSP_RESPONSE_free(resp);
-               resp = NULL;
-       }
-
-       if (bs != NULL) {
-               OCSP_BASICRESP_free(bs);
-               bs = NULL;
-       }
-
-       if (trustedStore != NULL) {
-               X509_STORE_free(trustedStore);
-               trustedStore = NULL;
-       }
-
        switch (ocspStatus) {
        case V_OCSP_CERTSTATUS_GOOD:
                return CKM_API_OCSP_STATUS_GOOD;