On 19/07/2016 7:14 p.m., Amos Jeffries wrote: > On 19/07/2016 6:58 a.m., Christos Tsantilas wrote: >> On 07/18/2016 08:32 PM, Alex Rousskov wrote: > > Dropping the non-locking constructor and forcing explicit resetFoo() is > probably for the best. Though it would not have helped in this case. I > did think the 'ssl' variable being passed in there was pre-locked so > would have coded as resetWithoutLocking(). > Would that difference in syntax have helped in the (brief) review? > > >>> >>> I can only repeat my earlier suggestions to hide that dangerous >>> constructor behind an explicit static method like >>> LockingPointer::NewWithoutLocking() and to assert that >>> resetWithoutLocking() method is fed a previously locked object. >> >> something like that. > > Nod. I'm about to try a build with dropped construtor to see what > breaks. That should highlight if a builder function is actually > necessary. I hope we can avoid it. >
Attached is a patch showing what that constructor removal would look like for current Squid trunk. I've added "// X locks" comments on new resetWithoutLocking() to be clearer where the non-locking comes from. OpenSSL API explicitly mentions that it "increments a reference" / locks. There are some new XXX points where I got ambiguous documentation about OpenSSL behaviour, or our code starts to show problems. Most notably client_side.cc shows up some weird behaviour going on with dynamicSslContext. Amos
=== modified file 'src/client_side.cc' --- src/client_side.cc 2016-07-18 12:36:38 +0000 +++ src/client_side.cc 2016-07-19 12:28:26 +0000 @@ -3043,43 +3043,50 @@ } getSslContextDone(NULL); } void ConnStateData::getSslContextDone(Security::ContextPtr sslContext, bool isNew) { // Try to add generated ssl context to storage. if (port->generateHostCertificates && isNew) { if (sslContext && (signAlgorithm == Ssl::algSignTrusted)) { Ssl::chainCertificatesToSSLContext(sslContext, *port); } else if (signAlgorithm == Ssl::algSignTrusted) { debugs(33, DBG_IMPORTANT, "WARNING: can not add signing certificate to SSL context chain because SSL context chain is invalid!"); } //else it is self-signed or untrusted do not attrach any certificate Ssl::LocalContextStorage *ssl_ctx_cache = Ssl::TheGlobalContextStorage.getLocalStorage(port->s); assert(sslBumpCertKey.size() > 0 && sslBumpCertKey[0] != '\0'); if (sslContext) { - if (!ssl_ctx_cache || !ssl_ctx_cache->add(sslBumpCertKey.termedBuf(), new Security::ContextPointer(sslContext))) { - // If it is not in storage delete after using. Else storage deleted it. + if (!ssl_ctx_cache) + // if there is no storage, just use fd_table[clientConnection->fd].dynamicSslContext = sslContext; + else { + Security::ContextPointer *ctx = new Security::ContextPointer; + ctx->resetWithoutLocking(sslContext); + if (!ssl_ctx_cache->add(sslBumpCertKey.termedBuf(), ctx)) { + // If it is not in storage delete after using. Else storage deleted it. + fd_table[clientConnection->fd].dynamicSslContext = sslContext; + } // XXX: why not set fd_table if that add() succeeded ? } } else { debugs(33, 2, HERE << "Failed to generate SSL cert for " << sslConnectHostOrIp); } } // If generated ssl context = NULL, try to use static ssl context. if (!sslContext) { if (!port->secure.staticContext) { debugs(83, DBG_IMPORTANT, "Closing " << clientConnection->remote << " as lacking TLS context"); clientConnection->close(); return; } else { debugs(33, 5, "Using static TLS context."); sslContext = port->secure.staticContext.get(); } } if (!httpsCreate(clientConnection, sslContext)) return; === modified file 'src/security/LockingPointer.h' --- src/security/LockingPointer.h 2016-07-13 12:39:22 +0000 +++ src/security/LockingPointer.h 2016-07-19 07:56:37 +0000 @@ -34,78 +34,80 @@ namespace Security { /** * A shared pointer to a reference-counting Object with library-specific * absorption, locking, and unlocking implementations. The API largely * follows std::shared_ptr. * * The constructor and the resetWithoutLocking() method import a raw Object pointer. * Normally, reset() would lock(), but libraries like OpenSSL * pre-lock objects before they are fed to LockingPointer, necessitating * this resetWithoutLocking() customization hook. */ template <typename T, void (*UnLocker)(T *t), int lockId> class LockingPointer { public: /// a helper label to simplify this objects API definitions below typedef Security::LockingPointer<T, UnLocker, lockId> SelfType; - /** - * Construct directly from a raw pointer. - * This action requires that the producer of that pointer has already - * created one reference lock for the object pointed to. - * Our destructor will do the matching unlock. - */ - explicit LockingPointer(T *t = nullptr): raw(nullptr) { - // de-optimized for clarity about non-locking - resetWithoutLocking(t); - } + explicit LockingPointer() : raw(nullptr) {} /// use the custom UnLocker to unlock any value still stored. ~LockingPointer() { unlock(); } + /** + * Construct directly from a raw pointer is forbidden. + * Use the appropriate reset method instead. + */ + explicit LockingPointer(T *) = delete; + // copy semantics are okay only when adding a lock reference explicit LockingPointer(const SelfType &o) : raw(nullptr) { resetAndLock(o.get()); } const SelfType &operator =(const SelfType &o) { resetAndLock(o.get()); return *this; } // move semantics are definitely okay, when possible explicit LockingPointer(SelfType &&) = default; SelfType &operator =(SelfType &&o) { if (o.get() != raw) resetWithoutLocking(o.release()); return *this; } bool operator !() const { return !raw; } explicit operator bool() const { return raw; } /// Returns raw and possibly nullptr pointer T *get() const { return raw; } - /// Reset raw pointer - unlock any previous one and save new one without locking. + /** + * Reset raw pointer - unlock any previous one and save new one without locking. + * This requires that the producer of that pointer has already + * created one reference lock for the object pointed to. + * Our destructor will do the matching unlock. + */ void resetWithoutLocking(T *t) { unlock(); raw = t; } void resetAndLock(T *t) { if (t != get()) { resetWithoutLocking(t); lock(t); } } /// Forget the raw pointer - unlock if any value was set. Become a nil pointer. void reset() { unlock(); } /// Forget the raw pointer without unlocking it. Become a nil pointer. T *release() { T *ret = raw; raw = nullptr; return ret; === modified file 'src/security/PeerOptions.cc' --- src/security/PeerOptions.cc 2016-06-15 14:31:34 +0000 +++ src/security/PeerOptions.cc 2016-07-19 11:23:48 +0000 @@ -198,40 +198,41 @@ add = "NO_SSLv3,NO_TLSv1_1,NO_TLSv1_2"; break; case 5: add = "NO_SSLv3,NO_TLSv1,NO_TLSv1_2"; break; case 6: add = "NO_SSLv3,NO_TLSv1,NO_TLSv1_1"; break; default: // nothing break; } if (add) { if (!sslOptions.isEmpty()) sslOptions.append(",",1); sslOptions.append(add, strlen(add)); } sslVersion = 0; // prevent sslOptions being repeatedly appended } } +// XXX: fix to return a Pointer instead of Ptr Security::ContextPtr Security::PeerOptions::createBlankContext() const { Security::ContextPtr t = nullptr; #if USE_OPENSSL Ssl::Initialize(); #if (OPENSSL_VERSION_NUMBER >= 0x10100000L) t = SSL_CTX_new(TLS_client_method()); #else t = SSL_CTX_new(SSLv23_client_method()); #endif if (!t) { const auto x = ERR_error_string(ERR_get_error(), nullptr); fatalf("Failed to allocate TLS client context: %s\n", x); } #elif USE_GNUTLS // Initialize for X.509 certificate exchange @@ -519,41 +520,43 @@ return fl; } /// Load a CRLs list stored in the file whose /path/name is in crlFile /// replaces any CRL loaded previously void Security::PeerOptions::loadCrlFile() { parsedCrl.clear(); if (crlFile.isEmpty()) return; #if USE_OPENSSL BIO *in = BIO_new_file(crlFile.c_str(), "r"); if (!in) { debugs(83, 2, "WARNING: Failed to open CRL file " << crlFile); return; } while (X509_CRL *crl = PEM_read_bio_X509_CRL(in,NULL,NULL,NULL)) { - parsedCrl.emplace_back(Security::CrlPointer(crl)); + Security::CrlPointer tmp; + tmp.resetWithoutLocking(crl); // XXX: does PEM_read_bio_X509_CRL lock ?? + parsedCrl.emplace_back(tmp); } BIO_free(in); #endif } #if USE_OPENSSL && defined(TLSEXT_TYPE_next_proto_neg) // Dummy next_proto_neg callback static int ssl_next_proto_cb(SSL *s, unsigned char **out, unsigned char *outlen, const unsigned char *in, unsigned int inlen, void *arg) { static const unsigned char supported_protos[] = {8, 'h','t','t', 'p', '/', '1', '.', '1'}; (void)SSL_select_next_proto(out, outlen, in, inlen, supported_protos, sizeof(supported_protos)); return SSL_TLSEXT_ERR_OK; } #endif void Security::PeerOptions::updateContextNpn(Security::ContextPtr &ctx) { if (!flags.tlsNpn) === modified file 'src/security/ServerOptions.cc' --- src/security/ServerOptions.cc 2016-07-07 19:03:02 +0000 +++ src/security/ServerOptions.cc 2016-07-19 11:21:35 +0000 @@ -105,41 +105,42 @@ #elif USE_GNUTLS // Initialize for X.509 certificate exchange if (const int x = gnutls_certificate_allocate_credentials(&t)) { debugs(83, DBG_CRITICAL, "ERROR: Failed to allocate TLS server context: error=" << x); } #else debugs(83, DBG_CRITICAL, "ERROR: Failed to allocate TLS server context: No TLS library"); #endif return t; } bool Security::ServerOptions::createStaticServerContext(AnyP::PortCfg &port) { updateTlsVersionLimits(); - Security::ContextPointer t(createBlankContext()); + Security::ContextPointer t; + t.resetWithoutLocking(createBlankContext()); // createBlankContext locks if (t) { #if USE_OPENSSL if (!Ssl::InitServerContext(t, port)) return false; #endif } staticContext = std::move(t); return bool(staticContext); } void Security::ServerOptions::loadDhParams() { if (dhParamsFile.isEmpty()) return; #if USE_OPENSSL DH *dhp = nullptr; if (FILE *in = fopen(dhParamsFile.c_str(), "r")) { === modified file 'src/ssl/PeekingPeerConnector.cc' --- src/ssl/PeekingPeerConnector.cc 2016-07-13 12:39:22 +0000 +++ src/ssl/PeekingPeerConnector.cc 2016-07-19 09:39:01 +0000 @@ -292,84 +292,86 @@ if (srvBio->bumpMode() == Ssl::bumpPeek && (resumingSession = srvBio->resumingSession())) { // we currently splice all resumed sessions unconditionally if (const bool spliceResumed = true) { bypassCertValidator(); checkForPeekAndSpliceMatched(Ssl::bumpSplice); return; } // else fall through to find a matching ssl_bump action (with limited info) } // If we are in peek-and-splice mode and still we did not write to // server yet, try to see if we should splice. // In this case the connection can be saved. // If the checklist decision is do not splice a new error will // occur in the next SSL_connect call, and we will fail again. // Abort on certificate validation errors to avoid splicing and // thus hiding them. // Abort if no certificate found probably because of malformed or // unsupported server Hello message (TODO: make configurable). if (!SSL_get_ex_data(ssl, ssl_ex_index_ssl_error_detail) && (srvBio->bumpMode() == Ssl::bumpPeek || srvBio->bumpMode() == Ssl::bumpStare) && srvBio->holdWrite()) { - Security::CertPointer serverCert(SSL_get_peer_certificate(ssl)); - if (serverCert.get()) { + Security::CertPointer serverCert; + serverCert.resetWithoutLocking(SSL_get_peer_certificate(ssl)); // SSL_get_peer_certificate locks. + if (serverCert) { debugs(81, 3, "Error (" << ERR_error_string(ssl_lib_error, NULL) << ") but, hold write on SSL connection on FD " << fd); checkForPeekAndSplice(); return; } } // else call parent noteNegotiationError to produce an error page Ssl::PeerConnector::noteSslNegotiationError(result, ssl_error, ssl_lib_error); } void Ssl::PeekingPeerConnector::handleServerCertificate() { if (serverCertificateHandled) return; if (ConnStateData *csd = request->clientConnectionManager.valid()) { const int fd = serverConnection()->fd; Security::SessionPtr ssl = fd_table[fd].ssl.get(); - Security::CertPointer serverCert(SSL_get_peer_certificate(ssl)); - if (!serverCert.get()) + Security::CertPointer serverCert; + serverCert.resetWithoutLocking(SSL_get_peer_certificate(ssl)); // SSL_get_peer_certificate locks + if (!serverCert) return; serverCertificateHandled = true; // remember the server certificate for later use if (Ssl::ServerBump *serverBump = csd->serverBump()) { serverBump->serverCert = std::move(serverCert); } } } void Ssl::PeekingPeerConnector::serverCertificateVerified() { if (ConnStateData *csd = request->clientConnectionManager.valid()) { Security::CertPointer serverCert; if(Ssl::ServerBump *serverBump = csd->serverBump()) serverCert.resetAndLock(serverBump->serverCert.get()); else { const int fd = serverConnection()->fd; Security::SessionPtr ssl = fd_table[fd].ssl.get(); - serverCert.resetWithoutLocking(SSL_get_peer_certificate(ssl)); + serverCert.resetWithoutLocking(SSL_get_peer_certificate(ssl)); // SSL_get_peer_certificate locks } if (serverCert.get()) { csd->resetSslCommonName(Ssl::CommonHostName(serverCert.get())); debugs(83, 5, "HTTPS server CN: " << csd->sslCommonName() << " bumped: " << *serverConnection()); } } } void Ssl::PeekingPeerConnector::tunnelInsteadOfNegotiating() { Must(callback != NULL); CbDialer *dialer = dynamic_cast<CbDialer*>(callback->getDialer()); Must(dialer); dialer->answer().tunneled = true; debugs(83, 5, "The SSL negotiation with server aborted"); } === modified file 'src/ssl/PeerConnector.cc' --- src/ssl/PeerConnector.cc 2016-07-13 12:39:22 +0000 +++ src/ssl/PeerConnector.cc 2016-07-19 09:47:20 +0000 @@ -279,41 +279,42 @@ for (SVCRECI i = resp.errors.begin(); i != resp.errors.end(); ++i) { debugs(83, 7, "Error item: " << i->error_no << " " << i->error_reason); assert(i->error_no != SSL_ERROR_NONE); if (!errDetails) { bool allowed = false; if (check) { check->sslErrors = new Ssl::CertErrors(Ssl::CertError(i->error_no, i->cert.get(), i->error_depth)); if (check->fastCheck() == ACCESS_ALLOWED) allowed = true; } // else the Config.ssl_client.cert_error access list is not defined // and the first error will cause the error page if (allowed) { debugs(83, 3, "bypassing SSL error " << i->error_no << " in " << "buffer"); } else { debugs(83, 5, "confirming SSL error " << i->error_no); X509 *brokenCert = i->cert.get(); - Security::CertPointer peerCert(SSL_get_peer_certificate(ssl)); + Security::CertPointer peerCert;; + peerCert.resetWithoutLocking(SSL_get_peer_certificate(ssl)); // SSL_get_peer_certificate locks const char *aReason = i->error_reason.empty() ? NULL : i->error_reason.c_str(); errDetails = new Ssl::ErrorDetail(i->error_no, peerCert.get(), brokenCert, aReason); } if (check) { delete check->sslErrors; check->sslErrors = NULL; } } if (!errs) errs = new Ssl::CertErrors(Ssl::CertError(i->error_no, i->cert.get(), i->error_depth)); else errs->push_back_unique(Ssl::CertError(i->error_no, i->cert.get(), i->error_depth)); } if (check) delete check; return errs; } === modified file 'src/ssl/cert_validate_message.h' --- src/ssl/cert_validate_message.h 2016-01-01 00:12:18 +0000 +++ src/ssl/cert_validate_message.h 2016-07-19 09:14:48 +0000 @@ -31,79 +31,79 @@ std::string domainName; ///< The server name CertValidationRequest() : ssl(NULL), errors(NULL) {} }; /** * This class is used to store informations found in certificate validation * response messages read from certificate validator helper */ class CertValidationResponse: public RefCountable { public: typedef RefCount<CertValidationResponse> Pointer; /** * This class used to hold error informations returned from * cert validator helper. */ class RecvdError { public: - RecvdError(): id(0), error_no(SSL_ERROR_NONE), cert(NULL), error_depth(-1) {} + RecvdError(): id(0), error_no(SSL_ERROR_NONE), error_depth(-1) {} RecvdError(const RecvdError &); RecvdError & operator =(const RecvdError &); void setCert(X509 *); ///< Sets cert to the given certificate int id; ///< The id of the error ssl_error_t error_no; ///< The OpenSSL error code std::string error_reason; ///< A string describing the error Security::CertPointer cert; ///< The broken certificate int error_depth; ///< The error depth }; typedef std::vector<RecvdError> RecvdErrors; /// Search in errors list for the error item with id=errorId. /// If none found a new RecvdError item added with the given id; RecvdError &getError(int errorId); RecvdErrors errors; ///< The list of parsed errors Helper::ResultCode resultCode; ///< The helper result code }; /** * This class is responsible for composing or parsing messages destined to * or comming from a cert validator helper. * The messages format is: * response/request-code SP body-length SP [key=value ...] \x01 */ class CertValidationMsg : public CrtdMessage { private: /** * This class used to hold the certId/cert pairs found * in cert validation messages. */ class CertItem { public: std::string name; ///< The certificate Id to use Security::CertPointer cert; ///< A pointer to certificate - CertItem(): cert(NULL) {} + CertItem() = default; CertItem(const CertItem &); CertItem & operator =(const CertItem &); void setCert(X509 *); ///< Sets cert to the given certificate }; public: CertValidationMsg(MessageKind kind): CrtdMessage(kind) {} /// Build a request message for the cert validation helper /// using informations provided by vcert object void composeRequest(CertValidationRequest const &vcert); /// Parse a response message and fill the resp object with parsed informations bool parseResponse(CertValidationResponse &resp, STACK_OF(X509) *peerCerts, std::string &error); /// Search a CertItems list for the certificate with ID "name" X509 *getCertByName(std::vector<CertItem> const &, std::string const & name); /// String code for "cert_validate" messages static const std::string code_cert_validate; === modified file 'src/ssl/gadgets.cc' --- src/ssl/gadgets.cc 2016-07-08 06:24:33 +0000 +++ src/ssl/gadgets.cc 2016-07-19 11:11:52 +0000 @@ -1,51 +1,52 @@ /* * Copyright (C) 1996-2016 The Squid Software Foundation and contributors * * Squid software is distributed under GPLv2+ license and includes * contributions from numerous individuals and organizations. * Please see the COPYING and CONTRIBUTORS files for details. */ #include "squid.h" #include "ssl/gadgets.h" #if HAVE_OPENSSL_X509V3_H #include <openssl/x509v3.h> #endif EVP_PKEY * Ssl::createSslPrivateKey() { - Ssl::EVP_PKEY_Pointer pkey(EVP_PKEY_new()); + Ssl::EVP_PKEY_Pointer pkey; + pkey.resetWithoutLocking(EVP_PKEY_new()); // EVP_PKEY_new locks if (!pkey) return NULL; Ssl::RSA_Pointer rsa(RSA_generate_key(1024, RSA_F4, NULL, NULL)); if (!rsa) return NULL; if (!EVP_PKEY_assign_RSA(pkey.get(), (rsa.get()))) return NULL; - rsa.release(); + rsa.release(); // XXX: because EVP_PKEY_assign_RSA steals ptr without locking? we leak? return pkey.release(); } /** \ingroup ServerProtocolSSLInternal * Set serial random serial number or set random serial number. */ static bool setSerialNumber(ASN1_INTEGER *ai, BIGNUM const* serial) { if (!ai) return false; Ssl::BIGNUM_Pointer bn(BN_new()); if (serial) { bn.reset(BN_dup(serial)); } else { if (!bn) return false; if (!BN_pseudo_rand(bn.get(), 64, 0, 0)) return false; @@ -499,41 +500,42 @@ // According to RFC 5280, using extensions requires v3 certificate. if (addedExtensions) X509_set_version(cert.get(), 2); // value 2 means v3 } return true; } static bool generateFakeSslCertificate(Security::CertPointer & certToStore, Ssl::EVP_PKEY_Pointer & pkeyToStore, Ssl::CertificateProperties const &properties, Ssl::BIGNUM_Pointer const &serial) { Ssl::EVP_PKEY_Pointer pkey; // Use signing certificates private key as generated certificate private key if (properties.signWithPkey.get()) pkey.resetAndLock(properties.signWithPkey.get()); else // if not exist generate one pkey.resetWithoutLocking(Ssl::createSslPrivateKey()); if (!pkey) return false; - Security::CertPointer cert(X509_new()); + Security::CertPointer cert; + cert.resetWithoutLocking(X509_new()); // X509_new locks if (!cert) return false; // Set pub key and serial given by the caller if (!X509_set_pubkey(cert.get(), pkey.get())) return false; if (!setSerialNumber(X509_get_serialNumber(cert.get()), serial.get())) return false; // Fill the certificate with the required properties if (!buildCertificate(cert, properties)) return false; int ret = 0; // Set issuer name, from CA or our subject name for self signed cert if (properties.signAlgorithm != Ssl::algSignSelf && properties.signWithX509.get()) ret = X509_set_issuer_name(cert.get(), X509_get_subject_name(properties.signWithX509.get())); else // Self signed certificate, set issuer to self ret = X509_set_issuer_name(cert.get(), X509_get_subject_name(cert.get())); if (!ret) === modified file 'src/ssl/support.cc' --- src/ssl/support.cc 2016-07-11 10:57:11 +0000 +++ src/ssl/support.cc 2016-07-19 10:08:19 +0000 @@ -904,44 +904,45 @@ for (i = 0; i < sk_X509_num(chain); ++i) { X509 *cert = sk_X509_value(chain, i); PEM_write_bio_X509(mem, cert); } len = BIO_get_mem_data(mem, &ptr); str = (char *)xmalloc(len + 1); memcpy(str, ptr, len); str[len] = '\0'; BIO_free(mem); return str; } /// Create SSL context and apply ssl certificate and private key to it. Security::ContextPtr Ssl::createSSLContext(Security::CertPointer & x509, Ssl::EVP_PKEY_Pointer & pkey, AnyP::PortCfg &port) { + Security::ContextPointer sslContext; #if (OPENSSL_VERSION_NUMBER >= 0x10100000L) - Security::ContextPointer sslContext(SSL_CTX_new(TLS_server_method())); + sslContext.resetWithoutLocking(SSL_CTX_new(TLS_server_method())); // SSL_CTX_new locks #else - Security::ContextPointer sslContext(SSL_CTX_new(SSLv23_server_method())); + sslContext.resetWithoutLocking(SSL_CTX_new(SSLv23_server_method())); // SSL_CTX_new locks #endif if (!SSL_CTX_use_certificate(sslContext.get(), x509.get())) return NULL; if (!SSL_CTX_use_PrivateKey(sslContext.get(), pkey.get())) return NULL; if (!configureSslContext(sslContext.get(), port)) return NULL; return sslContext.release(); } Security::ContextPtr Ssl::generateSslContextUsingPkeyAndCertFromMemory(const char * data, AnyP::PortCfg &port) { Security::CertPointer cert; Ssl::EVP_PKEY_Pointer pkey; if (!readCertAndPrivateKeyFromMemory(cert, pkey, data) || !cert || !pkey)
_______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev