Multiple certificate_db/ssl_crtd fixes - Try to update the index file in all cases the database modified - The find operator in database should not modify the database. Currently if an entry is expired, ssl_crtd removes the cert file but does not update the index file. - Use OPENSSL_malloc and OPENSSL_free to allocate/release memory for TXT_DB rows. Currently we are using the new operator. - Fix a small memory leak when remove entries from database: A row object removed from TXT_DB indexes but never released. This patch: * Use OPENSSL_malloc and OPENSSL_free to allocate/release memory for TXT_DB rows. OpenSSL SDK assumes that always allocated using these functions. * Add code in Ssl::CertificateDb::Row destructor to correctly release a TXT_DB row. * Add the sq_TXT_DB_delete and sq_TXT_DB_delete_row functions which removes a row from TXT_DB indexes. This is a Measurement Factory project === modified file 'src/ssl/certificate_db.cc' --- src/ssl/certificate_db.cc 2012-10-08 05:21:11 +0000 +++ src/ssl/certificate_db.cc 2012-11-06 18:02:02 +0000 @@ -86,78 +86,147 @@ } Ssl::Locker::Locker(Lock &aLock, const char *aFileName, int aLineNo): weLocked(false), lock(aLock), fileName(aFileName), lineNo(aLineNo) { if (!lock.locked()) { lock.lock(); weLocked = true; } } Ssl::Locker::~Locker() { if (weLocked) lock.unlock(); } Ssl::CertificateDb::Row::Row() : width(cnlNumber) { - row = new char *[width + 1]; + row = (char **)OPENSSL_malloc(sizeof(char *) * (width + 1)); for (size_t i = 0; i < width + 1; ++i) row[i] = NULL; } +Ssl::CertificateDb::Row::Row(char **aRow, size_t aWidth): width(aWidth) +{ + row = aRow; +} + Ssl::CertificateDb::Row::~Row() { - if (row) { + if (!row) + return; + + void *max; + if ((max = (void *)row[width]) != NULL) { + // It is an openSSL allocated row. The TXT_DB_read function stores the + // index and row items one one memory segment. The row[width] points + // to the end of buffer. We have to check for items in the array which + // are not stored in this segment. These items should released. for (size_t i = 0; i < width + 1; ++i) { - delete[](row[i]); + if (((row[i] < (char *)row) || (row[i] > max)) && (row[i] != NULL)) + OPENSSL_free(row[i]); } - delete[](row); + OPENSSL_free(row); + } else { + for (size_t i = 0; i < width + 1; ++i) { + if (row[i]) + OPENSSL_free(row[i]); + } + OPENSSL_free(row); } } void Ssl::CertificateDb::Row::reset() { row = NULL; } void Ssl::CertificateDb::Row::setValue(size_t cell, char const * value) { assert(cell < width); if (row[cell]) { free(row[cell]); } if (value) { - row[cell] = static_cast(malloc(sizeof(char) * (strlen(value) + 1))); + row[cell] = static_cast(OPENSSL_malloc(sizeof(char) * (strlen(value) + 1))); memcpy(row[cell], value, sizeof(char) * (strlen(value) + 1)); } else row[cell] = NULL; } char ** Ssl::CertificateDb::Row::getRow() { return row; } +void Ssl::CertificateDb::sq_TXT_DB_delete(TXT_DB *db, const char **row) +{ + if (!db) + return; + +#if OPENSSL_VERSION_NUMBER >= 0x1000004fL + for (int i = 0; i < sk_OPENSSL_PSTRING_num(db->data); ++i) { + const char ** current_row = ((const char **)sk_OPENSSL_PSTRING_value(db->data, i)); +#else + for (int i = 0; i < sk_num(db->data); ++i) { + const char ** current_row = ((const char **)sk_value(db->data, i)); +#endif + if (current_row == row) { + sq_TXT_DB_delete_row(db, i); + return; + } + } +} + +#define countof(arr) (sizeof(arr)/sizeof(*arr)) +void Ssl::CertificateDb::sq_TXT_DB_delete_row(TXT_DB *db, int idx) +{ + char **rrow; +#if OPENSSL_VERSION_NUMBER >= 0x1000004fL + rrow = (char **)sk_OPENSSL_PSTRING_delete(db->data, idx); +#else + rrow = (char **)sk_delete(db->data, idx); +#endif + + if (!rrow) + return; + + Row row(rrow, cnlNumber); // row wrapper used to free the rrow + + const Columns db_indexes[]={cnlSerial, cnlName}; + for (unsigned int i = 0; i < countof(db_indexes); ++i) { + void *data = NULL; +#if OPENSSL_VERSION_NUMBER >= 0x1000004fL + if (LHASH_OF(OPENSSL_STRING) *fieldIndex = db->index[db_indexes[i]]) + data = lh_OPENSSL_STRING_delete(fieldIndex, rrow); +#else + if (LHASH *fieldIndex = db->index[db_indexes[i]]) + data = lh_delete(fieldIndex, rrow); +#endif + if (data) + assert(data == rrow); + } +} + unsigned long Ssl::CertificateDb::index_serial_hash(const char **a) { const char *n = a[Ssl::CertificateDb::cnlSerial]; while (*n == '0') ++n; return lh_strhash(n); } int Ssl::CertificateDb::index_serial_cmp(const char **a, const char **b) { const char *aa, *bb; for (aa = a[Ssl::CertificateDb::cnlSerial]; *aa == '0'; ++aa); for (bb = b[Ssl::CertificateDb::cnlSerial]; *bb == '0'; ++bb); return strcmp(aa, bb); } unsigned long Ssl::CertificateDb::index_name_hash(const char **a) { return(lh_strhash(a[Ssl::CertificateDb::cnlName])); } @@ -210,80 +279,102 @@ } bool Ssl::CertificateDb::addCertAndPrivateKey(Ssl::X509_Pointer & cert, Ssl::EVP_PKEY_Pointer & pkey, std::string const & useName) { const Locker locker(dbLock, Here); load(); if (!db || !cert || !pkey) return false; Row row; ASN1_INTEGER * ai = X509_get_serialNumber(cert.get()); std::string serial_string; Ssl::BIGNUM_Pointer serial(ASN1_INTEGER_to_BN(ai, NULL)); { TidyPointer hex_bn(BN_bn2hex(serial.get())); serial_string = std::string(hex_bn.get()); } row.setValue(cnlSerial, serial_string.c_str()); char ** rrow = TXT_DB_get_by_index(db.get(), cnlSerial, row.getRow()); // We are creating certificates with unique serial numbers. If the serial // number is found in the database, the same certificate is already stored. - if (rrow != NULL) + if (rrow != NULL) { + // TODO: check if the stored row is valid. return true; + } { TidyPointer subject(X509_NAME_oneline(X509_get_subject_name(cert.get()), NULL, 0)); - if (pure_find(useName.empty() ? subject.get() : useName, cert, pkey)) + Ssl::X509_Pointer findCert; + Ssl::EVP_PKEY_Pointer findPkey; + if (pure_find(useName.empty() ? subject.get() : useName, findCert, findPkey)) { + // Replace with database certificate + cert.reset(findCert.release()); + pkey.reset(findPkey.release()); return true; + } + // pure_find may fail because the entry is expired, or because the + // certs file is corrupted. Remove any entry with given hostname + deleteByHostname(useName.empty() ? subject.get() : useName); } // 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()) + 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; } row.setValue(cnlType, "V"); ASN1_UTCTIME * tm = X509_get_notAfter(cert.get()); row.setValue(cnlExp_date, std::string(reinterpret_cast(tm->data), tm->length).c_str()); row.setValue(cnlFile, "unknown"); if (!useName.empty()) row.setValue(cnlName, useName.c_str()); else { TidyPointer subject(X509_NAME_oneline(X509_get_subject_name(cert.get()), NULL, 0)); row.setValue(cnlName, subject.get()); } - if (!TXT_DB_insert(db.get(), row.getRow())) + if (!TXT_DB_insert(db.get(), row.getRow())) { + // failed to add index (???) but we may have already modified + // the database so save before exit + save(); return false; - + } + rrow = row.getRow(); row.reset(); + std::string filename(cert_full + "/" + serial_string + ".pem"); - if (!writeCertAndPrivateKeyToFile(cert, pkey, filename.c_str())) + if (!writeCertAndPrivateKeyToFile(cert, pkey, filename.c_str())) { + //remove row from txt_db and save + sq_TXT_DB_delete(db.get(), (const char **)rrow); + save(); return false; + } addSize(filename); save(); return true; } void Ssl::CertificateDb::create(std::string const & db_path) { if (db_path == "") throw std::runtime_error("Path to db is empty"); std::string db_full(db_path + "/" + db_file); std::string cert_full(db_path + "/" + cert_dir); std::string size_full(db_path + "/" + size_file); if (mkdir(db_path.c_str(), 0777)) throw std::runtime_error("Cannot create " + db_path); if (mkdir(cert_full.c_str(), 0777)) throw std::runtime_error("Cannot create " + cert_full); @@ -298,44 +389,42 @@ } void Ssl::CertificateDb::check(std::string const & db_path, size_t max_db_size) { CertificateDb db(db_path, max_db_size, 0); db.load(); } bool Ssl::CertificateDb::pure_find(std::string const & host_name, Ssl::X509_Pointer & cert, Ssl::EVP_PKEY_Pointer & pkey) { if (!db) return false; Row row; row.setValue(cnlName, host_name.c_str()); char **rrow = TXT_DB_get_by_index(db.get(), cnlName, row.getRow()); if (rrow == NULL) return false; - if (!sslDateIsInTheFuture(rrow[cnlExp_date])) { - deleteByHostname(rrow[cnlName]); + if (!sslDateIsInTheFuture(rrow[cnlExp_date])) return false; - } // read cert and pkey from file. std::string filename(cert_full + "/" + rrow[cnlSerial] + ".pem"); readCertAndPrivateKeyFromFiles(cert, pkey, filename.c_str(), NULL); if (!cert || !pkey) return false; return true; } size_t Ssl::CertificateDb::size() const { return readSize(); } void Ssl::CertificateDb::addSize(std::string const & filename) { writeSize(readSize() + getFileSize(filename)); } void Ssl::CertificateDb::subSize(std::string const & filename) @@ -401,60 +490,44 @@ throw std::runtime_error("The SSL certificate database " + db_path + " is corrupted. Please rebuild"); db.reset(temp_db.release()); } void Ssl::CertificateDb::save() { if (!db) throw std::runtime_error("The certificates database is not loaded");; // To save the db to file, create a new BIO with BIO file methods. Ssl::BIO_Pointer out(BIO_new(BIO_s_file())); if (!out || !BIO_write_filename(out.get(), const_cast(db_full.c_str()))) throw std::runtime_error("Failed to initialize " + db_full + " file for writing");; if (TXT_DB_write(out.get(), db.get()) < 0) throw std::runtime_error("Failed to write " + db_full + " file"); } // Normally defined in defines.h file -#define countof(arr) (sizeof(arr)/sizeof(*arr)) void Ssl::CertificateDb::deleteRow(const char **row, int rowIndex) { const std::string filename(cert_full + "/" + row[cnlSerial] + ".pem"); -#if OPENSSL_VERSION_NUMBER >= 0x1000004fL - sk_OPENSSL_PSTRING_delete(db.get()->data, rowIndex); -#else - sk_delete(db.get()->data, rowIndex); -#endif - - const Columns db_indexes[]={cnlSerial, cnlName}; - for (unsigned int i = 0; i < countof(db_indexes); ++i) { -#if OPENSSL_VERSION_NUMBER >= 0x1000004fL - if (LHASH_OF(OPENSSL_STRING) *fieldIndex = db.get()->index[db_indexes[i]]) - lh_OPENSSL_STRING_delete(fieldIndex, (char **)row); -#else - if (LHASH *fieldIndex = db.get()->index[db_indexes[i]]) - lh_delete(fieldIndex, row); -#endif - } + sq_TXT_DB_delete_row(db.get(), rowIndex); subSize(filename); int ret = remove(filename.c_str()); if (ret < 0 && errno != ENOENT) throw std::runtime_error("Failed to remove certficate file " + filename + " from db"); } bool Ssl::CertificateDb::deleteInvalidCertificate() { if (!db) return false; bool removed_one = false; #if OPENSSL_VERSION_NUMBER >= 0x1000004fL for (int i = 0; i < sk_OPENSSL_PSTRING_num(db.get()->data); ++i) { const char ** current_row = ((const char **)sk_OPENSSL_PSTRING_value(db.get()->data, i)); #else for (int i = 0; i < sk_num(db.get()->data); ++i) { const char ** current_row = ((const char **)sk_value(db.get()->data, i)); #endif === modified file 'src/ssl/certificate_db.h' --- src/ssl/certificate_db.h 2012-10-08 05:21:11 +0000 +++ src/ssl/certificate_db.h 2012-11-06 17:52:36 +0000 @@ -60,40 +60,42 @@ class CertificateDb { public: /// Names of db columns. enum Columns { cnlType = 0, cnlExp_date, cnlRev_date, cnlSerial, cnlFile, cnlName, cnlNumber }; /// A wrapper for OpenSSL database row of TXT_DB database. class Row { public: /// Create row wrapper. Row(); + ///Create row wrapper for row with width items + Row(char **row, size_t width); /// Delete all row. ~Row(); void setValue(size_t number, char const * value); ///< Set cell's value in row char ** getRow(); ///< Raw row void reset(); ///< Abandon row and don't free memory private: char **row; ///< Raw row size_t width; ///< Number of cells in the row }; CertificateDb(std::string const & db_path, size_t aMax_db_size, size_t aFs_block_size); /// Find certificate and private key for host name bool find(std::string const & host_name, Ssl::X509_Pointer & cert, Ssl::EVP_PKEY_Pointer & pkey); /// Delete a certificate from database bool purgeCert(std::string const & key); /// Save certificate to disk. bool addCertAndPrivateKey(Ssl::X509_Pointer & cert, Ssl::EVP_PKEY_Pointer & pkey, std::string const & useName); /// Create and initialize a database under the db_path static void create(std::string const & db_path); /// Check the database stored under the db_path. @@ -101,40 +103,45 @@ bool IsEnabledDiskStore() const; ///< Check enabled of dist store. private: void load(); ///< Load db from disk. void save(); ///< Save db to disk. size_t size() const; ///< Get db size on disk in bytes. /// Increase db size by the given file size and update size_file void addSize(std::string const & filename); /// Decrease db size by the given file size and update size_file void subSize(std::string const & filename); size_t readSize() const; ///< Read size from file size_file void writeSize(size_t db_size); ///< Write size to file size_file. size_t getFileSize(std::string const & filename); ///< get file size on disk. /// Only find certificate in current db and return it. bool pure_find(std::string const & host_name, Ssl::X509_Pointer & cert, Ssl::EVP_PKEY_Pointer & pkey); void deleteRow(const char **row, int rowIndex); ///< Delete a row from TXT_DB bool deleteInvalidCertificate(); ///< Delete invalid certificate. bool deleteOldestCertificate(); ///< Delete oldest certificate. bool deleteByHostname(std::string const & host); ///< Delete using host name. + /// Removes the first matching row from TXT_DB. Ignores failures. + static void sq_TXT_DB_delete(TXT_DB *db, const char **row); + /// Remove the row on position idx from TXT_DB. Ignores failures. + static void sq_TXT_DB_delete_row(TXT_DB *db, int idx); + /// Callback hash function for serials. Used to create TXT_DB index of serials. static unsigned long index_serial_hash(const char **a); /// Callback compare function for serials. Used to create TXT_DB index of serials. static int index_serial_cmp(const char **a, const char **b); /// Callback hash function for names. Used to create TXT_DB index of names.. static unsigned long index_name_hash(const char **a); /// Callback compare function for names. Used to create TXT_DB index of names.. static int index_name_cmp(const char **a, const char **b); /// Definitions required by openSSL, to use the index_* functions defined above ///with TXT_DB_create_index. #if OPENSSL_VERSION_NUMBER > 0x10000000L static unsigned long index_serial_LHASH_HASH(const void *a) { return index_serial_hash((const char **)a); } static int index_serial_LHASH_COMP(const void *arg1, const void *arg2) { return index_serial_cmp((const char **)arg1, (const char **)arg2); } static unsigned long index_name_LHASH_HASH(const void *a) { return index_name_hash((const char **)a);