From: Christos Tsantilas Date: Thu, 16 Jul 2015 07:24:24 +0000 (-0700) Subject: Avoid SSL certificate db corruption with empty index.txt as a symptom. X-Git-Tag: SQUID_3_5_7~15 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f00a2b08e2ab71c4d53fc7e959bc49a6614f59f8;p=thirdparty%2Fsquid.git Avoid SSL certificate db corruption with empty index.txt as a symptom. * Detect cases where the size file is corrupted or has a clearly wrong value. Automatically rebuild the database in such cases. * Teach ssl_crtd to keep running if it is unable to store the generated certificate in the database. Return the generated certificate to Squid and log an error message in such cases. Background: There are cases where ssl_crtd may corrupt its certificate database. The known cases manifest themselves with an empty db index file. When that happens, ssl_crtd helpers quit, SSL bumping does not work any more, and the certificate DB has to be deleted and re-initialized. We do not know exactly what causes corruption in deployments, but one known trigger that is easy to reproduce in a lab is the block size change in the ssl_crtd configuration. That change has the following side-effects: 1. When ssl_crtd removes certificates, it computes their size using a different block size than the one used to store the certificates. This is may result in negative database sizes. 2. Signed/unsigned conversion results in a huge number near LONG_MAX, which is then written to the "size" file. 3. The ssl_crtd helper remoces all certificates from database trying to make space for new certificates. 4. The ssl_crtd helper refuses to store new certificates because the database size (as described by the "size" file) still exceeds the configured limit. 5. The ssl_crtd helper exits because it cannot store a new certificates to the database. No helper response is sent to Squid in this case. Most likely, there are other corruption triggers -- the database management code is of an overall poor quality. This change resolves some of the underlying problems in hope to address at least some of the unknown triggers as well as the known one. This is a Measurement Factory project. --- diff --git a/src/ssl/certificate_db.cc b/src/ssl/certificate_db.cc index 175243ce56..fc2cf6197b 100644 --- a/src/ssl/certificate_db.cc +++ b/src/ssl/certificate_db.cc @@ -320,18 +320,25 @@ bool Ssl::CertificateDb::addCertAndPrivateKey(Ssl::X509_Pointer & cert, Ssl::EVP } // check db size while trying to minimize calls to size() - while (size() > max_db_size) { - if (deleteInvalidCertificate()) - continue; // try to find another invalid certificate if needed - - // there are no more invalid ones, but there must be valid certificates - do { - if (!deleteOldestCertificate()) { - save(); // Some entries may have been removed. Update the index file. - return false; // errors prevented us from freeing enough space - } - } while (size() > max_db_size); - break; + size_t dbSize = size(); + if ((dbSize == 0 && hasRows()) || + (dbSize > 0 && !hasRows()) || + (dbSize > 10 * max_db_size)) { + // Invalid database size, rebuild + dbSize = rebuildSize(); + } + while (dbSize > max_db_size && deleteInvalidCertificate()) { + dbSize = size(); // get the current database size + // and try to find another invalid certificate if needed + } + // there are no more invalid ones, but there must be valid certificates + while (dbSize > max_db_size) { + if (!deleteOldestCertificate()) { + rebuildSize(); // No certificates in database.Update the size file. + save(); // Some entries may have been removed. Update the index file. + return false; // errors prevented us from freeing enough space + } + dbSize = size(); // get the current database size } row.setValue(cnlType, "V"); @@ -456,7 +463,8 @@ void Ssl::CertificateDb::addSize(std::string const & filename) { void Ssl::CertificateDb::subSize(std::string const & filename) { // readSize will rebuild 'size' file if missing or it is corrupted size_t dbSize = readSize(); - dbSize -= getFileSize(filename); + const size_t fileSize = getFileSize(filename); + dbSize = dbSize > fileSize ? dbSize - fileSize : 0; writeSize(dbSize); } @@ -480,8 +488,10 @@ size_t Ssl::CertificateDb::getFileSize(std::string const & filename) { if (!file) return 0; file.seekg(0, std::ios_base::end); - size_t file_size = file.tellg(); - return ((file_size + fs_block_size - 1) / fs_block_size) * fs_block_size; + const std::streampos file_size = file.tellg(); + if (file_size < 0) + return 0; + return ((static_cast(file_size) + fs_block_size - 1) / fs_block_size) * fs_block_size; } void Ssl::CertificateDb::load() { @@ -561,15 +571,9 @@ bool Ssl::CertificateDb::deleteInvalidCertificate() { return true; } -bool Ssl::CertificateDb::deleteOldestCertificate() { - if (!db) - return false; - -#if SQUID_SSLTXTDB_PSTRINGDATA - if (sk_OPENSSL_PSTRING_num(db.get()->data) == 0) -#else - if (sk_num(db.get()->data) == 0) -#endif +bool Ssl::CertificateDb::deleteOldestCertificate() +{ + if (!hasRows()) return false; #if SQUID_SSLTXTDB_PSTRINGDATA @@ -610,6 +614,20 @@ bool Ssl::CertificateDb::deleteByHostname(std::string const & host) { return false; } +bool Ssl::CertificateDb::hasRows() const +{ + if (!db) + return false; + +#if SQUID_SSLTXTDB_PSTRINGDATA + if (sk_OPENSSL_PSTRING_num(db.get()->data) == 0) +#else + if (sk_num(db.get()->data) == 0) +#endif + return false; + return true; +} + bool Ssl::CertificateDb::IsEnabledDiskStore() const { return enabled_disk_store; } diff --git a/src/ssl/certificate_db.h b/src/ssl/certificate_db.h index 3062fa6d41..96e8e8bec9 100644 --- a/src/ssl/certificate_db.h +++ b/src/ssl/certificate_db.h @@ -127,6 +127,7 @@ private: bool deleteInvalidCertificate(); ///< Delete invalid certificate. bool deleteOldestCertificate(); ///< Delete oldest certificate. bool deleteByHostname(std::string const & host); ///< Delete using host name. + bool hasRows() const; ///< Whether the TXT_DB has stored items. /// Removes the first matching row from TXT_DB. Ignores failures. static void sq_TXT_DB_delete(TXT_DB *db, const char **row); diff --git a/src/ssl/ssl_crtd.cc b/src/ssl/ssl_crtd.cc index f0181fa421..41042ebdbc 100644 --- a/src/ssl/ssl_crtd.cc +++ b/src/ssl/ssl_crtd.cc @@ -205,7 +205,13 @@ static bool proccessNewRequest(Ssl::CrtdMessage & request_message, std::string c Ssl::EVP_PKEY_Pointer pkey; std::string &cert_subject = certProperties.dbKey(); - db.find(cert_subject, cert, pkey); + bool dbFailed = false; + try { + db.find(cert_subject, cert, pkey); + } catch (std::runtime_error &err) { + dbFailed = true; + error = err.what(); + } if (cert.get()) { if (!Ssl::certificateMatchesProperties(cert.get(), certProperties)) { @@ -221,10 +227,22 @@ static bool proccessNewRequest(Ssl::CrtdMessage & request_message, std::string c if (!Ssl::generateSslCertificate(cert, pkey, certProperties)) throw std::runtime_error("Cannot create ssl certificate or private key."); - if (!db.addCertAndPrivateKey(cert, pkey, cert_subject) && db.IsEnabledDiskStore()) - throw std::runtime_error("Cannot add certificate to db."); + if (!dbFailed && db.IsEnabledDiskStore()) { + try { + if (!db.addCertAndPrivateKey(cert, pkey, cert_subject)) { + dbFailed = true; + error = "Cannot add certificate to db."; + } + } catch (const std::runtime_error &err) { + dbFailed = true; + error = err.what(); + } + } } + if (dbFailed) + std::cerr << "ssl_crtd helper database '" << db_path << "' failed: " << error << std::endl; + std::string bufferToWrite; if (!Ssl::writeCertAndPrivateKeyToMemory(cert, pkey, bufferToWrite)) throw std::runtime_error("Cannot write ssl certificate or/and private key to memory.");