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.
  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

diff --git a/src/ssl/certificate_db.cc b/src/ssl/certificate_db.cc
index addd853dd2a45fef400de0f882d89c1a5ab3cd6b..303c11917caf49553b648a7f32317910dd858899 100644 (file)
--- a/src/ssl/certificate_db.cc
+++ b/src/ssl/certificate_db.cc
@@ -103,19 +103,38 @@ Ssl::Locker::~Locker()
 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]);
+        }
+    } else {
+        for (size_t i = 0; i < width + 1; ++i) {
+            if (row[i])
+                OPENSSL_free(row[i]);
         }
-        delete[](row);
     }
+    OPENSSL_free(row);
 }
 
 void Ssl::CertificateDb::Row::reset()
@@ -130,7 +149,7 @@ void Ssl::CertificateDb::Row::setValue(size_t cell, char const * value)
         free(row[cell]);
     }
     if (value) {
-        row[cell] = static_cast<char *>(malloc(sizeof(char) * (strlen(value) + 1)));
+        row[cell] = static_cast<char *>(OPENSSL_malloc(sizeof(char) * (strlen(value) + 1)));
         memcpy(row[cell], value, sizeof(char) * (strlen(value) + 1));
     } else
         row[cell] = NULL;
@@ -141,6 +160,55 @@ 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];
@@ -227,13 +295,24 @@ bool Ssl::CertificateDb::addCertAndPrivateKey(Ssl::X509_Pointer & cert, Ssl::EVP
     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<char, tidyFree> 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()
@@ -243,8 +322,10 @@ bool Ssl::CertificateDb::addCertAndPrivateKey(Ssl::X509_Pointer & cert, Ssl::EVP
 
         // 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;
     }
@@ -260,13 +341,22 @@ bool Ssl::CertificateDb::addCertAndPrivateKey(Ssl::X509_Pointer & cert, Ssl::EVP
         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();
@@ -315,10 +405,8 @@ bool Ssl::CertificateDb::pure_find(std::string const & host_name, Ssl::X509_Poin
     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");
@@ -418,26 +506,10 @@ void Ssl::CertificateDb::save()
 }
 
 // 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());

diff --git a/src/ssl/certificate_db.h b/src/ssl/certificate_db.h
index c817fa3390cb446243b490b05987f29a11c9693d..fa571f429389d4b0e65a66ed90588ffd9399659e 100644 (file)
--- a/src/ssl/certificate_db.h
+++ b/src/ssl/certificate_db.h
@@ -77,6 +77,8 @@ public:
     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
@@ -118,6 +120,11 @@ private:
     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.