[squid-dev] [RFC] [PREVIEW] LockingPointer round 3.
Amos Jeffries
squid3 at treenet.co.nz
Thu Jul 21 13:58:07 UTC 2016
On 21/07/2016 5:54 a.m., Alex Rousskov wrote:
> On 07/19/2016 10:45 PM, Amos Jeffries wrote:
>> 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:
>>>>> 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.
>
> Creating a nil pointer just to reset it on the next line looks like bad
> style to me. It makes it impossible to make the pointer object constant
> and increases the chances that the two lines will get separated in the
> future for now good reason, possibly causing bugs.
>
> Why resist naming an unusual/special construction method?
>
Just considering all the options without jumping to one too quickly.
How about?
static SelfType&& MakeFromLockedOpenSslPointer(T *t) {
SelfType out;
out.resetWithoutLocking(t);
return std::move(out);
}
Theres something I'm not quite understanding going on with the
return-by-value for template types giving me compiler errors. Thus the
std::move, it seems to build but I'm not sure if it will run properly at
all.
>
>> 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.
>
> The "resetWithoutLocking(X()); // X locks" approach creates a redundant
> distraction IMO. If you feel that we need those comments, then perhaps
> the reset method needs a new name, like resetUsingLocked(X()) or
> resetToLocked(X()).
>
> Note that you actually did not add a comment where one would be useful:
>
> ctx->resetWithoutLocking(sslContext); // XXX: how do we know its locked?
>
Yeah, identifying this ambiguous use (yay, no others) was the point of
the comment experiment.
>
>> // XXX: why not set fd_table if that add() succeeded ?
>
> The simple answer is that if you set fd_table after cache add()ing
> succeeded, then both fd_table and the cache will delete the same
> pointer, crashing Squid. Note that ctx is a raw pointer [to a smart
> pointer]. However, I doubt this is the correct/best answer.
>
> AFAICT, the poor-quality comments in the original code were supposed to
> answer that question like this: The _only_ reason we add ctx to the
> table is so that it gets eventually destroyed. The context object is not
> needed in the table and should not be accessed from there. If we
> successfully add ctx to the cache, then the cache becomes responsible
> for its destruction and we do not need to add it to the table.
>
So that means the fd_table member can be set *always*, and should be a
Pointer already. An extra +1 and -1 cycle happening on some objects is a
trivial cost to pay for reliable existence of contexts.
That will also protect any logic that may depend on one or the other
storage place existing from non-obvious nullptr values.
On the other hand. I suspect that the use fd_table is being put to is
better served by one of the MasterXaction / AccessLogEntry /
ConnStateData objects relevant to the transaction using that context.
>
>> + if (!ssl_ctx_cache)
>> + // if there is no storage, just use
>> fd_table[clientConnection->fd].dynamicSslContext = sslContext;
>> + else {
>
> Missing {} around the multiline if-statement body.
Re-written this section in light of the above explanation.
>
>> + explicit LockingPointer() : raw(nullptr) {}
>
> Default explicit constructors is a convoluted C++ issue. Unless you have
> a specific need for it, I suggest removing "explicit" here. Otherwise,
> document that need in a source code comment.
>
ok.
>
>> + * Our destructor will do the matching unlock.
>
> This comment is inaccurate because we may unlock in places other than
> the destructor. You can say "We become responsible for the matching
> unlock()." or something like that.
>
ok.
>
>> + tmp.resetWithoutLocking(crl); // XXX: does PEM_read_bio_X509_CRL lock ??
>
> One more reason to assert that resetWithoutLocking() is fed a locked
> OpenSSL object and also assert that unlock() has a locked OpenSSL object
> (if any).
Asserting that its a locked object, sure and done***. Though that itself
is not a guarantee the caller did the proper locking. Some other things
(including self) might be holding locks that disappear later on us. I've
used >1 for that case but still no certainty.
If you are referring to the self-reset assert(t != raw) and so adamant
that is right, please feel free to apply a patch doing so. I'm sure
enough about the outcome of that not to want my name against it.
*** though I should point out that OpenSSL 1.1.0 is making a lot of
things opaque and that includes the reference counting. We may not be
able to access t->references for long.
>
> To answer your question, yes, I think it does lock because the BUGS
> section of the PEM_read_bio_X509_CRL manual page contains an example
> that frees the result:
>
>> X509_free(x);
>> x = PEM_read_bio_X509(bp, NULL, 0, NULL);
>
Updated PREVIEW patch attached. Its still building here, but seems to
build okay for the most part.
Amos
-------------- next part --------------
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2016-07-18 12:36:38 +0000
+++ src/client_side.cc 2016-07-21 09:23:17 +0000
@@ -3043,43 +3043,48 @@
}
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.
- fd_table[clientConnection->fd].dynamicSslContext = sslContext;
+ // ensure the context stays around for at least the duration of this connection.
+ // fd_table is now responsible for reference counting of the pointer.
+ fd_table[clientConnection->fd].dynamicSslContext.resetWithoutLocking(sslContext);
+ if (ssl_ctx_cache) {
+ // hold another reference in cache (if any) to re-use the context for other connections
+ auto * tmp = new Security::ContextPointer(fd_table[clientConnection->fd].dynamicSslContext);
+ if (!ssl_ctx_cache->add(sslBumpCertKey.termedBuf(), tmp))
+ delete tmp; // dont leak tmp on failures
}
} 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;
@@ -3286,41 +3291,41 @@
Http::StreamPointer context = pipeline.front();
ClientHttpRequest *http = context ? context->http : NULL;
if (sslServerBump->step == Ssl::bumpStep1) {
sslServerBump->step = Ssl::bumpStep2;
// Run a accessList check to check if want to splice or continue bumping
ACLFilledChecklist *acl_checklist = new ACLFilledChecklist(Config.accessList.ssl_bump, sslServerBump->request.getRaw(), NULL);
acl_checklist->al = http ? http->al : NULL;
//acl_checklist->src_addr = params.conn->remote;
//acl_checklist->my_addr = s->s;
acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpNone));
acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpClientFirst));
acl_checklist->banAction(allow_t(ACCESS_ALLOWED, Ssl::bumpServerFirst));
acl_checklist->nonBlockingCheck(httpsSslBumpStep2AccessCheckDone, this);
return;
}
// will call httpsPeeked() with certificate and connection, eventually
auto unConfiguredCTX = Ssl::createSSLContext(port->signingCert, port->signPkey, *port);
- fd_table[clientConnection->fd].dynamicSslContext = unConfiguredCTX;
+ fd_table[clientConnection->fd].dynamicSslContext.resetWithoutLocking(unConfiguredCTX);
if (!httpsCreate(clientConnection, unConfiguredCTX))
return;
switchedToHttps_ = true;
auto ssl = fd_table[clientConnection->fd].ssl.get();
BIO *b = SSL_get_rbio(ssl);
Ssl::ClientBio *bio = static_cast<Ssl::ClientBio *>(b->ptr);
bio->setReadBufData(inBuf);
bio->hold(true);
// Here squid should have all of the client hello message so the
// Squid_SSL_accept should return 0;
// This block exist only to force openSSL parse client hello and detect
// ERR_SECURE_ACCEPT_FAIL error, which should be checked and splice if required.
int ret = 0;
if ((ret = Squid_SSL_accept(this, NULL)) < 0) {
debugs(83, 2, "SSL_accept failed.");
const err_type err = ERR_SECURE_ACCEPT_FAIL;
=== modified file 'src/fde.h'
--- src/fde.h 2016-07-05 23:37:12 +0000
+++ src/fde.h 2016-07-21 09:21:02 +0000
@@ -103,41 +103,41 @@
#if USE_DELAY_POOLS
ClientInfo * clientInfo;/* pointer to client info used in client write limiter or NULL if not present */
#endif
unsigned epoll_state;
_fde_disk disk;
PF *read_handler;
void *read_data;
PF *write_handler;
void *write_data;
AsyncCall::Pointer timeoutHandler;
time_t timeout;
time_t writeStart;
void *lifetime_data;
AsyncCall::Pointer closeHandler;
AsyncCall::Pointer halfClosedReader; /// read handler for half-closed fds
READ_HANDLER *read_method;
WRITE_HANDLER *write_method;
Security::SessionPointer ssl;
- Security::ContextPtr dynamicSslContext; ///< cached and then freed when fd is closed
+ Security::ContextPointer dynamicSslContext; ///< cached and then freed when fd is closed
#if _SQUID_WINDOWS_
struct {
long handle;
} win32;
#endif
tos_t tosFromServer; /**< Stores the TOS flags of the packets from the remote server.
See FwdState::dispatch(). Note that this differs to
tosToServer in that this is the value we *receive* from the,
connection, whereas tosToServer is the value to set on packets
*leaving* Squid. */
unsigned int nfmarkFromServer; /**< Stores the Netfilter mark value of the connection from the remote
server. See FwdState::dispatch(). Note that this differs to
nfmarkToServer in that this is the value we *receive* from the,
connection, whereas nfmarkToServer is the value to set on packets
*leaving* Squid. */
/** Clear the fde class back to NULL equivalent. */
inline void clear() {
type = 0;
remote_port = 0;
@@ -150,39 +150,39 @@
memset(&flags,0,sizeof(_fde_flags));
bytes_read = 0;
bytes_written = 0;
pconn.uses = 0;
#if USE_DELAY_POOLS
clientInfo = NULL;
#endif
epoll_state = 0;
read_handler = NULL;
read_data = NULL;
write_handler = NULL;
write_data = NULL;
timeoutHandler = NULL;
timeout = 0;
writeStart = 0;
lifetime_data = NULL;
closeHandler = NULL;
halfClosedReader = NULL;
read_method = NULL;
write_method = NULL;
- ssl.resetWithoutLocking(nullptr);
- dynamicSslContext = NULL;
+ ssl.reset();
+ dynamicSslContext.reset();
#if _SQUID_WINDOWS_
win32.handle = (long)NULL;
#endif
tosFromServer = '\0';
nfmarkFromServer = 0;
}
};
#define fd_table fde::Table
int fdNFree(void);
#define FD_READ_METHOD(fd, buf, len) (*fd_table[fd].read_method)(fd, buf, len)
#define FD_WRITE_METHOD(fd, buf, len) (*fd_table[fd].write_method)(fd, buf, len)
#endif /* SQUID_FDE_H */
=== modified file 'src/security/LockingPointer.h'
--- src/security/LockingPointer.h 2016-07-13 12:39:22 +0000
+++ src/security/LockingPointer.h 2016-07-21 13:02:50 +0000
@@ -34,117 +34,135 @@
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);
- }
+ 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.
+ * We will be responsible for the matching unlock.
+ */
void resetWithoutLocking(T *t) {
+#if USE_OPENSSL
+ assert(!t || t->references > 0);
+ assert(!raw || raw->references > 0);
+ if (raw && t == raw) {
+ assert(raw->references > 1); // us plus caller locks
+ }
+#endif
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;
}
+ static SelfType&& MakeFromLockedOpenSslPointer(T *t) {
+ SelfType out;
+ out.resetWithoutLocking(t);
+ return std::move(out);
+ }
+
private:
/// The lock() method increments Object's reference counter.
void lock(T *t) {
#if USE_OPENSSL
if (t)
CRYPTO_add(&t->references, 1, lockId);
#elif USE_GNUTLS
// XXX: GnuTLS does not provide locking ?
#else
assert(false);
#endif
}
/// Become a nil pointer. Decrements any pointed-to Object's reference counter
/// using UnLocker which ideally destroys the object when the counter reaches zero.
void unlock() {
if (raw) {
+#if USE_OPENSSL
+ assert(raw->references > 0);
+#endif
UnLocker(raw);
raw = nullptr;
}
}
/**
* Normally, no other code will have this raw pointer.
*
* However, OpenSSL does some strange and not always consistent things.
* OpenSSL library may keep its own internal raw pointers and manage
* their reference counts independently, or it may not. This varies between
* API functions, though it is usually documented.
*
* This means the caller code needs to be carefuly written to use the correct
* reset method and avoid the raw-pointer constructor unless OpenSSL function
* producing the pointer is clearly documented as incrementing a lock for it.
*/
T *raw;
};
=== modified file 'src/security/PeerOptions.cc'
--- src/security/PeerOptions.cc 2016-06-15 14:31:34 +0000
+++ src/security/PeerOptions.cc 2016-07-21 12:42:37 +0000
@@ -519,41 +519,41 @@
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));
+ parsedCrl.emplace_back(Security::CrlPointer::MakeFromLockedOpenSslPointer(crl));
}
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-21 13:43:19 +0000
@@ -105,41 +105,41 @@
#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(Security::ContextPointer::MakeFromLockedOpenSslPointer(createBlankContext()));
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-21 11:22:05 +0000
@@ -263,113 +263,113 @@
Ssl::PeekingPeerConnector::noteWantWrite()
{
const int fd = serverConnection()->fd;
Security::SessionPtr ssl = fd_table[fd].ssl.get();
BIO *b = SSL_get_rbio(ssl);
Ssl::ServerBio *srvBio = static_cast<Ssl::ServerBio *>(b->ptr);
if ((srvBio->bumpMode() == Ssl::bumpPeek || srvBio->bumpMode() == Ssl::bumpStare) && srvBio->holdWrite()) {
debugs(81, 3, "hold write on SSL connection on FD " << fd);
checkForPeekAndSplice();
return;
}
Ssl::PeerConnector::noteWantWrite();
}
void
Ssl::PeekingPeerConnector::noteSslNegotiationError(const int result, const int ssl_error, const int ssl_lib_error)
{
const int fd = serverConnection()->fd;
- Security::SessionPtr ssl = fd_table[fd].ssl.get();
+ SSL *ssl = fd_table[fd].ssl.get();
BIO *b = SSL_get_rbio(ssl);
Ssl::ServerBio *srvBio = static_cast<Ssl::ServerBio *>(b->ptr);
// In Peek mode, the ClientHello message sent to the server. If the
// server resuming a previous (spliced) SSL session with the client,
// then probably we are here because local SSL object does not know
// anything about the session being resumed.
//
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(Security::CertPointer::MakeFromLockedOpenSslPointer(SSL_get_peer_certificate(ssl)));
+ 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())
+ SSL *ssl = fd_table[fd].ssl.get();
+ Security::CertPointer serverCert(Security::CertPointer::MakeFromLockedOpenSslPointer(SSL_get_peer_certificate(ssl)));
+ 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-21 12:39:03 +0000
@@ -279,41 +279,41 @@
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(Security::CertPointer::MakeFromLockedOpenSslPointer(SSL_get_peer_certificate(ssl)));
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-21 09:06:30 +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-21 12:50:53 +0000
@@ -1,51 +1,51 @@
/*
* 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(Ssl::EVP_PKEY_Pointer::MakeFromLockedOpenSslPointer(EVP_PKEY_new()));
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 +499,41 @@
// 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(Security::CertPointer::MakeFromLockedOpenSslPointer(X509_new()));
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-21 12:47:30 +0000
@@ -905,43 +905,43 @@
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)
{
#if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
- Security::ContextPointer sslContext(SSL_CTX_new(TLS_server_method()));
+ Security::ContextPointer sslContext(Security::ContextPointer::MakeFromLockedOpenSslPointer(SSL_CTX_new(TLS_server_method())));
#else
- Security::ContextPointer sslContext(SSL_CTX_new(SSLv23_server_method()));
+ Security::ContextPointer sslContext(Security::ContextPointer::MakeFromLockedOpenSslPointer(SSL_CTX_new(SSLv23_server_method())));
#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)
More information about the squid-dev
mailing list